Custom Code Quality Rules

This page describes the custom code quality rules executed by Cloud Manager created based on best practices from AEM Engineering.

SonarQube Rules

The following section highlights the SonarQube Rules:

Do not use potentially dangerous functions

Key: CQRules:CWE-676

Type: Vulnerability

Severity: Major

Since: Version 2018.4.0

The methods Thread.stop() and Thread.interrupt() can produce hard-to-reproduce issues and, in some cases, security vulnerabilities. Their usage should be tightly monitored and validated. In general, message passing is a safer way to accomplish similar goals.

Non-Compliant Code

public class DontDoThis implements Runnable {
  private Thread thread;
 
  public void start() {
    thread = new Thread(this);
    thread.start();
  }
 
  public void stop() {
    thread.stop();  // UNSAFE!
  }
 
  public void run() {
    while (true) {
        somethingWhichTakesAWhileToDo();
    }
  }
}

Compliant Code

public class DoThis implements Runnable {
  private Thread thread;
  private boolean keepGoing = true;
 
  public void start() {
    thread = new Thread(this);
    thread.start();
  }
 
  public void stop() {
    keepGoing = false;
  }
 
  public void run() {
    while (this.keepGoing) {
        somethingWhichTakesAWhileToDo();
    }
  }
}

Do not use format strings which may be externally controlled

Key: CQRules:CWE-134

Type: Vulnerability

Severity: Major

Since: Version 2018.4.0

Using a format string from an external source (such a request parameter or user-generated content) can produce can expose an application to denial of service attacks. There are circumstances where a format string may be externally controlled, but is only allowed from trusted sources.

Non-Compliant Code

protected void doPost(SlingHttpServletRequest request, SlingHttpServletResponse response) {
  String messageFormat = request.getParameter("messageFormat");
  request.getResource().getValueMap().put("some property", String.format(messageFormat, "some text"));
  response.sendStatus(HttpServletResponse.SC_OK);
}

HTTP requests should always have socket and connect timeouts

Key: CQRules:ConnectionTimeoutMechanism

Type: Bug

Severity: Critical

Since: Version 2018.6.0

When executing HTTP requests from inside an AEM application, it is critical to ensure that proper timeouts are configured in order to avoid unnecessary thread consumption. Unfortunately, the default behavior of both Java’s default HTTP Client (java.net.HttpUrlConnection) and the commonly used Apache HTTP Components client is to never timeout, so timeouts must be explicitly set. Further, as a best practice, these timeouts should be no more than 60 seconds.

Non-Compliant Code

@Reference
private HttpClientBuilderFactory httpClientBuilderFactory;
 
public void dontDoThis() {
  HttpClientBuilder builder = httpClientBuilderFactory.newBuilder();
  HttpClient httpClient = builder.build();

  // do something with the client
}

public void dontDoThisEither() {
  URL url = new URL("http://www.google.com");
  URLConnection urlConnection = url.openConnection();
 
  BufferedReader in = new BufferedReader(new InputStreamReader(
    urlConnection.getInputStream()));
 
  String inputLine;
  while ((inputLine = in.readLine()) != null) {
    logger.info(inputLine);
  }
 
  in.close();
}

Compliant Code

@Reference
private HttpClientBuilderFactory httpClientBuilderFactory;
 
public void doThis() {
  HttpClientBuilder builder = httpClientBuilderFactory.newBuilder();
  RequestConfig requestConfig = RequestConfig.custom()
    .setConnectTimeout(5000)
    .setSocketTimeout(5000)
    .build();
  builder.setDefaultRequestConfig(requestConfig);
 
  HttpClient httpClient = builder.build();
   
  // do something with the client
}

public void orDoThis() {
  URL url = new URL("http://www.google.com");
  URLConnection urlConnection = url.openConnection();
  urlConnection.setConnectTimeout(5000);
  urlConnection.setReadTimeout(5000);
 
  BufferedReader in = new BufferedReader(new InputStreamReader(
    urlConnection.getInputStream()));
 
  String inputLine;
  while ((inputLine = in.readLine()) != null) {
    logger.info(inputLine);
  }
 
  in.close();
}

Product APIs annotated with @ProviderType should not be implemented or extended by customers

Key: CQBP-84, CQBP-84-dependencies

Type: Bug

Severity: Critical

Since: Version 2018.7.0

The AEM API contains Java interfaces and classes which are only meant to be used, but not implemented, by custom code. For example, the interface com.day.cq.wcm.api.Page is designed to be implemented by AEM only.

When new methods are added to these interfaces, those additional methods do not impact existing code which uses these interfaces and, as a result, the addition of new methods to these interfaces are considered to be backwards-compatible. However, if custom code implements one of these interfaces, that custom code has introduced a backwards-compatibility risk for the customer.

Interfaces (and classes) which are only intended to be implemented by AEM are annotated with org.osgi.annotation.versioning.ProviderType (or, in some cases, a similar legacy annotation aQute.bnd.annotation.ProviderType). This rule identifies the cases where such an interface is implemented (or a class is extended) by custom code.

Non-Compliant Code

import com.day.cq.wcm.api.Page;

public class DontDoThis implements Page {
// implementation here
}

ResourceResolver objects should always be closed

Key: CQRules:CQBP-72

Type: Code Smell

Severity: Major

Since: Version 2018.4.0

ResourceResolver objects obtained from the ResourceResolverFactory consume system resources. Although there are measures in place to reclaim these resources when a ResourceResolver is no longer in use, it is more efficient to explicitly close any opened ResourceResolver objects by calling the close() method.

One relatively common misconception is that ResourceResolver objects created using an existing JCR Session should not be explicitly closed or that doing so will close the underlying JCR Session. This is not the case - regardless of how a ResourceResolver is opened, it should be closed when no longer used. Since ResourceResolver implements the Closeable interface, it is also possible to use the try-with-resources syntax instead of explicitly invoking close().

Non-Compliant Code

public void dontDoThis(Session session) throws Exception {
  ResourceResolver resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object)session));
  // do some stuff with the resolver
}

Compliant Code

public void doThis(Session session) throws Exception {
  ResourceResolver resolver = null;
  try {
    resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object)session));
    // do something with the resolver
  } finally {
    if (resolver != null) {
      resolver.close();
    }
  }
}

public void orDoThis(Session session) throws Exception {
  try (ResourceResolver resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object) session))){
    // do something with the resolver
  }
}

Do not use Sling servlet paths to register servlets

Key: CQRules:CQBP-75

Type: Code Smell

Severity: Major

Since: Version 2018.4.0

As described in the Sling documentation, bindings servlets by paths is discouraged. Path-bound servlets cannot use standard JCR access controls and, as a result, require additional security rigor. Rather than using path-bound servlets, it is recommended to create nodes in the repository and register servlets by resource type.

Non-Compliant Code

@Component(property = {
  "sling.servlet.paths=/apps/myco/endpoint"
})
public class DontDoThis extends SlingAllMethodsServlet {
 // implementation
}

Caught Exceptions should be logged or thrown, but not both

Key: CQRules:CQBP-44—CatchAndEitherLogOrThrow

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

In general, an exception should be logged exactly one time. Logging exceptions multiple times can cause confusion as it is unclear how many times an exception occurred. The most common pattern which leads to this is logging and throwing a caught exception.

Non-Compliant Code

public void dontDoThis() throws Exception {
  try {
    someOperation();
  } catch (Exception e) {
    logger.error("something went wrong", e);
    throw e;
  }
}

Compliant Code

public void doThis() {
  try {
    someOperation();
  } catch (Exception e) {
    logger.error("something went wrong", e);
  }
}

public void orDoThis() throws MyCustomException {
  try {
    someOperation();
  } catch (Exception e) {
    throw new MyCustomException(e);
  }
}

Avoid having a log statement immediately followed by a throw statement

Key: CQRules:CQBP-44—ConsecutivelyLogAndThrow

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

Another common pattern to avoid is to log a message and then immediately throw an exception. This generally indicates that the exception message will end up duplicated in log files.

Non-Compliant Code

public void dontDoThis() throws Exception {
  logger.error("something went wrong");
  throw new RuntimeException("something went wrong");
}

Compliant Code

public void doThis() throws Exception {
  throw new RuntimeException("something went wrong");
}

Avoid logging at INFO when handling GET or HEAD requests

Key: CQRules:CQBP-44—LogInfoInGetOrHeadRequests

Type: Code Smell

Severity: Minor

In general, the INFO log level should be used to demarcate important actions and, by default, AEM is configured to log at the INFO level or above. GET and HEAD methods should only ever be read-only operations and thus do not constitute important actions. Logging at the INFO level in response to GET or HEAD requests is likely to create significant log noise thereby making it harder to identify useful information in log files. Logging when handling GET or HEAD requests should be either at the WARN or ERROR levels when something has gone wrong or at the DEBUG or TRACE levels if deeper troubleshooting information would be helpful.

Non-Compliant Code

public void doGet() throws Exception {
  logger.info("handling a request from the user");
}

Compliant Code

public void doGet() throws Exception {
  logger.debug("handling a request from the user.");
}

Do not use Exception.getMessage() as the first parameter of a logging statement

Key: CQRules:CQBP-44—ExceptionGetMessageIsFirstLogParam

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

As a best practice, log messages should provide contextual information about where in the application an exception has occurred. While context can also be determined through the use of stack traces, in general the log message is going to be easier to read and understand. As a result, when logging an exception, it is a bad practice to use the exception’s message as the log message - the exception message will contain what went wrong whereas the log message should be used to tell a log reader what the application was doing when the exception happened. The exception message will still be logged; by specifying your own message the logs will just be easier to understand.

Non-Compliant Code

public void dontDoThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.error(e.getMessage(), e);
  }
}

Compliant Code

public void doThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.error("Unable to do something", e);
  }
}

Logging in catch blocks should be at the WARN or ERROR level

Key: CQRules:CQBP-44—WrongLogLevelInCatchBlock

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

As the name suggests, Java exceptions should always be used in exceptional circumstances. As a result, when an exception is caught, it is important to ensure that log messages are logged at the appropriate level - either WARN or ERROR. This ensures that those messages appear correctly in the logs.

Non-Compliant Code

public void dontDoThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.debug(e.getMessage(), e);
  }
}

Compliant Code

public void doThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.error("Unable to do something", e);
  }
}

Do not print stack traces to the console

Key: CQRules:CQBP-44—ExceptionPrintStackTrace

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

As mentioned, context is critical when understanding log messages. Using Exception.printStackTrace() causes only the stack trace to be output to the standard error stream thereby losing all context. Further, in a multi-threaded application like AEM if multiple exceptions are printed using this method in parallel, their stack traces may overlap which produces significant confusion. Exceptions should be logged through the logging framework only.

Non-Compliant Code

public void dontDoThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    e.printStackTrace();
  }
}

Compliant Code

public void doThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.error("Unable to do something", e);
  }
}

Do not output to Standard Output or Standard Error

Key: CQRules:CQBP-44—LogLevelConsolePrinters

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

Logging in AEM should always be done through the logging framework (SLF4J). Outputting directly to the standard output or standard error streams loses the structural and contextual information provided by the logging framework and may, in some cases, cause performance issues.

Non-Compliant Code

public void dontDoThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    System.err.println("Unable to do something");
  }
}

Compliant Code

public void doThis() {
  try {
    someMethodThrowingAnException();
  } catch (Exception e) {
    logger.error("Unable to do something", e);
  }
}

Avoid Hardcoded /apps and /libs Paths

Key: CQRules:CQBP-71

Type: Code Smell

Severity: Minor

Since: Version 2018.4.0

In general, paths which start with /libs and /apps should not be hardcoded as the paths they refer to are most commonly stored as paths relative to the Sling search path (which is set to /libs,/apps by default). Using the absolute path may introduce subtle defects which would only appear later in the project lifecycle.

Non-Compliant Code

public boolean dontDoThis(Resource resource) {
  return resource.isResourceType("/libs/foundation/components/text");
}

Compliant Code

public void doThis(Resource resource) {
  return resource.isResourceType("foundation/components/text");
}

Sling Scheduler Should Not Be Used

Key: CQRules:AMSCORE-554

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

The Sling Scheduler must not be used for tasks that require a guaranteed execution. Sling Scheduled Jobs guarantee execution and better suited for both clustered and non-clustered environments.

Refer to Apache Sling Eventing and Job Handling to learn more about how Sling Jobs are handled in a clustered environments.

AEM Deprecated APIs Should Not Be Used

Key: AMSCORE-553

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

The AEM API surface is under constant revision to identify APIs for which usage is discouraged and thus considered deprecated.

In many cases, these APIs are deprecated using the standard Java @Deprecated annotation and, as such, as identified by squid:CallToDeprecatedMethod.

However, there are cases where an an API is deprecated in the context of AEM but may not be deprecated in other contexts. This rule identifies this second class.

OakPAL Content Rules

Please find below the OakPAL checks executed by Cloud Manager.

Customer Packages Should Not Create or Modify Nodes Under /libs

Key: BannedPaths

Type: Bug

Severity: Blocker

Since: Version 2019.6.0

It has been a long-standing best practice that the /libs content tree in the AEM content repository should be considered read-only by customers. Modifying nodes and properties under /libs creates significant risk for major and minor updates. Modifications to /libs should only be made by Adobe through official channels.

Packages Should Not Contain Duplicate OSGi Configurations

Key: DuplicateOsgiConfigurations

Type: Bug

Severity: Major

Since: Version 2019.6.0

A common problem that occurs on complex projects is where the same OSGi component is configured multiple times. This creates an ambiguity as to which configuration will be operable. This rule is “runmode-aware” in that it will only identify issues where the same component is configured multiple times in the same runmode (or combination of runmodes).

Non Compliant Code

  + projectA
    + config
      + com.day.cq.commons.impl.ExternalizerImpl
  + projectB
    + config
      + com.day.cq.commons.impl.ExternalizerImpl

Compliant Code

  + shared-config
    + config
      + com.day.cq.commons.impl.ExternalizerImpl

Config and Install Folders Should Only Contain OSGi Nodes

Key: ConfigAndInstallShouldOnlyContainOsgiNodes

Type: Bug

Severity: Major

Since: Version 2019.6.0

For security reasons, paths containing /config/ and /install/ are only readable by administrative users in AEM and should be used only for OSGi configuration and OSGi bundles. Placing other types of content under paths which contain these segments results in application behavior which unintentionally varies between administrative and non-administrative users.

A common problem is use of nodes named config within component dialogs or when specifying the rich text editor configuration for inline editing. To resolve this the offending node should be renamed to a compliant name. For the rich text editor configuration make use of the configPath property on the cq:inplaceEditing node to specify the new location.

Non Compliant Code

+ cq:editConfig [cq:EditConfig]
  + cq:inplaceEditing [cq:InplaceEditConfig]
    + config [nt:unstructured]
      + rtePlugins [nt:unstructured]

Compliant Code

+ cq:editConfig [cq:EditConfig]
  + cq:inplaceEditing [cq:InplaceEditConfig]
    ./configPath = inplaceEditingConfig (String)
    + inplaceEditingConfig [nt:unstructured]
      + rtePlugins [nt:unstructured]

Packages Should Not Overlap

Key: PackageOverlaps

Type: Bug

Severity: Major

Since: Version 2019.6.0

Similar to the Packages Should Not Contain Duplicate OSGi Configurations this is a common problem on complex projects where the same node path is written to by multiple separate content packages. While using content package dependencies can be used to ensure a consistent result, it is better to avoid overlaps entirely.

Default Authoring Mode Should Not Be Classic UI

Key: ClassicUIAuthoringMode

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

The OSGi configuration com.day.cq.wcm.core.impl.AuthoringUIModeServiceImpl defines the default authoring mode within AEM. As Classic UI has been deprecated since AEM 6.4, an issue will now be raised when the default authoring mode is configured to Classic UI.

Components With Dialogs Should Have Touch UI Dialogs

Key: ComponentWithOnlyClassicUIDialog

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

AEM Components which have a Classic UI dialog should always have a corresponding Touch UI dialog both to provide an optimal authoring experience and to be compatible with the Cloud Service deployment model, where Classic UI is not supported. This rule verifies the following scenarios:

• A component with a Classic UI dialog (that is, a dialog child node) must have a corresponding Touch UI dialog (that is, a cq:dialog child node).
• A component with a Classic UI design dialog (i.e. a design_dialog node) must have a corresponding Touch UI design dialog (that is, a cq:design_dialog child node).
• A component with both a Classic UI dialog and a Classic UI design dialog must have both a corresponding Touch UI dialog and a corresponding Touch UI design dialog.

The AEM Modernization Tools documentation provides documentation and tooling for how to convert components from Classic UI to Touch UI. Refer to The AEM Modernization Tools for more details.

Packages Should Not Mix Mutable and Immutable Content

Key: ImmutableMutableMixedPackage

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

In order to be compatible with the Cloud Service deployment model, individual content packages must contain either content for the immutable areas of the repository (that is, /apps and /libs, although /libs should not be modified by customer code and will cause a separate violation) or the mutable area (that is, everything else), but not both. For example, a package which includes both /apps/myco/components/text and /etc/clientlibs/myco is not compatible with Cloud Service and will cause an issue to be reported.

Refer to AEM Project Structure for more details.

Reverse Replication Agents Should Not Be Used

Key: ReverseReplication

Type: Code Smell/Cloud Service Compatibility

Severity: Minor

Since: Version 2020.5.0

Support for Reverse Replication is not available in Cloud Service deployments, as described in Release Notes: Removal of Replication Agents.

Customers using reverse replication should contact Adobe for alternative solutions.