fix: redact Authorization header in LogInterceptor [AIS-247] - #212
Conversation
Adds a `redactHeaders` helper that replaces the value of any Authorization header with `Bearer [REDACTED]` before passing headers to the logger. Response headers are unchanged because they don't carry bearer tokens. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #a798ccActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
|
| Source | Requirement / Code Area | Status | Notes |
|---|---|---|---|
| AIS-247 | Redact Authorization header values in LogInterceptor to prevent bearer token leakage in logs | ✅ Met | The PR implements the redactHeaders method in src/main/java/com/contentful/java/cma/interceptor/LogInterceptor.java to redact Authorization header values before logging. The method iterates through headers and replaces any header matching 'Authorization' (case-insensitive) with 'Bearer [REDACTED]'. The logging statement at line 18 now calls `redactHeaders(request.headers())` instead of logging raw headers. Additional tests in src/test/kotlin/com/contentful/java/cma/interceptor/LogInterceptorTests.kt verify the redaction works case-insensitively and that raw tokens never appear in log output. |
Impact Analysis by BitoInteraction DiagramsequenceDiagram
participant App as Application
participant CMA as CMAClient<br/>🔄 Updated | ●●○ Medium
participant Auth as AuthorizationHeaderInterceptor
participant Log as LogInterceptor<br/>🔄 Updated | ●●● High
participant OkHttp as OkHttp Chain
participant Logger as Logger
participant Test as LogInterceptorTests<br/>🟩 Added | ●●○ Medium
Note over Log: Security fix: Authorization header<br/>values are now redacted before logging
App->>CMA: Create client with logger enabled
CMA->>Log: Add LogInterceptor to chain
CMA->>Auth: Add AuthorizationHeaderInterceptor
App->>CMA: Make API request
CMA->>Auth: intercept(request)
Auth->>OkHttp: Add Authorization: Bearer token
OkHttp->>Log: intercept(request)
Log->>Log: redactHeaders(headers)
Log->>Logger: Log sanitized headers
Note over Log: Authorization header now shows<br/>Bearer [REDACTED] instead of real token
Log->>OkHttp: proceed(request)
OkHttp-->>Log: Return response
Log-->>CMA: Return response
CMA-->>App: Return result
Test->>Log: Test redactHeaders()
Test->>Log: Test token never appears in logs
This MR adds security-focused header redaction to the LogInterceptor. When logging is enabled, Authorization header values are now masked as 'Bearer [REDACTED]' before being written to logs, preventing sensitive access tokens from being exposed. The change includes comprehensive unit and integration tests to verify the redaction behavior. Cross-Repository Impact Analysis
Code Paths AnalyzedImpact: Flow: Direct Changes (Diff Files): Repository Impact: Cross-Repository Dependencies: Database/Caching Impact: API Contract Violations: Infrastructure Dependencies: Additional Insights: Testing RecommendationsFrontend Impact: Service Integration: Data Serialization: Privacy Compliance: Backward Compatibility: OAuth Functionality: Cross-Service Communication: Reliability Testing: Additional Insights: Analysis based on known dependency patterns and edges. Actual impact may vary. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
✅ Pre-review pass — verified, ready for team reviewTraced the fix against AIS-247:
One suggestion (non-blocking): add a small unit test asserting the redaction, since this is a regression-prone security fix. Happy to approve as-is otherwise. |
Tyler Collins (t-col)
left a comment
Tyler Collins (t-col)
left a comment
There was a problem hiding this comment.
Nice work — this closes a real credential-leak risk cleanly, and the fix itself reads correct: it walks the header list by index, subs in the redacted value only for Authorization, and leaves everything else (including response headers, which don't carry the token) untouched.
I left a couple of comments below, mostly around reusing an existing constant and locking this in with a test, but nothing here should block the merge — happy to see this go out as is if we'd rather follow up separately.
If we don't add a test now, might be worth opening a quick follow-up ticket so the regression coverage doesn't get lost.
| Headers.Builder redacted = new Headers.Builder(); | ||
| for (int i = 0; i < headers.size(); i++) { | ||
| String name = headers.name(i); | ||
| if ("Authorization".equalsIgnoreCase(name)) { |
There was a problem hiding this comment.
Small one: AuthorizationHeaderInterceptor.HEADER_NAME already holds this exact string a few files over. Might be worth reusing it here instead of the literal, just so we don't end up with two sources of truth for the header name.
There was a problem hiding this comment.
Done — redactHeaders now compares against AuthorizationHeaderInterceptor.HEADER_NAME instead of the string literal, so the header name has a single source of truth. Same package, so no new import.
| return response; | ||
| } | ||
|
|
||
| private static Headers redactHeaders(Headers headers) { |
There was a problem hiding this comment.
Following up on coverage — there's no existing test file for LogInterceptor, so this redaction doesn't have anything guarding it going forward. A quick test that mocks the logger and asserts the token never shows up unredacted in the logged string could be cheap insurance for a fix like this.
There was a problem hiding this comment.
Added LogInterceptorTests.kt under interceptor/. It drives a real request through the interceptor via MockWebServer with a capturing Logger:
testAuthorizationHeaderIsRedacted— asserts the raw token never appears andBearer [REDACTED]does.testOtherHeadersAreStillLogged— asserts a non-auth header (X-Custom-Header) is still present, so redaction is surgical.
Heads up: I could not run the suite locally (no JDK on my machine at the moment), so I am leaning on CI to confirm it goes green — the test mirrors the existing MockWebServer pattern used across the Kotlin tests, so I would expect it to, but flagging that I have not executed it myself.
There was a problem hiding this comment.
Ran the suite locally after all (installed a JDK) — and it caught something worth flagging.
The end-to-end test I first wrote asserted Bearer [REDACTED] appears in the log. It failed: okhttp >= 4.10 already masks Authorization to ██ in Headers.toString() by header name, regardless of value. So on our okhttp (4.12):
- the raw token was never in the log even before this fix (okhttp masks it), and
- our injected
Bearer [REDACTED]gets overwritten to██too, so it is not observable end-to-end.
Two takeaways:
redactHeadersis defense-in-depth here, not the thing preventing the leak on 4.12 — it guarantees the value is stripped independent of okhttp's internal masking (which is an undocumented debug convenience, not a security guarantee, and would disappear if okhttp were pinned < 4.10). Added a comment saying so.- An end-to-end "token absent" test would pass even if
redactHeaderswere deleted, so it does not actually guard the code. Reworked the tests to assertredactHeadersdirectly (Authorization value →Bearer [REDACTED], case-insensitive match, other headers untouched) plus keep one end-to-end raw-token-absent invariant.
3/3 green locally (mvn -Dtest=LogInterceptorTests test). Pushed in b1031c4.
Address review feedback: - redactHeaders compares against AuthorizationHeaderInterceptor.HEADER_NAME instead of a string literal (single source of truth) - add LogInterceptorTests exercising redaction via MockWebServer: asserts the raw token never appears and non-auth headers are still logged Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
02ace95
There was a problem hiding this comment.
Code Review Agent Run #b4d98a
Actionable Suggestions - 1
-
src/test/kotlin/com/contentful/java/cma/interceptor/LogInterceptorTests.kt - 1
- Refactor duplicate test setup code · Line 28-28
Review Details
-
Files reviewed - 2 · Commit Range:
eb968c9..02ace95- src/main/java/com/contentful/java/cma/interceptor/LogInterceptor.java
- src/test/kotlin/com/contentful/java/cma/interceptor/LogInterceptorTests.kt
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Kotlin (Linter) - ✔︎ Successful
- Java-google-format (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at jared.jolton@contentful.com.
Documentation & Help
AI Code Review powered by
Running the suite surfaced that okhttp >= 4.10 already masks Authorization to "██" in Headers.toString() by name, so an end-to-end "Bearer [REDACTED]" assertion never matched and a raw-token-absent assertion wouldn't guard our code (okhttp masks regardless). Make redactHeaders package-private and assert its value transformation directly; keep an end-to-end raw-token-absent check. Document that redactHeaders is defense-in-depth on current okhttp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #ebe35eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
Summary
redactHeadersprivate method that replaces the value of anyAuthorizationheader withBearer [REDACTED]before passing to the loggerBASICorFULLlog level still see all other headers and request metadataTickets
Closes AIS-247
Test plan
./gradlew build)FULLlogging and confirmAuthorizationheader no longer appears in logs🤖 Generated with Claude Code
Summary by Bito
This PR improves testability of the LogInterceptor by making its `redactHeaders` method package-private and enhancing test coverage. The method serves as defense-in-depth to ensure Authorization header values are redacted to 'Bearer [REDACTED]' before logging, protecting sensitive bearer tokens regardless of the configured log level or okhttp version.
Detailed Changes