Implement RID safety passthrough based on the locator (#2202)

Implement RID safety passthrough based on the locator component
palantir · Apr 18, 2022 · 4ea77f9 · 4ea77f9
1 parent 69d5ab0
commit 4ea77f9
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 1 deletion.
diff --git a/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
@@ -196,13 +196,20 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
     private static final Matcher<ExpressionTree> STATIC_STREAM_FACTORIES =
             MethodMatchers.staticMethod().onClass(Stream.class.getName()).namedAnyOf("of", "ofNullable", "concat");
 
+    private static final Matcher<ExpressionTree> RID_FACTORY = MethodMatchers.staticMethod()
+            .onClass("com.palantir.ri.ResourceIdentifier")
+            // capture all overloads -- in the case of the multi-parameter methods, only the 'locator' components
+            // really matter, however the rest are required to be safe, so they cannot poison results.
+            .namedAnyOf("of", "valueOf");
+
     // These methods do not take the receiver (generally a static class) into account, only the inputs.
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_COMBINATION_OF_ARGS = Matchers.anyOf(
             STRING_FORMAT,
             OBJECTS_TO_STRING,
             IMMUTABLE_COLLECTION_FACTORY,
             OPTIONAL_FACTORIES,
-            STATIC_STREAM_FACTORIES);
+            STATIC_STREAM_FACTORIES,
+            RID_FACTORY);
 
     private static final Matcher<ExpressionTree> OPTIONAL_ACCESSORS = Matchers.anyOf(
             MethodMatchers.instanceMethod()
@@ -258,6 +265,11 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
                     .onDescendantOf(Iterator.class.getName())
                     .named("next")
                     .withNoParameters(),
+            // Rid components are considered safe, except for 'locator' which inherits the safety of the rid object.
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf("com.palantir.ri.ResourceIdentifier")
+                    .named("getLocator")
+                    .withNoParameters(),
             OPTIONAL_ACCESSORS,
             STREAM_ACCESSORS);
 

diff --git a/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
@@ -1189,6 +1189,66 @@ public void testRawTypes() {
                 .doTest();
     }
 
+    @Test
+    public void testResourceIdentifierComponentSafety() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import java.util.function.*;",
+                        "import com.palantir.ri.*;",
+                        "class Test {",
+                        "  void f(@Unsafe ResourceIdentifier rid) {",
+                        "    fun(rid.getService());",
+                        "    fun(rid.getInstance());",
+                        "    fun(rid.getType());",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(rid.getLocator());",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(rid);",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(rid.toString());",
+                        "  }",
+                        "  void fun(@Safe Object in) {}",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testResourceIdentifierCreationSafety() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import java.util.function.*;",
+                        "import com.palantir.ri.*;",
+                        "class Test {",
+                        "  void f(@Unsafe String value) {",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    safe(ResourceIdentifier.of(value));",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    safe(ResourceIdentifier.valueOf(value));",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    safe(ResourceIdentifier.of(\"service\", \"instance\", \"type\", value));",
+                        "    ResourceIdentifier varArgs = ResourceIdentifier.of(",
+                        "      \"service\", \"instance\", \"type\", \"loc\", value);",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    safe(varArgs);",
+                        "    safe(ResourceIdentifier.valueOf(\"ri.service.instance.type.name\"));",
+                        "    safe(ResourceIdentifier.of(\"ri.service.instance.type.name\"));",
+                        "    safe(ResourceIdentifier.of(\"service\", \"instance\", \"type\", \"safe-locator\"));",
+                        "    safe(ResourceIdentifier.of(\"service\", \"instance\", \"type\", \"safe\", \"locator\"));",
+                        "  }",
+                        "  void safe(@Safe Object in) {}",
+                        "}")
+                .doTest();
+    }
+
     @Test
     public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() {
         refactoringHelper()

diff --git a/changelog/@unreleased/pr-2202.v2.yml b/changelog/@unreleased/pr-2202.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Implement RID safety passthrough based on the locator component
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2202