This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 ed7aaa7b59 [Cherry-pick to branch-1.3] [#12872] fix(core): Capture and
redact request query parameters in audit log entries (#12891) (#12932)
ed7aaa7b59 is described below
commit ed7aaa7b593fa89d390e1087d7bec39ea6e6bc8e
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Fri Sep 4 22:41:32 2026 +0800
[Cherry-pick to branch-1.3] [#12872] fix(core): Capture and redact request
query parameters in audit log entries (#12891) (#12932)
**Cherry-pick Information:**
- Original commit: 15259af5d7d4124fa367df3c5106bbc75744b426
- Target branch: `branch-1.3`
- Status: ✅ Conflicts resolved manually
Conflicts arose because this commit's changes assume two prerequisite
PRs (#12691 and #12722) that were never fully backported to `branch-1.3`
(#12722's backport, #12726, was closed unmerged). Resolved by keeping
this commit's own change (request query-parameter capture, sealed
`customInfo()`/`ownCustomInfo()`) while not pulling in the unrelated
audit-extras functionality from those two PRs that branch-1.3 doesn't
have. See the "Resolve branch-1.3 cherry-pick conflicts" commit for the
file-by-file rationale.
Verified: affected modules (core, iceberg-rest-server,
lance-rest-server, server-common, server) compile cleanly, their unit
tests pass, and `spotlessApply` makes no changes.
---------
Co-authored-by: Jerry Shao <[email protected]>
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Jerry Shao <[email protected]>
---
.../apache/gravitino/audit/AuditLogRedactor.java | 69 ++++++-
.../org/apache/gravitino/listener/EventBus.java | 19 ++
.../apache/gravitino/listener/api/event/Event.java | 55 ++++++
.../listener/api/event/GrantGroupRolesEvent.java | 2 +-
.../api/event/GrantGroupRolesFailureEvent.java | 2 +-
.../listener/api/event/GrantUserRolesEvent.java | 2 +-
.../api/event/GrantUserRolesFailureEvent.java | 2 +-
.../listener/api/event/RevokeGroupRolesEvent.java | 2 +-
.../api/event/RevokeGroupRolesFailureEvent.java | 2 +-
.../listener/api/event/RevokeUserRolesEvent.java | 2 +-
.../api/event/RevokeUserRolesFailureEvent.java | 2 +-
.../server/AuthorizationDenialFailureEvent.java | 5 +-
...uestFailureEvent.java => HttpRequestEvent.java} | 81 ++++-----
.../api/event/server/HttpRequestFailureEvent.java | 23 ++-
.../org/apache/gravitino/utils/RequestContext.java | 157 ++++++++++++++--
.../gravitino/audit/TestAuditLogRedactor.java | 159 ++++++++++++++++
.../gravitino/audit/TestJsonAuditFormatter.java | 4 +-
.../gravitino/audit/v2/TestSimpleAuditLogV2.java | 31 +++-
.../apache/gravitino/listener/TestEventBus.java | 37 ++++
.../gravitino/listener/api/event/TestEvent.java | 102 +++++++++++
.../TestAuthorizationDenialFailureEvent.java | 25 +++
.../apache/gravitino/utils/TestRequestContext.java | 147 +++++++++++++++
docs/gravitino-server-config.md | 30 +++-
.../org/apache/gravitino/iceberg/RESTService.java | 4 +
.../gravitino/listener/api/event/IcebergEvent.java | 3 +-
.../listener/api/event/IcebergFailureEvent.java | 3 +-
.../apache/gravitino/lance/LanceRESTService.java | 4 +
.../gravitino/server/web/HttpAuditFilter.java | 67 ++++---
.../gravitino/server/web/RequestContextFilter.java | 130 +++++++++++++-
.../gravitino/server/web/TestHttpAuditFilter.java | 91 +++++++++-
.../server/web/TestRequestContextFilter.java | 199 +++++++++++++++++++++
.../apache/gravitino/server/GravitinoServer.java | 2 +-
32 files changed, 1344 insertions(+), 119 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java
b/core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java
index a60dece5d4..ac49e131d8 100644
--- a/core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java
+++ b/core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java
@@ -25,16 +25,61 @@ import java.util.Locale;
import java.util.Map;
import java.util.Set;
-/** Utility methods for redacting sensitive values in audit logs. */
+/**
+ * Utility methods for redacting sensitive values in audit logs.
+ *
+ * <p>This is the single place redaction happens: {@code customInfo()} entries
are captured raw
+ * everywhere they originate (dispatcher extras, request query parameters,
HTTP headers, etc.) and
+ * redacted only here, once, right before a log line is written — by {@link
+ * org.apache.gravitino.audit.v2.SimpleAuditLogV2} and {@link
JsonAuditFormatter}, the only two
+ * renderers that expose {@code customInfo()}. Keeping exactly one redaction
pass, applied uniformly
+ * to the fully-merged map regardless of which layer contributed which key,
avoids the alternative
+ * of multiple redaction call sites drifting out of sync with different
keyword lists.
+ */
public final class AuditLogRedactor {
/** Redacted value used for sensitive audit fields. */
public static final String REDACTED_VALUE = "***";
+ /** Case-insensitive exact matches against known internal semantic keys. */
private static final Set<String> MASKED_CUSTOM_INFO_KEYS =
ImmutableSet.of(
"authorization", "cookie", "x-amz-security-token",
"s3.access-key-id", "jdbc-password");
+ /**
+ * Case-insensitive substrings of a {@code customInfo} key that mark it as
sensitive, checked
+ * (against a separator-stripped form of the key — see {@link
#isSensitiveKey(String)}) in
+ * addition to the exact-match {@link #MASKED_CUSTOM_INFO_KEYS}. This exists
because some keys —
+ * notably request query-parameter names, which become {@code customInfo}
entries via {@link
+ * org.apache.gravitino.listener.api.event.Event#customInfo()} — are
supplied by the caller and
+ * can use arbitrary naming conventions (e.g. {@code accessToken}, {@code
s3-secret-access-key},
+ * {@code api-key}, {@code x_api_key}) that an exact-match list alone would
miss. Extend this set
+ * as new sensitive key names are identified.
+ */
+ private static final Set<String> SENSITIVE_KEY_SUBSTRINGS =
+ ImmutableSet.of(
+ "password",
+ "secret",
+ "token",
+ "credential",
+ "apikey",
+ "accesskey",
+ "privatekey",
+ "auth",
+ "signature");
+
+ /**
+ * Case-insensitive exact matches that are never sensitive, checked before
{@link
+ * #SENSITIVE_KEY_SUBSTRINGS}. These are fixed key names the codebase itself
chooses (e.g. {@code
+ * ownCustomInfo()} in {@code AuthorizationDenialFailureEvent}, {@code
HttpRequestEvent}), not
+ * caller-supplied data — unlike query-parameter names, a coincidental
substring match here (e.g.
+ * {@code auth.method} contains "auth") is always a false positive that
would destroy genuinely
+ * useful audit data instead of protecting anything. Add a key here only
when it is a fixed string
+ * literal in the codebase, never for a name that could come from external
input.
+ */
+ private static final Set<String> NEVER_SENSITIVE_KEYS =
+ ImmutableSet.of("http.method", "http.uri", "http.status", "auth.method",
"auth.expression");
+
private AuditLogRedactor() {}
/**
@@ -61,10 +106,26 @@ public final class AuditLogRedactor {
* @return the redacted value for sensitive keys, otherwise the original
value
*/
public static String redactValue(String key, String value) {
- return isSensitiveCustomInfoKey(key) ? REDACTED_VALUE : value;
+ return isSensitiveKey(key) ? REDACTED_VALUE : value;
}
- private static boolean isSensitiveCustomInfoKey(String key) {
- return key != null &&
MASKED_CUSTOM_INFO_KEYS.contains(key.toLowerCase(Locale.ROOT));
+ private static boolean isSensitiveKey(String key) {
+ if (key == null) {
+ return false;
+ }
+ // Exact-match checks run against the raw (only case-folded) key:
NEVER_SENSITIVE_KEYS and
+ // MASKED_CUSTOM_INFO_KEYS are fixed literals, so there is nothing to
normalize away.
+ String lowerCaseKey = key.toLowerCase(Locale.ROOT);
+ if (NEVER_SENSITIVE_KEYS.contains(lowerCaseKey)) {
+ return false;
+ }
+ if (MASKED_CUSTOM_INFO_KEYS.contains(lowerCaseKey)) {
+ return true;
+ }
+ // Caller-supplied key names vary in separator style (api-key, api_key,
x-api-key, ...), so the
+ // substring check strips everything but letters and digits before
matching; otherwise a
+ // hyphenated or underscored spelling of a multi-word entry like "apikey"
would never match.
+ String normalizedKey = lowerCaseKey.replaceAll("[^a-z0-9]", "");
+ return SENSITIVE_KEY_SUBSTRINGS.stream().anyMatch(normalizedKey::contains);
}
}
diff --git a/core/src/main/java/org/apache/gravitino/listener/EventBus.java
b/core/src/main/java/org/apache/gravitino/listener/EventBus.java
index 17af8d718a..33de94ce42 100644
--- a/core/src/main/java/org/apache/gravitino/listener/EventBus.java
+++ b/core/src/main/java/org/apache/gravitino/listener/EventBus.java
@@ -31,6 +31,7 @@ import org.apache.gravitino.listener.api.event.Event;
import org.apache.gravitino.listener.api.event.FailureEvent;
import org.apache.gravitino.listener.api.event.PreEvent;
import org.apache.gravitino.listener.api.event.SupportsChangingPreEvent;
+import org.apache.gravitino.listener.api.event.server.HttpRequestEvent;
import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent;
import org.apache.gravitino.utils.RequestContext;
import org.slf4j.Logger;
@@ -126,7 +127,25 @@ public class EventBus {
return eventListeners;
}
+ /**
+ * Dispatches a success event to listeners.
+ *
+ * <p>For every success event that is <em>not</em> an {@link
HttpRequestEvent}, this method also
+ * sets {@link RequestContext#markOperationSuccessFired()} on the current
thread. This lets {@code
+ * HttpAuditFilter} detect that an operation-layer success event has already
been dispatched and
+ * skip emitting a redundant HTTP-level fallback event for the same request.
Tracked independently
+ * from the failure-side flag so an operation that succeeds but whose HTTP
response delivery later
+ * fails still produces both a success and a failure audit entry.
+ *
+ * @param postEvent the success event to dispatch
+ */
private void dispatchPostEvent(Event postEvent) {
+ // Mark the request thread so HttpAuditFilter skips emitting its own
fallback event.
+ // HttpRequestEvent is exempt — it IS the HTTP-layer fallback event and
must not set the flag,
+ // otherwise it would suppress itself on the next pass through the finally
block.
+ if (!(postEvent instanceof HttpRequestEvent)) {
+ RequestContext.markOperationSuccessFired();
+ }
eventListeners.forEach(eventListener ->
eventListener.onPostEvent(postEvent));
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
index cb2b0829b7..799eeacfc8 100644
--- a/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
+++ b/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
@@ -19,6 +19,9 @@
package org.apache.gravitino.listener.api.event;
+import com.google.common.collect.ImmutableMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
@@ -30,16 +33,38 @@ import org.apache.gravitino.utils.RequestContext;
* <p>The client remote address is captured from {@link RequestContext} at
construction time on the
* servlet thread, so async listener threads can safely call {@link
#remoteAddress()} without
* accessing thread-local storage.
+ *
+ * <p>The current request's query parameters are captured the same way, raw
and <b>unredacted</b>
+ * (redaction happens later — see {@code
org.apache.gravitino.audit.AuditLogRedactor}'s class doc),
+ * and merged into {@link #customInfo()} automatically. This gives every event
in the system —
+ * including ones with no subclass-specific {@code customInfo} — automatic
audit coverage of the
+ * parameters that produced it, with no per-event-class wiring required. It
also means every {@link
+ * org.apache.gravitino.listener.api.EventListenerPlugin} — not only the two
built-in audit-log
+ * formatters — receives these parameters unredacted; a plugin that forwards
{@code customInfo()}
+ * elsewhere is responsible for its own redaction if that matters for its
destination.
+ *
+ * <p>{@link #customInfo()} itself is {@code final}: a subclass that wants to
contribute its own
+ * facts must override {@link #ownCustomInfo()} instead, never {@code
customInfo()} directly. This
+ * is deliberate — an earlier version of this class let subclasses override
{@code customInfo()}
+ * directly, which let several of them (accidentally) discard the
automatically captured query
+ * parameters instead of merging with them. Sealing the merge here makes that
specific class of bug
+ * impossible to reintroduce for any subclass of {@code Event} (this class
does not affect {@link
+ * BaseEvent#customInfo()} directly, which stays overridable, or {@link
PreEvent}, which extends
+ * {@code BaseEvent} rather than this class). Note for API consumers: this
class is annotated {@link
+ * DeveloperApi}, and sealing an existing non-final method is a source- and
binary-incompatible
+ * change for any external subclass that overrode {@code customInfo()}
directly.
*/
@DeveloperApi
public abstract class Event extends BaseEvent {
private final String remoteAddress;
+ private final Map<String, String> autoCustomInfo;
protected Event(String user, NameIdentifier identifier) {
super(user, identifier);
String addr = RequestContext.getRemoteAddress();
this.remoteAddress = StringUtils.isNoneBlank(addr) ? addr : "unknown";
+ this.autoCustomInfo = RequestContext.getRequestQueryParams();
}
/**
@@ -51,4 +76,34 @@ public abstract class Event extends BaseEvent {
public String remoteAddress() {
return remoteAddress;
}
+
+ /**
+ * Returns the current request's (raw) query parameters merged with this
event's own facts (from
+ * {@link #ownCustomInfo()}); the subclass's own values win on a key
collision. This method is
+ * {@code final} — override {@link #ownCustomInfo()} to contribute
subclass-specific facts.
+ *
+ * @return the merged custom info, or just the automatically captured query
parameters if the
+ * subclass contributes nothing of its own.
+ */
+ @Override
+ public final Map<String, String> customInfo() {
+ Map<String, String> own = ownCustomInfo();
+ if (own.isEmpty()) {
+ return autoCustomInfo;
+ }
+ Map<String, String> merged = new LinkedHashMap<>(autoCustomInfo);
+ merged.putAll(own);
+ return ImmutableMap.copyOf(merged);
+ }
+
+ /**
+ * Returns this event's own explicit facts, to be merged with the
automatically captured request
+ * query parameters by {@link #customInfo()}. Override this — not {@code
customInfo()} — to
+ * contribute subclass-specific audit facts.
+ *
+ * @return this event's own facts, or an empty map if it has none of its own.
+ */
+ protected Map<String, String> ownCustomInfo() {
+ return ImmutableMap.of();
+ }
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesEvent.java
index e3d181357d..dfdf992192 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesEvent.java
@@ -68,7 +68,7 @@ public class GrantGroupRolesEvent extends GroupEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesFailureEvent.java
index 23c0b4eb02..3006ece5df 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantGroupRolesFailureEvent.java
@@ -74,7 +74,7 @@ public class GrantGroupRolesFailureEvent extends
GroupFailureEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesEvent.java
index 705877b484..508c3455f9 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesEvent.java
@@ -69,7 +69,7 @@ public class GrantUserRolesEvent extends UserEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesFailureEvent.java
index 2cf48293e1..e88f2e3774 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/GrantUserRolesFailureEvent.java
@@ -68,7 +68,7 @@ public class GrantUserRolesFailureEvent extends
UserFailureEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesEvent.java
index 6a7af8106c..c69985bbd6 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesEvent.java
@@ -69,7 +69,7 @@ public class RevokeGroupRolesEvent extends GroupEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesFailureEvent.java
index 8d1d993fce..f05ac9688d 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeGroupRolesFailureEvent.java
@@ -74,7 +74,7 @@ public class RevokeGroupRolesFailureEvent extends
GroupFailureEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesEvent.java
index 76091ab704..172d6f6b5c 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesEvent.java
@@ -69,7 +69,7 @@ public class RevokeUserRolesEvent extends UserEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesFailureEvent.java
index 4f25e51395..4f9d34d5e1 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/RevokeUserRolesFailureEvent.java
@@ -69,7 +69,7 @@ public class RevokeUserRolesFailureEvent extends
UserFailureEvent {
/** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return RoleAssignmentAuditInfos.of(roles);
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/AuthorizationDenialFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/AuthorizationDenialFailureEvent.java
index ef9613ec20..60b9f3446a 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/AuthorizationDenialFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/AuthorizationDenialFailureEvent.java
@@ -108,6 +108,9 @@ public final class AuthorizationDenialFailureEvent extends
FailureEvent {
/**
* Returns authorization-specific context that distinguishes this event from
HTTP-level events.
+ * Merged automatically by {@link
org.apache.gravitino.listener.api.event.Event#customInfo()} with
+ * the request's automatically captured query parameters; these keys always
win on a collision
+ * with a query parameter of the same name.
*
* <ul>
* <li>{@code auth.method} — the intercepted Java method name
@@ -119,7 +122,7 @@ public final class AuthorizationDenialFailureEvent extends
FailureEvent {
* here to avoid duplication.
*/
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.of(
"auth.method", methodName,
"auth.expression", expression);
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
similarity index 56%
copy from
core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
copy to
core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
index 2556dba7bd..3347574c38 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
@@ -22,27 +22,28 @@ package org.apache.gravitino.listener.api.event.server;
import com.google.common.collect.ImmutableMap;
import java.util.Map;
import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.listener.api.event.Event;
import org.apache.gravitino.listener.api.event.EventSource;
-import org.apache.gravitino.listener.api.event.FailureEvent;
+import org.apache.gravitino.listener.api.event.OperationStatus;
import org.apache.gravitino.listener.api.event.OperationType;
/**
- * Represents an HTTP-level request failure that occurred before or outside
the operation dispatcher
- * layer — for example, a 401 authentication rejection, a 400 malformed JSON
body, or a 404 unknown
- * route. It is emitted by {@code HttpAuditFilter} when the HTTP response
status is 4xx or 5xx and
- * no operation-layer {@link FailureEvent} was already dispatched for the same
request.
+ * A fallback audit event for HTTP requests that complete with a 2xx/3xx
status but for which no
+ * operation-layer success {@link Event} was dispatched — for example, a REST
endpoint that has not
+ * (yet) been wired into the operation dispatcher/event system. It is emitted
by {@code
+ * HttpAuditFilter} so that such endpoints still produce an audit record
(method, URI, status, query
+ * parameters) instead of none at all.
*
- * <p>Unlike operation-layer failure events, this event carries no {@link
- * org.apache.gravitino.NameIdentifier} (the resource was never resolved) and
its {@link
- * #operationType()} is always {@link OperationType#UNKNOWN}. HTTP-specific
context (method, URI,
- * status code) is available via {@link #customInfo()}.
+ * <p>Unlike operation-layer success events, this event carries no {@link
+ * org.apache.gravitino.NameIdentifier} (the resource was never resolved as a
structured entity) and
+ * its {@link #operationType()} is always {@link OperationType#UNKNOWN}. Once
an endpoint gains its
+ * own structured operation-layer event, that event suppresses this fallback
for the same request —
+ * see {@code RequestContext#markOperationSuccessFired()}.
*
- * <p>This event extends {@link FailureEvent} so it is routed through {@code
- * EventBus.dispatchFailureEvent()}, which swallows listener exceptions and
prevents audit failures
- * from masking the original HTTP error.
+ * <p>This is the success-path counterpart of {@link HttpRequestFailureEvent}.
*/
@DeveloperApi
-public final class HttpRequestFailureEvent extends FailureEvent {
+public final class HttpRequestEvent extends Event {
private final String explicitRemoteAddress;
private final String httpMethod;
@@ -51,25 +52,28 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
private final EventSource explicitEventSource;
/**
- * Constructs an {@code HttpRequestFailureEvent}.
+ * Constructs an {@code HttpRequestEvent}.
*
* @param user the authenticated user, or {@code "unknown"} if
authentication had not completed.
* @param remoteAddress the client IP resolved by the filter
(X-Forwarded-For or raw socket
- * address). Stored explicitly because Iceberg and Lance servers do not
install {@code
- * RequestContextFilter}, so {@code RequestContext.getRemoteAddress()}
may be unset.
+ * address). Stored explicitly, rather than relying on the base {@link
Event} behaviour of
+ * reading {@code RequestContext.getRemoteAddress()} at construction
time, so this event still
+ * carries a correct address even on a server that does not install
{@code
+ * RequestContextFilter} at all (all servers in this codebase currently
do, but this keeps the
+ * event resilient to one that doesn't).
* @param httpMethod the HTTP method (e.g. {@code "GET"}, {@code "POST"}).
- * @param requestUri the request URI path (e.g. {@code
"/api/metalakes/m1/catalogs"}).
- * @param statusCode the HTTP response status code (e.g. {@code 401}, {@code
404}).
+ * @param requestUri the request URI path (e.g. {@code "/search/query"}).
+ * @param statusCode the HTTP response status code (e.g. {@code 200}).
* @param eventSource identifies which server produced the event.
*/
- public HttpRequestFailureEvent(
+ public HttpRequestEvent(
String user,
String remoteAddress,
String httpMethod,
String requestUri,
int statusCode,
EventSource eventSource) {
- super(user, null, new HttpRequestException(httpMethod, requestUri,
statusCode));
+ super(user, null);
this.explicitRemoteAddress = remoteAddress != null ? remoteAddress :
"unknown";
this.httpMethod = httpMethod;
this.requestUri = requestUri;
@@ -83,11 +87,17 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
return OperationType.UNKNOWN;
}
+ /** {@inheritDoc} */
+ @Override
+ public OperationStatus operationStatus() {
+ return OperationStatus.SUCCESS;
+ }
+
/**
- * Returns the explicitly-resolved client remote address. This overrides the
base {@link
- * org.apache.gravitino.listener.api.event.Event} behaviour (which reads
from {@link
- * org.apache.gravitino.utils.RequestContext}) so that events emitted by
servers that do not
- * install {@code RequestContextFilter} still carry a correct address.
+ * Returns the explicitly-resolved client remote address supplied at
construction time, rather
+ * than the base {@link Event} behaviour of reading it from {@link
+ * org.apache.gravitino.utils.RequestContext}. See the constructor's {@code
remoteAddress}
+ * parameter doc for why.
*/
@Override
public String remoteAddress() {
@@ -101,7 +111,9 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
}
/**
- * Returns HTTP-specific context that distinguishes this event from
operation-layer events.
+ * Returns HTTP-specific context that distinguishes this event from
operation-layer events. Merged
+ * automatically by {@link Event#customInfo()} with the request's
automatically captured query
+ * parameters; these keys always win on a collision with a query parameter
of the same name.
*
* <ul>
* <li>{@code http.method} — the HTTP verb
@@ -110,7 +122,7 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
* </ul>
*/
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.of(
"http.method", httpMethod,
"http.uri", requestUri,
@@ -131,21 +143,4 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
public int statusCode() {
return statusCode;
}
-
- /**
- * A lightweight synthetic exception used as the {@link
FailureEvent#exception()} carrier for
- * {@link HttpRequestFailureEvent}. It encodes the HTTP method, URI, and
status so that log
- * formatters that render the exception message get readable output without
needing to inspect
- * {@link HttpRequestFailureEvent#customInfo()} separately.
- */
- public static final class HttpRequestException extends RuntimeException {
-
- private HttpRequestException(String httpMethod, String requestUri, int
statusCode) {
- super(
- String.format("HTTP request failed: %s %s -> %d", httpMethod,
requestUri, statusCode),
- null,
- true,
- false /* suppress stack trace — this is a synthetic carrier, not a
real exception */);
- }
- }
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
index 2556dba7bd..22485e12f9 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
@@ -55,8 +55,12 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
*
* @param user the authenticated user, or {@code "unknown"} if
authentication had not completed.
* @param remoteAddress the client IP resolved by the filter
(X-Forwarded-For or raw socket
- * address). Stored explicitly because Iceberg and Lance servers do not
install {@code
- * RequestContextFilter}, so {@code RequestContext.getRemoteAddress()}
may be unset.
+ * address). Stored explicitly, rather than relying on the base {@link
+ * org.apache.gravitino.listener.api.event.Event} behaviour of reading
{@code
+ * RequestContext.getRemoteAddress()} at construction time, so this
event still carries a
+ * correct address even on a server that does not install {@code
RequestContextFilter} at all
+ * (all servers in this codebase currently do, but this keeps the event
resilient to one that
+ * doesn't).
* @param httpMethod the HTTP method (e.g. {@code "GET"}, {@code "POST"}).
* @param requestUri the request URI path (e.g. {@code
"/api/metalakes/m1/catalogs"}).
* @param statusCode the HTTP response status code (e.g. {@code 401}, {@code
404}).
@@ -84,10 +88,10 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
}
/**
- * Returns the explicitly-resolved client remote address. This overrides the
base {@link
- * org.apache.gravitino.listener.api.event.Event} behaviour (which reads
from {@link
- * org.apache.gravitino.utils.RequestContext}) so that events emitted by
servers that do not
- * install {@code RequestContextFilter} still carry a correct address.
+ * Returns the explicitly-resolved client remote address supplied at
construction time, rather
+ * than the base {@link org.apache.gravitino.listener.api.event.Event}
behaviour of reading it
+ * from {@link org.apache.gravitino.utils.RequestContext}. See the
constructor's {@code
+ * remoteAddress} parameter doc for why.
*/
@Override
public String remoteAddress() {
@@ -101,7 +105,10 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
}
/**
- * Returns HTTP-specific context that distinguishes this event from
operation-layer events.
+ * Returns HTTP-specific context that distinguishes this event from
operation-layer events. Merged
+ * automatically by {@link
org.apache.gravitino.listener.api.event.Event#customInfo()} with the
+ * request's automatically captured query parameters; these keys always win
on a collision with a
+ * query parameter of the same name.
*
* <ul>
* <li>{@code http.method} — the HTTP verb
@@ -110,7 +117,7 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
* </ul>
*/
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.of(
"http.method", httpMethod,
"http.uri", requestUri,
diff --git a/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
index bfdbb9c60e..01b6e4f607 100644
--- a/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
+++ b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
@@ -19,20 +19,39 @@
package org.apache.gravitino.utils;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+
/**
* Holds per-request context data in a {@link ThreadLocal} so that event
classes constructed on the
* servlet thread can capture it without carrying a servlet dependency.
*
- * <p>Currently tracks two pieces of state:
+ * <p>Currently tracks four pieces of state:
*
* <ul>
* <li><b>remoteAddress</b> — the client IP resolved from {@code
X-Forwarded-For} or {@link
* javax.servlet.http.HttpServletRequest#getRemoteAddr()}.
- * <li><b>operationFailureFired</b> — set to {@code true} by {@link
- * org.apache.gravitino.listener.EventBus} when an operation-layer {@link
+ * <li><b>operationOutcome</b> — set by {@link
org.apache.gravitino.listener.EventBus} when an
+ * operation-layer {@link org.apache.gravitino.listener.api.event.Event}
or {@link
* org.apache.gravitino.listener.api.event.FailureEvent} is dispatched,
so that {@code
- * HttpAuditFilter} can skip emitting a redundant HTTP-level failure
event for the same
- * request.
+ * HttpAuditFilter} can skip emitting a redundant HTTP-level fallback
event for the same
+ * request. Exposed as two independent-looking flags ({@code
operationFailureFired}/{@code
+ * operationSuccessFired}) for callers, but backed by a single tri-state
value — normally a
+ * request's operation layer dispatches at most one terminal event
(success or failure), never
+ * both, so there is nothing to track independently. That is an
expectation on callers, not
+ * something this class enforces, so failure is treated as sticky: {@link
+ * #markOperationSuccessFired()} is a no-op once a failure has been
recorded on this thread,
+ * so a later success dispatch can never erase it and cause a 4xx/5xx
response to be
+ * double-logged as if no operation-layer failure had fired. {@code
HttpAuditFilter} still
+ * produces both a success and a failure entry for the "operation
succeeded but HTTP delivery
+ * failed" case, because that failure entry comes from the filter's own
HTTP-status check, not
+ * from a second operation-layer flag.
+ * <li><b>auditExtras</b> — optional {@code customInfo} facts stashed by an
inner dispatcher and
+ * consumed by the outer event dispatcher so one operation still
produces one event.
+ * <li><b>requestQueryParams</b> — the current request's query parameters,
captured raw (not
+ * redacted — see {@code AuditLogRedactor}) once per request by {@code
RequestContextFilter}
+ * and read (non-destructively) by every {@link
org.apache.gravitino.listener.api.event.Event}
+ * constructor.
* </ul>
*
* <p><b>Threading contract:</b> values must be set and cleared on the same
(servlet) thread. Event
@@ -41,8 +60,16 @@ package org.apache.gravitino.utils;
*/
public class RequestContext {
+ /** The operation-layer event outcome recorded for the current request, if
any. */
+ private enum OperationOutcome {
+ SUCCESS,
+ FAILURE
+ }
+
private static final ThreadLocal<String> REMOTE_ADDRESS = new
ThreadLocal<>();
- private static final ThreadLocal<Boolean> OPERATION_FAILURE_FIRED = new
ThreadLocal<>();
+ private static final ThreadLocal<OperationOutcome> OPERATION_OUTCOME = new
ThreadLocal<>();
+ private static final ThreadLocal<Map<String, String>> AUDIT_EXTRAS = new
ThreadLocal<>();
+ private static final ThreadLocal<Map<String, String>> REQUEST_QUERY_PARAMS =
new ThreadLocal<>();
private RequestContext() {}
@@ -71,7 +98,7 @@ public class RequestContext {
* {@code HttpRequestFailureEvent}.
*/
public static void markOperationFailureFired() {
- OPERATION_FAILURE_FIRED.set(Boolean.TRUE);
+ OPERATION_OUTCOME.set(OperationOutcome.FAILURE);
}
/**
@@ -81,16 +108,118 @@ public class RequestContext {
* @return {@code true} if the flag is set, {@code false} otherwise.
*/
public static boolean isOperationFailureFired() {
- return Boolean.TRUE.equals(OPERATION_FAILURE_FIRED.get());
+ return OPERATION_OUTCOME.get() == OperationOutcome.FAILURE;
}
/**
- * Clears the operation-failure flag for the current request thread. Must be
called in a {@code
- * finally} block by {@code HttpAuditFilter} at the end of each request to
prevent stale values
- * from leaking to the next request on the same Jetty thread.
+ * Clears the recorded operation outcome for the current request thread.
Must be called in a
+ * {@code finally} block by {@code HttpAuditFilter} at the end of each
request to prevent stale
+ * values from leaking to the next request on the same Jetty thread.
+ *
+ * <p>Named after the failure flag for symmetry with {@link
#resetOperationSuccessFired()}, but
+ * clears the single underlying outcome either way — see the class-level doc.
*/
public static void resetOperationFailureFired() {
- OPERATION_FAILURE_FIRED.remove();
+ OPERATION_OUTCOME.remove();
+ }
+
+ /**
+ * Marks that an operation-layer success {@code Event} has been dispatched
for the current
+ * request. Called by {@code EventBus.dispatchPostEvent()} for every success
event that is not
+ * itself an {@code HttpRequestEvent}.
+ *
+ * <p>A no-op if a failure was already recorded on this thread: the failure
outcome is monotonic
+ * (mirroring the pre-existing {@code operationFailureFired} boolean this
replaced), so a success
+ * event dispatched after a failure event for the same request — which
should not normally happen,
+ * but is not structurally prevented — can never erase it and cause {@code
HttpAuditFilter} to
+ * double-log a 4xx/5xx response as if no operation-layer failure had fired.
+ */
+ public static void markOperationSuccessFired() {
+ if (OPERATION_OUTCOME.get() != OperationOutcome.FAILURE) {
+ OPERATION_OUTCOME.set(OperationOutcome.SUCCESS);
+ }
+ }
+
+ /**
+ * Returns {@code true} if an operation-layer success event has already been
dispatched on this
+ * thread for the current request.
+ *
+ * @return {@code true} if the flag is set, {@code false} otherwise.
+ */
+ public static boolean isOperationSuccessFired() {
+ return OPERATION_OUTCOME.get() == OperationOutcome.SUCCESS;
+ }
+
+ /**
+ * Clears the recorded operation outcome for the current request thread.
Must be called in a
+ * {@code finally} block by {@code HttpAuditFilter} at the end of each
request to prevent stale
+ * values from leaking to the next request on the same Jetty thread.
+ *
+ * <p>Named after the success flag for symmetry with {@link
#resetOperationFailureFired()}, but
+ * clears the single underlying outcome either way — see the class-level doc.
+ */
+ public static void resetOperationSuccessFired() {
+ OPERATION_OUTCOME.remove();
+ }
+
+ /**
+ * Stashes optional audit extras for the current request thread. An inner
dispatcher calls this
+ * before returning or throwing so the outer event dispatcher can attach the
extras to the
+ * existing table event.
+ *
+ * <p>A {@code null} or empty map clears any previously stashed extras.
+ *
+ * @param extras optional {@code customInfo} facts, or {@code null} to clear
+ */
+ public static void setAuditExtras(Map<String, String> extras) {
+ if (extras == null || extras.isEmpty()) {
+ AUDIT_EXTRAS.remove();
+ return;
+ }
+ AUDIT_EXTRAS.set(ImmutableMap.copyOf(extras));
+ }
+
+ /**
+ * Returns and clears audit extras stashed on this thread. The outer event
dispatcher calls this
+ * when constructing the terminal table event.
+ *
+ * @return an immutable extras map, or an empty map when none were stashed
+ */
+ public static Map<String, String> takeAuditExtras() {
+ Map<String, String> extras = AUDIT_EXTRAS.get();
+ AUDIT_EXTRAS.remove();
+ return extras == null ? ImmutableMap.of() : extras;
+ }
+
+ /**
+ * Stashes the current request's raw (not redacted) query parameters for the
current request
+ * thread. Called once per request by {@code RequestContextFilter}.
Redaction happens later, at
+ * audit-log format time, applied uniformly to the full merged {@code
customInfo()} map — see
+ * {@code AuditLogRedactor}'s class doc for why.
+ *
+ * <p>Unlike {@link #setAuditExtras(Map)}/{@link #takeAuditExtras()}, this
is read
+ * non-destructively: every {@link
org.apache.gravitino.listener.api.event.Event} constructed on
+ * this thread during the request reads the same snapshot.
+ *
+ * @param params the raw query parameters, or {@code null} to clear
+ */
+ public static void setRequestQueryParams(Map<String, String> params) {
+ if (params == null || params.isEmpty()) {
+ REQUEST_QUERY_PARAMS.remove();
+ return;
+ }
+ REQUEST_QUERY_PARAMS.set(ImmutableMap.copyOf(params));
+ }
+
+ /**
+ * Returns the current request's raw (not redacted) query parameters
previously set on this
+ * thread, without clearing them.
+ *
+ * @return an immutable query-parameter map, or an empty map when none were
set
+ */
+ public static Map<String, String> getRequestQueryParams() {
+ Map<String, String> params = REQUEST_QUERY_PARAMS.get();
+ return params == null ? ImmutableMap.of() : params;
}
/**
@@ -99,6 +228,8 @@ public class RequestContext {
*/
public static void clear() {
REMOTE_ADDRESS.remove();
- OPERATION_FAILURE_FIRED.remove();
+ OPERATION_OUTCOME.remove();
+ AUDIT_EXTRAS.remove();
+ REQUEST_QUERY_PARAMS.remove();
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/audit/TestAuditLogRedactor.java
b/core/src/test/java/org/apache/gravitino/audit/TestAuditLogRedactor.java
new file mode 100644
index 0000000000..99623a68bf
--- /dev/null
+++ b/core/src/test/java/org/apache/gravitino/audit/TestAuditLogRedactor.java
@@ -0,0 +1,159 @@
+/*
+ * 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.audit;
+
+import java.util.Collections;
+import java.util.Map;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestAuditLogRedactor {
+
+ @Test
+ public void testNullCustomInfoReturnsEmptyMap() {
+ Assertions.assertTrue(AuditLogRedactor.redactCustomInfo(null).isEmpty());
+ }
+
+ @Test
+ public void testNonSensitiveKeyKeptAsIs() {
+ Map<String, String> info = Collections.singletonMap("details", "true");
+ Assertions.assertEquals("true",
AuditLogRedactor.redactCustomInfo(info).get("details"));
+ }
+
+ // ─── exact-match (MASKED_CUSTOM_INFO_KEYS)
───────────────────────────────────
+
+ @Test
+ public void testExactMatchKeyIsRedacted() {
+ Map<String, String> info = Collections.singletonMap("authorization",
"Bearer secret-token");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("authorization"));
+ }
+
+ @Test
+ public void testExactMatchIsCaseInsensitive() {
+ Map<String, String> info = Collections.singletonMap("AUTHORIZATION",
"Bearer secret-token");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("AUTHORIZATION"));
+ }
+
+ /**
+ * "cookie" is a known internal key with no sensitive substring in its name
(it doesn't contain
+ * "password", "token", etc.), so only the exact-match list catches it.
+ */
+ @Test
+ public void testExactMatchCatchesKeyWithNoSensitiveSubstring() {
+ Map<String, String> info = Collections.singletonMap("cookie",
"session=abc123");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
AuditLogRedactor.redactCustomInfo(info).get("cookie"));
+ }
+
+ // ─── substring match (SENSITIVE_KEY_SUBSTRINGS)
──────────────────────────────
+
+ /**
+ * Request query-parameter names become customInfo entries via {@link
+ * org.apache.gravitino.listener.api.event.Event#customInfo()} with whatever
naming convention the
+ * caller chose, so redaction here must also catch names the exact-match
list was never designed
+ * for (e.g. "accessToken" isn't in MASKED_CUSTOM_INFO_KEYS, but it contains
"token").
+ */
+ @Test
+ public void testSubstringMatchCatchesCallerSuppliedKeyNames() {
+ Map<String, String> info = Collections.singletonMap("accessToken",
"abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("accessToken"));
+ }
+
+ @Test
+ public void testSubstringMatchCatchesHyphenatedKeyName() {
+ Map<String, String> info =
Collections.singletonMap("s3-secret-access-key", "abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("s3-secret-access-key"));
+ }
+
+ @Test
+ public void testSubstringMatchIsCaseInsensitive() {
+ Map<String, String> info = Collections.singletonMap("PASSWORD", "hunter2");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
AuditLogRedactor.redactCustomInfo(info).get("PASSWORD"));
+ }
+
+ /**
+ * The substring list has multi-word entries ("apikey", "accesskey",
"privatekey"), but
+ * caller-supplied key names almost always separate those words with a
hyphen or underscore
+ * ("api-key", "x-api-key", "access-key-id", "private_key"). Pins that the
match is done against a
+ * separator-stripped form of the key, not just a lowercased one, so these
realistic spellings are
+ * actually caught instead of silently passing through in cleartext.
+ */
+ @Test
+ public void testSubstringMatchIgnoresSeparators() {
+ Map<String, String> info = Collections.singletonMap("api-key", "abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
AuditLogRedactor.redactCustomInfo(info).get("api-key"));
+
+ Map<String, String> info2 = Collections.singletonMap("x-api-key",
"abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
AuditLogRedactor.redactCustomInfo(info2).get("x-api-key"));
+
+ Map<String, String> info3 = Collections.singletonMap("access-key-id",
"abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info3).get("access-key-id"));
+
+ Map<String, String> info4 = Collections.singletonMap("private_key",
"abcd1234");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info4).get("private_key"));
+ }
+
+ @Test
+ public void testKeyIsPreservedEvenWhenValueIsRedacted() {
+ Map<String, String> info = Collections.singletonMap("token",
"secretvalue");
+
Assertions.assertTrue(AuditLogRedactor.redactCustomInfo(info).containsKey("token"));
+ }
+
+ // ─── NEVER_SENSITIVE_KEYS (codebase-chosen keys that coincidentally
contain a substring) ────
+
+ /**
+ * "auth.method"/"auth.expression" are fixed key literals
AuthorizationDenialFailureEvent always
+ * uses — not caller-supplied data — but they contain "auth", which is in
+ * SENSITIVE_KEY_SUBSTRINGS. Without an explicit exemption, the substring
check (added to also
+ * cover caller-supplied query-parameter names) would redact genuinely
useful audit data: which
+ * method was denied and what expression was evaluated, exactly what this
event exists to record.
+ */
+ @Test
+ public void testCodebaseChosenAuthKeysAreNeverRedacted() {
+ Map<String, String> info = Collections.singletonMap("auth.method",
"loadTable");
+ Assertions.assertEquals(
+ "loadTable",
AuditLogRedactor.redactCustomInfo(info).get("auth.method"));
+
+ Map<String, String> info2 = Collections.singletonMap("auth.expression",
"TABLE:LOAD");
+ Assertions.assertEquals(
+ "TABLE:LOAD",
AuditLogRedactor.redactCustomInfo(info2).get("auth.expression"));
+ }
+
+ @Test
+ public void testCodebaseChosenHttpKeysAreNeverRedacted() {
+ Map<String, String> info = Collections.singletonMap("http.method", "GET");
+ Assertions.assertEquals("GET",
AuditLogRedactor.redactCustomInfo(info).get("http.method"));
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/audit/TestJsonAuditFormatter.java
b/core/src/test/java/org/apache/gravitino/audit/TestJsonAuditFormatter.java
index a22e953dfd..6178efaac2 100644
--- a/core/src/test/java/org/apache/gravitino/audit/TestJsonAuditFormatter.java
+++ b/core/src/test/java/org/apache/gravitino/audit/TestJsonAuditFormatter.java
@@ -138,7 +138,7 @@ public class TestJsonAuditFormatter {
static class StubEventWithSensitiveCustomInfo extends StubEvent {
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.<String, String>builder()
.put("Authorization", "Bearer token")
.put("cookie", "a=b")
@@ -161,7 +161,7 @@ public class TestJsonAuditFormatter {
}
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.of("env", "prod");
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
b/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
index 8c68859ef1..7c2d89d420 100644
--- a/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
+++ b/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
@@ -35,11 +35,18 @@ import
org.apache.gravitino.listener.api.event.OperationStatus;
import org.apache.gravitino.listener.api.event.OperationType;
import
org.apache.gravitino.listener.api.event.server.AuthorizationDenialFailureEvent;
import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent;
+import org.apache.gravitino.utils.RequestContext;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
public class TestSimpleAuditLogV2 {
+ @AfterEach
+ void cleanup() {
+ RequestContext.clear();
+ }
+
@Test
public void testTimestampHasMillisecondPrecision() {
SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEvent());
@@ -70,6 +77,28 @@ public class TestSimpleAuditLogV2 {
Assertions.assertEquals("", fields[7], "Last field should be empty when
customInfo is absent");
}
+ /**
+ * End-to-end proof that redaction happens exactly once, at format time: an
auto-captured query
+ * parameter is stashed raw (as RequestContextFilter now does), reaches
Event.customInfo()
+ * unredacted, and is only masked here, when SimpleAuditLogV2 renders it via
AuditLogRedactor.
+ */
+ @Test
+ public void testAutoCapturedQueryParamIsRedactedAtFormatTime() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("token",
"raw-secret-value"));
+ SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEvent());
+
+ // Not redacted yet on the event itself — redaction is the formatter's
job, not capture's.
+ Assertions.assertEquals("raw-secret-value", log.customInfo().get("token"));
+
+ String output = log.toString();
+ String[] fields = output.split("\t", -1);
+ Assertions.assertTrue(
+ fields[7].contains("***"), "Rendered output must redact the token
value: " + fields[7]);
+ Assertions.assertFalse(
+ fields[7].contains("raw-secret-value"),
+ "Rendered output must not contain the raw token value: " + fields[7]);
+ }
+
@Test
public void testRoleAssignmentIncludesRoleNames() {
GrantUserRolesFailureEvent event =
@@ -315,7 +344,7 @@ public class TestSimpleAuditLogV2 {
static class StubEventWithCustomInfo extends StubEvent {
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return ImmutableMap.of("k1", "v1");
}
}
diff --git a/core/src/test/java/org/apache/gravitino/listener/TestEventBus.java
b/core/src/test/java/org/apache/gravitino/listener/TestEventBus.java
index a1b5c5ae0f..5fab308bc7 100644
--- a/core/src/test/java/org/apache/gravitino/listener/TestEventBus.java
+++ b/core/src/test/java/org/apache/gravitino/listener/TestEventBus.java
@@ -29,6 +29,7 @@ import org.apache.gravitino.listener.api.event.EventSource;
import org.apache.gravitino.listener.api.event.FailureEvent;
import org.apache.gravitino.listener.api.event.OperationStatus;
import org.apache.gravitino.listener.api.event.PreEvent;
+import org.apache.gravitino.listener.api.event.server.HttpRequestEvent;
import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent;
import org.apache.gravitino.utils.RequestContext;
import org.junit.jupiter.api.AfterEach;
@@ -101,6 +102,7 @@ public class TestEventBus {
@AfterEach
void clearRequestContext() {
RequestContext.resetOperationFailureFired();
+ RequestContext.resetOperationSuccessFired();
RequestContext.clear();
}
@@ -140,6 +142,41 @@ public class TestEventBus {
+ "it is the HTTP-layer event and must not suppress itself");
}
+ // ─── operationSuccessFired flag marking
──────────────────────────────────────
+
+ @Test
+ void testOperationLayerSuccessEventSetsOperationSuccessFiredFlag() {
+ EventBus eventBus = new EventBus(Collections.emptyList());
+ Event successEvent =
+ new Event("user", NameIdentifier.of("test")) {
+ @Override
+ public OperationStatus operationStatus() {
+ return OperationStatus.SUCCESS;
+ }
+ };
+
+ Assertions.assertFalse(RequestContext.isOperationSuccessFired(), "Flag
must start as false");
+ eventBus.dispatchEvent(successEvent);
+ Assertions.assertTrue(
+ RequestContext.isOperationSuccessFired(),
+ "Flag must be set after dispatching an operation-layer success event");
+ }
+
+ @Test
+ void testHttpRequestEventDoesNotSetOperationSuccessFiredFlag() {
+ EventBus eventBus = new EventBus(Collections.emptyList());
+ HttpRequestEvent httpEvent =
+ new HttpRequestEvent(
+ "alice", "1.2.3.4", "GET", "/api/metalakes", 200,
EventSource.GRAVITINO_SERVER);
+
+ eventBus.dispatchEvent(httpEvent);
+
+ Assertions.assertFalse(
+ RequestContext.isOperationSuccessFired(),
+ "Flag must NOT be set when dispatching HttpRequestEvent — "
+ + "it is the HTTP-layer fallback event and must not suppress
itself");
+ }
+
static class ThrowingEventListener implements EventListenerPlugin {
private final RuntimeException exceptionToThrow;
diff --git
a/core/src/test/java/org/apache/gravitino/listener/api/event/TestEvent.java
b/core/src/test/java/org/apache/gravitino/listener/api/event/TestEvent.java
new file mode 100644
index 0000000000..b17931c0f7
--- /dev/null
+++ b/core/src/test/java/org/apache/gravitino/listener/api/event/TestEvent.java
@@ -0,0 +1,102 @@
+/*
+ * 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.listener.api.event;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.utils.RequestContext;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Exercises {@link Event}'s automatic capture of the request's query
parameters at construction
+ * time. Uses {@link ListCatalogEvent} as a representative concrete subclass
that does not override
+ * {@code customInfo()} itself — any of the ~300 event classes without their
own {@code customInfo}
+ * go through the exact same {@link Event} constructor, so this one test
stands in for all of them.
+ */
+public class TestEvent {
+
+ @AfterEach
+ void cleanup() {
+ RequestContext.clear();
+ }
+
+ @Test
+ void testCustomInfoCapturesQueryParamsAtConstructionTime() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("details", "true"));
+ Event event = new ListCatalogEvent("user", Namespace.of("metalake"), 3);
+ Assertions.assertEquals("true", event.customInfo().get("details"));
+ }
+
+ @Test
+ void testCustomInfoIsEmptyWhenNoQueryParamsCaptured() {
+ Event event = new ListCatalogEvent("user", Namespace.of("metalake"), 3);
+ Assertions.assertTrue(event.customInfo().isEmpty());
+ }
+
+ @Test
+ void testCustomInfoSnapshotIsFixedAtConstructionNotAtReadTime() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("details", "true"));
+ Event event = new ListCatalogEvent("user", Namespace.of("metalake"), 3);
+ RequestContext.setRequestQueryParams(ImmutableMap.of("details", "false"));
+ Assertions.assertEquals(
+ "true",
+ event.customInfo().get("details"),
+ "event must keep the snapshot taken at construction time, not a later
thread-local value");
+ }
+
+ /**
+ * customInfo() is final in Event precisely so this merge direction can't be
gotten wrong by a
+ * subclass: a subclass's own fact must win over an automatically captured
query parameter of the
+ * same name, otherwise a caller could overwrite an audit-critical field
(e.g. a query parameter
+ * literally named "http.status") just by naming a query parameter after it.
+ */
+ @Test
+ void testOwnCustomInfoOverridesAutomaticQueryParamOnKeyCollision() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("outcome",
"attacker-supplied"));
+ Event event =
+ new Event("user", NameIdentifier.of("metalake")) {
+ @Override
+ protected Map<String, String> ownCustomInfo() {
+ return ImmutableMap.of("outcome", "real-value");
+ }
+ };
+
+ Assertions.assertEquals("real-value", event.customInfo().get("outcome"));
+ }
+
+ @Test
+ void testOwnCustomInfoIsMergedAlongsideAutomaticQueryParams() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("details", "true"));
+ Event event =
+ new Event("user", NameIdentifier.of("metalake")) {
+ @Override
+ protected Map<String, String> ownCustomInfo() {
+ return ImmutableMap.of("http.status", "200");
+ }
+ };
+
+ Assertions.assertEquals("true", event.customInfo().get("details"));
+ Assertions.assertEquals("200", event.customInfo().get("http.status"));
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/listener/api/event/server/TestAuthorizationDenialFailureEvent.java
b/core/src/test/java/org/apache/gravitino/listener/api/event/server/TestAuthorizationDenialFailureEvent.java
index b91696fc4d..d03d470f9d 100644
---
a/core/src/test/java/org/apache/gravitino/listener/api/event/server/TestAuthorizationDenialFailureEvent.java
+++
b/core/src/test/java/org/apache/gravitino/listener/api/event/server/TestAuthorizationDenialFailureEvent.java
@@ -19,10 +19,13 @@
package org.apache.gravitino.listener.api.event.server;
+import com.google.common.collect.ImmutableMap;
import java.util.Map;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.listener.api.event.EventSource;
import org.apache.gravitino.listener.api.event.OperationType;
+import org.apache.gravitino.utils.RequestContext;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -31,6 +34,11 @@ public class TestAuthorizationDenialFailureEvent {
private static final NameIdentifier TABLE_IDENT =
NameIdentifier.of("metalake1", "catalog1", "schema1", "table1");
+ @AfterEach
+ void cleanup() {
+ RequestContext.clear();
+ }
+
@Test
public void testFieldsStoredCorrectly() {
AuthorizationDenialFailureEvent event =
@@ -70,6 +78,23 @@ public class TestAuthorizationDenialFailureEvent {
Assertions.assertFalse(info.containsKey("auth.resource"));
}
+ /**
+ * Pins the fix for a bug where customInfo() overrode and discarded Event's
automatically captured
+ * request query parameters, exactly like the bug already fixed in
TableEvent,
+ * HttpRequestFailureEvent, and Iceberg's event classes.
+ */
+ @Test
+ public void testCustomInfoMergesAutomaticQueryParamsWithAuthFields() {
+ RequestContext.setRequestQueryParams(ImmutableMap.of("details", "true"));
+ AuthorizationDenialFailureEvent event =
+ new AuthorizationDenialFailureEvent("alice", TABLE_IDENT, "loadTable",
"TABLE:LOAD");
+
+ Map<String, String> info = event.customInfo();
+ Assertions.assertEquals("true", info.get("details"));
+ Assertions.assertEquals("loadTable", info.get("auth.method"));
+ Assertions.assertEquals("TABLE:LOAD", info.get("auth.expression"));
+ }
+
@Test
public void testNullExpressionNormalisedToEmptyString() {
AuthorizationDenialFailureEvent event =
diff --git
a/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
b/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
index 5c70284446..253280a360 100644
--- a/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
+++ b/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
@@ -19,6 +19,8 @@
package org.apache.gravitino.utils;
+import java.util.Collections;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
@@ -67,4 +69,149 @@ public class TestRequestContext {
Assertions.assertEquals(
"main-thread-ip", RequestContext.getRemoteAddress(), "Main thread
value unchanged");
}
+
+ /**
+ * Callers read the stash unconditionally on every table operation, so the
common case is that
+ * nothing was stashed. Pins that this returns an empty map rather than
{@code null}, which keeps
+ * the dispatcher free of a null check on the hot path.
+ */
+ @Test
+ public void testTakeAuditExtrasReturnsEmptyWhenUnset() {
+ Assertions.assertTrue(RequestContext.takeAuditExtras().isEmpty());
+ }
+
+ /**
+ * The stash is a hand-off between two dispatcher layers, not a
request-scoped register: exactly
+ * one reader is expected to consume each set. Pins the take-and-clear
semantics that the
+ * dispatcher relies on to avoid attributing a fact to a later operation.
+ */
+ @Test
+ public void testSetAndTakeAuditExtras() {
+ RequestContext.setAuditExtras(Collections.singletonMap("audit.reason",
"policy-applied"));
+ Assertions.assertEquals("policy-applied",
RequestContext.takeAuditExtras().get("audit.reason"));
+ Assertions.assertTrue(RequestContext.takeAuditExtras().isEmpty(), "take
must clear extras");
+ }
+
+ /**
+ * A contributor that decides it has nothing to report will naturally pass
an empty or {@code
+ * null} map. Pins that this retracts an earlier stash instead of being a
no-op, so a stale fact
+ * from an earlier decision cannot be attached to the event.
+ */
+ @Test
+ public void testEmptyOrNullAuditExtrasClearsStash() {
+ RequestContext.setAuditExtras(Collections.singletonMap("k", "v"));
+ RequestContext.setAuditExtras(Collections.emptyMap());
+ Assertions.assertTrue(RequestContext.takeAuditExtras().isEmpty());
+
+ RequestContext.setAuditExtras(Collections.singletonMap("k", "v"));
+ RequestContext.setAuditExtras(null);
+ Assertions.assertTrue(RequestContext.takeAuditExtras().isEmpty());
+ }
+
+ /**
+ * Extras are a third {@link ThreadLocal} on a pooled servlet thread, so the
request-teardown
+ * sweep has to know about them. Pins that {@code clear()} was extended
alongside the new state
+ * and cannot silently leave it bound to the thread.
+ */
+ @Test
+ public void testClearRemovesAuditExtras() {
+ RequestContext.setAuditExtras(Collections.singletonMap("k", "v"));
+ RequestContext.clear();
+ Assertions.assertTrue(RequestContext.takeAuditExtras().isEmpty());
+ }
+
+ /**
+ * The stash is only safe because a contributor and the dispatcher share one
servlet thread. Pins
+ * that a fact set on one thread is invisible to another, which is the
assumption that makes a
+ * {@link ThreadLocal} acceptable here in place of a threaded-through
parameter.
+ */
+ @Test
+ public void testAuditExtrasAreThreadConfined() throws InterruptedException {
+ RequestContext.setAuditExtras(Collections.singletonMap("audit.reason",
"policy-applied"));
+ AtomicReference<Map<String, String>> childValue = new AtomicReference<>();
+
+ Thread child = new Thread(() ->
childValue.set(RequestContext.takeAuditExtras()));
+ child.start();
+ child.join();
+
+ Assertions.assertTrue(childValue.get().isEmpty(), "Child thread must not
see stashed extras");
+ Assertions.assertEquals(
+ "policy-applied",
+ RequestContext.takeAuditExtras().get("audit.reason"),
+ "Child thread must not consume the parent's stash");
+ }
+
+ /**
+ * Unlike audit extras, the query-param snapshot is read by every {@code
Event} constructed during
+ * the request, not consumed once. Pins that reading it does not clear it.
+ */
+ @Test
+ public void testGetRequestQueryParamsDoesNotClear() {
+ RequestContext.setRequestQueryParams(Collections.singletonMap("details",
"true"));
+ Assertions.assertEquals("true",
RequestContext.getRequestQueryParams().get("details"));
+ Assertions.assertEquals(
+ "true",
+ RequestContext.getRequestQueryParams().get("details"),
+ "a second read must still see the value");
+ }
+
+ @Test
+ public void testGetRequestQueryParamsReturnsEmptyWhenUnset() {
+ Assertions.assertTrue(RequestContext.getRequestQueryParams().isEmpty());
+ }
+
+ @Test
+ public void testEmptyOrNullRequestQueryParamsClearsStash() {
+ RequestContext.setRequestQueryParams(Collections.singletonMap("k", "v"));
+ RequestContext.setRequestQueryParams(Collections.emptyMap());
+ Assertions.assertTrue(RequestContext.getRequestQueryParams().isEmpty());
+
+ RequestContext.setRequestQueryParams(Collections.singletonMap("k", "v"));
+ RequestContext.setRequestQueryParams(null);
+ Assertions.assertTrue(RequestContext.getRequestQueryParams().isEmpty());
+ }
+
+ @Test
+ public void testClearRemovesRequestQueryParams() {
+ RequestContext.setRequestQueryParams(Collections.singletonMap("k", "v"));
+ RequestContext.clear();
+ Assertions.assertTrue(RequestContext.getRequestQueryParams().isEmpty());
+ }
+
+ /**
+ * Mirrors the existing {@code operationFailureFired} flag on the success
path. Kept as an
+ * independent flag (not merged with the failure one) so that an operation
which succeeds but
+ * whose HTTP response delivery later fails can still produce both a success
and a failure audit
+ * entry.
+ */
+ @Test
+ public void testOperationSuccessFiredLifecycle() {
+ Assertions.assertFalse(RequestContext.isOperationSuccessFired());
+ RequestContext.markOperationSuccessFired();
+ Assertions.assertTrue(RequestContext.isOperationSuccessFired());
+ RequestContext.resetOperationSuccessFired();
+ Assertions.assertFalse(RequestContext.isOperationSuccessFired());
+ }
+
+ @Test
+ public void testClearRemovesOperationSuccessFired() {
+ RequestContext.markOperationSuccessFired();
+ RequestContext.clear();
+ Assertions.assertFalse(RequestContext.isOperationSuccessFired());
+ }
+
+ /**
+ * Failure is sticky: a success dispatch that happens to follow a failure
dispatch on the same
+ * thread (not expected in practice, but not structurally prevented either)
must not overwrite it
+ * — otherwise HttpAuditFilter would see isOperationFailureFired() == false
and double-log a
+ * 4xx/5xx response with its own fallback failure event, on top of the real
operation-layer one.
+ */
+ @Test
+ public void testMarkOperationSuccessFiredDoesNotOverwriteFailure() {
+ RequestContext.markOperationFailureFired();
+ RequestContext.markOperationSuccessFired();
+ Assertions.assertTrue(RequestContext.isOperationFailureFired(), "failure
must remain recorded");
+ Assertions.assertFalse(
+ RequestContext.isOperationSuccessFired(), "success must not overwrite
a recorded failure");
+ }
}
diff --git a/docs/gravitino-server-config.md b/docs/gravitino-server-config.md
index f85f56a6d3..2b1802e08c 100644
--- a/docs/gravitino-server-config.md
+++ b/docs/gravitino-server-config.md
@@ -400,9 +400,27 @@ pipeline can replace either.
`SimpleFormatterV2` is the default formatter. `JsonAuditFormatter` is
available where structured
output is wanted: it emits one JSON object per line, serializes `customInfo`,
and writes timestamps
-as ISO 8601 with millisecond precision and a zone offset. Both formatters
replace the value of a
-sensitive `customInfo` key with `***`. The masked keys are `authorization`,
`cookie`,
-`x-amz-security-token`, `s3.access-key-id`, and `jdbc-password`.
+as ISO 8601 with millisecond precision and a zone offset.
+
+`customInfo` always includes the request's query parameters, captured
automatically for every
+event — not just the ones an operation dispatcher explicitly reports. For
example, a listing
+endpoint's `?details=true` shows up in the audit entry for that request even
though no dispatcher
+code added it. Both formatters redact a `customInfo` value, replacing it with
`***`, when its key
+either exactly matches `authorization`, `cookie`, `x-amz-security-token`,
`s3.access-key-id`, or
+`jdbc-password`, or contains (case-insensitively) `password`, `secret`,
`token`, `credential`,
+`apikey`, `accesskey`, `privatekey`, `auth`, or `signature` — so a
caller-named parameter like
+`?token=...` or `?myApiKey=...` is masked even though its exact name was never
enumerated. A short,
+fixed list of keys the server itself always uses (e.g. `http.method`,
`http.status`, `auth.method`)
+is exempt from that substring check, since otherwise `auth.method` would be
masked for merely
+containing "auth".
+
+Every request that reaches the server produces at least one audit entry, even
one whose operation
+has no dedicated `Event` subclass: `HttpAuditFilter` dispatches a generic
fallback event (method,
+URI, status code, and the same auto-captured query parameters) for any request
where no
+operation-layer event fired. On a server that sees a high rate of
otherwise-unaudited calls to the
+same endpoint — for example an Iceberg REST catalog's `/v1/config`, which some
clients poll
+frequently — this can measurably increase audit log volume; size log rotation
and retention in
+`conf/log4j2.properties` (below) accordingly.
`FileAuditWriter` is the default writer, and it manages no files itself.
Rotation, compression, and
retention are delegated to Log4j2 through a logger named `gravitino.audit`,
configured by the
@@ -454,6 +472,12 @@ package.
Throwing a `ForbiddenException` from a pre-event handler stops the operation
before it runs, which
makes pre-events a veto point rather than a notification.
+`customInfo()` on every event includes the request's query parameters, and a
custom listener
+receives them **unredacted** — the masking described under "Audit Logging"
above is applied only by
+the two built-in audit-log formatters at format time, not to the event object
itself. A listener
+that forwards `customInfo()` elsewhere (logs, a metrics pipeline, a downstream
service) is
+responsible for its own redaction if that matters for its destination.
+
A plugin declares how its events are dispatched:
| Mode | Behavior
|
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 7e37b184cb..af732fbea1 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
@@ -60,6 +60,7 @@ import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.HttpServerMetricsSource;
import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
+import org.apache.gravitino.server.web.RequestContextFilter;
import
org.apache.gravitino.server.web.filter.IcebergRESTAuthInterceptionService;
import org.glassfish.hk2.api.InterceptionService;
import org.glassfish.hk2.utilities.binding.AbstractBinder;
@@ -204,6 +205,9 @@ public class RESTService implements
GravitinoAuxiliaryService {
Servlet servlet = new ServletContainer(config);
server.addServlet(servlet, ICEBERG_SPEC);
+ // Registered before HttpAuditFilter so audit events dispatched during
this request carry the
+ // request's query parameters and remote address, exactly as on the main
server.
+ server.addFilter(new RequestContextFilter(eventBus), ICEBERG_SPEC);
server.addFilter(
new HttpAuditFilter(
eventBus,
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergEvent.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergEvent.java
index 08d7cb1743..25307cd1ab 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergEvent.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergEvent.java
@@ -53,8 +53,9 @@ public abstract class IcebergEvent extends Event {
return icebergRequestContext.remoteHostName();
}
+ /** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return icebergRequestContext.httpHeaders();
}
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergFailureEvent.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergFailureEvent.java
index 3ffc40ff99..92db423190 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergFailureEvent.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergFailureEvent.java
@@ -53,8 +53,9 @@ public abstract class IcebergFailureEvent extends
FailureEvent {
return icebergRequestContext.remoteHostName();
}
+ /** {@inheritDoc} */
@Override
- public Map<String, String> customInfo() {
+ protected Map<String, String> ownCustomInfo() {
return icebergRequestContext.httpHeaders();
}
}
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 407350c000..041e038198 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
@@ -36,6 +36,7 @@ import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.HttpServerMetricsSource;
import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
+import org.apache.gravitino.server.web.RequestContextFilter;
import org.glassfish.hk2.utilities.binding.AbstractBinder;
import org.glassfish.jersey.jackson.JacksonFeature;
import org.glassfish.jersey.server.ResourceConfig;
@@ -93,6 +94,9 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
Servlet container = new ServletContainer(resourceConfig);
server.addServlet(container, LANCE_SPEC);
+ // Registered before HttpAuditFilter so audit events dispatched during
this request carry the
+ // request's query parameters and remote address, exactly as on the main
server.
+ server.addFilter(new RequestContextFilter(eventBus), LANCE_SPEC);
server.addFilter(
new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_LANCE_REST_SERVER), LANCE_SPEC);
server.addCustomFilters(LANCE_SPEC);
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
index 6f6d53202f..29f401bd92 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
@@ -37,6 +37,7 @@ import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.api.event.EventSource;
+import org.apache.gravitino.listener.api.event.server.HttpRequestEvent;
import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent;
import org.apache.gravitino.utils.RequestContext;
import org.slf4j.Logger;
@@ -44,8 +45,10 @@ import org.slf4j.LoggerFactory;
/**
* A servlet filter that emits an {@link HttpRequestFailureEvent} for every
HTTP request that
- * completes with a 4xx or 5xx status code and for which no operation-layer
failure event was
- * already dispatched on the same request thread.
+ * completes with a 4xx or 5xx status code, or an {@link HttpRequestEvent} for
one that completes
+ * with a 2xx/3xx status code, and for which no matching operation-layer event
was already
+ * dispatched on the same request thread. This guarantees every request
produces at least one audit
+ * entry, even for endpoints that are not (yet) wired into the operation-layer
event system.
*
* <p><strong>Filter chain position:</strong> this filter must be registered
<em>after</em> {@link
* RequestContextFilter} (so the remote address is already populated) but
<em>before</em> both
@@ -61,17 +64,21 @@ import org.slf4j.LoggerFactory;
* with {@link DispatcherType#ERROR} to render an error page. This filter
detects that case and
* passes the request straight through without emitting a second event,
preventing double-logging.
*
- * <p><strong>Double-logging prevention:</strong> when an operation-layer
failure event (e.g. {@code
- * LoadTableFailureEvent}, {@code AuthorizationDenialFailureEvent}) has
already been dispatched via
- * {@link EventBus}, {@link RequestContext#markOperationFailureFired()} is set
on the request
- * thread. This filter checks that flag in the {@code finally} block and skips
emitting its own
- * {@link HttpRequestFailureEvent} if the flag is set.
+ * <p><strong>Double-logging prevention:</strong> when an operation-layer
event has already been
+ * dispatched via {@link EventBus} for this request — a failure (e.g. {@code
LoadTableFailureEvent},
+ * {@code AuthorizationDenialFailureEvent}) via {@link
RequestContext#markOperationFailureFired()},
+ * or a success via {@link RequestContext#markOperationSuccessFired()} — this
filter skips emitting
+ * its own fallback event for the same outcome: no {@link
HttpRequestFailureEvent} on a 4xx/5xx
+ * response if a failure was already recorded, and no {@link HttpRequestEvent}
on a 2xx/3xx response
+ * if either a success or a failure was already recorded (a recorded failure
means the fallback
+ * success event would be misleading even if the HTTP layer ultimately
returned a non-error status).
*
- * <p><strong>Success + 5xx edge case:</strong> if an operation dispatcher
emits a {@code
- * SuccessEvent} (flag not set) but the HTTP layer subsequently fails with a
5xx (e.g. JSON
- * serialization error), this filter will emit an {@link
HttpRequestFailureEvent} in addition to the
- * success event already in the audit log. Both entries are correct — the
operation itself succeeded
- * but the response delivery failed — and are intentionally preserved.
+ * <p><strong>Success + 5xx edge case:</strong> if an operation dispatcher
emits a success event but
+ * the HTTP layer subsequently fails with a 5xx (e.g. JSON serialization
error), this filter still
+ * emits an {@link HttpRequestFailureEvent} in addition to the success event
already in the audit
+ * log. Both entries are correct — the operation itself succeeded but the
response delivery failed —
+ * and are intentionally preserved: the 5xx branch above checks only the
failure flag, not the
+ * success one, so a recorded success never suppresses this filter's own
failure fallback.
*
* <p><strong>Health check exclusion:</strong> requests matched by the
configured {@link
* HealthCheckPathMatcher} are silently passed through without audit logging
to avoid polluting the
@@ -96,11 +103,11 @@ public class HttpAuditFilter implements Filter {
/**
* Constructs an {@code HttpAuditFilter} using the default {@link
HealthCheckPathMatcher}.
*
- * @param eventBus the event bus used to dispatch {@link
HttpRequestFailureEvent}s; may be {@code
- * null}, in which case the filter is a pass-through no-op (useful when
no audit listener is
- * configured).
+ * @param eventBus the event bus used to dispatch {@link
HttpRequestFailureEvent}s and {@link
+ * HttpRequestEvent}s; may be {@code null}, in which case the filter is
a pass-through no-op
+ * (useful when no audit listener is configured).
* @param eventSource identifies which server this filter instance is
installed on; included in
- * every emitted {@link HttpRequestFailureEvent}.
+ * every emitted event.
*/
public HttpAuditFilter(@Nullable EventBus eventBus, EventSource eventSource)
{
this(eventBus, eventSource, new HealthCheckPathMatcher());
@@ -109,8 +116,8 @@ public class HttpAuditFilter implements Filter {
/**
* Constructs an {@code HttpAuditFilter} with a custom {@link
HealthCheckPathMatcher}.
*
- * @param eventBus the event bus used to dispatch {@link
HttpRequestFailureEvent}s; may be {@code
- * null}, in which case the filter is a pass-through no-op.
+ * @param eventBus the event bus used to dispatch {@link
HttpRequestFailureEvent}s and {@link
+ * HttpRequestEvent}s; may be {@code null}, in which case the filter is
a pass-through no-op.
* @param eventSource identifies which server this filter instance is
installed on.
* @param healthCheckMatcher determines which URI paths are health check
probes; those paths are
* excluded from audit logging to avoid polluting the audit log with
probe traffic.
@@ -146,11 +153,13 @@ public class HttpAuditFilter implements Filter {
}
// Defensive cleanup at request entry in case a pooled thread leaked stale
state.
RequestContext.resetOperationFailureFired();
+ RequestContext.resetOperationSuccessFired();
if (healthCheckMatcher.isHealthCheckPath(httpRequest.getRequestURI())) {
try {
chain.doFilter(request, response);
} finally {
RequestContext.resetOperationFailureFired();
+ RequestContext.resetOperationSuccessFired();
}
return;
}
@@ -168,9 +177,9 @@ public class HttpAuditFilter implements Filter {
}
} finally {
try {
- if (!RequestContext.isOperationFailureFired()) {
- int status = wrappedResponse.getCapturedStatus();
- if (status >= 400) {
+ int status = wrappedResponse.getCapturedStatus();
+ if (status >= 400) {
+ if (!RequestContext.isOperationFailureFired()) {
String user = resolveUser(httpRequest);
String remoteAddress = resolveClientAddress(httpRequest);
HttpRequestFailureEvent event =
@@ -183,6 +192,19 @@ public class HttpAuditFilter implements Filter {
eventSource);
eventBus.get().dispatchEvent(event);
}
+ } else if (!RequestContext.isOperationFailureFired()
+ && !RequestContext.isOperationSuccessFired()) {
+ String user = resolveUser(httpRequest);
+ String remoteAddress = resolveClientAddress(httpRequest);
+ HttpRequestEvent event =
+ new HttpRequestEvent(
+ user,
+ remoteAddress,
+ httpRequest.getMethod(),
+ httpRequest.getRequestURI(),
+ status,
+ eventSource);
+ eventBus.get().dispatchEvent(event);
}
} catch (Exception e) {
LOG.error(
@@ -191,8 +213,9 @@ public class HttpAuditFilter implements Filter {
httpRequest.getRequestURI(),
e);
} finally {
- // Always clear the flag to prevent ThreadLocal leaks across pooled
threads.
+ // Always clear the flags to prevent ThreadLocal leaks across pooled
threads.
RequestContext.resetOperationFailureFired();
+ RequestContext.resetOperationSuccessFired();
}
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
index f5de5185bf..78016bcedb 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
@@ -19,7 +19,17 @@
package org.apache.gravitino.server.web;
+import com.google.common.collect.ImmutableMap;
import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import javax.annotation.Nullable;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
@@ -28,11 +38,14 @@ import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.audit.AuditLogRedactor;
+import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.utils.RequestContext;
/**
- * A servlet filter that captures the client remote address from each HTTP
request and stores it in
- * {@link RequestContext} so that audit event constructors can read it on the
same thread.
+ * A servlet filter that captures the client remote address and (raw,
unredacted) query parameters
+ * from each HTTP request and stores them in {@link RequestContext} so that
audit event constructors
+ * can read them on the same thread.
*
* <p>When a reverse proxy is in use, the real client IP is taken from the
first entry of the {@code
* X-Forwarded-For} header (a de-facto standard header set by reverse proxies;
note that it is
@@ -40,12 +53,59 @@ import org.apache.gravitino.utils.RequestContext;
* reverse proxy, should be aware that clients can spoof this header). If the
header is absent,
* {@link HttpServletRequest#getRemoteAddr()} is used instead.
*
- * <p>The stored value is always cleared in a {@code finally} block to prevent
thread-pool leaks.
+ * <p>Query parameters are parsed from {@link
HttpServletRequest#getQueryString()}, not from {@link
+ * HttpServletRequest#getParameterMap()}. The latter also parses an {@code
+ * application/x-www-form-urlencoded} request body when present, consuming the
request input stream
+ * in the process — a resource endpoint that later reads the body itself (a
future OAuth2
+ * token-style endpoint, for example) would see an empty body with no
indication this filter is the
+ * cause. Parsing the raw query string avoids that hazard entirely and matches
what this class
+ * actually claims to capture.
+ *
+ * <p>Query parameters are captured as-is, not redacted here. Redaction
happens exactly once, at
+ * audit-log format time, via {@link AuditLogRedactor}, applied uniformly to
the fully-merged {@code
+ * customInfo()} map regardless of which layer contributed which key — see
{@link
+ * AuditLogRedactor}'s class doc for why that single pass replaces redacting
at every source
+ * separately. Capturing the query string is skipped entirely when no {@link
EventBus} is supplied,
+ * since nothing will ever read the result.
+ *
+ * <p>The number of distinct parameter names captured is capped at {@value
#MAX_PARAMETERS}, and any
+ * single value is truncated to {@value #MAX_VALUE_LENGTH} characters (marked
with {@value
+ * #TRUNCATED_SUFFIX}) — a caller cannot grow every event constructed during a
request, and every
+ * audit line written for it, by attaching an arbitrarily large query string.
+ *
+ * <p>The stored values are always cleared in a {@code finally} block to
prevent thread-pool leaks.
*/
public class RequestContextFilter implements Filter {
private static final String X_FORWARDED_FOR = "X-Forwarded-For";
+ /** Maximum number of distinct query-parameter names captured per request. */
+ static final int MAX_PARAMETERS = 50;
+
+ /** Maximum length of a single captured (post-join) parameter value, before
truncation. */
+ static final int MAX_VALUE_LENGTH = 256;
+
+ /** Appended to a value that was cut short at {@link #MAX_VALUE_LENGTH}. */
+ static final String TRUNCATED_SUFFIX = "...(truncated)";
+
+ private final Optional<EventBus> eventBus;
+
+ /** Constructs a {@code RequestContextFilter} that never captures query
parameters. */
+ public RequestContextFilter() {
+ this(null);
+ }
+
+ /**
+ * Constructs a {@code RequestContextFilter}.
+ *
+ * @param eventBus the event bus that will consume the captured query
parameters; may be {@code
+ * null}, in which case query-parameter capture is skipped
(remote-address capture still
+ * happens, since {@code HttpAuditFilter}'s own fallback events need it
regardless).
+ */
+ public RequestContextFilter(@Nullable EventBus eventBus) {
+ this.eventBus = Optional.ofNullable(eventBus);
+ }
+
@Override
public void init(FilterConfig filterConfig) {}
@@ -54,7 +114,11 @@ public class RequestContextFilter implements Filter {
throws IOException, ServletException {
try {
if (request instanceof HttpServletRequest) {
-
RequestContext.setRemoteAddress(resolveClientAddress((HttpServletRequest)
request));
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ RequestContext.setRemoteAddress(resolveClientAddress(httpRequest));
+ if (eventBus.isPresent()) {
+
RequestContext.setRequestQueryParams(parseQueryString(httpRequest.getQueryString()));
+ }
}
chain.doFilter(request, response);
} finally {
@@ -72,4 +136,62 @@ public class RequestContextFilter implements Filter {
}
return request.getRemoteAddr();
}
+
+ /**
+ * Parses a raw HTTP query string into a name-to-value map (joining
multi-valued parameters with a
+ * comma), with no redaction — see the class doc for why that happens later.
Bounded by {@link
+ * #MAX_PARAMETERS} and {@link #MAX_VALUE_LENGTH} so a caller cannot inflate
every event
+ * constructed during this request with an arbitrarily large query string.
+ *
+ * @param queryString the raw (still percent-encoded) query string, or
{@code null}
+ * @return an immutable, bounded, decoded parameter map; empty if {@code
queryString} is {@code
+ * null} or blank
+ */
+ private static Map<String, String> parseQueryString(String queryString) {
+ if (StringUtils.isBlank(queryString)) {
+ return ImmutableMap.of();
+ }
+ Map<String, List<String>> multiValued = new LinkedHashMap<>();
+ for (String pair : queryString.split("&")) {
+ if (pair.isEmpty()) {
+ continue;
+ }
+ int equalsIndex = pair.indexOf('=');
+ String rawName = equalsIndex >= 0 ? pair.substring(0, equalsIndex) :
pair;
+ String rawValue = equalsIndex >= 0 ? pair.substring(equalsIndex + 1) :
"";
+ String name = decode(rawName);
+ // Once the cap is hit, only accumulate more values for names already
being tracked — never
+ // start tracking a new name.
+ if (!multiValued.containsKey(name) && multiValued.size() >=
MAX_PARAMETERS) {
+ continue;
+ }
+ multiValued.computeIfAbsent(name, k -> new
ArrayList<>()).add(decode(rawValue));
+ }
+ ImmutableMap.Builder<String, String> flattened = ImmutableMap.builder();
+ multiValued.forEach((name, values) -> flattened.put(name,
truncate(String.join(",", values))));
+ return flattened.build();
+ }
+
+ private static String truncate(String value) {
+ return value.length() > MAX_VALUE_LENGTH
+ ? value.substring(0, MAX_VALUE_LENGTH) + TRUNCATED_SUFFIX
+ : value;
+ }
+
+ /**
+ * Decodes an {@code application/x-www-form-urlencoded} query-string
component (the encoding every
+ * servlet container assumes for a query string). Malformed percent-encoding
— always possible,
+ * since this is attacker-influenced input — falls back to the raw component
instead of throwing,
+ * so one bad parameter cannot fail the whole request just to be audited.
+ */
+ private static String decode(String value) {
+ try {
+ return URLDecoder.decode(value, StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ // UTF-8 is always supported by the JVM; unreachable in practice.
+ return value;
+ } catch (IllegalArgumentException e) {
+ return value;
+ }
+ }
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
index 5cd2c651bf..dd025492ab 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
@@ -38,6 +38,7 @@ import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.api.event.BaseEvent;
import org.apache.gravitino.listener.api.event.EventSource;
+import org.apache.gravitino.listener.api.event.server.HttpRequestEvent;
import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent;
import org.apache.gravitino.utils.RequestContext;
import org.junit.jupiter.api.AfterEach;
@@ -50,6 +51,7 @@ public class TestHttpAuditFilter {
@AfterEach
public void cleanup() {
RequestContext.resetOperationFailureFired();
+ RequestContext.resetOperationSuccessFired();
RequestContext.clear();
}
@@ -127,10 +129,13 @@ public class TestHttpAuditFilter {
verify(eventBus, never()).dispatchEvent(any());
}
- // ─── Successful 200 — no event
───────────────────────────────────────────────
+ // ─── Successful 200 — fallback HttpRequestEvent
──────────────────────────────
@Test
- public void test200ResponseNoEvent() throws Exception {
+ public void test200ResponseWithNoOperationEventEmitsHttpRequestEvent()
throws Exception {
+ // No operation-layer event was dispatched for this request (e.g. an
endpoint that is not yet
+ // wired into the event system), so the filter must emit a fallback
HttpRequestEvent rather than
+ // leaving the request completely unaudited.
EventBus eventBus = mock(EventBus.class);
HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
HttpServletRequest req = mockRequest("GET", "/api/metalakes", null,
"1.2.3.4");
@@ -139,9 +144,76 @@ public class TestHttpAuditFilter {
filter.doFilter(req, resp, chain);
+ ArgumentCaptor<BaseEvent> captor =
ArgumentCaptor.forClass(BaseEvent.class);
+ verify(eventBus).dispatchEvent(captor.capture());
+ Assertions.assertInstanceOf(HttpRequestEvent.class, captor.getValue());
+ HttpRequestEvent event = (HttpRequestEvent) captor.getValue();
+ Assertions.assertEquals(200, event.statusCode());
+ Assertions.assertEquals("GET", event.httpMethod());
+ Assertions.assertEquals("/api/metalakes", event.requestUri());
+ Assertions.assertEquals(EventSource.GRAVITINO_SERVER, event.eventSource());
+ }
+
+ @Test
+ public void testOperationSuccessFiredFlagPreventsHttpRequestEvent() throws
Exception {
+ // An operation-layer success event (e.g. LoadTableEvent) was already
dispatched for this
+ // request, so the filter must not emit a redundant fallback event.
+ EventBus eventBus = mock(EventBus.class);
+ HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
+ HttpServletRequest req = mockRequest("GET", "/api/metalakes/m1", null,
"1.2.3.4");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ FilterChain chain =
+ (request, response) -> {
+ RequestContext.markOperationSuccessFired();
+ ((HttpServletResponse) response).setStatus(200);
+ };
+
+ filter.doFilter(req, resp, chain);
+
verify(eventBus, never()).dispatchEvent(any());
}
+ /**
+ * Pins a fix to the success-fallback guard: it now also checks the failure
flag, not just the
+ * success flag, so a recorded operation-layer failure never gets a
contradictory success-shaped
+ * fallback event just because the HTTP layer happened to resolve to a
non-error status.
+ */
+ @Test
+ public void testOperationFailureFiredFlagAlsoPreventsHttpRequestEvent()
throws Exception {
+ EventBus eventBus = mock(EventBus.class);
+ HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
+ HttpServletRequest req = mockRequest("POST",
"/api/catalogs/test-connection", null, "1.2.3.4");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ FilterChain chain =
+ (request, response) -> {
+ RequestContext.markOperationFailureFired();
+ ((HttpServletResponse) response).setStatus(200);
+ };
+
+ filter.doFilter(req, resp, chain);
+
+ verify(eventBus, never()).dispatchEvent(any());
+ }
+
+ @Test
+ public void testSuccessFlagClearedAfterNormalCompletion() throws Exception {
+ EventBus eventBus = mock(EventBus.class);
+ HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
+ HttpServletRequest req = mockRequest("GET", "/api/metalakes/m1", null,
"1.2.3.4");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ FilterChain chain =
+ (request, response) -> {
+ RequestContext.markOperationSuccessFired();
+ ((HttpServletResponse) response).setStatus(200);
+ };
+
+ filter.doFilter(req, resp, chain);
+
+ Assertions.assertFalse(
+ RequestContext.isOperationSuccessFired(),
+ "operationSuccessFired flag must be cleared after filter completes");
+ }
+
// ─── 4xx via sendError
───────────────────────────────────────────────────────
@Test
@@ -371,15 +443,20 @@ public class TestHttpAuditFilter {
@Test
public void testSuccessEventFollowedBy5xxStillEmitsHttpFailureEvent() throws
Exception {
- // An operation dispatcher emits a SuccessEvent (operationFailureFired
stays false),
- // but the HTTP response ends up as 500 (e.g. JSON serialization failure).
- // HttpAuditFilter must still emit HttpRequestFailureEvent for the
HTTP-layer failure.
+ // An operation dispatcher emits a SuccessEvent (operationSuccessFired set,
+ // operationFailureFired
+ // stays false), but the HTTP response ends up as 500 (e.g. JSON
serialization failure).
+ // HttpAuditFilter must still emit HttpRequestFailureEvent for the
HTTP-layer failure — the
+ // success flag must not suppress it, since the two flags are tracked
independently.
EventBus eventBus = mock(EventBus.class);
HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
HttpServletRequest req = mockRequest("GET", "/api/metalakes/m1", null,
"1.2.3.4");
HttpServletResponse resp = mock(HttpServletResponse.class);
- // operationFailureFired is NOT set (success event path)
- FilterChain chain = (request, response) -> ((HttpServletResponse)
response).sendError(500);
+ FilterChain chain =
+ (request, response) -> {
+ RequestContext.markOperationSuccessFired();
+ ((HttpServletResponse) response).sendError(500);
+ };
filter.doFilter(req, resp, chain);
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
index 1b69f03ac4..14ab58acaf 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
@@ -23,11 +23,16 @@ import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.utils.RequestContext;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
@@ -35,7 +40,11 @@ import org.junit.jupiter.api.Test;
public class TestRequestContextFilter {
+ // No EventBus: exercises the remote-address-only path shared by every
request.
private final RequestContextFilter filter = new RequestContextFilter();
+ // With an EventBus configured: exercises query-parameter capture, which is
otherwise skipped.
+ private final RequestContextFilter filterWithEventBus =
+ new RequestContextFilter(new EventBus(Collections.emptyList()));
@AfterEach
public void cleanup() {
@@ -117,4 +126,194 @@ public class TestRequestContextFilter {
Assertions.assertNull(
RequestContext.getRemoteAddress(), "ThreadLocal must be cleared even
when chain throws");
}
+
+ /**
+ * Query parameters are captured raw here, not redacted — redaction happens
once, uniformly, at
+ * audit-log format time (see AuditLogRedactor's class doc). So a
sensitive-looking parameter like
+ * "token" must come through as its real value at this layer, not already
masked.
+ */
+ @Test
+ public void testQueryParamsCapturedRaw() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("details=true&token=secret-value");
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ filterWithEventBus.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("true", captured.get().get("details"));
+ Assertions.assertEquals("secret-value", captured.get().get("token"));
+ }
+
+ @Test
+ public void testMultiValuedQueryParamIsJoinedWithComma() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("keyword=a&keyword=b&keyword=c");
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ filterWithEventBus.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("a,b,c", captured.get().get("keyword"));
+ }
+
+ /**
+ * Query strings are percent-encoded ({@code
application/x-www-form-urlencoded}); both the name
+ * and the value must come back decoded, including {@code +} decoding to a
space.
+ */
+ @Test
+ public void testQueryParamsAreUrlDecoded() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("full+name=Alice+Smith&sym=%26");
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ filterWithEventBus.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("Alice Smith", captured.get().get("full name"));
+ Assertions.assertEquals("&", captured.get().get("sym"));
+ }
+
+ /**
+ * A name with no {@code =} (e.g. {@code ?flag}) is a valid query string; it
must be captured with
+ * an empty value rather than crashing the whole request.
+ */
+ @Test
+ public void testValuelessParamDoesNotThrow() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("flag");
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ Assertions.assertDoesNotThrow(() -> filterWithEventBus.doFilter(req, resp,
chain));
+ Assertions.assertEquals("", captured.get().get("flag"));
+ }
+
+ /**
+ * The query string is attacker-influenced input; malformed percent-encoding
(e.g. a truncated
+ * {@code %} escape) must not crash the whole request — it falls back to the
raw component.
+ */
+ @Test
+ public void testMalformedPercentEncodingDoesNotThrow() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("bad=100%");
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ Assertions.assertDoesNotThrow(() -> filterWithEventBus.doFilter(req, resp,
chain));
+ Assertions.assertEquals("100%", captured.get().get("bad"));
+ }
+
+ /**
+ * Pins the bound on parameter count: a query string with more distinct
names than {@link
+ * RequestContextFilter#MAX_PARAMETERS} must not grow the captured map
without limit.
+ */
+ @Test
+ public void testParameterCountIsCapped() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ String queryString =
+ IntStream.range(0, RequestContextFilter.MAX_PARAMETERS + 20)
+ .mapToObj(i -> "p" + i + "=v")
+ .collect(Collectors.joining("&"));
+ when(req.getQueryString()).thenReturn(queryString);
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ filterWithEventBus.doFilter(req, resp, chain);
+
+ Assertions.assertEquals(RequestContextFilter.MAX_PARAMETERS,
captured.get().size());
+ }
+
+ /**
+ * Pins the bound on value length: a single oversized value must be
truncated with an explicit
+ * marker, not copied in full into every event constructed during the
request.
+ */
+ @Test
+ public void testValueLengthIsTruncated() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ String hugeValue =
+ String.join("",
Collections.nCopies(RequestContextFilter.MAX_VALUE_LENGTH + 100, "a"));
+ when(req.getQueryString()).thenReturn("big=" + hugeValue);
+
+ AtomicReference<Map<String, String>> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRequestQueryParams());
+
+ filterWithEventBus.doFilter(req, resp, chain);
+
+ String capturedBig = captured.get().get("big");
+
Assertions.assertTrue(capturedBig.endsWith(RequestContextFilter.TRUNCATED_SUFFIX),
capturedBig);
+ Assertions.assertEquals(
+ RequestContextFilter.MAX_VALUE_LENGTH +
RequestContextFilter.TRUNCATED_SUFFIX.length(),
+ capturedBig.length());
+ }
+
+ @Test
+ public void testQueryParamsClearedAfterChain() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("details=true");
+
+ filterWithEventBus.doFilter(req, resp, (request, response) -> {});
+
+ Assertions.assertTrue(
+ RequestContext.getRequestQueryParams().isEmpty(),
+ "query-param ThreadLocal must be cleared after chain completes");
+ }
+
+ /**
+ * Pins the efficiency fix: when no EventBus is configured, nothing will
ever read the captured
+ * query parameters, so capture (and its redaction-list scanning) is skipped
entirely — remote
+ * address is still captured, since HttpAuditFilter's own fallback events
need it regardless of
+ * whether any listener is configured.
+ */
+ @Test
+ public void testQueryParamsNotCapturedWithoutEventBus() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+ when(req.getQueryString()).thenReturn("details=true");
+
+ AtomicReference<Map<String, String>> capturedParams = new
AtomicReference<>();
+ AtomicReference<String> capturedAddress = new AtomicReference<>();
+ FilterChain chain =
+ (request, response) -> {
+ capturedParams.set(RequestContext.getRequestQueryParams());
+ capturedAddress.set(RequestContext.getRemoteAddress());
+ };
+
+ filter.doFilter(req, resp, chain);
+
+ Assertions.assertTrue(capturedParams.get().isEmpty());
+ Assertions.assertEquals("1.2.3.4", capturedAddress.get());
+ }
}
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 831422ebc6..0f83f8d482 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -194,7 +194,7 @@ public class GravitinoServer extends ResourceConfig {
server.addServlet(new HealthAliasServlet(), "/health/*");
server.addServlet(new HealthAliasServlet(), "/health.html");
- server.addFilter(new RequestContextFilter(), API_ANY_PATH);
+ 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);
