This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new c2526d8b9e [Cherry-pick to branch-1.3] [#12760] fix(server): Cover
root-mounted servlets with the request-context, audit, and custom filter chain
(#12922) (#12945)
c2526d8b9e is described below
commit c2526d8b9ebdb4dc38867fccbeb393a9bcca9c2a
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Mon Sep 7 18:01:16 2026 +0800
[Cherry-pick to branch-1.3] [#12760] fix(server): Cover root-mounted
servlets with the request-context, audit, and custom filter chain (#12922)
(#12945)
**Cherry-pick Information:**
- Original commit: b639f5c12f458817949a13ea3af1dfb7a9e9dd0c
- Target branch: `branch-1.3`
- Status: ✅ Conflicts resolved
**Conflict resolution notes:**
`branch-1.3`'s `LanceRESTService` predates several main-only Lance
features (metadata authorization, `LanceServiceIdentityFilter`,
`LanceHealthCheckPathMatcher`, `HealthAliasServlet`, `auxMode`-aware
namespace loading), so the cherry-picked diff's context couldn't match.
Resolved by dropping those unrelated additions from the "theirs" side
while keeping the actual fix: the extracted
`registerMetricsPathFilters()` helper wiring `RequestContextFilter` and
`HttpAuditFilter` onto `JettyServer.METRICS_PATH_SPECS`, and
consolidating custom-filter registration into a single
`addCustomFilters()` call across `METRICS_PATH_SPECS` plus `LANCE_SPEC`.
Similarly, `TestGravitinoServer.java` on main carries secret-provider
discovery tests (`SecretProviderRegistry`, `InMemorySecretsProvider`, an
HTTP-based `fetchSecretProviders` helper) that don't exist on
`branch-1.3`. Dropped that unrelated test content while keeping the two
new GH-12760 regression tests
(`testEveryServletPathIsCoveredByAuditFilter`,
`testEveryServletPathIsEitherAuthenticatedOrDeliberatelyPublic`) and
their helpers.
Also added the `testArtifacts` configuration to
`server-common/build.gradle.kts`, which main already had from an earlier
change never backported to `branch-1.3`; without it, consumers
(`server`, `lance-rest-server`, `iceberg-rest-server`) couldn't resolve
their existing `testImplementation(project(":server-common",
"testArtifacts"))` dependency needed to reach the new
`JettyServerTestUtils` class.
Verified `:server:compileTestJava`,
`:lance:lance-rest-server:compileJava`,
`:lance:lance-rest-server:compileTestJava`, and
`:iceberg:iceberg-rest-server:compileTestJava` all succeed, and
`TestGravitinoServer`, `TestLanceRESTService`, and `TestRESTService` all
pass.
---------
Co-authored-by: Jerry Shao <[email protected]>
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Jerry Shao <[email protected]>
---
iceberg/iceberg-rest-server/build.gradle.kts | 1 +
.../org/apache/gravitino/iceberg/RESTService.java | 37 ++++++-
.../apache/gravitino/iceberg/TestRESTService.java | 77 +++++++++++++
lance/lance-rest-server/build.gradle.kts | 1 +
.../apache/gravitino/lance/LanceRESTService.java | 32 +++++-
.../gravitino/lance/TestLanceRESTService.java | 77 +++++++++++++
server-common/build.gradle.kts | 13 +++
.../apache/gravitino/server/web/JettyServer.java | 29 ++++-
.../gravitino/server/web/JettyServerTestUtils.java | 73 ++++++++++++
server/build.gradle.kts | 1 +
.../apache/gravitino/server/GravitinoServer.java | 49 +++++++-
.../gravitino/server/TestGravitinoServer.java | 123 +++++++++++++++++++++
12 files changed, 504 insertions(+), 9 deletions(-)
diff --git a/iceberg/iceberg-rest-server/build.gradle.kts
b/iceberg/iceberg-rest-server/build.gradle.kts
index 59485de5ca..f69bb0ea66 100644
--- a/iceberg/iceberg-rest-server/build.gradle.kts
+++ b/iceberg/iceberg-rest-server/build.gradle.kts
@@ -84,6 +84,7 @@ dependencies {
testImplementation(project(":bundles:iceberg-azure-bundle"))
testImplementation(project(":core", "testArtifacts"))
testImplementation(project(":integration-test-common", "testArtifacts"))
+ testImplementation(project(":server-common", "testArtifacts"))
testImplementation(project(":server"))
testImplementation("org.scala-lang.modules:scala-collection-compat_$scalaVersion:$scalaCollectionCompatVersion")
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index af732fbea1..89f42ca0c2 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -19,6 +19,7 @@
package org.apache.gravitino.iceberg;
import com.google.common.collect.Lists;
+import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -214,13 +215,45 @@ public class RESTService implements
GravitinoAuxiliaryService {
EventSource.GRAVITINO_ICEBERG_REST_SERVER,
new IcebergHealthCheckPathMatcher()),
ICEBERG_SPEC);
- server.addCustomFilters(ICEBERG_SPEC);
server.addSystemFilters(ICEBERG_SPEC);
// Root-level aliases for health checks to improve compatibility with
various monitoring
- // systems that expect a /health endpoint.
+ // systems that expect a /health endpoint. Not part of
JettyServer.METRICS_PATH_SPECS below:
+ // HealthAliasServlet forwards every request into /iceberg/health*, which
ICEBERG_SPEC already
+ // covers via the servlet container's FORWARD dispatcher type, so binding
the filter again
+ // here would double-log every probe.
server.addServlet(new HealthAliasServlet("/iceberg"), "/health/*");
server.addServlet(new HealthAliasServlet("/iceberg"), "/health.html");
+
+ registerMetricsPathFilters(server, eventBus);
+
+ // Custom filters are registered once, across every filtered path in a
single call, so a
+ // filter whose init() isn't safe to run more than once per JVM only runs
it once rather than
+ // once per pathSpec.
+ List<String> customFilterPaths = new
ArrayList<>(JettyServer.METRICS_PATH_SPECS);
+ customFilterPaths.add(ICEBERG_SPEC);
+ server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+ }
+
+ /**
+ * Registers request-context tracking and audit-on-failure coverage on {@link
+ * JettyServer#METRICS_PATH_SPECS}. {@code /metrics} and {@code
/prometheus/metrics} used to
+ * receive no such coverage at all, with nothing in the build catching it;
{@code
+ * RequestContextFilter} is included too so query-parameter capture applies
uniformly, matching
+ * {@link #ICEBERG_SPEC}. Package-private and static so a unit test can
exercise it directly
+ * against a plain {@link JettyServer}, without booting the rest of {@link
#initServer}. See
+ * GH-12760.
+ *
+ * @param server the Jetty server whose {@link
JettyServer#METRICS_PATH_SPECS} need filter
+ * coverage
+ * @param eventBus the event bus audit events are dispatched through
+ */
+ static void registerMetricsPathFilters(JettyServer server, EventBus
eventBus) {
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ server.addFilter(new RequestContextFilter(eventBus), pathSpec);
+ server.addFilter(
+ new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_ICEBERG_REST_SERVER), pathSpec);
+ }
}
@Override
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
new file mode 100644
index 0000000000..36b2b5e6bd
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
@@ -0,0 +1,77 @@
+/*
+ * 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.util.Collections;
+import java.util.Set;
+import org.apache.gravitino.iceberg.common.IcebergConfig;
+import org.apache.gravitino.listener.EventBus;
+import org.apache.gravitino.server.web.HttpAuditFilter;
+import org.apache.gravitino.server.web.JettyServer;
+import org.apache.gravitino.server.web.JettyServerConfig;
+import org.apache.gravitino.server.web.JettyServerTestUtils;
+import org.apache.gravitino.server.web.RequestContextFilter;
+import org.eclipse.jetty.servlet.ServletHandler;
+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. Rather than parse source text, this
exercises the extracted
+ * RESTService#registerMetricsPathFilters against a plain JettyServer and
inspects the real
+ * ServletHandler filter mappings it produces, so a broken
JettyServer.METRICS_PATH_SPECS list or
+ * a filter that merely appears in a comment cannot pass. See GH-12760.
+ */
+ @Test
+ public void testMetricsPathsHaveAuditFilterCoverage() throws Exception {
+ JettyServer server = new JettyServer();
+ JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(new
IcebergConfig());
+ server.initialize(jettyServerConfig, "test-iceberg-rest", false);
+ EventBus eventBus = new EventBus(Collections.emptyList());
+
+ try {
+ RESTService.registerMetricsPathFilters(server, eventBus);
+
+ ServletHandler servletHandler =
+
JettyServerTestUtils.getServletContextHandler(server).getServletHandler();
+ Set<String> auditedPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
HttpAuditFilter.class);
+ Set<String> requestContextPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
RequestContextFilter.class);
+
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ assertTrue(
+ auditedPathSpecs.contains(pathSpec),
+ "'" + pathSpec + "' must be covered by HttpAuditFilter, see
GH-12760");
+ assertTrue(
+ requestContextPathSpecs.contains(pathSpec),
+ "'"
+ + pathSpec
+ + "' must be covered by RequestContextFilter for
query-parameter "
+ + "capture, see GH-12760");
+ }
+ } finally {
+ server.stop();
+ }
+ }
+}
diff --git a/lance/lance-rest-server/build.gradle.kts
b/lance/lance-rest-server/build.gradle.kts
index 127eac64d5..55ede19641 100644
--- a/lance/lance-rest-server/build.gradle.kts
+++ b/lance/lance-rest-server/build.gradle.kts
@@ -103,6 +103,7 @@ dependencies {
testImplementation(project(":clients:client-java"))
testImplementation(project(":server"))
testImplementation(project(":integration-test-common", "testArtifacts"))
+ testImplementation(project(":server-common", "testArtifacts"))
testImplementation(libs.lance)
lanceSparkBundleVersions.forEach { version ->
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
index 041e038198..98f86aaae5 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
@@ -21,6 +21,8 @@ package org.apache.gravitino.lance;
import static
org.apache.gravitino.lance.common.config.LanceConfig.NAMESPACE_BACKEND;
import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Map;
import javax.servlet.Servlet;
import org.apache.gravitino.GravitinoEnv;
@@ -99,9 +101,17 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
server.addFilter(new RequestContextFilter(eventBus), LANCE_SPEC);
server.addFilter(
new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_LANCE_REST_SERVER), LANCE_SPEC);
- server.addCustomFilters(LANCE_SPEC);
server.addSystemFilters(LANCE_SPEC);
+ registerMetricsPathFilters(server, eventBus);
+
+ // Custom filters are registered once, across every filtered path in a
single call, so a
+ // filter whose init() isn't safe to run more than once per JVM only runs
it once rather than
+ // once per pathSpec.
+ List<String> customFilterPaths = new
ArrayList<>(JettyServer.METRICS_PATH_SPECS);
+ customFilterPaths.add(LANCE_SPEC);
+ server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+
LOG.info(
"Initialized Lance REST service for backend {} in {} mode",
lanceConfig.getNamespaceBackend(),
@@ -133,6 +143,26 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
}
}
+ /**
+ * Registers request-context tracking and audit-on-failure coverage on {@link
+ * JettyServer#METRICS_PATH_SPECS}. {@code /metrics} and {@code
/prometheus/metrics} used to
+ * receive no such coverage at all, with nothing in the build catching it;
{@code
+ * RequestContextFilter} is included too so query-parameter capture applies
uniformly, matching
+ * {@link #LANCE_SPEC}. Package-private and static so a unit test can
exercise it directly against
+ * a plain {@link JettyServer}, without booting the rest of {@link
#serviceInit}. See GH-12760.
+ *
+ * @param server the Jetty server whose {@link
JettyServer#METRICS_PATH_SPECS} need filter
+ * coverage
+ * @param eventBus the event bus audit events are dispatched through
+ */
+ static void registerMetricsPathFilters(JettyServer server, EventBus
eventBus) {
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ server.addFilter(new RequestContextFilter(eventBus), pathSpec);
+ server.addFilter(
+ new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_LANCE_REST_SERVER), pathSpec);
+ }
+ }
+
private NamespaceWrapper loadNamespaceImpl(LanceConfig lanceConfig) {
String backendType = lanceConfig.get(NAMESPACE_BACKEND);
LanceNamespaceBackend lanceNamespaceBackend =
LanceNamespaceBackend.fromType(backendType);
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
new file mode 100644
index 0000000000..bda74859f0
--- /dev/null
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
@@ -0,0 +1,77 @@
+/*
+ * 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.lance;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Collections;
+import java.util.Set;
+import org.apache.gravitino.lance.common.config.LanceConfig;
+import org.apache.gravitino.listener.EventBus;
+import org.apache.gravitino.server.web.HttpAuditFilter;
+import org.apache.gravitino.server.web.JettyServer;
+import org.apache.gravitino.server.web.JettyServerConfig;
+import org.apache.gravitino.server.web.JettyServerTestUtils;
+import org.apache.gravitino.server.web.RequestContextFilter;
+import org.eclipse.jetty.servlet.ServletHandler;
+import org.junit.jupiter.api.Test;
+
+public class TestLanceRESTService {
+
+ /**
+ * LanceRESTService.serviceInit() previously registered /metrics and
/prometheus/metrics (added by
+ * JettyServer#initialize() itself, outside LANCE_SPEC) with no audit
coverage at all, and nothing
+ * in the build caught it. Rather than parse source text, this exercises the
extracted
+ * LanceRESTService#registerMetricsPathFilters against a plain JettyServer
and inspects the real
+ * ServletHandler filter mappings it produces, so a broken
JettyServer.METRICS_PATH_SPECS list or
+ * a filter that merely appears in a comment cannot pass. See GH-12760.
+ */
+ @Test
+ public void testMetricsPathsHaveAuditFilterCoverage() throws Exception {
+ JettyServer server = new JettyServer();
+ JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(new
LanceConfig());
+ server.initialize(jettyServerConfig, "test-lance-rest", false);
+ EventBus eventBus = new EventBus(Collections.emptyList());
+
+ try {
+ LanceRESTService.registerMetricsPathFilters(server, eventBus);
+
+ ServletHandler servletHandler =
+
JettyServerTestUtils.getServletContextHandler(server).getServletHandler();
+ Set<String> auditedPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
HttpAuditFilter.class);
+ Set<String> requestContextPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
RequestContextFilter.class);
+
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ assertTrue(
+ auditedPathSpecs.contains(pathSpec),
+ "'" + pathSpec + "' must be covered by HttpAuditFilter, see
GH-12760");
+ assertTrue(
+ requestContextPathSpecs.contains(pathSpec),
+ "'"
+ + pathSpec
+ + "' must be covered by RequestContextFilter for
query-parameter "
+ + "capture, see GH-12760");
+ }
+ } finally {
+ server.stop();
+ }
+ }
+}
diff --git a/server-common/build.gradle.kts b/server-common/build.gradle.kts
index fdcdf2f931..aa48f71060 100644
--- a/server-common/build.gradle.kts
+++ b/server-common/build.gradle.kts
@@ -72,3 +72,16 @@ tasks {
environment("GRAVITINO_TEST", "true")
}
}
+
+val testJar by tasks.registering(Jar::class) {
+ archiveClassifier.set("tests")
+ from(sourceSets["test"].output)
+}
+
+configurations {
+ create("testArtifacts")
+}
+
+artifacts {
+ add("testArtifacts", testJar)
+}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
index 260791a4ca..2385b49a1f 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.server.web;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.servlets.MetricsServlet;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
import java.io.File;
import java.io.IOException;
import java.net.BindException;
@@ -71,6 +72,16 @@ public class JettyServer {
private static final String HTTPS = "https";
private static final String HTTP_PROTOCOL = "http/1.1";
+ /**
+ * The pathSpecs {@link #initialize} registers directly on the shared
servlet context when a
+ * {@link MetricsSystem} is available, outside of whatever pathspec the
caller itself filters.
+ * Callers that need request-context tracking or audit coverage on these
paths (see GH-12760) must
+ * apply their own filters to them explicitly, using this constant rather
than re-declaring the
+ * literal path strings, so the two can never drift apart.
+ */
+ public static final ImmutableList<String> METRICS_PATH_SPECS =
+ ImmutableList.of("/metrics", "/prometheus/metrics");
+
private Server server;
private ServletContextHandler servletContextHandler;
@@ -174,10 +185,10 @@ public class JettyServer {
MetricRegistry metricRegistry = metricsSystem.getMetricRegistry();
servletContextHandler.setAttribute(
"com.codahale.metrics.servlets.MetricsServlet.registry",
metricRegistry);
- servletContextHandler.addServlet(MetricsServlet.class, "/metrics");
+ servletContextHandler.addServlet(MetricsServlet.class,
METRICS_PATH_SPECS.get(0));
servletContextHandler.addServlet(
- new ServletHolder(metricsSystem.getPrometheusServlet()),
"/prometheus/metrics");
+ new ServletHolder(metricsSystem.getPrometheusServlet()),
METRICS_PATH_SPECS.get(1));
}
HandlerCollection handlers = new HandlerCollection();
@@ -493,7 +504,14 @@ public class JettyServer {
return server.getThreadPool();
}
- public void addCustomFilters(String pathSpec) {
+ /**
+ * Registers every configured custom filter, binding each one's single
{@link FilterHolder} to all
+ * of {@code pathSpecs} in one pass — so a filter whose {@code init()} isn't
safe to run more than
+ * once per JVM only runs it once, regardless of how many paths it's bound
to.
+ *
+ * @param pathSpecs the pathSpecs to bind each configured custom filter to
+ */
+ public void addCustomFilters(String... pathSpecs) {
for (String filterName : serverConfig.getCustomFilters()) {
if (StringUtils.isBlank(filterName)) {
continue;
@@ -504,7 +522,10 @@ public class JettyServer {
serverConfig.getAllWithPrefix(String.format("%s.param.",
filterName)).entrySet()) {
filterHolder.setInitParameter(entry.getKey(), entry.getValue());
}
- servletContextHandler.addFilter(filterHolder, pathSpec,
EnumSet.allOf(DispatcherType.class));
+ for (String pathSpec : pathSpecs) {
+ servletContextHandler.addFilter(
+ filterHolder, pathSpec, EnumSet.allOf(DispatcherType.class));
+ }
}
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/JettyServerTestUtils.java
b/server-common/src/test/java/org/apache/gravitino/server/web/JettyServerTestUtils.java
new file mode 100644
index 0000000000..d9706f6f7e
--- /dev/null
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/JettyServerTestUtils.java
@@ -0,0 +1,73 @@
+/*
+ * 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.server.web;
+
+import java.lang.reflect.Field;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.servlet.Filter;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+import org.eclipse.jetty.servlet.ServletHandler;
+
+/**
+ * Shared reflection/introspection helpers for tests that need to inspect a
live {@link
+ * JettyServer}'s real servlet and filter mappings, rather than parsing source
text or mocking
+ * Jetty. Published via server-common's {@code testArtifacts} configuration so
tests in other
+ * modules built on {@link JettyServer} (the main server, Iceberg REST, Lance
REST) don't each
+ * re-implement the same reflection and stream logic.
+ */
+public final class JettyServerTestUtils {
+
+ private JettyServerTestUtils() {}
+
+ /**
+ * Reflects into {@code server}'s private {@code servletContextHandler}
field, which is otherwise
+ * only populated after {@link JettyServer#initialize} runs and has no
public getter.
+ *
+ * @param server the Jetty server to inspect
+ * @return the server's live servlet context handler
+ * @throws ReflectiveOperationException if the field cannot be accessed
+ */
+ public static ServletContextHandler getServletContextHandler(JettyServer
server)
+ throws ReflectiveOperationException {
+ Field handlerField =
JettyServer.class.getDeclaredField("servletContextHandler");
+ handlerField.setAccessible(true);
+ return (ServletContextHandler) handlerField.get(server);
+ }
+
+ /**
+ * Returns every pathSpec that {@code filterClass} is bound to on {@code
servletHandler}.
+ *
+ * @param servletHandler the live servlet handler to inspect
+ * @param filterClass the filter class to look for
+ * @return the set of pathSpecs the filter is registered on
+ */
+ public static Set<String> filterPathSpecsFor(
+ ServletHandler servletHandler, Class<? extends Filter> filterClass) {
+ return Arrays.stream(servletHandler.getFilterMappings())
+ .filter(
+ filterMapping ->
+ filterClass
+ .getName()
+
.equals(servletHandler.getFilter(filterMapping.getFilterName()).getClassName()))
+ .flatMap(filterMapping -> Arrays.stream(filterMapping.getPathSpecs()))
+ .collect(Collectors.toSet());
+ }
+}
diff --git a/server/build.gradle.kts b/server/build.gradle.kts
index c5627f62b1..895c64a511 100644
--- a/server/build.gradle.kts
+++ b/server/build.gradle.kts
@@ -54,6 +54,7 @@ dependencies {
testCompileOnly(libs.lombok)
testImplementation(libs.commons.io)
+ testImplementation(project(":server-common", "testArtifacts"))
testImplementation(libs.jersey.test.framework.core) {
exclude(group = "org.junit.jupiter")
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index 0f83f8d482..f2ef1e77a8 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -18,9 +18,12 @@
*/
package org.apache.gravitino.server;
+import com.google.common.collect.ImmutableList;
import java.io.File;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.HashSet;
+import java.util.List;
import java.util.Properties;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.inject.Singleton;
@@ -83,6 +86,18 @@ public class GravitinoServer extends ResourceConfig {
private static final String API_ANY_PATH = "/api/*";
+ // Servlets mounted outside API_ANY_PATH that still need request-context
tracking and
+ // audit-on-failure coverage. VersioningFilter is intentionally not applied
to these: it
+ // negotiates Gravitino's "application/vnd.gravitino.vN+json" media type for
the Jersey-managed
+ // REST surface, and none of these servlets are part of it or read that
header. /metrics and
+ // /prometheus/metrics are registered by JettyServer#initialize() itself
(see server-common's
+ // JettyServer), outside GravitinoServer's own control entirely. See
GH-12760.
+ private static final ImmutableList<String> ROOT_MOUNTED_PATHS =
+ ImmutableList.<String>builder()
+ .add("/configs", "/configs/secrets/providers")
+ .addAll(JettyServer.METRICS_PATH_SPECS)
+ .build();
+
public static final String CONF_FILE = "gravitino.conf";
public static final String WEBSERVER_CONF_PREFIX =
"gravitino.server.webserver.";
@@ -190,15 +205,45 @@ public class GravitinoServer extends ResourceConfig {
// Root-level aliases for enterprise GTMs that require probes at
well-known root paths.
// Forwards /health, /health/live, /health/ready, and /health.html to the
canonical
- // /api/health/* endpoints.
+ // /api/health/* endpoints. Not part of ROOT_MOUNTED_PATHS below:
HealthAliasServlet forwards
+ // every request into /api/health*, which API_ANY_PATH already covers via
the servlet
+ // container's FORWARD dispatcher type, so binding the filters again here
would double-log
+ // every probe.
server.addServlet(new HealthAliasServlet(), "/health/*");
server.addServlet(new HealthAliasServlet(), "/health.html");
+ // API_ANY_PATH keeps its original, unabridged filter set: this is the
only pathspec that is
+ // part of the Jersey-managed REST surface, so it's the only one
VersioningFilter applies to.
server.addFilter(new RequestContextFilter(gravitinoEnv.eventBus()),
API_ANY_PATH);
server.addFilter(
new HttpAuditFilter(gravitinoEnv.eventBus(),
EventSource.GRAVITINO_SERVER), API_ANY_PATH);
- server.addCustomFilters(API_ANY_PATH);
server.addFilter(new VersioningFilter(), API_ANY_PATH);
+
+ // GH-12760: servlets mounted outside API_ANY_PATH used to receive none of
the filters below
+ // (no request-context tracking, no audit-on-failure, no custom filters),
with nothing in the
+ // build catching it. Every pathSpec a servlet is registered under above
must appear in
+ // ROOT_MOUNTED_PATHS, unless it forwards into an already-covered path
(see HealthAliasServlet
+ // above) or is added to the exemption list documented on
+ // TestGravitinoServer#testEveryServletPathIsCoveredByAuditFilter.
RequestContextFilter is
+ // given gravitinoEnv.eventBus() here too (not just API_ANY_PATH) so
query-parameter capture
+ // (see RequestContextFilter's class doc) applies uniformly, not only to
the REST API.
+ for (String pathSpec : ROOT_MOUNTED_PATHS) {
+ server.addFilter(new RequestContextFilter(gravitinoEnv.eventBus()),
pathSpec);
+ server.addFilter(
+ new HttpAuditFilter(gravitinoEnv.eventBus(),
EventSource.GRAVITINO_SERVER), pathSpec);
+ }
+
+ // Custom filters are registered once, across every filtered path in a
single call, so a
+ // filter whose init() isn't safe to run more than once per JVM only runs
it once rather than
+ // once per pathSpec.
+ List<String> customFilterPaths = new ArrayList<>(ROOT_MOUNTED_PATHS);
+ customFilterPaths.add(API_ANY_PATH);
+ server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+
+ // Only API_ANY_PATH requires authentication today. /configs must stay
open for the Web UI's
+ // pre-login OAuth bootstrap (see docs/gravitino-server-config.md);
/configs/secrets/providers
+ // is open pending GH-12921, which will add an operator-controlled
authorization gate for it
+ // specifically.
server.addSystemFilters(API_ANY_PATH);
if (server.isWebUiEnabled()) {
server.addFilter(new WebUIFilter(), "/"); // Redirect to the /ui/index
html page.
diff --git
a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
index 0c5cfa48e6..d9d2408731 100644
--- a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
+++ b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
@@ -23,15 +23,26 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import java.io.IOException;
+import java.lang.reflect.Field;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.util.Set;
import org.apache.commons.io.FileUtils;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.auxiliary.AuxiliaryServiceManager;
import org.apache.gravitino.rest.RESTUtils;
+import org.apache.gravitino.server.authentication.AuthenticationFilter;
+import org.apache.gravitino.server.web.HttpAuditFilter;
+import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
+import org.apache.gravitino.server.web.JettyServerTestUtils;
+import org.eclipse.jetty.http.pathmap.ServletPathSpec;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+import org.eclipse.jetty.servlet.ServletHandler;
+import org.eclipse.jetty.servlet.ServletMapping;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
@@ -44,6 +55,37 @@ import org.mockito.Mockito;
@TestInstance(Lifecycle.PER_CLASS)
public class TestGravitinoServer {
+ // Static-asset and forwarding paths shared by both exemption sets below,
each for the same
+ // reason in both: WebUIFilter's static assets and HealthAliasServlet's
forwarded probes need
+ // no direct filter binding of their own on this path. See GH-12760.
+ private static final Set<String> STATIC_AND_FORWARDING_PATHS =
+ ImmutableSet.of(
+ "/", // DefaultServlet / WebUIFilter: serves static UI assets, no
server-side logic.
+ "/ui/*", // WebUIFilter: serves static UI assets, no server-side
logic.
+ // HealthAliasServlet forwards both of the next two into
/api/health*, which is already
+ // covered via the servlet container's FORWARD dispatcher type.
+ "/health/*",
+ "/health.html");
+
+ // Paths that legitimately have no HttpAuditFilter binding of their own.
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 =
+ STATIC_AND_FORWARDING_PATHS;
+
+ // Paths that are deliberately reachable without authentication. GH-12760:
extending this set
+ // is a deliberate, reviewed decision, not a default — every other servlet
path must be covered
+ // by AuthenticationFilter. This is a distinct invariant from
+ // PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE above: e.g. /configs is fully
audited but
+ // intentionally unauthenticated, so it belongs in this set but not that one.
+ private static final Set<String> KNOWN_PUBLIC_PATHS =
+ ImmutableSet.<String>builder()
+ .addAll(STATIC_AND_FORWARDING_PATHS)
+ .add("/configs") // Intentionally public: backs the Web UI's
pre-login OAuth bootstrap.
+ .add("/configs/secrets/providers") // Open pending GH-12921 (tracked
authz gate).
+ .addAll(JettyServer.METRICS_PATH_SPECS) // Conventionally scraped
without credentials.
+ .build();
+
private GravitinoServer gravitinoServer;
private ServerConfig spyServerConfig;
@@ -136,4 +178,85 @@ public class TestGravitinoServer {
hookBlock.contains("server.gracefulStop()"),
"Shutdown hook should invoke server.gracefulStop() so app-level
cleanup runs on SIGTERM");
}
+
+ @Test
+ public void testEveryServletPathIsCoveredByAuditFilter() throws Exception {
+ gravitinoServer.initialize();
+
+ ServletHandler servletHandler =
getServletContextHandler(gravitinoServer).getServletHandler();
+ Set<String> auditedPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
HttpAuditFilter.class);
+
+ for (ServletMapping servletMapping : servletHandler.getServletMappings()) {
+ for (String pathSpec : servletMapping.getPathSpecs()) {
+ if (PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE.contains(pathSpec)) {
+ continue;
+ }
+ assertTrue(
+ isPathSpecCovered(pathSpec, auditedPathSpecs),
+ "Servlet path '"
+ + pathSpec
+ + "' is registered without HttpAuditFilter coverage. See
GH-12760: every "
+ + "servlet mounted outside /api/* must be wired into
GravitinoServer's "
+ + "ROOT_MOUNTED_PATHS filter loop, or added to "
+ + "PATHS_EXEMPT_FROM_DIRECT_AUDIT_COVERAGE above with a
documented reason.");
+ }
+ }
+ }
+
+ @Test
+ public void testEveryServletPathIsEitherAuthenticatedOrDeliberatelyPublic()
throws Exception {
+ gravitinoServer.initialize();
+
+ ServletHandler servletHandler =
getServletContextHandler(gravitinoServer).getServletHandler();
+ Set<String> authenticatedPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
AuthenticationFilter.class);
+
+ for (ServletMapping servletMapping : servletHandler.getServletMappings()) {
+ for (String pathSpec : servletMapping.getPathSpecs()) {
+ if (isPathSpecCovered(pathSpec, authenticatedPathSpecs)) {
+ continue;
+ }
+ assertTrue(
+ KNOWN_PUBLIC_PATHS.contains(pathSpec),
+ "Servlet path '"
+ + pathSpec
+ + "' is neither covered by AuthenticationFilter nor listed in "
+ + "KNOWN_PUBLIC_PATHS. See GH-12760: a servlet must either
require "
+ + "authentication or be a deliberate, reviewed public
exception.");
+ }
+ }
+ }
+
+ /**
+ * Whether every request a servlet registered under {@code servletPathSpec}
can receive is also
+ * matched by at least one of {@code filterPathSpecs}, using Jetty's own
path-spec matching
+ * semantics rather than exact string equality — so a servlet mounted at,
say, {@code
+ * /api/internal/*} is correctly recognized as already covered by a filter
bound to {@code
+ * /api/*}.
+ *
+ * @param servletPathSpec the servlet's registered pathSpec
+ * @param filterPathSpecs the pathSpecs a filter is bound to
+ * @return true if {@code servletPathSpec} is covered by one of {@code
filterPathSpecs}
+ */
+ private static boolean isPathSpecCovered(String servletPathSpec, Set<String>
filterPathSpecs) {
+ // A path-prefix spec like "/api/*" matches any concrete path under it;
substitute a
+ // representative concrete path so ServletPathSpec#matches can evaluate a
real request path
+ // instead of a pattern.
+ String representativePath =
+ servletPathSpec.endsWith("/*")
+ ? servletPathSpec.substring(0, servletPathSpec.length() - 1) +
"probe"
+ : servletPathSpec;
+ return filterPathSpecs.stream()
+ .anyMatch(
+ filterPathSpec -> new
ServletPathSpec(filterPathSpec).matches(representativePath));
+ }
+
+ private static ServletContextHandler
getServletContextHandler(GravitinoServer gravitinoServer)
+ throws Exception {
+ Field serverField = GravitinoServer.class.getDeclaredField("server");
+ serverField.setAccessible(true);
+ JettyServer jettyServer = (JettyServer) serverField.get(gravitinoServer);
+ return JettyServerTestUtils.getServletContextHandler(jettyServer);
+ }
}
