xiangfu0 commented on code in PR #19899:
URL: https://github.com/apache/pinot/pull/19899#discussion_r4289167653
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ControllerResponseFilter.java:
##########
@@ -42,12 +53,50 @@ public class ControllerResponseFilter implements
ContainerResponseFilter {
public void filter(ContainerRequestContext requestContext,
ContainerResponseContext responseContext)
throws IOException {
final String method = requestContext.getMethod();
- final String uri = requestContext.getUriInfo().getRequestUri().toString();
+ final String uri =
redactQueryParameters(requestContext.getUriInfo().getRequestUri());
final int respStatus = responseContext.getStatus();
final String reasonPhrase =
responseContext.getStatusInfo().getReasonPhrase();
final String srcIpAddr = _request.get().getRemoteAddr();
final String contentType =
requestContext.getHeaderString(HttpHeaders.CONTENT_TYPE);
LOGGER.info("Handled request from {} {} {}, content-type {} status code {}
{}", srcIpAddr, method, uri, contentType,
respStatus, reasonPhrase);
}
+
+ /// Returns the request URI with the values of credential-bearing query
parameters masked, as decided by
+ /// [Obfuscator#DEFAULT]. This line is logged at INFO for every request, so
a token passed as a query
+ /// parameter would otherwise be written to the controller log verbatim.
Other parameters are kept in
+ /// their original order with their values as sent, since they are what
makes the line useful when
+ /// debugging. Names are shown decoded, and repeated parameters are grouped
under their first occurrence.
+ @VisibleForTesting
+ static String redactQueryParameters(URI requestUri) {
+ String rawQuery = requestUri.getRawQuery();
+ String uri = requestUri.toString();
+ if (rawQuery == null || rawQuery.isEmpty()) {
+ return uri;
+ }
+ String withoutQuery = uri.substring(0, uri.indexOf('?'));
+ // Keyed by the decoded name, which is what the patterns judge; the values
stay raw. A URI has already
+ // rejected malformed escapes, so decoding cannot fail here.
+ Map<String, List<String>> parameters = new LinkedHashMap<>();
+ for (String pair : rawQuery.split("&")) {
+ int separator = pair.indexOf('=');
+ String rawName = separator < 0 ? pair : pair.substring(0, separator);
+ String rawValue = separator < 0 ? "" : pair.substring(separator + 1);
+ parameters.computeIfAbsent(URLDecoder.decode(rawName,
StandardCharsets.UTF_8), k -> new ArrayList<>())
Review Comment:
Minor: grouping by name regroups interleaved duplicates, so `?a=1&b=2&a=3`
is logged as `?a=1&a=3&b=2`. That slightly contradicts the "kept in their
original order" claim in the doc comment above for the duplicate-key case. It
only affects readability of the log line, so fine to leave — maybe just soften
the doc wording to say repeats are grouped under the first occurrence (which it
already hints at).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ControllerResponseFilter.java:
##########
@@ -42,12 +53,50 @@ public class ControllerResponseFilter implements
ContainerResponseFilter {
public void filter(ContainerRequestContext requestContext,
ContainerResponseContext responseContext)
throws IOException {
final String method = requestContext.getMethod();
- final String uri = requestContext.getUriInfo().getRequestUri().toString();
+ final String uri =
redactQueryParameters(requestContext.getUriInfo().getRequestUri());
final int respStatus = responseContext.getStatus();
final String reasonPhrase =
responseContext.getStatusInfo().getReasonPhrase();
final String srcIpAddr = _request.get().getRemoteAddr();
final String contentType =
requestContext.getHeaderString(HttpHeaders.CONTENT_TYPE);
LOGGER.info("Handled request from {} {} {}, content-type {} status code {}
{}", srcIpAddr, method, uri, contentType,
respStatus, reasonPhrase);
}
+
+ /// Returns the request URI with the values of credential-bearing query
parameters masked, as decided by
+ /// [Obfuscator#DEFAULT]. This line is logged at INFO for every request, so
a token passed as a query
+ /// parameter would otherwise be written to the controller log verbatim.
Other parameters are kept in
+ /// their original order with their values as sent, since they are what
makes the line useful when
+ /// debugging. Names are shown decoded, and repeated parameters are grouped
under their first occurrence.
+ @VisibleForTesting
+ static String redactQueryParameters(URI requestUri) {
+ String rawQuery = requestUri.getRawQuery();
+ String uri = requestUri.toString();
+ if (rawQuery == null || rawQuery.isEmpty()) {
+ return uri;
+ }
+ String withoutQuery = uri.substring(0, uri.indexOf('?'));
Review Comment:
Minor: when the URI carries a fragment, this drops it. `withoutQuery` is the
part before `?` and the rebuilt string is `withoutQuery + query`, so
`?a=b#frag` logs as `?a=b` with the `#frag` gone. In practice request URIs
almost never carry a fragment (clients don't send it), so this is low-impact —
just flagging that the reconstruction isn't lossless.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
