jerryshao commented on code in PR #12922:
URL: https://github.com/apache/gravitino/pull/12922#discussion_r3933726727
##########
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:
Fixed — added testEveryServletPathIsEitherAuthenticatedOrDeliberatelyPublic,
which mirrors the audit test but checks AuthenticationFilter coverage against a
new KNOWN_PUBLIC_PATHS exemption set (distinct from the audit exemption set,
since e.g. /configs is audited but intentionally unauthenticated).
##########
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:
Fixed — both coverage checks now go through a shared isPathSpecCovered
helper that uses org.eclipse.jetty.http.pathmap.ServletPathSpec#matches against
a representative concrete path, instead of exact string equality. A servlet at
/api/internal/* is now correctly recognized as already covered by a filter
bound to /api/*.
--
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]
