yuqi1129 commented on code in PR #12922:
URL: https://github.com/apache/gravitino/pull/12922#discussion_r3933242409
##########
server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java:
##########
@@ -174,6 +184,62 @@ public void
testSecretProvidersDiscoveryWithMemoryProvider() throws Exception {
assertFalse(providers.get(0).containsKey("className"));
}
+ // Paths that legitimately have no HttpAuditFilter binding of their own,
each with the reason
+ // it's still safe. GH-12760: extending this set is a deliberate, reviewed
decision, not a
+ // default — everything else registered on the servlet context must be
covered.
+ private static final Set<String> PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE =
+ ImmutableSet.of(
+ "/", // DefaultServlet / WebUIFilter: serves static UI assets, no
server-side logic.
+ "/ui/*", // WebUIFilter: serves static UI assets, no server-side
logic.
+ "/health/*", // HealthAliasServlet forwards into /api/health*,
already covered via the
+ "/health.html" // FORWARD dispatcher type; binding again here would
double-log probes.
+ );
+
+ @Test
+ public void testEveryServletPathIsCoveredByAuditFilter() throws Exception {
+ gravitinoServer.initialize();
+
+ ServletHandler servletHandler =
getServletContextHandler(gravitinoServer).getServletHandler();
+
+ Set<String> auditedPathSpecs =
+ Arrays.stream(servletHandler.getFilterMappings())
+ .filter(
+ filterMapping ->
+ HttpAuditFilter.class
Review Comment:
Could we also enforce the authentication/public-path invariant here? This
test only collects `HttpAuditFilter` mappings, while `addSystemFilters` remains
bound solely to `/api/*`. A future root-mounted servlet can be added to
`ROOT_MOUNTED_NON_API_PATHS` and make this test pass while remaining
anonymously accessible—the exact fail-open case #12760 asks us to prevent.
Please introduce an explicit public-path set and assert that every servlet
mapping is either covered by `AuthenticationFilter` or deliberately listed as
public.
##########
server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java:
##########
@@ -174,6 +184,62 @@ public void
testSecretProvidersDiscoveryWithMemoryProvider() throws Exception {
assertFalse(providers.get(0).containsKey("className"));
}
+ // Paths that legitimately have no HttpAuditFilter binding of their own,
each with the reason
+ // it's still safe. GH-12760: extending this set is a deliberate, reviewed
decision, not a
+ // default — everything else registered on the servlet context must be
covered.
+ private static final Set<String> PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE =
+ ImmutableSet.of(
+ "/", // DefaultServlet / WebUIFilter: serves static UI assets, no
server-side logic.
+ "/ui/*", // WebUIFilter: serves static UI assets, no server-side
logic.
+ "/health/*", // HealthAliasServlet forwards into /api/health*,
already covered via the
+ "/health.html" // FORWARD dispatcher type; binding again here would
double-log probes.
+ );
+
+ @Test
+ public void testEveryServletPathIsCoveredByAuditFilter() throws Exception {
+ gravitinoServer.initialize();
+
+ ServletHandler servletHandler =
getServletContextHandler(gravitinoServer).getServletHandler();
+
+ Set<String> auditedPathSpecs =
+ Arrays.stream(servletHandler.getFilterMappings())
+ .filter(
+ filterMapping ->
+ HttpAuditFilter.class
+ .getName()
+ .equals(
+
servletHandler.getFilter(filterMapping.getFilterName()).getClassName()))
+ .flatMap(filterMapping ->
Arrays.stream(filterMapping.getPathSpecs()))
+ .collect(Collectors.toSet());
+
+ for (ServletMapping servletMapping : servletHandler.getServletMappings()) {
+ for (String pathSpec : servletMapping.getPathSpecs()) {
+ if (PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE.contains(pathSpec)) {
+ continue;
+ }
+ assertTrue(
+ auditedPathSpecs.contains(pathSpec),
Review Comment:
Could we check path-spec coverage rather than exact string equality? For
example, a servlet mapped to `/api/internal/*` is already covered by the
`/api/*` audit mapping, but `contains(pathSpec)` returns false and would
encourage adding a redundant filter mapping. Please use Jetty/Servlet path-spec
matching semantics for this assertion.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.gravitino.iceberg;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+
+public class TestRESTService {
+
+ /**
+ * RESTService.initServer() previously registered /metrics and
/prometheus/metrics (added by
+ * JettyServer#initialize() itself, outside ICEBERG_SPEC) with no audit
coverage at all, and
+ * nothing in the build caught it. This test pins the fix by inspecting the
source directly, since
+ * RESTService's initServer() has too many external dependencies (catalog
backends, config
+ * providers) to boot in a plain unit test. See GH-12760.
+ */
+ @Test
+ public void testMetricsPathsHaveAuditFilterCoverage() throws IOException {
+ Path sourceFile =
Path.of("src/main/java/org/apache/gravitino/iceberg/RESTService.java");
+ String source = Files.readString(sourceFile);
+
+ int loopStart = source.indexOf("for (String pathSpec : METRICS_PATHS)");
+ assertTrue(
+ loopStart >= 0,
+ "RESTService must wire filters onto every path in METRICS_PATHS, see
GH-12760");
+
+ int loopEnd = source.indexOf("}", loopStart);
+ assertTrue(loopEnd > loopStart, "Malformed METRICS_PATHS filter loop");
+
+ String loopBody = source.substring(loopStart, loopEnd);
Review Comment:
This source-text check can pass even when endpoint coverage is broken—for
example, if `METRICS_PATHS` is changed to contain only `/metrics`, or if
`HttpAuditFilter` merely appears in a comment. Since this is a
security/compliance regression test, please inspect the actual `ServletHandler`
mappings, or extract filter registration into a method that can be tested
directly. The same concern applies to `TestLanceRESTService`.
--
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]
