diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 15803d7c8..f31379d29 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -196,13 +196,20 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< private static final Matcher STATIC_STREAM_FACTORIES = MethodMatchers.staticMethod().onClass(Stream.class.getName()).namedAnyOf("of", "ofNullable", "concat"); + private static final Matcher 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 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 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/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 8f1e70fb7..741c7e49c 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-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 new file mode 100644 index 000000000..6c1cbb5ea --- /dev/null +++ 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