[Feat][SDK-601] Add built in scrubbing#377
Conversation
|
@claude review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52d5d40da7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (body == null || fieldPatterns.isEmpty()) { | ||
| return body; | ||
| } | ||
| BodyContent content = body.getContents(); |
There was a problem hiding this comment.
When logging a Throwable with a Thread, the default wrapping path lets BodyFactory populate Body.rollbarThreads with trace chains in addition to bodyContent, and the initial thread can carry the same Frame.locals from ThrowableCache. This method only reads body.getContents(), so redactedKeys scrub the top-level trace but the serialized threads entry can still contain password/token locals. Please walk body.getRollbarThreads() and rebuild those groups as well.
Useful? React with 👍 / 👎.
| String key = eq >= 0 ? pair.substring(0, eq) : pair; | ||
| if (matchesAny(key, patterns)) { |
There was a problem hiding this comment.
When Request.queryString is populated from servlet getQueryString(), parameter names are still percent-encoded. Matching the raw substring here means a request such as pass%77ord=hunter2 is semantically the password parameter and the parsed get map is scrubbed, but the raw query_string sent alongside it remains unchanged and leaks the value for a normal redactedKeys(["password"]) configuration.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| String originalQueryString = req.getQueryString(); | ||
|
|
||
| String scrubbedUrl = originalUrl != null ? urlSanitizer.sanitize(originalUrl) : null; | ||
| Map<String, String> scrubbedHeaders = scrubStringMap(originalHeaders); | ||
| Map<String, List<String>> scrubbedGet = scrubMultiMap(originalGet, fieldPatterns); | ||
| Map<String, Object> scrubbedPost = scrubObjectMap(originalPost, fieldPatterns, 0); | ||
| String scrubbedQueryString = scrubQueryString(originalQueryString, fieldPatterns); | ||
|
|
||
| boolean changed = !equal(originalUrl, scrubbedUrl) | ||
| || scrubbedHeaders != originalHeaders | ||
| || scrubbedGet != originalGet | ||
| || scrubbedPost != originalPost | ||
| || !equal(originalQueryString, scrubbedQueryString); | ||
|
|
||
| if (!changed) { | ||
| return req; | ||
| } | ||
|
|
||
| return new Request.Builder(req) | ||
| .url(scrubbedUrl) | ||
| .headers(scrubbedHeaders) | ||
| .get(scrubbedGet) | ||
| .post(scrubbedPost) | ||
| .queryString(scrubbedQueryString) | ||
| .build(); | ||
| } | ||
|
|
||
| private Body scrubBody(Body body) { | ||
| if (body == null || fieldPatterns.isEmpty()) { |
There was a problem hiding this comment.
🔴 The built-in scrubber has three coverage gaps where user-populated data flows around redactedKeys:
Request.params— routing parameters (e.g./reset/:token) are copied throughnew Request.Builder(req)and serialized under"params"butscrubRequestnever touches them.Request.metadata— flattened directly into the outgoing JSON viavalues.putAll(metadata)(Request.java:141-143), structurally identical tocustomwhich is scrubbed.Frame.localsinbody.rollbarThreads— the JVMTI-cached locals attached byBodyFactory.updateInitialRollbarThreadare duplicated underbody.threads[].group.trace_chain.traces[].frames[].localsand shipped in the clear even though the same values underbody.trace_chainare scrubbed.
Gap #3 is the load-bearing one — it is a silent data leak on JVMTI-enabled deployments that use redactedKeys. Fix: reuse scrubStringMap for params, scrubObjectMap for metadata, and iterate body.getRollbarThreads() alongside the existing Trace/TraceChain branches in scrubBody.
Extended reasoning...
What the bug is
ScrubDataTransformer promises redaction across "query params, POST params, custom data, and Frame.locals" (matching the PR description). But three payload slots that carry user-populated data bypass it entirely because they are copied through unchanged by the respective Builder(other) copy constructors.
1. Request.params (routing parameters)
Request has a params field of type Map<String, String> (Request.java:29), exposed via getParams(), serialized under the JSON key "params" (Request.java:153-155), and copied through by new Request.Builder(request) (Request.java:319). ScrubDataTransformer.scrubRequest() reads url/headers/get/post/queryString but never touches params. Frameworks that expose routing templates (e.g. path parameter /reset/:token) can populate this map with values matching a user's redactedKeys, and those values reach Rollbar untouched.
2. Request.metadata (user extension slot)
Request.metadata (Map<String, Object>, Request.java:41) is flattened directly into the outgoing JSON via values.putAll(metadata) at Request.java:141-143 — so any user-supplied key ends up as a top-level entry under request. It is copied through by Request.Builder(request) and never read by scrubRequest. It is structurally identical to custom, which is scrubbed. The refutation notes correctly that no in-tree provider populates metadata — but that is exactly why fixing this is cheap: a user who plumbs a custom provider with metadata still reasonably expects their configured redactedKeys to apply, since the PR contract says "custom data" is covered and metadata is the same shape.
3. Frame.locals in body.rollbarThreads (the load-bearing gap)
This is where the leak is not hypothetical. Body.getRollbarThreads() (Body.java:45) is serialized under the JSON key "threads" (Body.java:71). BodyFactory.updateInitialRollbarThread (BodyFactory.java:145-152) builds the primary thread's TraceChain via traceChain(throwableWrapper, description), which uses the frames(ThrowableWrapper) overload at BodyFactory.java:211-235 — line 228 attaches JVMTI-cached locals via cachedFrames[j].getLocals(). ScrubDataTransformer.scrubBody() only iterates body.getContents() (Trace/TraceChain branches, lines 152-176 of the transformer) and Body.Builder(body) copies rollbarThreads through unchanged (Body.java:146).
Step-by-step proof (case 3, the real leak)
- User configures
.redactedKeys(Arrays.asList("password"))on a deployment runningrollbar-jvmtiwith locals capture enabled. - A method with a
String password = "hunter2"local throws an uncaught exception on a worker thread. BodyFactory.from(...)builds bothbodyContent(Trace/TraceChain) and callsmakeRollbarThreads()/updateInitialRollbarThread, which rebuilds the TraceChain and attaches the JVMTI locals tobody.rollbarThreads[0].group.traceChain.traces[].frames[].locals.Data.getBody()reachesScrubDataTransformer.transform → scrubBody.scrubBodyrecurses intobody.getContents()and correctly replacespassword → "***"underbody.trace_chain.trace.frames[].locals.scrubBodyreturns without ever callingbody.getRollbarThreads(). The duplicatedpassword: "hunter2"underbody.threads[0].group.trace_chain.traces[].frames[].localsis copied through byBody.Builder(body).build()and serialized to Rollbar in the clear.
Net effect: the exact same key/value the user asked to be redacted is present twice in the payload — scrubbed under trace_chain, plaintext under threads.
Why existing code doesn't prevent it
There is no code path in ScrubDataTransformer that traverses body.getRollbarThreads(). The Body.Builder(Body) copy constructor preserves the field verbatim (Body.java:146), so even if scrubBody rebuilds the body it does not re-scrub the threads list. The same argument applies to Request.params / Request.metadata: their respective builder copy constructors preserve the maps unchanged (Request.java:319, 325).
Addressing the refutation on metadata
The refutation is right that metadata is a user-controlled extension slot with no in-tree populator, and that a user could scrub it themselves in their provider. But the same argument holds for custom, which IS scrubbed — the whole point of this PR is that the SDK provides scrubbing so users do not have to do it in their own providers. If custom is in scope, metadata (same shape, same population pattern, same key-space semantics) should be too. This one is fair to defer as a followup, but the fix is a two-line addition.
How to fix
Three small additions localized to ScrubDataTransformer:
// in scrubRequest, alongside the existing scrubStringMap/scrubObjectMap calls:
Map<String, String> scrubbedParams = scrubStringMap(req.getParams());
Map<String, Object> scrubbedMetadata = scrubObjectMap(req.getMetadata(), fieldPatterns, 0);
// ...
return new Request.Builder(req)
.url(scrubbedUrl)
.headers(scrubbedHeaders)
.params(scrubbedParams)
.get(scrubbedGet)
.post(scrubbedPost)
.queryString(scrubbedQueryString)
.metadata(scrubbedMetadata)
.build();
// in scrubBody, iterate body.getRollbarThreads() and rebuild each RollbarThread whose
// Group.traceChain has scrubbable frames, then set it via Body.Builder.rollbarThreads(...).Severity normal because case 3 is a silent, security-relevant leak in a first-party feature (JVMTI locals capture) that the redactedKeys config exists specifically to prevent.
1 participant
Description of the change
Adds a scrubbing layer that runs on every payload, so sensitive data is redacted by default rather than only when a user wires up their own transformer.
What it does
ScrubDataTransformerruns after any user-supplied transformer and cannot bebypassed by replacing it. It redacts values with
***in:Authorization,Cookie,Set-Cookie,X-Api-Key,X-Auth-Token,X-Access-Token,X-Secret,Proxy-Authorization,WWW-Authenticate). Always on, no config needed.Frame.locals— for keysmatching the new
redactedKeysconfig. Nested custom maps are traversed to adepth of 8.
Request.urlis sanitized unconditionally byDefaultUrlSanitizer, which stripsuserinfo, query string, and fragment.
Config
Added to both the core and reactive-streams builders. Defaults are an empty
redaction list and DefaultUrlSanitizer.
Warning
Behavior change
Query strings are now stripped from Request.url by default. Users who relied on
seeing them in Rollbar will need a custom urlSanitizer.
okhttp
RollbarOkHttpInterceptor now delegates its default URL sanitization to the
shared DefaultUrlSanitizer so the okhttp and notifier paths can't drift apart,
and gains a withSharedUrlSanitizer(recorder, sanitizer) factory to reuse the
sanitizer from a notifier config.
The sanitizer types live in rollbar-api (com.rollbar.api.scrubbing), not
rollbar-java, so rollbar-okhttp keeps its existing lightweight dependency.
The factory is static rather than a constructor overload — UrlSanitizer and
StringUrlSanitizer are both functional interfaces, so an overload would make a
lambda argument ambiguous and break existing callers at compile time.
Type of change
Related issues
Checklists
Development
Code review