Copilot commented on code in PR #25628:
URL: https://github.com/apache/pulsar/pull/25628#discussion_r3170421833
##########
pulsar-package-management/core/src/main/java/org/apache/pulsar/packages/management/core/common/PackageName.java:
##########
@@ -136,7 +137,13 @@ public String toString() {
}
public String toRestPath() {
- return String.format("%s/%s/%s/%s/%s", type, tenant, namespace, name,
version);
+ // Use Guava's urlPathSegmentEscaper to safely encode each segment and
prevents Path Traversal (CWE-22)
+ return String.format("%s/%s/%s/%s/%s",
+ type.toString(),
+ UrlEscapers.urlPathSegmentEscaper().escape(tenant),
+ UrlEscapers.urlPathSegmentEscaper().escape(namespace),
+ UrlEscapers.urlPathSegmentEscaper().escape(name),
+ UrlEscapers.urlPathSegmentEscaper().escape(version));
Review Comment:
`toRestPath()` calls `UrlEscapers.urlPathSegmentEscaper()` repeatedly.
Consider storing the escaper in a local variable or a `private static final`
field to avoid repeated lookups/allocations and make the intent clearer (encode
each segment with the same escaper).
##########
pulsar-package-management/core/src/test/java/org/apache/pulsar/packages/management/core/common/PackageNameTest.java:
##########
@@ -116,4 +116,22 @@ public void testPackageNameErrors() {
PackageName name = PackageName.get("function://public/default/test");
Assert.assertEquals("function://public/default/test@latest",
name.toString());
}
+
+ @Test
+ public void testPathTraversalBypassConstructor() throws Exception {
+ // Create a normal, valid package to bypass the splitter
+ PackageName packageName =
PackageName.get("function://tenant-a/ns/name@v1");
+
+ java.lang.reflect.Field tenantField =
PackageName.class.getDeclaredField("tenant");
+ tenantField.setAccessible(true);
+ tenantField.set(packageName, "tenant-a/../../system-tenant");
+ // Define what the SAFE, patched output should look like
+ String expectedSafePath =
"function/tenant-a%2F..%2F..%2Fsystem-tenant/ns/name/v1";
+
+ // Trigger the vulnerable method
+ String actualPath = packageName.toRestPath();
+ // The Trap: This will fail, because actualPath will still contain
unencoded slashes
+ Assert.assertEquals(actualPath, expectedSafePath,
+ "VULNERABILITY REPLICATED: The toRestPath method failed to
encode path traversal characters!");
Review Comment:
The inline comments and assertion failure message read as if the test is
meant to demonstrate that the vulnerability still exists (“The Trap: This will
fail…”, “VULNERABILITY REPLICATED…”). Since the assertion expects the
safe/encoded output, please update these to describe the intended security
property being verified to avoid confusion when reading test output.
```suggestion
// Create a valid package instance, then mutate an internal field to
verify REST path encoding.
PackageName packageName =
PackageName.get("function://tenant-a/ns/name@v1");
java.lang.reflect.Field tenantField =
PackageName.class.getDeclaredField("tenant");
tenantField.setAccessible(true);
tenantField.set(packageName, "tenant-a/../../system-tenant");
// Verify that path separators in package components are
percent-encoded in the generated REST path.
String expectedSafePath =
"function/tenant-a%2F..%2F..%2Fsystem-tenant/ns/name/v1";
// Invoke the method under test and confirm it preserves
path-segment boundaries safely.
String actualPath = packageName.toRestPath();
// This assertion verifies that traversal characters are encoded
rather than emitted as raw slashes.
Assert.assertEquals(actualPath, expectedSafePath,
"The toRestPath method must encode path traversal characters
in package components.");
```
##########
pulsar-package-management/core/src/test/java/org/apache/pulsar/packages/management/core/common/PackageNameTest.java:
##########
@@ -116,4 +116,22 @@ public void testPackageNameErrors() {
PackageName name = PackageName.get("function://public/default/test");
Assert.assertEquals("function://public/default/test@latest",
name.toString());
}
+
+ @Test
+ public void testPathTraversalBypassConstructor() throws Exception {
+ // Create a normal, valid package to bypass the splitter
+ PackageName packageName =
PackageName.get("function://tenant-a/ns/name@v1");
+
+ java.lang.reflect.Field tenantField =
PackageName.class.getDeclaredField("tenant");
+ tenantField.setAccessible(true);
+ tenantField.set(packageName, "tenant-a/../../system-tenant");
Review Comment:
This test mutates a `PackageName` instance obtained from
`PackageName.get(...)`, which is backed by a static `LoadingCache`. Modifying
the cached instance via reflection can leak state into other tests (or later
lookups of the same key) and cause flaky failures. Prefer constructing a
non-cached instance (e.g., via reflective access to the private constructor) or
otherwise ensure the mutated instance cannot be reused from the cache.
##########
pulsar-package-management/core/src/main/java/org/apache/pulsar/packages/management/core/common/PackageName.java:
##########
@@ -136,7 +137,13 @@ public String toString() {
}
public String toRestPath() {
- return String.format("%s/%s/%s/%s/%s", type, tenant, namespace, name,
version);
+ // Use Guava's urlPathSegmentEscaper to safely encode each segment and
prevents Path Traversal (CWE-22)
Review Comment:
Comment grammar: “...encode each segment and prevents...” should be
“...encode each segment and prevent...” (or rephrase). Also consider
capitalizing consistently (e.g., `URL`/`RFC 3986`) if you keep the comment.
```suggestion
// Use Guava's URL path segment escaper to safely encode each
segment and prevent path traversal (CWE-22).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
