Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
|
@claude review |
| request.method(), | ||
| request.url().toString(), |
There was a problem hiding this comment.
🔴 When a server redirects (e.g., 301 from /api/v1/users to /api/v2/users) and the final endpoint returns a 4xx/5xx, the telemetry records the original URL and method rather than the ones that actually produced the error. Lines 33–34 use request.method() and request.url().toString() where request is the pre-redirect chain.request(); the fix is to use response.request().method() and response.request().url().toString() to report the request that actually produced the final response.
Extended reasoning...
What the bug is and how it manifests
RollbarOkHttpInterceptor is registered as an application interceptor via addInterceptor(). Application interceptors sit above OkHttp's internal RetryAndFollowUpInterceptor, which handles redirect resolution transparently. This means chain.proceed(request) returns only after all redirects have been followed, and the returned Response is the final response in the chain.
At line 24, request = chain.request() captures the original pre-redirect request. Lines 33–34 then use request.method() and request.url().toString() to populate the telemetry event. Because these fields come from the original request — not the final redirected one — any redirect causes incorrect attribution.
The specific code path that triggers it
Why existing code doesn't prevent it
OkHttp's Response.request() returns the request that produced that specific response — for a redirect chain, this is the last request in the chain, not the first. The code never consults response.request(), so the mismatch between the status code source (final response) and the URL/method source (original request) goes uncorrected. The test suite confirms the gap: redirectResponse_doesNotRecordEvent() only tests a 301 with followRedirects(false), which does not follow the redirect at all. The redirect-then-error scenario (follow redirect -> final URL returns 5xx) is not tested.
What the impact would be
Two failure modes:
-
Wrong URL: A 301 from /api/v1/users -> /api/v2/users that ends in a 500 is logged in Rollbar as an error at /api/v1/users. Engineers investigating via Rollbar telemetry will look at the wrong endpoint.
-
Wrong method: Per the HTTP spec (and OkHttp's default behavior), a POST redirected with 301 or 302 becomes a GET for the final request. The telemetry would record method=POST when the actual failing request was GET, compounding the misattribution.
OkHttp follows redirects by default (followRedirects=true), so any application that hits a redirected endpoint with a 4xx/5xx final response is affected.
Step-by-step proof
- Client sends POST /api/v1/users via an OkHttpClient with RollbarOkHttpInterceptor.
- request = chain.request() -> records {method: "POST", url: "/api/v1/users"}.
- chain.proceed(request) -> OkHttp internally follows the 301 and sends GET /api/v2/users.
- The server returns 500 for /api/v2/users. response.code() = 500.
- recorder.recordNetworkEvent(CRITICAL, "POST", "/api/v1/users", "500") is called.
- Rollbar shows: error at POST /api/v1/users -> 500. Both the URL and method are wrong.
- response.request().method() -> "GET"; response.request().url().toString() -> "/api/v2/users". These are the correct values.
How to fix it
Replace lines 33–34 with:
This ensures the telemetry records the request that actually produced the error response, regardless of how many redirects occurred.
| recorder.recordNetworkEvent( | ||
| Level.CRITICAL, | ||
| request.method(), | ||
| request.url().toString(), | ||
| String.valueOf(response.code())); |
There was a problem hiding this comment.
🟡 The interceptor passes request.url().toString() (full URL including query string) to the user-supplied NetworkTelemetryRecorder, and the README example forwards this raw URL directly to Rollbar with no sanitization or warning. URLs routinely carry sensitive query parameters (API keys, OAuth tokens, PII), so users who copy-paste the README example will inadvertently send those secrets to Rollbar's servers. Add a security callout to the README and update the example to show query-parameter stripping (e.g. request.url().newBuilder().query(null).build().toString()) or at minimum note that callers are responsible for sanitizing the URL before forwarding it to Rollbar.
Extended reasoning...
What the bug is
RollbarOkHttpInterceptor.intercept() calls recorder.recordNetworkEvent(..., request.url().toString(), ...) at line 34. OkHttp's HttpUrl.toString() returns the complete URL including the query string — for example, https://api.example.com/search?api_key=sk-secret&email=user@example.com. Query parameters are a well-known carrier of sensitive data: API keys (?api_key=...), OAuth tokens (?access_token=...), session identifiers (?session_id=...), and PII such as email or SSN are all commonly passed this way.
Why the README makes this actionable
The NetworkTelemetryRecorder interface is user-controlled, so in principle users can sanitize the URL inside their own implementation. The refutation correctly identifies this. However, the README example (lines 37-38) shows exactly:
public void recordNetworkEvent(Level level, String method, String url, String statusCode) {
rollbar.recordNetworkEventFor(level, method, url, statusCode);
}This pattern passes the raw URL verbatim to Rollbar — an external cloud service — without any sanitization and without any comment warning that query parameters are included. There is no note in the README, no Javadoc on NetworkTelemetryRecorder, and no inline comment in the code that flags this risk. Users who follow this copy-paste example will inadvertently ship sensitive query parameters to Rollbar's servers.
How this differs from comparable libraries
The refutation compares this to OkHttp's HttpLoggingInterceptor, which also logs full URLs. The key difference: HttpLoggingInterceptor writes to a local logger under the developer's own control (and OkHttp's own docs recommend a HttpLoggingInterceptor.Logger that can be customized). This interceptor's primary documented use case, as shown in the README, is forwarding to Rollbar — a third-party SaaS. The data-flow destination changes the risk profile substantially.
Impact
Any developer who follows the README (likely the majority of new adopters) will send complete request URLs to Rollbar. If those URLs contain API keys or tokens, those credentials are now stored in a third party's servers, visible to anyone with Rollbar access, and potentially included in Rollbar exports or integrations. This is a silent data leak with no indication at the call site.
Proof by example
- App calls
GET https://payments.example.com/charge?token=sk_live_secret&amount=100→ server returns 500. response.code() >= 400is true → interceptor callsrecorder.recordNetworkEvent(CRITICAL, "GET", "https://payments.example.com/charge?token=sk_live_secret&amount=100", "500").- User's recorder (copied from README) calls
rollbar.recordNetworkEventFor(level, method, url, statusCode)with the full URL. - Rollbar stores
token=sk_live_secretas part of the telemetry event, visible in the Rollbar dashboard.
How to fix
This is primarily a documentation gap. The README should include a security callout explaining that url includes query parameters and showing how to strip them: e.g., request.url().newBuilder().query(null).build().toString() before passing to the recorder, or stripping inside the recorder implementation. The NetworkTelemetryRecorder Javadoc should note the same. Optionally, the interceptor itself could strip query parameters by default and provide an opt-in setUrlSanitizer(Function<HttpUrl, String>) for users who need them.
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