-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Redact sensitive headers from debug logs #2279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,8 @@ | |
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import static org.asynchttpclient.netty.handler.HttpMessageFormatter.REDACTED; | ||
| import static org.asynchttpclient.netty.handler.HttpMessageFormatter.isSensitiveHeader; | ||
| import static org.asynchttpclient.util.MiscUtils.isNonEmpty; | ||
|
|
||
| public class DefaultRequest implements Request { | ||
|
|
@@ -293,7 +295,7 @@ public String toString() { | |
| sb.append('\t'); | ||
| sb.append(header.getKey()); | ||
| sb.append(':'); | ||
| sb.append(header.getValue()); | ||
| sb.append(isSensitiveHeader(header.getKey()) ? REDACTED : header.getValue()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Headers are redacted here but formParams a bit further down in this same method still get appended in full. If a password ever comes through as a form field it would still show up in this output untouched, worth checking if that is in scope for the issues this closes. |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* | ||
| * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. | ||
| * | ||
| * Licensed 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.asynchttpclient.netty.handler; | ||
|
|
||
| import io.netty.handler.codec.http.HttpHeaderNames; | ||
| import io.netty.handler.codec.http.HttpHeaders; | ||
| import io.netty.handler.codec.http.HttpRequest; | ||
| import io.netty.handler.codec.http.HttpResponse; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Formats HTTP messages for logging. Sensitive header values are redacted unless the | ||
| * {@code org.asynchttpclient.enableSensitiveLogging} system property or {@code AHC_ENABLE_SENSITIVE_LOGGING} environment | ||
| * variable is set to {@code true} when this class is initialized. Enabling sensitive logging may expose credentials and | ||
| * session data. | ||
| */ | ||
| public final class HttpMessageFormatter { | ||
|
|
||
| private static final String ENABLE_SENSITIVE_LOGGING_PROPERTY = "org.asynchttpclient.enableSensitiveLogging"; | ||
| private static final String ENABLE_SENSITIVE_LOGGING_ENVIRONMENT_VARIABLE = "AHC_ENABLE_SENSITIVE_LOGGING"; | ||
| private static final boolean SENSITIVE_LOGGING_ENABLED = isSensitiveLoggingEnabled(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is read once when the class loads and never rechecked. Surefire in this repo reuses a single JVM across the whole client module by default, so whichever test touches this class first locks the value for the rest of the run. That means the EnabledIfSystemProperty and EnabledIfEnvironmentVariable tests further down just get skipped in a normal build instead of actually running, so the enabled path has no real coverage right now. Might be worth resolving this per call or behind a seam that a test can override. |
||
|
|
||
| /** The value used in place of sensitive header values. */ | ||
| public static final String REDACTED = "<redacted>"; | ||
|
|
||
| private HttpMessageFormatter() { | ||
| } | ||
|
|
||
| static String format(HttpRequest request) { | ||
| StringBuilder value = new StringBuilder() | ||
| .append(request.method()).append(' ') | ||
| .append(request.uri()).append(' ') | ||
| .append(request.protocolVersion()); | ||
| return appendHeaders(value, request.headers()).toString(); | ||
| } | ||
|
|
||
| static String format(HttpResponse response) { | ||
| StringBuilder value = new StringBuilder() | ||
| .append(response.protocolVersion()).append(' ') | ||
| .append(response.status()); | ||
| return appendHeaders(value, response.headers()).toString(); | ||
| } | ||
|
|
||
| private static StringBuilder appendHeaders(StringBuilder value, HttpHeaders headers) { | ||
| for (Map.Entry<String, String> header : headers) { | ||
| value.append('\n').append(header.getKey()).append(": ") | ||
| .append(isSensitiveHeader(header.getKey()) ? REDACTED : header.getValue()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This uses a colon followed by a space between header name and value, but DefaultRequest toString uses a colon with no space for the same kind of dump. Small thing, but the two renderings will drift if they stay separate. |
||
| } | ||
| return value; | ||
| } | ||
|
|
||
| private static boolean isSensitiveLoggingEnabled() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reads from an environment variable that can be inherited from a container or orchestration setup. Worth considering a one time warn log when this ends up enabled, so it shows up in the logs instead of only being discoverable after something has already leaked. |
||
| String propertyValue = System.getProperty(ENABLE_SENSITIVE_LOGGING_PROPERTY); | ||
| String value = propertyValue != null ? propertyValue : System.getenv(ENABLE_SENSITIVE_LOGGING_ENVIRONMENT_VARIABLE); | ||
| return Boolean.parseBoolean(value); | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether a header value must be redacted from logs. | ||
| * | ||
| * @param name the header name | ||
| * @return {@code true} for authentication and cookie headers when sensitive logging is disabled | ||
| */ | ||
| public static boolean isSensitiveHeader(CharSequence name) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DefaultRequest in the base package now depends on this method and REDACTED. Since the base Request model has not reached into netty handler before, it might be cleaner to move this predicate and the redacted constant into a neutral utility package and have this class consume it instead. |
||
| return !SENSITIVE_LOGGING_ENABLED && (HttpHeaderNames.AUTHORIZATION.contentEqualsIgnoreCase(name) | ||
| || HttpHeaderNames.PROXY_AUTHORIZATION.contentEqualsIgnoreCase(name) | ||
| || HttpHeaderNames.COOKIE.contentEqualsIgnoreCase(name) | ||
| || HttpHeaderNames.SET_COOKIE.contentEqualsIgnoreCase(name)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1105,7 +1105,8 @@ public boolean retry(NettyResponseFuture<?> future) { | |
| if (future.isReplayPossible()) { | ||
| future.setChannelState(ChannelState.RECONNECTED); | ||
|
|
||
| LOGGER.debug("Trying to recover request {}\n", future.getNettyRequest().getHttpRequest()); | ||
| HttpRequest request = future.getNettyRequest().getHttpRequest(); | ||
| LOGGER.debug("Trying to recover request '{}' to '{}'\n", request.method(), request.uri()); | ||
| try { | ||
| future.getAsyncHandler().onRetry(); | ||
| } catch (Exception e) { | ||
|
|
@@ -1413,7 +1414,7 @@ public void replayRequest(final NettyResponseFuture<?> future, FilterContext fc, | |
| future.setChannelState(ChannelState.NEW); | ||
| future.touch(); | ||
|
|
||
| LOGGER.debug("\n\nReplaying Request {}\n for Future {}\n", newRequest, future); | ||
| LOGGER.debug("\n\nReplaying request '{}' to '{}'\n for Future {}\n", newRequest.getMethod(), newRequest.getUri(), future); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked this one, NettyResponseFuture toString does not render the underlying NettyRequest headers since NettyRequest has no toString override, so this falls back to identity hash and does not leak anything today. Might be worth a short comment here noting that this is load bearing, so a future toString added to NettyRequest does not quietly reopen this. |
||
| try { | ||
| future.getAsyncHandler().onRetry(); | ||
| } catch (Exception e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| /* | ||
| * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. | ||
| * | ||
| * Licensed 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.asynchttpclient.netty.handler; | ||
|
|
||
| import io.netty.handler.codec.http.DefaultHttpRequest; | ||
| import io.netty.handler.codec.http.DefaultHttpResponse; | ||
| import io.netty.handler.codec.http.HttpMethod; | ||
| import io.netty.handler.codec.http.HttpRequest; | ||
| import io.netty.handler.codec.http.HttpResponse; | ||
| import io.netty.handler.codec.http.HttpResponseStatus; | ||
| import io.netty.handler.codec.http.HttpVersion; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; | ||
| import org.junit.jupiter.api.condition.EnabledIfSystemProperty; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class HttpMessageFormatterTest { | ||
|
|
||
| @Test | ||
| public void shouldRedactSensitiveRequestHeaders() { | ||
| HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/test"); | ||
| request.headers() | ||
| .set("Authorization", "Bearer request-secret") | ||
| .set("proxy-authorization", "Basic proxy-secret") | ||
| .set("Cookie", "session=cookie-secret") | ||
| .set("X-Request-Id", "request-id"); | ||
|
|
||
| String value = HttpMessageFormatter.format(request); | ||
|
|
||
| assertFalse(value.contains("request-secret")); | ||
| assertFalse(value.contains("proxy-secret")); | ||
| assertFalse(value.contains("cookie-secret")); | ||
| assertTrue(value.contains("Authorization: <redacted>")); | ||
| assertTrue(value.contains("proxy-authorization: <redacted>")); | ||
| assertTrue(value.contains("X-Request-Id: request-id")); | ||
| } | ||
|
|
||
| @Test | ||
| public void shouldRedactSensitiveResponseHeaders() { | ||
| HttpResponse response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); | ||
| response.headers() | ||
| .set("Set-Cookie", "session=response-secret") | ||
| .set("X-Request-Id", "request-id"); | ||
|
|
||
| String value = HttpMessageFormatter.format(response); | ||
|
|
||
| assertFalse(value.contains("response-secret")); | ||
| assertTrue(value.contains("Set-Cookie: <redacted>")); | ||
| assertTrue(value.contains("X-Request-Id: request-id")); | ||
| } | ||
|
|
||
| @Test | ||
| @EnabledIfSystemProperty(named = "org.asynchttpclient.enableSensitiveLogging", matches = "(?i)true") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concern as on HttpMessageFormatter, this test and the one below it get silently skipped in a normal run rather than exercised, so the enabled path is effectively untested in CI as configured. |
||
| public void shouldIncludeSensitiveHeadersWhenSystemPropertyEnabled() { | ||
| assertSensitiveHeadersIncluded(); | ||
| } | ||
|
|
||
| @Test | ||
| @EnabledIfEnvironmentVariable(named = "AHC_ENABLE_SENSITIVE_LOGGING", matches = "(?i)true") | ||
| public void shouldIncludeSensitiveHeadersWhenEnvironmentVariableEnabled() { | ||
| assertSensitiveHeadersIncluded(); | ||
| } | ||
|
|
||
| private static void assertSensitiveHeadersIncluded() { | ||
| HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/test"); | ||
| request.headers().set("Authorization", "Bearer request-secret"); | ||
|
|
||
| String value = HttpMessageFormatter.format(request); | ||
|
|
||
| assertFalse(HttpMessageFormatter.isSensitiveHeader("Authorization")); | ||
| assertTrue(value.contains("Authorization: Bearer request-secret")); | ||
| assertFalse(value.contains(HttpMessageFormatter.REDACTED)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pulls the base Request model into a dependency on the netty handler package, which has been treated as transport level internal up to now. See the note on HttpMessageFormatter about moving the shared predicate somewhere neutral instead.