jerryshao commented on code in PR #13057:
URL: https://github.com/apache/gravitino/pull/13057#discussion_r3976730040
##########
server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java:
##########
@@ -112,6 +112,17 @@ public final class JettyServerConfig {
.checkValue(value -> value > 0,
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
.createWithDefault(128 * 1024);
+ public static final ConfigEntry<Boolean> INCLUDE_ERROR_STACK_TRACE =
+ new ConfigBuilder("includeErrorStackTrace")
+ .doc(
+ "Whether to include server-side stack traces in HTTP error
responses. Set this to "
+ + "false in new deployments because stack traces can expose
internal "
+ + "implementation details. It remains true by default only
to avoid breaking "
+ + "legacy clients that expect the stack field")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .booleanConf()
Review Comment:
**correctness (high) — scope gap, verified**: This config only governs the
main Gravitino server's own `ErrorResponse` serialization path. Two other REST
services shipped in this repository are completely untouched and still
unconditionally leak full stack traces regardless of this setting:
1. `iceberg/iceberg-rest-server/.../service/IcebergRESTUtils.java:428-437` —
`errorResponse(Throwable, int)` (used by the globally-registered
`IcebergExceptionMapper`) does
`ErrorResponse.builder()....withStackTrace(ex).build()` unconditionally, on
Iceberg's own `org.apache.iceberg.rest.responses.ErrorResponse`, with zero
reference to `includeErrorStackTrace` anywhere in that file.
2. `lance/lance-rest-server/.../service/LanceExceptionMapper.java:70-87,131`
— every branch of `toResponse(Throwable)` embeds
`org.apache.commons.lang3.exception.ExceptionUtils.getStackTrace(ex)` into the
response's `detail` field, again completely independent of this flag.
An operator who sets `includeErrorStackTrace=false` on the main server —
following this PR's own recommendation — would reasonably believe the CWE-209
exposure is closed, but any client hitting the Iceberg REST catalog endpoint or
the Lance REST endpoint still gets a full server-side Java stack trace in every
error response. Given the stated goal of this PR, I'd consider this a release
blocker rather than a nice-to-have follow-up.
---
**altitude**: Separately, this flag defaults to `true` (stack traces
exposed) purely for legacy-client compatibility — the PR's own doc comment here
admits this and tells operators to flip it to `false`. That's an opt-in-safety
design: every fresh OSS install, Docker image, or Helm chart that doesn't
explicitly override this ships CWE-209-exposed by default. A safer design would
default to `false` with a documented legacy-compatibility opt-in, or move
`stack` out of the response body into server-side-only diagnostics entirely.
##########
server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java:
##########
@@ -112,6 +112,17 @@ public final class JettyServerConfig {
.checkValue(value -> value > 0,
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
.createWithDefault(128 * 1024);
+ public static final ConfigEntry<Boolean> INCLUDE_ERROR_STACK_TRACE =
Review Comment:
**conventions**: This new public field `INCLUDE_ERROR_STACK_TRACE` has no
Javadoc comment — only a `.doc(...)` builder string, which documents the config
value to operators, not the Java field/API itself. CLAUDE.md: "All new `public`
and `protected` classes, methods, and fields must have Javadoc" (checkstyle
enforces this with `-Werror` per this repo's CI).
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:
##########
@@ -183,7 +206,7 @@ protected void sendAuthErrorResponse(HttpServletResponse
response, Exception exc
response.setStatus(httpStatus);
response.setContentType("application/json");
response.setCharacterEncoding(StandardCharsets.UTF_8.name());
- ObjectMapperProvider.objectMapper().writeValue(response.getWriter(),
errorResponse);
+ objectMapper.writeValue(response.getWriter(), errorResponse);
Review Comment:
**correctness**: `includeErrorStackTrace` (via `objectMapper` here) only
gates the `stack` field; it does nothing for `message`. Every branch of
`sendAuthErrorResponse` above (lines 188-203) builds the response's `message`
field directly from the raw, uncontrolled `exception.getMessage()` — including
the catch-all `ErrorResponse.internalError(exception.getMessage(), exception)`
at line 203 for unanticipated exceptions. For a bug in a custom
`Authenticator`, an NPE, or a backend/token-store connection failure,
`getMessage()`/the exception's own text commonly includes internal class names,
file paths, or connection details — even with `includeErrorStackTrace=false`,
that text still ships in the response body. Same pattern exists in
`server/.../web/mapper/ErrorExceptionMapper.java:40-43` for uncaught
`java.lang.Error`. Worth considering whether `message` needs the same treatment
`stack` just got.
---
**correctness**: Separately, `doFilter`'s catch blocks (lines 168-171,
calling `sendAuthErrorResponse`) never log anything server-side — unlike the
sibling authorization path (`GravitinoInterceptionService`) that this same PR
updated to `LOG.warn(..., ex)`. Under the PR's own recommended config
(`includeErrorStackTrace=false`), an unexpected exception during authentication
is now both omitted from the client response *and* never logged server-side,
losing all diagnostics for authentication-layer bugs — exactly the failure mode
the PR description says it solved for the authorization path, just not carried
over to this one.
##########
server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java:
##########
@@ -28,38 +29,88 @@
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import javax.ws.rs.ext.ContextResolver;
import javax.ws.rs.ext.Provider;
+import org.apache.gravitino.dto.responses.ErrorResponse;
@Provider
public class ObjectMapperProvider implements ContextResolver<ObjectMapper> {
+ // Keep diagnostic stacks inside the server and accept legacy payloads while
allowing operators
+ // to omit them from responses.
+ @JsonIgnoreProperties(value = "stack", allowSetters = true)
+ private abstract static class ErrorResponseMixin {}
+
private static class ObjectMapperHolder {
- private static final ObjectMapper INSTANCE =
- JsonMapper.builder()
- .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false)
- .configure(EnumFeature.WRITE_ENUMS_TO_LOWERCASE, true)
- .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)
- .build()
- .setSerializationInclusion(JsonInclude.Include.NON_NULL)
- .registerModule(new JavaTimeModule())
- .registerModule(new Jdk8Module());
+ private static final ObjectMapper WITHOUT_ERROR_STACK_TRACE =
createObjectMapper(false);
+ private static final ObjectMapper WITH_ERROR_STACK_TRACE =
createObjectMapper(true);
+ }
+
+ private final ObjectMapper objectMapper;
+
+ /**
+ * Creates a provider using the backward-compatible server default, which
includes error stack
+ * traces.
+ */
+ public ObjectMapperProvider() {
+ this(JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue());
+ }
+
+ /**
+ * Creates a provider with explicit error stack-trace serialization behavior.
+ *
+ * @param includeErrorStackTrace whether HTTP error responses should include
diagnostic stack
+ * traces
+ */
+ public ObjectMapperProvider(boolean includeErrorStackTrace) {
+ this.objectMapper = objectMapper(includeErrorStackTrace);
}
/**
- * Retrieves a globally shared {@link ObjectMapper} instance.
+ * Retrieves the shared {@link ObjectMapper} using the backward-compatible
server default.
*
- * <p>Note: This ObjectMapper is a global single instance. If you need to
modify the default
- * serialization/deserialization settings, make changes within the INSTANCE
builder directly.
- * Avoid modifying properties of the returned {@code ObjectMapper} instance
to prevent unintended
- * side effects.
+ * <p>Do not modify the returned mapper. Use {@link #objectMapper(boolean)}
to select explicit
+ * error stack-trace behavior.
*
* @return the globally shared {@link ObjectMapper} instance
*/
public static ObjectMapper objectMapper() {
- return ObjectMapperHolder.INSTANCE;
+ return
objectMapper(JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue());
Review Comment:
**reuse**: This "resolve the backward-compatible default" expression
(`JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue()`) is
independently duplicated here and in the constructor a few lines up (line 54),
and again in `VersioningFilter`'s no-arg constructor
(`server/.../VersioningFilter.java:~102`), instead of one delegating to a
single shared resolution point — e.g. `VersioningFilter()` could just call
`ObjectMapperProvider.objectMapper()` (the way `AuthenticationFilter()` already
does) instead of re-deriving the same boolean and re-invoking the two-arg
overload itself. If the default-resolution logic ever changes (env-var
override, renamed config key), it's easy to update some call sites and miss
others, leaving them silently serving stale default behavior.
##########
server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java:
##########
@@ -542,6 +542,6 @@ public void addSystemFilters(String pathSpec) {
* custom authentication filter (e.g., one that returns Iceberg-spec JSON
error responses).
*/
protected Filter createAuthenticationFilter() {
Review Comment:
**altitude**: `createAuthenticationFilter()` is a plain overridable method
with nothing forcing an override to consult
`serverConfig.isIncludeErrorStackTrace()`. Confirmed both Iceberg's
`RESTService.java:96-97` and `LanceJettyServer.createAuthenticationFilter()`
override this and always construct their own auth filter via a no-arg
constructor, silently dropping the config flag (currently non-leaking only
because neither `IcebergAuthenticationFilter` nor `LanceAuthenticationFilter`'s
own DTO has a `stack` field — but that's incidental, not enforced).
This demonstrates the "thread the boolean through every constructor" pattern
this PR relies on isn't backed by any compiler check, test, or checkstyle rule
— a future REST module added the same way (copying the existing Iceberg/Lance
pattern) will silently reintroduce full stack-trace leakage in its auth-error
path with nothing catching it.
##########
server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java:
##########
@@ -28,38 +29,88 @@
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import javax.ws.rs.ext.ContextResolver;
import javax.ws.rs.ext.Provider;
+import org.apache.gravitino.dto.responses.ErrorResponse;
@Provider
public class ObjectMapperProvider implements ContextResolver<ObjectMapper> {
+ // Keep diagnostic stacks inside the server and accept legacy payloads while
allowing operators
+ // to omit them from responses.
+ @JsonIgnoreProperties(value = "stack", allowSetters = true)
Review Comment:
**efficiency**: The suppression mechanism is serialization-time only (this
Jackson mixin), not suppression at the point the stack string is produced.
`ErrorResponse`'s factory methods
(`common/.../dto/responses/ErrorResponse.java`, e.g.
`internalError`/`forbidden`/`unauthorized`) still call
`getStackTrace(throwable)` unconditionally to build the field, and this mixin
just discards the result when `includeErrorStackTrace=false`. So disabling the
config saves response bytes but not the CPU/allocation cost of building a
(potentially deep, multi-cause) stack-trace string on every failed request.
Threading the boolean into `ErrorResponse`'s construction (skip `getStackTrace`
entirely when disabled) would avoid the cost outright instead of just hiding
the result.
##########
server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java:
##########
@@ -116,7 +116,7 @@ public synchronized void initialize(
// Set error handler for Jetty Server
ErrorHandler errorHandler = new ErrorHandler();
- errorHandler.setShowStacks(true);
+ errorHandler.setShowStacks(serverConfig.isIncludeErrorStackTrace());
Review Comment:
**test-coverage**: This new conditional
(`errorHandler.setShowStacks(serverConfig.isIncludeErrorStackTrace())`,
replacing a hardcoded `true`) has zero test coverage in either direction —
`grep -rn "setShowStacks|showStacks" server-common/src/test server/src/test`
returns no matches anywhere. This is the pre-Jersey, raw Jetty error path (e.g.
malformed HTTP requests that never reach a Jersey resource); a regression here
(inverted boolean, or the call silently dropped in a future refactor) would
leak stack traces for a whole class of errors that bypass Jersey's exception
mappers entirely, with nothing to catch it.
--
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]
