[Feat] [SDK-399] Okhttp interceptor#367
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
|
@claude review |
…ent sensitive data leakage
…ent sensitive data leakage
…izer interface for API 21 compatibility
| If your URLs do not contain sensitive query parameters and you need them for debugging, you can opt in to the full URL by supplying a custom sanitizer: | ||
|
|
||
| ```java | ||
| OkHttpClient client = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(recorder, HttpUrl::toString)) | ||
| .build(); | ||
| ``` | ||
|
|
||
| When using a custom sanitizer, you are responsible for ensuring that sensitive query parameters are removed before the URL reaches Rollbar. |
There was a problem hiding this comment.
🟡 The opt-in example at README.md:91 uses HttpUrl::toString to re-enable the full URL, but the surrounding warning text at lines 87 and 95 only mentions "sensitive query parameters" — yet HttpUrl::toString simultaneously re-exposes userinfo (basic-auth credentials) and the fragment, which the Security paragraph at line 83 just promised the default strips. A reader whose URLs are query-param-clean but contain basic-auth credentials will follow the example and silently leak those credentials to Rollbar. Either change the example to a sanitizer that only opts into query (e.g. url -> url.newBuilder().username("").password("").fragment(null).build().toString()), or expand both warnings to enumerate all three components HttpUrl::toString re-exposes.
Extended reasoning...
What is wrong
The Security section frames URL sanitization as a defense for three sensitive URL components and the default sanitizer at RollbarOkHttpInterceptor.java:21-29 correctly strips all of them: username(""), password(""), query(null), fragment(null). README.md:83 documents this faithfully, listing userinfo (basic-auth credentials), query parameters, and the fragment. README.md:85 even illustrates with a URL that contains all three.
But the immediately following opt-in subsection narrows the framing back to one component:
- Line 87: "If your URLs do not contain sensitive query parameters and you need them for debugging, you can opt in to the full URL by supplying a custom sanitizer:"
- Line 91: example uses
HttpUrl::toString - Line 95: "When using a custom sanitizer, you are responsible for ensuring that sensitive query parameters are removed before the URL reaches Rollbar."
HttpUrl::toString is OkHttp's canonical, fully-unredacted URL serialization — it preserves all three components the previous paragraph just promised the default strips, not just query parameters.
Step-by-step proof of the silent credential leak
- Reader skims the Security section and confirms their app does not put sensitive data in query strings.
- Reader follows line 87's suggestion and copies the line 91 example:
new RollbarOkHttpInterceptor(recorder, HttpUrl::toString). - Some endpoint in their app issues a request to
https://alice:hunter2@api.example.com/charge(URL-embedded basic-auth — uncommon but legal and still seen in some legacy/internal integrations). - The server returns 500. The interceptor calls
urlSanitizer.sanitize(request.url())which is nowHttpUrl::toString. HttpUrl.toString()returnshttps://alice:hunter2@api.example.com/chargeverbatim, userinfo intact.- The README's recommended recorder forwards
urlstraight torollbar.recordNetworkEventFor(...). Basic-auth credentials are now stored in Rollbar.
The reader was never warned about userinfo or fragments because lines 87 and 95 only flag query parameters.
Why this PR is the right place to fix it
Commit 8af01d0 (docs(okhttp): update sanitizer docs to list all stripped URL components) already aligned the Security paragraph (line 83) and inline comment with the broadened sanitizer, but the opt-in paragraph and closing caveat were missed in that pass. Bringing them into alignment is a one-paragraph wording change that closes the gap before the docs are released.
Suggested fix — either of these:
- (a) Change the example to retain credential/fragment stripping while opting into query:
url -> url.newBuilder().username("").password("").fragment(null).build().toString(). This matches the framing the surrounding text already implies. - (b) Expand lines 87 and 95 to enumerate all three components and explicitly note that
HttpUrl::toStringre-exposes userinfo and the fragment in addition to query parameters.
Severity rationale
Docs-only, opt-in is deliberate, and URL-embedded basic-auth is rare in modern API usage — so this is a nit, not a normal-severity bug. It is still worth fixing in this same PR because (1) the section is explicitly framed as security, (2) the inconsistency with line 83 is internal to the same paragraph, and (3) the fix is trivial.
2 participants
Description of the change
rollbar-okhttpmodule with an OkHttp interceptor that records network telemetry (HTTP errors) and error events (connection failures/timeouts) via RollbarNetworkTelemetryRecorderinterface for bridging with any Rollbar instanceExamples
400/500
connection error logs the IOException
Type of change
Checklists
Development
Code review