Copilot commented on code in PR #4471:
URL: https://github.com/apache/solr/pull/4471#discussion_r3317030112


##########
solr/agent-sm/src/java/org/apache/solr/security/agent/AgentPolicy.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.security.agent;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Immutable singleton that holds the active security policy for the Solr JVM.
+ *
+ * <p>The policy is loaded once at JVM startup by {@link PolicyLoader} and 
must not be modified
+ * afterwards. Any attempt to replace the singleton after it has been set 
throws a {@link
+ * SecurityException}.
+ *
+ * <p>The singleton is stored as a plain {@code static volatile} field so that 
it is visible to all
+ * classloaders, including bootstrap-injected agent classes. The enforcement 
mode is read directly
+ * from the system property {@code solr.security.agent.mode} (set by the 
startup script from the
+ * environment variable {@code SOLR_SECURITY_AGENT_MODE}). {@code EnvUtils} 
from {@code solr:core}
+ * is intentionally not used here because the agent JAR has no compile-time 
dependency on Solr
+ * application code.
+ */
+public final class AgentPolicy {
+
+  /** Whether violations block the operation or are merely logged. */
+  public enum EnforcementMode {
+    /** Violations are logged at WARN level; the operation is allowed to 
proceed. */
+    WARN,
+    /** Violations are logged at ERROR level and blocked with a {@link 
SecurityException}. */
+    ENFORCE
+  }
+
+  // Singleton holder — set once at premain; never null after initialization.
+  private static volatile AgentPolicy instance;
+
+  private final List<PermittedPath> permittedPaths;
+  private final List<PermittedEndpoint> permittedEndpoints;
+  private final List<ApprovedCallSite> approvedExitCallers;
+  private final List<ApprovedCallSite> approvedExecCallers;
+  private final EnforcementMode enforcementMode;
+  private final Set<String> trustedFileSystems;
+  private final Set<String> trustedHosts;
+
+  /** Constructs the policy. Called exclusively by {@link 
PolicyLoader#load(java.nio.file.Path)}. */
+  AgentPolicy(
+      List<PermittedPath> permittedPaths,
+      List<PermittedEndpoint> permittedEndpoints,
+      List<ApprovedCallSite> approvedExitCallers,
+      List<ApprovedCallSite> approvedExecCallers,
+      EnforcementMode enforcementMode) {
+    this(
+        permittedPaths,
+        permittedEndpoints,
+        approvedExitCallers,
+        approvedExecCallers,
+        enforcementMode,
+        Set.of(),
+        Set.of());
+  }
+
+  /**
+   * Constructs the policy with explicit trusted filesystem schemes and 
trusted hosts.
+   *
+   * @param trustedFileSystems filesystem URI schemes exempt from path checks 
(e.g. {@code "jrt"},
+   *     {@code "memory"} used in tests)
+   * @param trustedHosts host strings exempt from network checks (e.g. 
loopback addresses)
+   */
+  AgentPolicy(
+      List<PermittedPath> permittedPaths,
+      List<PermittedEndpoint> permittedEndpoints,
+      List<ApprovedCallSite> approvedExitCallers,
+      List<ApprovedCallSite> approvedExecCallers,
+      EnforcementMode enforcementMode,
+      Set<String> trustedFileSystems,
+      Set<String> trustedHosts) {
+    this.permittedPaths = Collections.unmodifiableList(permittedPaths);
+    this.permittedEndpoints = Collections.unmodifiableList(permittedEndpoints);
+    this.approvedExitCallers = 
Collections.unmodifiableList(approvedExitCallers);
+    this.approvedExecCallers = 
Collections.unmodifiableList(approvedExecCallers);
+    this.enforcementMode = enforcementMode;
+    this.trustedFileSystems = Collections.unmodifiableSet(trustedFileSystems);
+    this.trustedHosts = Collections.unmodifiableSet(trustedHosts);
+  }
+
+  // 
---------------------------------------------------------------------------
+  // Singleton management
+  // 
---------------------------------------------------------------------------
+
+  /**
+   * Sets the global singleton policy. May only be called once; subsequent 
calls throw {@link
+   * SecurityException}.
+   */
+  public static void initialize(AgentPolicy policy) {
+    synchronized (AgentPolicy.class) {
+      if (instance != null) {
+        throw new SecurityException(
+            "AgentPolicy has already been initialized and cannot be replaced. "
+                + "This is a programming error; only 
SolrAgentEntryPoint.premain() should call initialize().");
+      }
+      instance = policy;
+    }
+  }
+
+  /**
+   * Returns the active global policy.
+   *
+   * @throws IllegalStateException if the policy has not yet been initialized
+   */
+  public static AgentPolicy getInstance() {
+    AgentPolicy p = instance;
+    if (p == null) {
+      throw new IllegalStateException(
+          "AgentPolicy has not been initialized. "
+              + "Ensure the Solr security agent JAR is on the -javaagent: 
command-line.");
+    }
+    return p;
+  }
+
+  /**
+   * Returns {@code true} if the singleton has already been initialized. Used 
by the agent entry
+   * point to detect double-loading.
+   */
+  public static boolean isInitialized() {
+    return instance != null;
+  }
+
+  /**
+   * Resets the singleton to {@code null} so that tests can re-initialize it 
between test methods.
+   *
+   * <p>This method is package-private and intended exclusively for unit tests 
in the {@code
+   * org.apache.solr.security.agent} package. Production code must never call 
this method.
+   *
+   * <p>The write is not synchronized: {@code instance} is {@code volatile}, 
so the assignment is
+   * immediately visible to all threads. Unlike {@link #initialize}, there is 
no invariant to
+   * protect here — tests call this only from a single thread during teardown.
+   */
+  static void resetForTesting() {
+    instance = null;
+  }
+
+  // 
---------------------------------------------------------------------------
+  // Policy accessors
+  // 
---------------------------------------------------------------------------
+
+  /** Permitted file-system paths derived from both the default policy and 
operator extensions. */
+  public List<PermittedPath> permittedPaths() {
+    return permittedPaths;
+  }
+
+  /** Permitted outbound network endpoints. */
+  public List<PermittedEndpoint> permittedEndpoints() {
+    return permittedEndpoints;
+  }
+
+  /** Classes approved to call {@code System.exit()} or {@code 
Runtime.halt()}. */
+  public List<ApprovedCallSite> approvedExitCallers() {
+    return approvedExitCallers;
+  }
+
+  /**
+   * Classes approved to spawn child processes via {@code ProcessBuilder} or 
{@code Runtime.exec()}.
+   */
+  public List<ApprovedCallSite> approvedExecCallers() {
+    return approvedExecCallers;
+  }
+
+  /**
+   * Current enforcement mode. {@link EnforcementMode#WARN} allows violations; 
{@link
+   * EnforcementMode#ENFORCE} blocks them.
+   */
+  public EnforcementMode enforcementMode() {
+    return enforcementMode;
+  }
+
+  /**
+   * Filesystem scheme names that are exempt from path-based checks (e.g. 
in-memory filesystems used
+   * in tests).
+   */
+  public Set<String> trustedFileSystems() {
+    return trustedFileSystems;
+  }
+
+  /**
+   * Host strings exempt from outbound network checks (e.g. {@code 
"localhost"}, {@code
+   * "127.0.0.1"}). Populated by {@link SolrAgentEntryPoint} at startup.
+   */
+  public Set<String> trustedHosts() {
+    return trustedHosts;
+  }
+
+  // 
---------------------------------------------------------------------------
+  // Policy checks (convenience helpers called by interceptors)
+  // 
---------------------------------------------------------------------------
+
+  /**
+   * Returns {@code true} if at least one {@link PermittedPath} in this policy 
permits the given
+   * action on the given resolved (real) path.
+   */
+  public boolean isPathPermitted(String resolvedPath, String action) {
+    for (PermittedPath p : permittedPaths) {
+      if (p.permits(resolvedPath, action)) return true;
+    }
+    return false;
+  }
+
+  /**
+   * Returns {@code true} if at least one {@link ApprovedCallSite} with {@link
+   * ApprovedCallSite.Operation#EXIT} matches the given class name.
+   */
+  public boolean isExitApproved(String className) {
+    for (ApprovedCallSite cs : approvedExitCallers) {
+      if (cs.operation() == ApprovedCallSite.Operation.EXIT && 
cs.matches(className)) return true;
+    }
+    return false;
+  }
+
+  /**
+   * Returns {@code true} if at least one {@link ApprovedCallSite} with {@link
+   * ApprovedCallSite.Operation#EXEC} matches the given class name.
+   */
+  public boolean isExecApproved(String className) {
+    for (ApprovedCallSite cs : approvedExecCallers) {
+      if (cs.operation() == ApprovedCallSite.Operation.EXEC && 
cs.matches(className)) return true;
+    }
+    return false;
+  }
+
+  /**
+   * Returns {@code true} if any class in the call chain is approved to call 
{@code System.exit()}
+   * or {@code Runtime.halt()}. Any approved class anywhere in the chain 
grants permission.
+   *
+   * <p>Class names are matched using {@link String#matches} (full regex), so 
the approved-caller
+   * list supports wildcard patterns such as {@code 
"org\\.apache\\.solr\\..*"}.

Review Comment:
   The Javadoc states "Class names are matched using `String#matches` (full 
regex), so the approved-caller list supports wildcard patterns such as 
`org\\.apache\\.solr\\..*`." This does not match the actual implementation: 
`ApprovedCallSite.matches` (PermittedCallSite line 73–80) uses 
`equals`/`startsWith` semantics with a `.*` suffix convention — not regex. A 
pattern like `org\\.apache\\.solr\\..*` would not match anything under the 
current implementation.
   
   Please update this Javadoc to describe the actual matching rules (exact 
match, `*` wildcard, or trailing `.*` package-prefix match).
   



##########
solr/agent-sm/src/java/org/apache/solr/security/agent/FileInterceptor.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.security.agent;
+
+import java.lang.reflect.Method;
+import java.nio.file.LinkOption;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.nio.file.spi.FileSystemProvider;
+import java.util.Collection;
+import java.util.Locale;
+import java.util.Set;
+import net.bytebuddy.asm.Advice;
+
+/**
+ * ByteBuddy {@link Advice} interceptor for file-system operations.
+ *
+ * <p>This file was derived from the OpenSearch project and modified. See 
{@code NOTICE.txt} for
+ * attribution.
+ */
+public class FileInterceptor {
+
+  /** FileInterceptor */
+  public FileInterceptor() {}
+
+  /**
+   * Intercepts file operations.
+   *
+   * @param args arguments
+   * @param method method
+   * @throws Exception exceptions
+   */
+  @Advice.OnMethodEnter
+  public static void intercept(@Advice.AllArguments Object[] args, 
@Advice.Origin Method method)
+      throws Exception {
+    if (!AgentPolicy.isInitialized()) return;
+    final AgentPolicy policy = AgentPolicy.getInstance();
+
+    FileSystemProvider provider = null;
+    String filePath = null;
+    if (args.length > 0 && args[0] instanceof String pathStr) {
+      filePath = Path.of(pathStr).toAbsolutePath().normalize().toString();
+    } else if (args.length > 0 && args[0] instanceof Path path) {
+      filePath = path.toAbsolutePath().normalize().toString();
+      provider = path.getFileSystem().provider();
+    }
+
+    if (filePath == null) {
+      return; // No valid file path found
+    }
+
+    if (provider != null && 
policy.trustedFileSystems().contains(provider.getScheme())) {
+      return;
+    }
+
+    final StackWalker walker = 
StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
+    final String caller = walker.getCallerClass().getName();
+
+    final String name = method.getName();
+    boolean isMutating = name.equals("move") || name.equals("write") || 
name.startsWith("create");
+    final boolean isDelete = isMutating == false ? name.startsWith("delete") : 
false;
+
+    // This is Windows implementation of UNIX Domain Sockets (close)
+    boolean isUnixSocketCaller = false;
+    if (isDelete == true) {
+      final Collection<Class<?>> chain = 
walker.walk(StackCallerClassChainExtractor.INSTANCE);
+      for (final Class<?> cls : chain) {
+        if 
(cls.getName().equalsIgnoreCase("sun.nio.ch.PipeImpl$Initializer$LoopbackConnector"))
 {
+          isUnixSocketCaller = true;
+          break;
+        }
+      }
+    }
+
+    if (isDelete == true && isUnixSocketCaller == true) {
+      // Unix domain socket cleanup — local IPC, always allow
+      return;
+    } else {
+      String targetFilePath = null;
+      if (isMutating == false && isDelete == false) {
+        if (name.equals("newByteChannel") == true || name.equals("open") == 
true) {
+          if (args.length > 1) {
+            if (args[1] instanceof OpenOption[] opts) {
+              for (final OpenOption opt : opts) {
+                if (opt != StandardOpenOption.READ) {
+                  isMutating = true;
+                  break;
+                }
+              }
+            } else if (args[1] instanceof Set<?> opts) {
+              @SuppressWarnings("unchecked")
+              final Set<OpenOption> options = (Set<OpenOption>) args[1];
+              for (final OpenOption opt : options) {
+                if (opt != StandardOpenOption.READ) {
+                  isMutating = true;
+                  break;
+                }
+              }
+            } else if (args[1] instanceof Object[] opts) {
+              for (final Object opt : opts) {
+                if (opt != StandardOpenOption.READ) {
+                  isMutating = true;
+                  break;
+                }
+              }
+            } else {
+              throw new SecurityException(
+                  "Unsupported argument type: " + 
args[1].getClass().getName());

Review Comment:
   The `else` branch at line 121 throws `SecurityException("Unsupported 
argument type: ...")` whenever `args[1]` is non-null but not one of the three 
handled types (`OpenOption[]`, `Set`, `Object[]`). Because this advice is 
inlined into all `FileSystemProvider` / `Files` / `FileChannel` 
`newByteChannel`/`open` call sites in the JDK, any present or future overload 
that passes a different second-argument type will cause legitimate file 
operations to fail with a non-policy-related `SecurityException` even when the 
agent is in `warn` mode (the throw is unconditional, not gated by enforcement 
mode).
   
   Consider treating an unrecognized argument type as `isMutating = true` 
(conservative — falls through to the write check) or silently skipping the 
option-decoding logic, rather than throwing. Throwing here also bypasses the 
violation counter and the structured log, so operators will see a raw stack 
trace with no policy context.
   



##########
solr/agent-sm/src/java/org/apache/solr/security/agent/SocketChannelInterceptor.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.security.agent;
+
+import java.lang.reflect.Method;
+import java.net.InetSocketAddress;
+import java.net.URL;
+import java.net.UnixDomainSocketAddress;
+import java.security.CodeSource;
+import java.util.Collection;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.asm.Advice.Origin;
+
+/**
+ * ByteBuddy {@link Advice} interceptor for outbound socket connections.
+ *
+ * <p>This file was derived from the OpenSearch project and modified. See 
{@code NOTICE.txt} for
+ * attribution.
+ */
+public class SocketChannelInterceptor {
+
+  /** SocketChannelInterceptor */
+  public SocketChannelInterceptor() {}
+
+  /**
+   * Interceptors
+   *
+   * @param args arguments
+   * @param method method
+   * @throws Exception exceptions
+   */
+  @Advice.OnMethodEnter
+  public static void intercept(@Advice.AllArguments Object[] args, @Origin 
Method method)
+      throws Exception {
+    if (!AgentPolicy.isInitialized()) return;
+    final AgentPolicy policy = AgentPolicy.getInstance();
+
+    final StackWalker walker = 
StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
+    final String caller = walker.getCallerClass().getName();
+
+    if (args[0] instanceof InetSocketAddress address) {
+      if (!policy.trustedHosts().contains(address.getHostString())) {
+        final String host = address.getHostString();
+        final int port = address.getPort();
+
+        if (!isEndpointPermitted(policy, host, port)) {
+          final String target = host + ":" + port;
+          ViolationMetricsReporter.incrementNetwork();
+          SecurityViolationLogger.log(
+              SecurityViolationLogger.ViolationType.NETWORK_CONNECT,
+              target,
+              caller,
+              policy.enforcementMode());
+          if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) 
{
+            throw new SecurityException(
+                "Outbound network connection denied by Solr security agent: " 
+ target);
+          }
+        }
+      }
+    } else if (args[0] instanceof UnixDomainSocketAddress) {
+      // Unix domain socket — local IPC, always allow
+      return;
+    }
+  }
+
+  // 
---------------------------------------------------------------------------
+  // Static helpers (used by advice and by tests)
+  // 
---------------------------------------------------------------------------
+
+  /**
+   * Checks whether the given remote address may be connected to under the 
active policy. Increments
+   * the network violation counter and logs on violation; throws {@link 
SecurityException} in
+   * enforce mode.
+   *
+   * <p>Used by tests to exercise the network check without ByteBuddy 
instrumentation.
+   */
+  public static void checkConnect(InetSocketAddress address) {
+    if (!AgentPolicy.isInitialized()) return;
+    if (address.isUnresolved()) return;
+    AgentPolicy policy = AgentPolicy.getInstance();
+    if (policy.trustedHosts().contains(address.getHostString())) return;
+    String caller = topCallerClassName();
+    String host = address.getHostString();
+    int port = address.getPort();
+    if (!isEndpointPermitted(policy, host, port)) {
+      String target = host + ":" + port;
+      ViolationMetricsReporter.incrementNetwork();
+      SecurityViolationLogger.log(
+          SecurityViolationLogger.ViolationType.NETWORK_CONNECT,
+          target,
+          caller,
+          policy.enforcementMode());
+      if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) {
+        throw new SecurityException(
+            "Outbound network connection denied by Solr security agent: " + 
target);
+      }
+    }
+  }
+
+  public static String topCallerClassName() {
+    try {
+      return StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
+          .getCallerClass()
+          .getName();
+    } catch (Exception e) {
+      return "<unknown>";
+    }
+  }
+
+  // 
---------------------------------------------------------------------------
+  // Endpoint matching helpers
+  // 
---------------------------------------------------------------------------
+
+  /**
+   * Returns {@code true} if at least one permitted endpoint entry in the 
policy covers the given
+   * host and port. Matching rules:
+   *
+   * <ul>
+   *   <li>Entry {@code *:port} — matches any host on that exact port
+   *   <li>Entry {@code host:port} — matches exact host and port
+   *   <li>Entry {@code host:low-high} — matches the host with a port in the 
inclusive range
+   *   <li>Entry {@code *} (no colon) — matches everything (broad wildcard)
+   * </ul>
+   *
+   * <p>Entries with a {@code codeBase} restriction are evaluated against the 
current call chain via
+   * {@link StackWalker}: the entry permits the connection only if at least 
one class in the chain
+   * was loaded from a code source under that codeBase path. The stack walk is 
performed lazily —
+   * only when a codeBase-restricted entry whose endpoint pattern matches is 
encountered.
+   */
+  public static boolean isEndpointPermitted(AgentPolicy policy, String host, 
int port) {
+    Collection<Class<?>> chain = null; // lazily populated
+
+    for (PermittedEndpoint entry : policy.permittedEndpoints()) {
+      if (!matchesEndpoint(entry.hostPort(), host, port)) continue;
+
+      if (entry.codeBase() == null) {
+        // Global grant — no code-source restriction
+        return true;
+      }
+
+      // codeBase-scoped grant: check if any class in the call chain was 
loaded from that codeBase
+      if (chain == null) {
+        chain =
+            StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
+                .walk(StackCallerClassChainExtractor.INSTANCE);
+      }
+      if (isCallerFromCodeBase(chain, entry.codeBase())) return true;
+    }
+    return false;
+  }
+
+  /**
+   * Returns {@code true} if any class in {@code chain} has a code source 
whose location is under
+   * the given {@code codeBase} path. Supports JDK policy file {@code 
codeBase} syntax:
+   *
+   * <ul>
+   *   <li>{@code file:/path/to/dir/-} — recursive: matches any JAR or class 
in that directory tree
+   *   <li>{@code file:/path/to/dir/} or {@code file:/path/to/dir} — exact 
directory
+   *   <li>{@code file:/path/to/specific.jar} — exact JAR file
+   * </ul>
+   */
+  static boolean isCallerFromCodeBase(Collection<Class<?>> chain, String 
codeBase) {
+    // Strip "file:" scheme prefix if present
+    String base = codeBase.startsWith("file:") ? codeBase.substring(5) : 
codeBase;
+    boolean recursive = base.endsWith("/-");
+    if (recursive) base = base.substring(0, base.length() - 2);
+    // Normalise: strip trailing "/" so startsWith checks are consistent
+    while (base.endsWith("/") || base.endsWith("\\")) base = base.substring(0, 
base.length() - 1);
+
+    for (Class<?> cls : chain) {
+      try {
+        CodeSource cs = cls.getProtectionDomain().getCodeSource();
+        if (cs == null) continue;
+        URL loc = cs.getLocation();
+        if (loc == null) continue;
+        String locPath = loc.getPath();
+        if (locPath == null) continue;
+        // Normalise: strip trailing separators
+        while (locPath.endsWith("/") || locPath.endsWith("\\"))
+          locPath = locPath.substring(0, locPath.length() - 1);
+
+        if (recursive) {
+          if (locPath.equals(base) || locPath.startsWith(base + "/")) return 
true;
+        } else {
+          if (locPath.equals(base)) return true;
+        }

Review Comment:
   `isCallerFromCodeBase` normalises trailing separators on both sides but only 
checks `startsWith(base + "/")` for recursive matches at line 197. On Windows, 
code-source URL paths may use forward slashes (URL convention) but the `base` 
may contain backslashes, and vice versa. To be robust cross-platform, normalize 
both sides to a single separator before the prefix comparison, or check both 
`base + "/"` and `base + "\"`.



##########
solr/agent-sm/src/java/org/apache/solr/security/agent/FileInterceptor.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.security.agent;
+
+import java.lang.reflect.Method;
+import java.nio.file.LinkOption;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.nio.file.spi.FileSystemProvider;
+import java.util.Collection;
+import java.util.Locale;
+import java.util.Set;
+import net.bytebuddy.asm.Advice;
+
+/**
+ * ByteBuddy {@link Advice} interceptor for file-system operations.
+ *
+ * <p>This file was derived from the OpenSearch project and modified. See 
{@code NOTICE.txt} for
+ * attribution.
+ */
+public class FileInterceptor {
+
+  /** FileInterceptor */
+  public FileInterceptor() {}
+
+  /**
+   * Intercepts file operations.
+   *
+   * @param args arguments
+   * @param method method
+   * @throws Exception exceptions
+   */
+  @Advice.OnMethodEnter
+  public static void intercept(@Advice.AllArguments Object[] args, 
@Advice.Origin Method method)
+      throws Exception {
+    if (!AgentPolicy.isInitialized()) return;
+    final AgentPolicy policy = AgentPolicy.getInstance();
+
+    FileSystemProvider provider = null;
+    String filePath = null;
+    if (args.length > 0 && args[0] instanceof String pathStr) {
+      filePath = Path.of(pathStr).toAbsolutePath().normalize().toString();
+    } else if (args.length > 0 && args[0] instanceof Path path) {
+      filePath = path.toAbsolutePath().normalize().toString();
+      provider = path.getFileSystem().provider();
+    }

Review Comment:
   The production `intercept` advice (line 57–61) only calls 
`toAbsolutePath().normalize()` on the input path; it does **not** resolve 
symlinks. As a result, an attacker who places a symlink inside a permitted 
directory pointing to a file outside it will pass the policy check at runtime — 
even though `SymlinkEscapeTest` claims to verify this is blocked.
   
   The symlink-escape test calls `FileInterceptor.checkPath(...)` (the 
test-only helper at line 249), which uses `resolveRealPath()` (line 234) to 
call `toRealPath()`. The advice method does not. The comment on 
`resolveRealPath` (line 229–232) explicitly warns that `toRealPath()` must NOT 
be called from advice because it triggers re-entrant interception — meaning the 
symlink-escape protection cannot be added there as-is.
   
   This is a meaningful gap: the PR description states "Paths are normalised 
via `Path.normalize()` before the policy check to prevent `..`-traversal 
bypasses," but `SymlinkEscapeTest` tests for symlink escape that production 
code does not actually prevent. Please either (a) implement symlink escape 
prevention in the advice path (e.g. via a non-re-entrant resolution strategy), 
or (b) update the test and documentation to reflect that only path 
normalization — not symlink resolution — is enforced at runtime.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to