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:

1. 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.

2. 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

1. Client sends POST /api/v1/users via an OkHttpClient with RollbarOkHttpInterceptor.
2. request = chain.request() -> records {method: "POST", url: "/api/v1/users"}.
3. chain.proceed(request) -> OkHttp internally follows the 301 and sends GET /api/v2/users.
4. The server returns 500 for /api/v2/users. response.code() = 500.
5. recorder.recordNetworkEvent(CRITICAL, "POST", "/api/v1/users", "500") is called.
6. Rollbar shows: error at POST /api/v1/users -> 500. Both the URL and method are wrong.
7. 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.

What the bug is and how it manifests

In RollbarOkHttpInterceptor.intercept(), the inner try block (lines 42–47) wraps a single call: recorder.recordNetworkEvent(Level.CRITICAL, request.method(), urlSanitizer.apply(request.url()), String.valueOf(response.code())). Java evaluates method arguments left-to-right before the call is made, so urlSanitizer.apply(request.url()) (line 46) executes inside the try block. If a user-supplied sanitizer throws (e.g., NPE, IllegalStateException, or any custom failure), the exception is caught by the catch (Exception recorderException) at line 48, and the WARNING is logged with the message "NetworkTelemetryRecorder.recordNetworkEvent threw an exception; suppressing to preserve the interceptor contract." — even though recordNetworkEvent itself was never entered.

Why existing code doesn't prevent it

The argument-evaluation order is fundamental to Java semantics; there's no way to scope only the recorder call without lifting the sanitizer out. The catch block uses a single message because the original author likely assumed only the recorder could throw. The code path is real today: the urlSanitizer constructor parameter is part of the public API (see the README's "custom sanitizer" example, which encourages HttpUrl::toString and other user-defined functions).

Addressing the refutation

The refutation is correct that LOGGER.log(WARNING, message, throwable) includes the throwable's full stack trace, so a diligent debugger can identify the sanitizer as the source. It is also correct that the default sanitizer url.newBuilder().query(null).build().toString() is unlikely to throw in practice — only custom sanitizers are at meaningful risk. These mitigations are why this is a nit rather than a normal-severity bug. However, log message accuracy still matters for log scanning, alerting rules, and triage at scale: an operator filtering or aggregating on the WARNING header text will mis-bucket sanitizer failures as recorder failures, and the README explicitly invites users to plug in custom sanitizers. The fix is trivial and improves diagnostic quality.

Step-by-step proof

1. User constructs new RollbarOkHttpInterceptor(recorder, url -> { throw new IllegalStateException("bad url"); }).
2. A request returns a 500 → response.code() >= 400 && recorder != null is true.
3. JVM begins evaluating arguments to recorder.recordNetworkEvent(...) — Level.CRITICAL, then request.method(), then urlSanitizer.apply(request.url()).
4. The sanitizer throws IllegalStateException. recordNetworkEvent is never invoked.
5. Control transfers to the catch at line 48; line 49–52 logs WARNING with header "NetworkTelemetryRecorder.recordNetworkEvent threw an exception".
6. The header is factually incorrect — the recorder did not throw and was not even called.

How to fix it

Lift the sanitizer call out of the inner try so its failures are attributed correctly:

if (response.code() >= 400 && recorder != null) {
  String sanitizedUrl = urlSanitizer.apply(request.url());
  try {
    recorder.recordNetworkEvent(Level.CRITICAL, request.method(), sanitizedUrl, String.valueOf(response.code()));
  } catch (Exception recorderException) { ... }
}

If you also want to keep the interceptor robust to a buggy sanitizer (the current code happens to suppress sanitizer exceptions, which arguably is the desired behavior), wrap the sanitizer call in its own try/catch with a distinct message such as "urlSanitizer threw an exception". Either approach yields a log entry whose header matches its cause.

What the bug is

DEFAULT_URL_SANITIZER (line 19-20) is defined as:

private static final Function<HttpUrl, String> DEFAULT_URL_SANITIZER =
    url -> url.newBuilder().query(null).build().toString();

It only nulls out the query string. OkHttp's HttpUrl.toString() is the canonical, non-redacted serialization — it includes userinfo (username:password@) when present, and includes the fragment (#…). OkHttp provides a separate HttpUrl.redact() method specifically for masking these. The default sanitizer never touches them.

Step-by-step proof

1. Caller issues Request.Builder().url("https://alice:[email protected]/charge?token=abc#section").
2. Server returns 500. response.code() >= 400 is true → interceptor calls urlSanitizer.apply(request.url()).
3. url.newBuilder().query(null).build() produces an HttpUrl with username="alice", password="hunter2", fragment="section", query=null.
4. .toString() on that URL yields https://alice:[email protected]/charge#section.
5. The recorder receives a URL string containing the basic-auth credentials and the fragment. The README's recommended recorder forwards url straight to rollbar.recordNetworkEventFor(...), sending the credentials to Rollbar's servers.

Addressing the refutations

The strongest refutation argues this is intentional, documented behavior with a clear escape hatch: the README Security section states the default "strips query parameters," the Javadoc on NetworkTelemetryRecorder.recordNetworkEvent explicitly says "with query parameters stripped by default," and a constructor accepting a custom Function<HttpUrl, String> is provided. That is fair — the contract is narrowly scoped and matched by the implementation. URL-embedded basic-auth is also rare in modern API usage where Authorization headers dominate, and HTTP fragments are not transmitted to servers (they only ever appear in request.url() if a developer placed them there).

The reason this is still worth flagging is framing: this PR explicitly added URL sanitization as a security feature in response to a prior review comment. The README's Security heading reads as defense-in-depth against "accidental leakage of sensitive data," and a reasonable user might assume the default catches the obvious cases. Hardening the default to also clear userinfo and fragment is a one-line change with zero realistic regression cost (no legitimate caller needs basic-auth credentials echoed back into telemetry), and it brings the implementation in line with what HttpUrl.redact() already encapsulates for exactly this use case.

Suggested fix

Either tighten the default:

private static final Function<HttpUrl, String> DEFAULT_URL_SANITIZER =
    url -> url.newBuilder().username("").password("").query(null).fragment(null).build().toString();

…or add a sentence to the README Security section noting that userinfo and fragments are not stripped by the default and recommending a custom sanitizer when those may be present.

Severity rationale

Filing as nit: the contract is documented, an opt-out exists via the second constructor, and the practical exposure is narrow. But it is a worthwhile defensive hardening given the explicit security framing of this default.

What the bug is and how it manifests

The Installation section of rollbar-okhttp/README.md (lines 14–30) shows two and only two dependencies:

implementation("com.rollbar:rollbar-okhttp:<version>")
implementation("com.squareup.okhttp3:okhttp:<okhttp-version>")

Immediately below, the Usage section's recorder example calls rollbar.recordNetworkEventFor(level, method, url, statusCode) and rollbar.log(exception). Both methods live on com.rollbar.notifier.RollbarBase in the rollbar-java module — not in rollbar-okhttp and not in rollbar-api. A new user who follows the install snippet literally will not have any Rollbar class on their compile classpath and will see cannot find symbol: class Rollbar (and method recordNetworkEventFor / method log) when they try the example.

The specific code path

rollbar-okhttp/build.gradle.kts declares its public dependency tree as:

api(project(":rollbar-api"))

rollbar-api contains only payload data classes (Level, RollbarThread, etc.) — no notifier class. The notifier with recordNetworkEventFor and log is in rollbar-java/src/main/java/com/rollbar/notifier/RollbarBase.java. Consumers who pull in only rollbar-okhttp therefore inherit rollbar-api transitively but not rollbar-java, which the example silently assumes.

Why existing code doesn't prevent it

The example uses an unannotated, unconstructed rollbar variable — there is no import line, no Rollbar rollbar = ... initializer, and no callout that the variable comes from a different module. Nothing in the README points the reader to rollbar-java (or rollbar-web/spring/etc.) as a prerequisite, and nothing in the build script of rollbar-okhttp causes a notifier module to be pulled in transitively.

Impact

A first-time adopter of the OkHttp integration who lands on this README and copies the install snippet will get a compile error on the example. Severity is small because most realistic adopters of an OkHttp interceptor for Rollbar will already have rollbar-java configured elsewhere in their app (otherwise they'd have nothing to call recordNetworkEventFor on), but for a clean module-specific quickstart it is still a documentation gap.

Step-by-step proof

1. New user reads rollbar-okhttp/README.md and adds only the two listed dependencies to their build.gradle.kts: com.rollbar:rollbar-okhttp and com.squareup.okhttp3:okhttp.
2. Gradle resolves the compile classpath: rollbar-okhttp brings rollbar-api transitively (declared as api in its build script) plus okhttp.
3. User copies the recorder example, which references rollbar.recordNetworkEventFor(...) and rollbar.log(...).
4. grep -r "class Rollbar " rollbar-api → no match. grep -rn recordNetworkEventFor rollbar-api → no match.
5. grep -rn "recordNetworkEventFor" rollbar-java/src/main/java/com/rollbar/notifier/RollbarBase.java → defined at line 102 (and log(Throwable) is also defined here).
6. javac fails: error: cannot find symbol: class Rollbar and (after manually importing) error: cannot find symbol: method recordNetworkEventFor.

How to fix it

Add a third line to both the Kotlin DSL and Groovy install snippets — implementation("com.rollbar:rollbar-java:<version>") — or add a one-line note above the snippet stating that one of the rollbar-* notifier modules (e.g. rollbar-java, rollbar-web, rollbar-spring-boot-webmvc) must also be on the classpath to obtain the Rollbar notifier used by the example.

What the bug is

rollbar-okhttp/build.gradle.kts line 8 has testImplementation("org.mockito:mockito-core:5.23.0"), while the root build.gradle.kts subprojects block (line 52–56) already adds testImplementation "org.mockito:mockito-core:5.18.0" to every non-example, non-Android subproject. The result is two versions of mockito-core on the test classpath for this single module, and divergence from every other rollbar-* module that quietly inherits 5.18.0 (rollbar-reactive-streams, rollbar-web, rollbar-java, etc., none of which redeclare mockito locally).

Why the build still works today

Gradle's default conflict resolution strategy for the same module-coordinate with two versions is highest-wins. Both 5.18.0 and 5.23.0 are valid, the API surface used by these tests is stable across both, and so resolution picks 5.23.0 and tests run green. There is no compile or runtime error, and no test failure. That is exactly why the divergence is silent and why this is filed as a maintainability concern rather than a functional bug.

Why it still matters

Two concrete maintenance hazards:

1. Inversion on the next root bump. If someone bumps the root mockito (say to 5.24+) for a security or fix reason, every other module picks it up, but rollbar-okhttp stays pinned to 5.23.0 — a strictly older version than the root intended. The local override silently regresses the very thing the root bump was meant to fix.
2. Discoverability. A developer searching for "which mockito version do we use" finds 5.18.0 in the root and trusts it, then later discovers via dependency reports that one module quietly uses something else.

Why existing code doesn't prevent it

There is no version-alignment task or platform/BOM constraint enforcing a single mockito version across the SDK. The root subprojects block adds the dependency by string coordinate, and a local override on the same coordinate stacks rather than shadows; Gradle simply resolves the conflict at configuration time without any warning.

Step-by-step proof

1. Gradle evaluates rollbar-okhttp/build.gradle.kts after the root build script.
2. Root subprojects block has already added org.mockito:mockito-core:5.18.0 to testImplementation for rollbar-okhttp (line 55).
3. The local file then adds org.mockito:mockito-core:5.23.0 to testImplementation (line 8).
4. ./gradlew :rollbar-okhttp:dependencies --configuration testRuntimeClasspath will list both and report the resolution as 5.23.0 with a "selected by conflict resolution" annotation.
5. ./gradlew :rollbar-web:dependencies --configuration testRuntimeClasspath shows only 5.18.0 — a real per-module version skew.

How to fix it

Either drop line 8 entirely (preferred — matches every other module) so the inherited 5.18.0 is used, or, if 5.23.0 was deliberately needed for OkHttp 5 / mockwebserver 5 compatibility, bump the root version on line 55 of build.gradle.kts so all modules upgrade together. The current state guarantees the next mockito bump will diverge again.

What the bug is

RollbarOkHttpInterceptor.java (lines 8 import, 17 declaration, 49/63 usages) imports and uses java.util.logging.Logger:

import java.util.logging.Logger;
...
private static final Logger LOGGER = Logger.getLogger(RollbarOkHttpInterceptor.class.getName());

A grep across the repository shows that every other production module that emits logs uses SLF4J: Rollbar.java, RollbarBase.java, ObjectsUtils.java, AbstractSender.java, BufferedSender.java, DiskQueue.java, ConfigProviderHelper.java, RollbarFilter, the Struts2 interceptor, the Jakarta and javax web filters, the reactive-streams adapters, and the Android module — 23+ files spanning 8+ modules — all import org.slf4j.Logger / org.slf4j.LoggerFactory. rollbar-okhttp is the sole outlier.

Why SLF4J is the SDK convention

rollbar-java/build.gradle.kts declares api("org.slf4j:slf4j-api:1.7.25"), so SLF4J is part of the SDK's public dependency contract. Users who pull in any other Rollbar module already have SLF4J on the classpath, and most production Java applications standardize their logging through SLF4J + Logback / Log4j2. The choice of JUL in this single new module breaks that convention.

Impact

The WARNINGs emitted at lines 49 and 63 are the only signal a user gets when their NetworkTelemetryRecorder implementation throws an exception (e.g. an NPE inside the user's Rollbar wiring). Under JUL, those messages do reach stderr by the default ConsoleHandler, so they aren't completely silent — that's why this is a nit rather than a behavioral defect. However, users with sophisticated SLF4J configurations (structured JSON logging, log aggregators, alerting rules, level filtering, route-by-logger) will not see these warnings unless they explicitly install the jul-to-slf4j bridge. In a typical deployment that has only configured SLF4J, the WARNING ends up in a separate stderr stream that's not collected — making a broken recorder harder to detect than it would be in any other module.

How to fix

Swap the import and field declaration:

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
...
private static final Logger LOGGER = LoggerFactory.getLogger(RollbarOkHttpInterceptor.class);

…and update the call sites (LOGGER.log(WARNING, message, throwable) becomes LOGGER.warn(message, throwable)). One trade-off worth noting: rollbar-okhttp/build.gradle.kts currently depends only on :rollbar-api, which does not transitively expose SLF4J. Either (a) declare api(project(":rollbar-java")) (which already exposes slf4j-api), or (b) add api("org.slf4j:slf4j-api:1.7.25") directly, matching the rest of the SDK.

Step-by-step proof

1. User's app uses Logback as the SLF4J binding and has not installed jul-to-slf4j.
2. User configures Logback to write structured JSON to a centralized log aggregator and routes JUL output to /dev/null (a common production setup to avoid duplicate / unstructured log streams).
3. User's NetworkTelemetryRecorder.recordNetworkEvent throws an NPE due to a bug in their Rollbar wiring.
4. The interceptor's catch (Exception recorderException) block calls LOGGER.log(WARNING, ..., recorderException) — using JUL.
5. Because JUL is not bridged to SLF4J and JUL output is suppressed in this configuration, the WARNING is lost.
6. The user sees no telemetry events from 4xx/5xx responses and has no log signal indicating their recorder is broken.
7. If the same logger had used SLF4J, the WARNING would have been emitted via Logback and surfaced in the aggregator like every other warning the SDK emits.