jerryshao commented on code in PR #12891:
URL: https://github.com/apache/gravitino/pull/12891#discussion_r3932090729
##########
core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java:
##########
@@ -61,10 +105,18 @@ public static Map<String, String>
redactCustomInfo(Map<String, String> customInf
* @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;
+ }
+ String lowerCaseKey = key.toLowerCase(Locale.ROOT);
+ if (NEVER_SENSITIVE_KEYS.contains(lowerCaseKey)) {
+ return false;
+ }
+ return MASKED_CUSTOM_INFO_KEYS.contains(lowerCaseKey)
+ || SENSITIVE_KEY_SUBSTRINGS.stream().anyMatch(lowerCaseKey::contains);
Review Comment:
Fixed — added a normalization step (`replaceAll("[^a-z0-9]", "")`) before
the substring check, so `api-key`, `x-api-key`, `access-key-id`, `private_key`,
etc. are now caught. The exact-match/exemption checks still run against the raw
(only case-folded) key first, so `auth.method` is unaffected. See
`AuditLogRedactor.isSensitiveKey()` in commit 4afae98.
##########
core/src/test/java/org/apache/gravitino/audit/TestAuditLogRedactor.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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)
──────────────────────────────
Review Comment:
Added `testSubstringMatchIgnoresSeparators()` covering exactly the four
names you listed (`api-key`, `x-api-key`, `access-key-id`, `private_key`).
Commit 4afae98.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/Event.java:
##########
@@ -51,4 +69,34 @@ protected Event(String user, NameIdentifier identifier) {
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() {
Review Comment:
Went with your second option — documented the semantics rather than
reworking the design: added a paragraph to `Event`'s class Javadoc and a note
in `docs/gravitino-server-config.md`'s "Event Listeners" section stating
plainly that a custom `EventListenerPlugin` receives `customInfo()` unredacted,
and that a plugin forwarding it elsewhere is responsible for its own redaction.
Commit 4afae98.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/Event.java:
##########
@@ -30,16 +33,31 @@
* <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
(not redacted — see
+ * {@code org.apache.gravitino.audit.AuditLogRedactor}), 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.
+ *
+ * <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
class of bug
+ * impossible to reintroduce.
*/
@DeveloperApi
public abstract class Event extends BaseEvent {
Review Comment:
Agreed on both points. Softened the class doc's "impossible to reintroduce"
claim to scope it explicitly to the `Event` subtree (not
`BaseEvent`/`PreEvent`), and added a note about the source/binary compatibility
break for `@DeveloperApi` consumers. Also moved this under "Does this PR
introduce any user-facing change?" in the PR description, flagging it
explicitly against the `branch-1.3` label. Commit 4afae98.
##########
server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java:
##########
@@ -54,7 +86,11 @@ public void doFilter(ServletRequest request, ServletResponse
response, FilterCha
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(flattenParameterMap(httpRequest.getParameterMap()));
Review Comment:
Switched to parsing `getQueryString()` directly instead of
`getParameterMap()`, exactly per your suggestion — closer to what the class
already claims to capture and removes the form-body-consumption hazard
entirely. Commit 4afae98.
##########
server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java:
##########
@@ -72,4 +108,20 @@ private String resolveClientAddress(HttpServletRequest
request) {
}
return request.getRemoteAddr();
}
+
+ /**
+ * Flattens a servlet-style parameter map to a single value per name
(joining multi-valued
+ * parameters with a comma), with no redaction — see the class doc for why
that happens later.
+ */
+ private static Map<String, String> flattenParameterMap(Map<String, String[]>
parameterMap) {
Review Comment:
Added `MAX_PARAMETERS` (50) and `MAX_VALUE_LENGTH` (256, truncated with a
marker) caps in the same rewrite. Once the parameter-name cap is hit, only
additional values for already-tracked names are accumulated — no new names are
added. Commit 4afae98.
##########
core/src/main/java/org/apache/gravitino/utils/RequestContext.java:
##########
@@ -87,16 +104,50 @@ public static void markOperationFailureFired() {
* @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}.
+ */
+ public static void markOperationSuccessFired() {
+ OPERATION_OUTCOME.set(OperationOutcome.SUCCESS);
Review Comment:
Applied your one-line fix verbatim — `markOperationSuccessFired()` is now a
no-op if a failure was already recorded. Added
`testMarkOperationSuccessFiredDoesNotOverwriteFailure()` and updated the class
doc to state this as an enforced invariant rather than an unenforced comment.
Commit 4afae98.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
