-
Notifications
You must be signed in to change notification settings - Fork 74
[Feat] [SDK-399] Okhttp interceptor #367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e19ecf3
e141e7e
ab26f42
ed9af20
4f2e019
e7dd96f
ed91c57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| # Rollbar OkHttp Integration | ||
|
|
||
| This module provides an [OkHttp Interceptor](https://square.github.io/okhttp/features/interceptors/) that automatically captures network telemetry for the Rollbar Java SDK. | ||
|
|
||
| It records: | ||
|
|
||
| - **Network telemetry events** for HTTP responses with status code `>= 400` (client and server errors). | ||
| - **Error events** for connection failures, timeouts, and other I/O exceptions. | ||
|
|
||
| ## Installation | ||
|
|
||
| ### Gradle (Kotlin DSL) | ||
|
|
||
| ```kotlin | ||
| dependencies { | ||
| implementation("com.rollbar:rollbar-okhttp:<version>") | ||
| implementation("com.squareup.okhttp3:okhttp:<okhttp-version>") | ||
| } | ||
| ``` | ||
|
|
||
| ### Gradle (Groovy) | ||
|
|
||
| ```groovy | ||
| dependencies { | ||
| implementation 'com.rollbar:rollbar-okhttp:<version>' | ||
| implementation 'com.squareup.okhttp3:okhttp:<okhttp-version>' | ||
| } | ||
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| ### 1. Implement `NetworkTelemetryRecorder` | ||
|
|
||
| ```java | ||
| NetworkTelemetryRecorder recorder = new NetworkTelemetryRecorder() { | ||
| @Override | ||
| public void recordNetworkEvent(Level level, String method, String url, String statusCode) { | ||
| rollbar.recordNetworkEventFor(level, method, url, statusCode); | ||
| } | ||
|
|
||
| @Override | ||
| public void recordErrorEvent(Exception exception) { | ||
| rollbar.log(exception); | ||
| } | ||
| }; | ||
| ``` | ||
|
|
||
| ### 2. Add the interceptor to your OkHttpClient | ||
|
|
||
| ```java | ||
| OkHttpClient client = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(recorder)) | ||
| .build(); | ||
| ``` | ||
|
|
||
| ### 3. Make requests as usual | ||
|
|
||
| ```java | ||
| Request request = new Request.Builder() | ||
| .url("https://api.example.com/data") | ||
| .build(); | ||
|
|
||
| Response response = client.newCall(request).execute(); | ||
| ``` | ||
|
|
||
| The interceptor will automatically record telemetry events to Rollbar without interfering with the request/response flow. | ||
|
|
||
| ## Behavior | ||
|
|
||
| | Scenario | Action | | ||
| |-----------------------------------|---------------------------------------------------------| | ||
| | Recorder is `null` | No telemetry or log is recorded | | ||
| | Response status `< 400` | No telemetry recorded, response returned normally | | ||
| | Response status `>= 400` | Records a network telemetry event with `Level.CRITICAL` | | ||
| | Connection failure / timeout | Records an error event, then rethrows the `IOException` | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| group = "com.rollbar.okhttp" | ||
|
|
||
| dependencies { | ||
| testImplementation(platform("org.junit:junit-bom:5.14.3")) | ||
| testImplementation("org.junit.jupiter:junit-jupiter") | ||
| testRuntimeOnly("org.junit.platform:junit-platform-launcher") | ||
| testImplementation("com.squareup.okhttp3:mockwebserver:5.3.2") | ||
| testImplementation("org.mockito:mockito-core:5.23.0") | ||
| implementation("com.squareup.okhttp3:okhttp:5.3.2") | ||
|
buongarzoni marked this conversation as resolved.
|
||
| api(project(":rollbar-api")) | ||
| } | ||
|
|
||
| tasks.test { | ||
| useJUnitPlatform() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
|
|
||
| public interface NetworkTelemetryRecorder { | ||
| void recordNetworkEvent(Level level, String method, String url, String statusCode); | ||
|
|
||
| void recordErrorEvent(Exception exception); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.logging.Logger; | ||
|
|
||
| import okhttp3.Interceptor; | ||
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
|
|
||
| public class RollbarOkHttpInterceptor implements Interceptor { | ||
|
|
||
| private static final Logger LOGGER = Logger.getLogger(RollbarOkHttpInterceptor.class.getName()); | ||
|
|
||
| private final NetworkTelemetryRecorder recorder; | ||
|
|
||
| public RollbarOkHttpInterceptor(NetworkTelemetryRecorder recorder) { | ||
| this.recorder = recorder; | ||
| } | ||
|
|
||
| @Override | ||
| public Response intercept(Chain chain) throws IOException { | ||
| Request request = chain.request(); | ||
|
|
||
| try { | ||
| Response response = chain.proceed(request); | ||
|
|
||
| if (response.code() >= 400 && recorder != null) { | ||
| try { | ||
| recorder.recordNetworkEvent( | ||
| Level.CRITICAL, | ||
| request.method(), | ||
| request.url().toString(), | ||
|
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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:
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
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. |
||
| String.valueOf(response.code())); | ||
|
Comment on lines
+31
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The interceptor passes Extended reasoning...What the bug is
Why the README makes this actionable The 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 How this differs from comparable libraries The refutation compares this to OkHttp's 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
How to fix This is primarily a documentation gap. The README should include a security callout explaining that |
||
| } catch (Exception recorderException) { | ||
| LOGGER.log(java.util.logging.Level.WARNING, | ||
| "NetworkTelemetryRecorder.recordNetworkEvent threw an exception; " | ||
| + "suppressing to preserve the interceptor contract.", | ||
| recorderException); | ||
| } | ||
| } | ||
|
|
||
| return response; | ||
|
|
||
| } catch (IOException e) { | ||
| if (recorder != null) { | ||
| try { | ||
| recorder.recordErrorEvent(e); | ||
| } catch (Exception recorderException) { | ||
| LOGGER.log(java.util.logging.Level.WARNING, | ||
| "NetworkTelemetryRecorder.recordErrorEvent threw an exception; " | ||
| + "suppressing to preserve the original IOException.", | ||
| recorderException); | ||
| } | ||
| } | ||
|
|
||
| throw e; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
| import okhttp3.OkHttpClient; | ||
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
| import okhttp3.mockwebserver.MockResponse; | ||
| import okhttp3.mockwebserver.MockWebServer; | ||
| import okhttp3.mockwebserver.SocketPolicy; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| class RollbarOkHttpInterceptorTest { | ||
|
|
||
| private MockWebServer server; | ||
| private NetworkTelemetryRecorder recorder; | ||
| private OkHttpClient client; | ||
|
|
||
| @BeforeEach | ||
| void setUp() throws IOException { | ||
| server = new MockWebServer(); | ||
| server.start(); | ||
|
|
||
| recorder = mock(NetworkTelemetryRecorder.class); | ||
|
|
||
| client = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(recorder)) | ||
| .build(); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() throws IOException { | ||
| server.shutdown(); | ||
| } | ||
|
|
||
| @Test | ||
| void successfulResponse_doesNotRecordEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(200)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/ok")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(200, response.code()); | ||
| verifyNoInteractions(recorder); | ||
| } | ||
|
|
||
| @Test | ||
| void redirectResponse_doesNotRecordEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(301).addHeader("Location", "/other")); | ||
|
|
||
| OkHttpClient noFollowClient = client.newBuilder().followRedirects(false).build(); | ||
| Request request = new Request.Builder().url(server.url("/redirect")).build(); | ||
| Response response = noFollowClient.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(301, response.code()); | ||
| verifyNoInteractions(recorder); | ||
| } | ||
|
|
||
| @Test | ||
| void clientErrorResponse_recordsNetworkEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(404)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/not-found")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(404, response.code()); | ||
| verify(recorder).recordNetworkEvent( | ||
| eq(Level.CRITICAL), eq("GET"), contains("/not-found"), eq("404")); | ||
| verify(recorder, never()).recordErrorEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void serverErrorResponse_recordsNetworkEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/error")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(500, response.code()); | ||
| verify(recorder).recordNetworkEvent( | ||
| eq(Level.CRITICAL), eq("GET"), contains("/error"), eq("500")); | ||
| verify(recorder, never()).recordErrorEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void connectionFailure_recordsErrorEvent() { | ||
| server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/fail")).build(); | ||
|
|
||
| assertThrows(IOException.class, () -> client.newCall(request).execute()); | ||
|
|
||
| verify(recorder).recordErrorEvent(any(IOException.class)); | ||
| verify(recorder, never()).recordNetworkEvent(any(), any(), any(), any()); | ||
| } | ||
|
|
||
| @Test | ||
| void postRequest_recordsCorrectMethod() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| Request request = new Request.Builder() | ||
| .url(server.url("/post")) | ||
| .post(okhttp3.RequestBody.create("body", okhttp3.MediaType.parse("text/plain"))) | ||
| .build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| verify(recorder).recordNetworkEvent(eq(Level.CRITICAL), eq("POST"), any(), eq("500")); | ||
| } | ||
|
|
||
| @Test | ||
| void nullRecorder_errorResponse_doesNotThrowNPE() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| OkHttpClient nullRecorderClient = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(null)) | ||
| .build(); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/error")).build(); | ||
| Response response = nullRecorderClient.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(500, response.code()); | ||
| } | ||
|
|
||
| @Test | ||
| void nullRecorder_connectionFailure_doesNotThrow() { | ||
| server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); | ||
|
|
||
| OkHttpClient nullRecorderClient = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(null)) | ||
| .build(); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/fail")).build(); | ||
|
|
||
| assertThrows(IOException.class, () -> nullRecorderClient.newCall(request).execute()); | ||
| } | ||
|
|
||
| @Test | ||
| void recorderThrowsOnErrorResponse_responseStillReturned() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| doThrow(new RuntimeException("recorder boom")) | ||
| .when(recorder) | ||
| .recordNetworkEvent(any(), any(), any(), any()); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/error")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(500, response.code()); | ||
| } | ||
|
|
||
| @Test | ||
| void recorderThrowsOnConnectionFailure_originalIOExceptionPropagates() { | ||
| server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); | ||
|
|
||
| doThrow(new RuntimeException("recorder boom")) | ||
| .when(recorder) | ||
| .recordErrorEvent(any()); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/fail")).build(); | ||
|
|
||
| assertThrows(IOException.class, () -> client.newCall(request).execute()); | ||
| verify(recorder).recordErrorEvent(any(IOException.class)); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.