Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java.io.IOException;

import okhttp3.Headers;
import okhttp3.Interceptor;
import okhttp3.Request;
import okhttp3.Response;
Expand Down Expand Up @@ -46,7 +47,7 @@ public LogInterceptor(Logger logger) {

long t1 = System.nanoTime();
logger.log(String.format("Sending request %s on %s%n%s",
request.url(), chain.connection(), request.headers()));
request.url(), chain.connection(), redactHeaders(request.headers())));

Response response = chain.proceed(request);

Expand All @@ -56,4 +57,17 @@ public LogInterceptor(Logger logger) {

return response;
}

private static Headers redactHeaders(Headers headers) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and Bearer [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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. redactHeaders is 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.
  2. An end-to-end "token absent" test would pass even if redactHeaders were deleted, so it does not actually guard the code. Reworked the tests to assert redactHeaders directly (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.

Headers.Builder redacted = new Headers.Builder();
for (int i = 0; i < headers.size(); i++) {
String name = headers.name(i);
if (AuthorizationHeaderInterceptor.HEADER_NAME.equalsIgnoreCase(name)) {
redacted.add(name, "Bearer [REDACTED]");
} else {
redacted.add(name, headers.value(i));
}
}
return redacted.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package com.contentful.java.cma.interceptor

import com.contentful.java.cma.Logger
import okhttp3.OkHttpClient
import okhttp3.Request
import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.junit.After
import org.junit.Before
import org.junit.Test
import kotlin.test.assertFalse
import kotlin.test.assertTrue

class LogInterceptorTests {
private lateinit var server: MockWebServer

@Before fun setUp() {
server = MockWebServer()
server.start()
}

@After fun tearDown() {
server.shutdown()
}

@Test fun testAuthorizationHeaderIsRedacted() {
val token = "super-secret-token"
val logs = mutableListOf<String>()
Comment thread
tylerpina marked this conversation as resolved.
Outdated
val logger = Logger { message -> logs.add(message) }

val client = OkHttpClient.Builder()
.addInterceptor(LogInterceptor(logger))
.build()

server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))

val request = Request.Builder()
.url(server.url("/"))
.header(AuthorizationHeaderInterceptor.HEADER_NAME, "Bearer $token")
.build()

client.newCall(request).execute().use { it.body?.string() }

val requestLog = logs.first { it.startsWith("Sending request") }

assertFalse(requestLog.contains(token), "Raw token must not appear in logs")
assertTrue(requestLog.contains("Bearer [REDACTED]"), "Redacted marker expected")
}

@Test fun testOtherHeadersAreStillLogged() {
val logs = mutableListOf<String>()
val logger = Logger { message -> logs.add(message) }

val client = OkHttpClient.Builder()
.addInterceptor(LogInterceptor(logger))
.build()

server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))

val request = Request.Builder()
.url(server.url("/"))
.header(AuthorizationHeaderInterceptor.HEADER_NAME, "Bearer secret")
.header("X-Custom-Header", "custom-value")
.build()

client.newCall(request).execute().use { it.body?.string() }

val requestLog = logs.first { it.startsWith("Sending request") }

assertTrue(requestLog.contains("X-Custom-Header: custom-value"),
"Non-authorization headers must still be logged")
}
}
Loading