This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new b639f5c12f [#12760] fix(server): Cover root-mounted servlets with the
request-context, audit, and custom filter chain (#12922)
b639f5c12f is described below
commit b639f5c12f458817949a13ea3af1dfb7a9e9dd0c
Author: Jerry Shao <[email protected]>
AuthorDate: Mon Sep 7 14:00:07 2026 +0800
[#12760] fix(server): Cover root-mounted servlets with the request-context,
audit, and custom filter chain (#12922)
### What changes were proposed in this pull request?
`GravitinoServer` bound `RequestContextFilter`, `HttpAuditFilter`,
custom filters, and `VersioningFilter` to `/api/*` only. `ConfigServlet`
(`/configs`), `SecretProvidersConfigServlet`
(`/configs/secrets/providers`), and the `/metrics`/`/prometheus/metrics`
servlets are all mounted outside that pathspec and were silently
bypassing every one of those filters, with nothing in the build catching
it.
This PR binds the same filters to those paths too, skipping
`VersioningFilter` for `/metrics`/`/prometheus/metrics` since that
filter negotiates Gravitino's own REST API media type and has no meaning
for metrics output. Authentication stays `/api/*`-only: `/configs` must
remain open for the Web UI's pre-login OAuth bootstrap, and
`/configs/secrets/providers` is left open pending #12921, which will add
an operator-controlled authorization gate for it specifically.
A regression test
(`TestGravitinoServer#testEveryServletPathIsCoveredByAuditFilter`)
introspects the live `ServletContextHandler` after `initialize()` and
fails if any registered servlet path lacks `HttpAuditFilter` coverage,
unless it's in a small documented exemption list — so a future servlet
added the same way gets caught at test time instead of silently
repeating this bug.
**Also fixed the same gap in the Iceberg and Lance REST servers.** Both
extend the shared `JettyServer`, whose `initialize()` unconditionally
registers `/metrics` and `/prometheus/metrics` directly on the servlet
context handler, outside whatever spec path each server filters
(`/iceberg/*`, `/lance/*`). Neither `RESTService` nor `LanceRESTService`
wired any filter onto those two paths, so failures there went completely
unaudited there too. Added the same `HttpAuditFilter` + custom-filter
coverage to both, plus `RequestContextFilter(eventBus)` (matching the
wiring #12891 added to `ICEBERG_SPEC`/`LANCE_SPEC`) so query-parameter
capture applies there too. A source-inspection regression test in each
module pins the `HttpAuditFilter` coverage
(`RESTService`/`LanceRESTService` have too many external dependencies —
catalog backends, namespace backends, Gravitino env state — to boot in a
plain unit test the way `GravitinoServer` can).
### Why are the changes needed?
An endpoint that answers without appearing in the audit log is a
compliance gap independent of what it returns.
`/configs/secrets/providers` in particular can expose internal
secret-provider endpoint URIs (e.g. Vault/OpenBao) with zero audit trail
today.
Fix: #12760
### Does this PR introduce _any_ user-facing change?
Yes: requests to `/configs`, `/configs/secrets/providers`, `/metrics`,
and `/prometheus/metrics` (on the main Gravitino server, and
`/metrics`/`/prometheus/metrics` on the Iceberg and Lance REST servers)
now produce an audit log entry on both success and failure, with query
parameters captured and redacted, same as `/api/*`-family requests
already do. No config keys added, removed, or renamed. No change to
authentication — those endpoints remain open exactly as before.
### How was this patch tested?
- New unit test
`TestGravitinoServer#testEveryServletPathIsCoveredByAuditFilter`;
verified it actually catches the regression by temporarily removing a
path from coverage and confirming the test fails with an actionable
message.
- New unit tests
`TestRESTService#testMetricsPathsHaveAuditFilterCoverage` (Iceberg) and
`TestLanceRESTService#testMetricsPathsHaveAuditFilterCoverage` (Lance);
same verify-it-actually-fails check performed on each.
- Full module suites after rebasing onto #12891: `:server` 449 tests,
`:iceberg:iceberg-rest-server` 423 tests, `:lance:lance-rest-server` 92
tests — 0 failures across all three.
- Manual verification against a compiled distribution
(`compileDistribution -PskipWeb=true -x test`) with
`gravitino.audit.enabled=true`, running the main server plus both
Iceberg and Lance auxiliary services: confirmed `GET /configs`, `GET
/configs/secrets/providers?debug=true&token=...`, `GET
/metrics?apiKey=...` (main server), and `GET /metrics?token=...` on both
the Iceberg (port 9001) and Lance (port 9101) REST servers all produce
`SUCCESS` audit entries with the sensitive query parameter redacted to
`***` and the non-sensitive one passed through.
---------
Co-authored-by: Claude Sonnet 5 <[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 | 37 ++++++-
.../gravitino/lance/TestLanceRESTService.java | 77 +++++++++++++
.../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 +++++++++++++++++++++
11 files changed, 495 insertions(+), 10 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 3235b0dc17..176c4f16b7 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 67b4065406..8a45ce4939 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
@@ -22,7 +22,9 @@ import static
org.apache.gravitino.lance.common.config.LanceConfig.NAMESPACE_BAC
import static
org.apache.gravitino.lance.service.authorization.LanceRESTAuthInterceptionService.METALAKE_BINDING;
import java.lang.reflect.Constructor;
+import java.util.ArrayList;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import javax.inject.Singleton;
import javax.servlet.Servlet;
@@ -137,7 +139,6 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
new HttpAuditFilter(
eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
LANCE_SPEC);
- server.addCustomFilters(LANCE_SPEC);
server.addSystemFilters(LANCE_SPEC);
if (auxMode) {
server.addFilter(
@@ -146,10 +147,22 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
}
// 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 /lance/health*, which
LANCE_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("/lance"), "/health/*");
server.addServlet(new HealthAliasServlet("/lance"), "/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(LANCE_SPEC);
+ server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+
LOG.info(
"Initialized Lance REST service for backend {} in {} mode, metadata
authorization {}",
lanceConfig.getNamespaceBackend(),
@@ -182,6 +195,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, boolean
auxMode) {
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/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 c4891ba859..628da89fd2 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;
@@ -85,6 +88,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.";
@@ -198,15 +213,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 7b8dbca075..12e8eb68d4 100644
--- a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
+++ b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
@@ -26,7 +26,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
import com.fasterxml.jackson.core.type.TypeReference;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import java.io.IOException;
+import java.lang.reflect.Field;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
@@ -37,14 +39,23 @@ import java.nio.file.Paths;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+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.secret.SecretProviderRegistry;
import org.apache.gravitino.secret.memory.InMemorySecretsProvider;
+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.apache.gravitino.server.web.ObjectMapperProvider;
+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;
@@ -57,6 +68,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;
@@ -174,6 +216,87 @@ public class TestGravitinoServer {
assertFalse(providers.get(0).containsKey("className"));
}
+ @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);
+ }
+
private static ServerConfig serverConfigWithMemoryProvider() throws
IOException {
Map<String, String> configs = new HashMap<>();
configs.put(
