Switch order of literals to prevent NullPointerException #868

citizenjosh · 2024-01-23T20:05:32Z

This change defensively switches the order of literals in comparison expressions to ensure that no null pointer exceptions are unexpectedly thrown. Runtime exceptions especially can cause exceptional and unexpected code paths to be taken, and this can result in unexpected behavior.

Both simple vulnerabilities (like information disclosure) and complex vulnerabilities (like business logic flaws) can take advantage of these unexpected code paths.

Our changes look something like this:

  String fieldName = header.getFieldName();
  String fieldValue = header.getFieldValue();
- if(fieldName.equals("requestId")) {
+ if("requestId".equals(fieldName)) {
    logRequest(fieldValue);
  }

More reading

Powered by: pixeebot (codemod ID: pixee:java/switch-literal-first)

…-java/switch-literal-first Switch order of literals to prevent NullPointerException

CLAassistant · 2024-01-23T20:05:38Z

All committers have signed the CLA.

ibodrov

4/9

ibodrov · 2024-01-23T20:47:36Z

client2/src/main/java/com/walmartlabs/concord/client2/impl/ResponseBodyHandler.java

@@ -77,7 +77,7 @@ public static <T> T handle(ObjectMapper objectMapper,

    private static boolean isJsonMime(String mime) {
        String jsonMime = "(?i)^(application/json|[^;/ \t]+/[^;/ \t]+[+]json)[ \t]*(;.*)?$";
-        return mime != null && (mime.matches(jsonMime) || mime.equals("*/*"));
+        return mime != null && (mime.matches(jsonMime) || "*/*".equals(mime));


Useless. Non-null value here is already asserted.

The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:48:55Z

plugins/tasks/lock/src/main/java/com/walmartlabs/concord/plugins/lock/LockTaskCommon.java

@@ -82,8 +82,8 @@ private String checkScope(String scope) {
            return null;
        }

-        if (!scope.equalsIgnoreCase("ORG")
-                && !scope.equalsIgnoreCase("PROJECT")) {
+        if (!"ORG".equalsIgnoreCase(scope)


Checked before.

The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:49:53Z

runtime/v1/impl/src/main/java/com/walmartlabs/concord/runner/ConcordSecurityManager.java

@@ -60,7 +60,7 @@ public void checkPermission(Permission perm) {
            String path = perm.getName();

            // allow all local paths
-            if (!path.startsWith("/") && !path.equals(ALL_FILES_TOKEN)) {
+            if (!path.startsWith("/") && !ALL_FILES_TOKEN.equals(path)) {


Cannot be null here.

The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:50:36Z

...roject-model/src/main/java/com/walmartlabs/concord/project/yaml/converter/StepConverter.java

@@ -278,7 +278,7 @@ default Set<VariableMapping> getVarMap(Map<String, Object> options, String key)
            result.add(new VariableMapping(null, sourceExpr, sourceValue, target, true));
        }

-        if (key.equals("in") && getWithItems(options) != null) {
+        if ("in".equals(key) && getWithItems(options) != null) {


The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:51:02Z

server/impl/src/main/java/com/walmartlabs/concord/server/org/project/ProjectValidator.java

@@ -121,7 +121,7 @@ private static void validateSpec(Trigger t, List<String> errors) {

    private static void validateTimezone(Trigger t, List<String> errors) {
        String triggerName = t.name();
-        if (!triggerName.equals("cron")) {
+        if (!"cron".equals(triggerName)) {


Maybe useful?

The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:52:08Z

server/impl/src/main/java/com/walmartlabs/concord/server/user/UserManager.java

@@ -260,7 +260,7 @@ private UserType assertUserType(String username, String domain, UserType type) {
    }

    private UserType assertSsoUserType(UserPrincipal u, UserType type) {
-        if (u.getRealm().equals(SSO_REALM_NAME)) {
+        if (SSO_REALM_NAME.equals(u.getRealm())) {


ibodrov · 2024-01-23T20:52:22Z

...iquibase-ext/src/main/java/com/walmartlabs/concord/server/liquibase/ext/ApiTokenCreator.java

@@ -118,7 +118,7 @@ public String getConfirmationMessage() {

    @Override
    public void setUp() {
-        if (this.skip.equals("true")) {
+        if ("true".equals(this.skip)) {


ibodrov · 2024-01-23T20:52:39Z

...idc/src/main/java/com/walmartlabs/concord/server/plugins/oidc/OidcAuthenticationHandler.java

@@ -70,7 +70,7 @@ public AuthenticationToken createToken(ServletRequest request, ServletResponse r
        String header = req.getHeader(AUTHORIZATION_HEADER);
        if (header != null) {
            String[] as = header.split(" ");
-            if (as.length != 2 || !as[0].equals(HEADER_PREFIX)) {
+            if (as.length != 2 || !HEADER_PREFIX.equals(as[0])) {


Cannot be null.

The purpose of this change is to be proactively defensive. Though it may be negligibly faster to run this way, it also conforms to a best practice and makes the code easier to read. Also, since the behavior is otherwise equivalent, there is no cost to this change.

ibodrov · 2024-01-23T20:52:51Z

agent/src/main/java/com/walmartlabs/concord/agent/executors/runner/JobDependencies.java

@@ -186,7 +186,7 @@ private static Map<String, String> getDependencyVersionsFromFile(RunnerJob job)
    }

    private static boolean isLatestVersion(String v) {
-        return v.equalsIgnoreCase("latest");
+        return "latest".equalsIgnoreCase(v);


ibodrov · 2024-01-23T20:54:00Z

This bot need more training.

drdavella · 2024-03-01T15:24:54Z

@ibodrov thanks for the feedback on this PR!

I'm a maintainer of @pixeebot and just wanted to point out that @pixeebot is not actually using AI to make this change. We use an OSS framework called Codemodder: https://github.com/pixee/codemodder-java

This particular codemod is implemented here: https://github.com/pixee/codemodder-java/blob/main/core-codemods/src/main/java/io/codemodder/codemods/SwitchLiteralFirstComparisonsCodemod.java

A big part of our philosophy is to introduce proactively defensive changes that have no downside. In this particular case, it seems you've identified four places where the code potentially benefits from this hardening and five places where maybe it's not as obviously useful.

Given that, I'm curious whether you see any downside to merging the code as-is given that it's a defensive change that also reflects a best practice? Your feedback is appreciated.

pixeebot and others added 2 commits January 20, 2024 01:21

Switch order of literals to prevent NullPointerException

e2d1f71

Merge pull request #6 from citizenjosh/pixeebot/drip-2024-01-20-pixee…

4064ddf

…-java/switch-literal-first Switch order of literals to prevent NullPointerException

ibodrov requested changes Jan 23, 2024

View reviewed changes

remove change

be1ab4d

citizenjosh requested a review from ibodrov January 25, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch order of literals to prevent NullPointerException #868

Switch order of literals to prevent NullPointerException #868

citizenjosh commented Jan 23, 2024

CLAassistant commented Jan 23, 2024 •

edited

ibodrov left a comment

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024

ibodrov Jan 23, 2024

ibodrov Jan 23, 2024

ibodrov Jan 23, 2024

citizenjosh Jan 24, 2024 •

edited

ibodrov Jan 23, 2024

ibodrov commented Jan 23, 2024

drdavella commented Mar 1, 2024 •

edited

Switch order of literals to prevent NullPointerException #868

Are you sure you want to change the base?

Switch order of literals to prevent NullPointerException #868

Conversation

citizenjosh commented Jan 23, 2024

CLAassistant commented Jan 23, 2024 • edited

ibodrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citizenjosh Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibodrov commented Jan 23, 2024

drdavella commented Mar 1, 2024 • edited

CLAassistant commented Jan 23, 2024 •

edited

citizenjosh Jan 24, 2024 •

edited

drdavella commented Mar 1, 2024 •

edited