[ignore] Fix Codacy field-placement and setAccessible() findings

duncdrum · claude · duncdrum · commit 6a509e0f8500 · 2026-06-19T21:51:09.000+02:00
Move XSD11_DETECTION_CACHE/XSD11_DETECTION_CACHE_MAX_ENTRIES to the
top of Jaxp, alongside its other fields, instead of declaring them
mid-class next to the methods that use them.

Make isXsd11Schema package-private instead of private, the same
pattern already used for clearXsd11DetectionCache(), so
JaxpSchemaLocationSecurityTest/JaxpXsd11DetectionCacheTest (both in
the same package) can call it directly instead of via
setAccessible(true).

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;

Also scopes XSD11_DETECTION_CACHE by the requesting Subject's name (a
cache hit skips isXsd11Schema's permission-checked openStream() entirely,
so without this a Subject lacking read permission on a schema resource
could observe a boolean populated by a different Subject's earlier fetch),
and replaces the hand-rolled synchronized LinkedHashMap-based LRU with a
Caffeine cache (already a project dependency) for the same bound with less
hand-written locking.

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/exist-core/src/main/java/org/exist/xquery/functions/validation/Jaxp.java b/exist-core/src/main/java/org/exist/xquery/functions/validation/Jaxp.java
@@ -29,12 +29,14 @@
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
 import javax.annotation.Nullable;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.parsers.SAXParser;
@@ -109,6 +111,33 @@ public class Jaxp extends BasicFunction {
     private static final String XSI_NS = "http://www.w3.org/2001/XMLSchema-instance";
     private static final String XSD_VERSIONING_NS = "http://www.w3.org/2007/XMLSchema-versioning";
 
+    /**
+     * Bound on the size of {@link #XSD11_DETECTION_CACHE} (see there for what it caches).
+     */
+    private static final int XSD11_DETECTION_CACHE_MAX_ENTRIES = 256;
+
+    /**
+     * Cache key for {@link #XSD11_DETECTION_CACHE}: the requesting Subject's name plus the
+     * resolved schema URI. Including the Subject prevents a Subject without read permission on
+     * the schema resource from observing a boolean populated by a different (permitted)
+     * Subject's earlier, permission-checked fetch -- a cache hit skips {@link
+     * #isXsd11Schema(String, String, String)}'s {@code openStream()} entirely, so without this
+     * the cache itself would bypass whatever permission check that open would otherwise perform.
+     */
+    private record Xsd11DetectionCacheKey(String subjectName, String resolvedSchemaUri) {
+    }
+
+    /**
+     * Bounded (see {@link #XSD11_DETECTION_CACHE_MAX_ENTRIES}), LRU-evicted cache of
+     * "does the schema at this resolved URI declare vc:minVersion 1.1?", so that validating many
+     * documents against the same schema doesn't re-fetch and re-peek it every time. Cleared by
+     * {@code validation:clear-grammar-cache()} (see {@link GrammarTooling}) alongside the Xerces
+     * grammar pool, so operators have one function to clear every validation-related cache.
+     */
+    private static final Cache<Xsd11DetectionCacheKey, Boolean> XSD11_DETECTION_CACHE = Caffeine.newBuilder()
+            .maximumSize(XSD11_DETECTION_CACHE_MAX_ENTRIES)
+            .build();
+
     private static final String simpleFunctionTxt = """
             Validate document by parsing $instance. Optionally \
             grammar caching can be enabled. Supported grammars types \
@@ -445,12 +474,12 @@ private record ParseTarget(ContentHandler contenthandler, @Nullable MemTreeBuild
 
     /**
      * Acquires a fresh, disposable {@link InputSource} for the instance and runs
-     * {@link #detectXsd11ViaSchemaLocation(InputSource)} against it.
+     * {@link #detectXsd11ViaSchemaLocation(String, InputSource)} against it.
      */
     private boolean peekIsXsd11ViaSchemaLocation(final Sequence[] args) throws XPathException, IOException {
         final InputSource peekInstance = Shared.getInputSource(args[0].itemAt(0), context);
         try {
-            return detectXsd11ViaSchemaLocation(peekInstance);
+            return detectXsd11ViaSchemaLocation(context.getSubject().getName(), peekInstance);
         } finally {
             Shared.closeInputSource(peekInstance);
         }
@@ -531,9 +560,11 @@ private ParseTarget retryWithXsd11ValidatorIfNeeded(final Sequence[] args, final
      * retry-after-failure check in {@code eval()} remains the safety net for those cases
      * (catalog-mediated locations, an unresolvable hint, etc.).
      *
+     * @param subjectName the requesting Subject's name, used to scope {@link #XSD11_DETECTION_CACHE}
+     *                     (see there for why).
      * @param peekInstance a fresh, not-yet-consumed InputSource for the same instance document.
      */
-    private static boolean detectXsd11ViaSchemaLocation(final InputSource peekInstance) {
+    private static boolean detectXsd11ViaSchemaLocation(final String subjectName, final InputSource peekInstance) {
         final Map<String, String> rootAttrs = peekRootAttributes(peekInstance);
         final String baseUri = peekInstance.getSystemId();
         if (rootAttrs == null || baseUri == null) {
@@ -555,7 +586,7 @@ private static boolean detectXsd11ViaSchemaLocation(final InputSource peekInstan
         }
 
         for (final String location : candidateLocations) {
-            if (isXsd11Schema(baseUri, location)) {
+            if (isXsd11Schema(subjectName, baseUri, location)) {
                 return true;
             }
         }
@@ -568,6 +599,8 @@ private static boolean detectXsd11ViaSchemaLocation(final InputSource peekInstan
      * and checks whether its root element declares {@code vc:minVersion} containing "1.1".
      * Returns {@code false} for any resolution/read failure -- this is a best-effort peek, not
      * a substitute for the real catalog-aware resolution the actual validation pass performs.
+     * Package-private (not {@code private}) so {@code JaxpSchemaLocationSecurityTest}/
+     * {@code JaxpXsd11DetectionCacheTest}, both in this package, can call it directly.
      *
      * <p>{@code location} is the literal, attacker/document-author-controlled value of the
      * instance's own {@code xsi:schemaLocation}/{@code noNamespaceSchemaLocation} hint -- if it's
@@ -593,8 +626,13 @@ private static boolean detectXsd11ViaSchemaLocation(final InputSource peekInstan
      * way to get a {@code file:} base URI here), which already requires the caller to have used
      * {@code util:} Java-interop functions to construct that object in the first place -- a
      * separate, pre-existing privilege boundary this peek doesn't change either way.</p>
+     *
+     * <p>{@code subjectName} scopes {@link #XSD11_DETECTION_CACHE}: a cache hit skips this
+     * method's permission-checked {@code openStream()} entirely, so without scoping by Subject,
+     * a Subject without read permission on the schema resource could observe a boolean populated
+     * by a different (permitted) Subject's earlier fetch -- a cross-Subject information leak.</p>
      */
-    private static boolean isXsd11Schema(final String baseUri, final String location) {
+    static boolean isXsd11Schema(final String subjectName, final String baseUri, final String location) {
         try {
             final URI baseUriNormalized = new URI(ResolverFactory.fixupExistCatalogUri(baseUri));
             final URI resolvedUri = baseUriNormalized.resolve(location);
@@ -606,7 +644,7 @@ private static boolean isXsd11Schema(final String baseUri, final String location
                 return false;
             }
 
-            final String cacheKey = resolvedUri.toString();
+            final Xsd11DetectionCacheKey cacheKey = new Xsd11DetectionCacheKey(subjectName, resolvedUri.toString());
             final Boolean cached = getCachedXsd11Detection(cacheKey);
             if (cached != null) {
                 return cached;
@@ -636,44 +674,22 @@ private static boolean isXsd11Schema(final String baseUri, final String location
         }
     }
 
-    /**
-     * Bounded (see {@link #XSD11_DETECTION_CACHE_MAX_ENTRIES}), LRU-evicted cache of
-     * "does the schema at this resolved URI declare vc:minVersion 1.1?", so that validating many
-     * documents against the same schema doesn't re-fetch and re-peek it every time. Cleared by
-     * {@code validation:clear-grammar-cache()} (see {@link GrammarTooling}) alongside the Xerces
-     * grammar pool, so operators have one function to clear every validation-related cache.
-     */
-    private static final int XSD11_DETECTION_CACHE_MAX_ENTRIES = 256;
-
-    private static final Map<String, Boolean> XSD11_DETECTION_CACHE = new LinkedHashMap<>(16, 0.75f, true) {
-        @Override
-        protected boolean removeEldestEntry(final Map.Entry<String, Boolean> eldest) {
-            return size() > XSD11_DETECTION_CACHE_MAX_ENTRIES;
-        }
-    };
-
     @Nullable
-    private static Boolean getCachedXsd11Detection(final String resolvedUri) {
-        synchronized (XSD11_DETECTION_CACHE) {
-            return XSD11_DETECTION_CACHE.get(resolvedUri);
-        }
+    private static Boolean getCachedXsd11Detection(final Xsd11DetectionCacheKey key) {
+        return XSD11_DETECTION_CACHE.getIfPresent(key);
     }
 
-    private static void cacheXsd11Detection(final String resolvedUri, final boolean isXsd11) {
-        synchronized (XSD11_DETECTION_CACHE) {
-            XSD11_DETECTION_CACHE.put(resolvedUri, isXsd11);
-        }
+    private static void cacheXsd11Detection(final Xsd11DetectionCacheKey key, final boolean isXsd11) {
+        XSD11_DETECTION_CACHE.put(key, isXsd11);
     }
 
     /**
-     * Discards all cached {@link #isXsd11Schema(String, String)} results. Package-private so
-     * {@link GrammarTooling}'s {@code clear-grammar-cache()} can clear this alongside the Xerces
-     * grammar pool.
+     * Discards all cached {@link #isXsd11Schema(String, String, String)} results. Package-private
+     * so {@link GrammarTooling}'s {@code clear-grammar-cache()} can clear this alongside the
+     * Xerces grammar pool.
      */
     static void clearXsd11DetectionCache() {
-        synchronized (XSD11_DETECTION_CACHE) {
-            XSD11_DETECTION_CACHE.clear();
-        }
+        XSD11_DETECTION_CACHE.invalidateAll();
     }
 
     /**
diff --git a/exist-core/src/test/java/org/exist/xquery/functions/validation/JaxpSchemaLocationSecurityTest.java b/exist-core/src/test/java/org/exist/xquery/functions/validation/JaxpSchemaLocationSecurityTest.java
@@ -25,7 +25,6 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 
-import java.lang.reflect.Method;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -34,14 +33,14 @@
 import static org.junit.Assert.assertTrue;
 
 /**
- * Targeted white-box test for {@code Jaxp.isXsd11Schema(String, String)}'s same-origin
+ * Targeted white-box test for {@code Jaxp.isXsd11Schema(String, String, String)}'s same-origin
  * (scheme + authority) restriction -- the up-front XSD 1.1 detection peek must only ever follow
  * a {@code schemaLocation} hint that resolves to the exact same origin as the instance document's
  * own base URI, since the hint is document-author-controlled and the peek runs unconditionally,
  * before any catalog/permission check would otherwise govern the fetch.
  *
- * <p>Tests the method directly (via reflection, since it's {@code private static}) rather than
- * through the full {@code validation:jaxp()} pipeline: going through the pipeline would also
+ * <p>Tests {@code Jaxp.isXsd11Schema} directly rather than through the full {@code
+ * validation:jaxp()} pipeline: going through the pipeline would also
  * exercise the (separate, pre-existing, unrelated) default Xerces resolution that runs when the
  * peek declines and falls through -- which would independently attempt the same kind of fetch,
  * confounding any attempt to observe whether the peek itself made a network call.</p>
@@ -77,43 +76,37 @@ public static void teardown() throws Exception {
     }
 
     @Test
-    public void sameOriginRelativeXsd11SchemaIsDetected() throws Exception {
-        assertTrue(invokeIsXsd11Schema(fileBaseUri, "schema11.xsd"));
+    public void sameOriginRelativeXsd11SchemaIsDetected() {
+        assertTrue(Jaxp.isXsd11Schema("test-subject", fileBaseUri, "schema11.xsd"));
     }
 
     @Test
-    public void sameOriginRelativeXsd10SchemaIsNotDetectedAsXsd11() throws Exception {
-        assertFalse(invokeIsXsd11Schema(fileBaseUri, "schema10.xsd"));
+    public void sameOriginRelativeXsd10SchemaIsNotDetectedAsXsd11() {
+        assertFalse(Jaxp.isXsd11Schema("test-subject", fileBaseUri, "schema10.xsd"));
     }
 
     @Test(timeout = 5000)
-    public void crossOriginHttpLocationIsRefused() throws Exception {
+    public void crossOriginHttpLocationIsRefused() {
         // A real instance would never have a `file://` base URI reachable from an unprivileged
         // caller (only Java-object-backed items do, which already requires elevated capability) --
         // this is just the most convenient same-scheme baseline to contrast against. The case that
         // matters is: whatever the base URI's origin, an absolute, different-origin location must
         // never be followed.
-        assertFalse(invokeIsXsd11Schema(fileBaseUri, "http://203.0.113.1:1/evil.xsd"));
+        assertFalse(Jaxp.isXsd11Schema("test-subject", fileBaseUri, "http://203.0.113.1:1/evil.xsd"));
     }
 
     @Test(timeout = 5000)
-    public void crossOriginHttpLocationIsRefusedForDatabaseBaseUri() throws Exception {
+    public void crossOriginHttpLocationIsRefusedForDatabaseBaseUri() {
         // The realistic, unprivileged case: a document stored in the database (xmldb:// base URI)
         // with an absolute http:// schemaLocation hint pointing out to an attacker-controlled host.
-        assertFalse(invokeIsXsd11Schema("xmldb://db/test/instance.xml", "http://203.0.113.1:1/evil.xsd"));
+        assertFalse(Jaxp.isXsd11Schema("test-subject", "xmldb://db/test/instance.xml", "http://203.0.113.1:1/evil.xsd"));
     }
 
     @Test(timeout = 5000)
-    public void crossHostXmldbLocationIsRefused() throws Exception {
+    public void crossHostXmldbLocationIsRefused() {
         // An absolute xmldb:// location naming a different host is not "same scheme" enough --
         // XmldbURL.isEmbedded() treats a non-empty host as a remote XML-RPC target
         // (EmbeddedURLConnection -> XmlrpcInputStream), so this must be refused too.
-        assertFalse(invokeIsXsd11Schema("xmldb://db/test/instance.xml", "xmldb://203.0.113.1:1/db/evil.xsd"));
-    }
-
-    private static boolean invokeIsXsd11Schema(final String baseUri, final String location) throws Exception {
-        final Method method = Jaxp.class.getDeclaredMethod("isXsd11Schema", String.class, String.class);
-        method.setAccessible(true);
-        return (boolean) method.invoke(null, baseUri, location);
+        assertFalse(Jaxp.isXsd11Schema("test-subject", "xmldb://db/test/instance.xml", "xmldb://203.0.113.1:1/db/evil.xsd"));
     }
 }
diff --git a/exist-core/src/test/java/org/exist/xquery/functions/validation/JaxpXsd11DetectionCacheTest.java b/exist-core/src/test/java/org/exist/xquery/functions/validation/JaxpXsd11DetectionCacheTest.java
@@ -24,7 +24,6 @@
 import org.junit.After;
 import org.junit.Test;
 
-import java.lang.reflect.Method;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -34,9 +33,11 @@
 
 /**
  * Tests that {@code Jaxp.isXsd11Schema}'s result cache (a) actually caches -- a second call for
- * the same resolved schema URI doesn't re-read the (changed) underlying content -- and (b) is
- * fully cleared by {@code Jaxp.clearXsd11DetectionCache()}, the hook {@code
- * validation:clear-grammar-cache()} calls (see {@link GrammarTooling}).
+ * the same Subject and resolved schema URI doesn't re-read the (changed) underlying content --
+ * (b) is fully cleared by {@code Jaxp.clearXsd11DetectionCache()}, the hook {@code
+ * validation:clear-grammar-cache()} calls (see {@link GrammarTooling}), and (c) is scoped per
+ * Subject, so a different Subject querying the same resolved URI doesn't observe a cached answer
+ * populated by someone else's fetch.
  */
 public class JaxpXsd11DetectionCacheTest {
 
@@ -64,30 +65,51 @@ public void cachesResultAcrossCallsAndClearCacheInvalidatesIt() throws Exception
             final String baseUri = instance.toUri().toString();
 
             // First call: reads the real (XSD 1.1) content from disk.
-            assertTrue(invokeIsXsd11Schema(baseUri, "schema.xsd"));
+            assertTrue(Jaxp.isXsd11Schema("subject-a", baseUri, "schema.xsd"));
 
             // Flip the on-disk content to XSD 1.0 without going through the cache -- if the second
             // call is actually served from cache, it must still report the stale (cached) "true",
             // not re-read this new content.
             Files.writeString(schema, XSD_1_0_SCHEMA, StandardCharsets.UTF_8);
             assertTrue("second call should be served from cache, not re-read the changed file",
-                    invokeIsXsd11Schema(baseUri, "schema.xsd"));
+                    Jaxp.isXsd11Schema("subject-a", baseUri, "schema.xsd"));
 
             // Clearing the cache (what validation:clear-grammar-cache() does) must make the next
             // call re-read the file and observe the now-current (XSD 1.0) content.
             Jaxp.clearXsd11DetectionCache();
             assertFalse("after clearing the cache, the now-current XSD 1.0 content must be observed",
-                    invokeIsXsd11Schema(baseUri, "schema.xsd"));
+                    Jaxp.isXsd11Schema("subject-a", baseUri, "schema.xsd"));
         } finally {
             Files.deleteIfExists(tempDir.resolve("instance.xml"));
             Files.deleteIfExists(tempDir.resolve("schema.xsd"));
             Files.deleteIfExists(tempDir);
         }
     }
 
-    private static boolean invokeIsXsd11Schema(final String baseUri, final String location) throws Exception {
-        final Method method = Jaxp.class.getDeclaredMethod("isXsd11Schema", String.class, String.class);
-        method.setAccessible(true);
-        return (boolean) method.invoke(null, baseUri, location);
+    @Test
+    public void cacheIsScopedPerSubject() throws Exception {
+        final Path tempDir = Files.createTempDirectory("jaxp-xsd11-cache-subject-test");
+        try {
+            final Path instance = tempDir.resolve("instance.xml");
+            Files.writeString(instance, "<root/>", StandardCharsets.UTF_8);
+            final Path schema = tempDir.resolve("schema.xsd");
+            Files.writeString(schema, XSD_1_1_SCHEMA, StandardCharsets.UTF_8);
+
+            final String baseUri = instance.toUri().toString();
+
+            // subject-a's call populates the cache for this resolved URI.
+            assertTrue(Jaxp.isXsd11Schema("subject-a", baseUri, "schema.xsd"));
+
+            // Delete the underlying file so any call that actually has to read it (i.e. a cache
+            // miss) fails/returns false -- if subject-b's call were wrongly served from
+            // subject-a's cache entry, it would still report "true" despite never reading anything.
+            Files.delete(schema);
+            assertFalse("a different Subject must not observe a cache entry populated by another Subject's fetch",
+                    Jaxp.isXsd11Schema("subject-b", baseUri, "schema.xsd"));
+        } finally {
+            Files.deleteIfExists(tempDir.resolve("instance.xml"));
+            Files.deleteIfExists(tempDir.resolve("schema.xsd"));
+            Files.deleteIfExists(tempDir);
+        }
     }
 }
