[HDX-4127] Browser SDK: mask request/response headers and bodies#239
[HDX-4127] Browser SDK: mask request/response headers and bodies#239
Conversation
🦋 Changeset detectedLatest commit: 3f08340 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Add a maskFields option to HyperDX.init in the Browser SDK. When advancedNetworkCapture is enabled, matching headers and JSON body fields are replaced with '***' before they are recorded as span attributes, so sensitive data never leaves the client. Header matches are case-insensitive. Body matches walk through nested JSON objects and accept dotted paths (e.g. 'creditCard.number'). Non-JSON request/response bodies are passed through unchanged. Drive-by: switch headerCapture from for...of over a Map to Map.forEach so the function is exercisable by the node-mocha test runner, which currently resolves an ES5-targeted ts-node config and silently no-ops Map iterators without downlevelIteration. Production behaviour is identical. Refs HDX-4127
|
It seems like the Maybe a deno version mismatch? |
| body: unknown, | ||
| fieldsToMask: string[] | undefined, | ||
| ): string { | ||
| const original = typeof body === 'string' ? body : jsonToString(body); |
There was a problem hiding this comment.
Seems odd to convert an object into a string, only to parse it back into an object. Couldn't we just do
let parsed: unknown;
try {
parsed = typeof body === 'string' ? JSON.parse(body) : body;
} catch {
return jsonToString(body);
}
and then eliminate line 244? It would skip one stringify call cycle on every call to maskBody.
There was a problem hiding this comment.
Good catch — fixed in abb4e33. The new maskBody skips jsonToString when body is already an object and only calls JSON.parse on string bodies, so the redundant round-trip is gone.
| try { | ||
| return JSON.stringify(masked); | ||
| } catch { | ||
| return original; |
There was a problem hiding this comment.
Since the catch behavior is the same for both, why not expand the try { ... } block to cover everything from line 250 through 262. Unless we specifically want exceptions from maskJsonValue to not be caught.
There was a problem hiding this comment.
Done in abb4e33. The two try/catch blocks are collapsed into one wider block covering parse → clone → mask → stringify. Any failure now uniformly falls back to the original body.
| const lowerPath = path.toLowerCase(); | ||
| for (const field of fieldsToMask) { | ||
| const lowerField = field.toLowerCase(); | ||
| if (lowerField === lowerPath || lowerField === lowerKey) { |
There was a problem hiding this comment.
I'm not understanding why we need to check both the path and the key here. In fact it seems that including the key in the or is problematic.
Case 1: No nesting
fieldsToMask = ['myField']
inputObject = {
"myField": "xyz"
}
This correctly results in a call of matchesField("myField", "myField", fieldsToMask). The second equality check is redundant since both the path and the key are the same value.
Case 2: Nesting
fieldsToMask = ['inner.myField']
inputObject = {
"inner": {
"myField": "xyz"
}
}
This correctly results in a call of matchesField("myField", "inner.myField", fieldsToMask). In this kind of nested setup, the key will never match.
Case 3: Nesting with multiple fields
fieldsToMask = ['myField']
inputObject = {
"myField": "abc",
"inner": {
"myField": "xyz"
}
}
This is going to have two calls here
matchesField("myField", "myField", fieldsToMask) is going to correctly match and mask the field.
matchesField("myField", "inner.myField", fieldsToMask) is where the implementation seems broken. Since the key is going to match a value in fieldsToMask, it will become masked even though the config did not include the nested path.
This could be fine if we allowed something with a leading dot to anchor to the root, like .myField. But we set currentPath to an empty string and then line 287 will never add a dot prefix when traversing. I think we either need to just match on the path or use a default starting value of '.' in order to handle root anchoring. There could be situations where ambiguous field names should be redacted based on context (e.g. nesting level).
There was a problem hiding this comment.
Agreed, this was inconsistent with the dotted-path docs. Fixed in abb4e33: body matching is now path-exact via lodash.has / lodash.set. A bare key like 'token' only matches a root-level token; nested fields require their full path (e.g. inner.token). Body matching also became case-sensitive as a side effect — JSON object keys are case-sensitive by spec, so this is more correct anyway. Header matching stays case-insensitive. README and JSDoc updated to make the contract explicit, and a new test asserts the Case 3 expectation (nested token is left alone when only 'token' is configured).
One ergonomic regression worth flagging: under the new semantics, masking the same field at multiple paths/array indices requires enumerating each path. Wildcards (e.g. **.password) are tracked as part of the broader unification effort in HDX-4148 alongside the requestHook/responseHook escape hatch.
| assert.deepStrictEqual(span.attributes['http.request.header.authorization'], [ | ||
| DEFAULT_MASK_PLACEHOLDER, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Probably needs a test case to handle a request where there are multiple headers and the values would be an array. There's code to mask each array value but nothing to validate it.
There was a problem hiding this comment.
Added in abb4e33: new test 'masks each element when the header value is an array' passes getHeader returning a string[] for a set-cookie-style header and asserts every element is replaced with the placeholder.
- Switch body field matching from bare-key-or-path to path-exact via lodash.has/lodash.set. Bare keys like 'password' now only match root-level fields; nested fields require their full dotted path. Aligns the JSDoc and README with the actual behavior. - Skip the redundant stringify->parse round-trip in maskBody when the body is already an object. - Collapse the two try/catch blocks in maskBody into one wider try so a fault in cloneDeep/has/set or stringify falls back to the original body uniformly. - Body matching becomes case-sensitive (JSON object keys are case-sensitive by spec). Headers stay case-insensitive. - New test exercises array-valued header masking (e.g. set-cookie). - README updated with header vs. body semantics, indexed-array example, case-sensitivity note. Refs HDX-4127, PR #239 review by @dhable
Modern Chromium routes errors thrown inside setTimeout callbacks and `<img>` load failures through 'unhandledrejection' rather than the window 'error' or document 'error' events, depending on async-task runtime semantics that have shifted across Chrome versions. The HyperDXErrorInstrumentation captures all three sources equivalently; only the resulting span name differs. Loosen the assertions in the 'test error' and 'test unloaded img' suites so either span name is accepted. The instrumentation behavior under test is unchanged. Refs HDX-4127
|
Update: pushed CI is now green: unit run ✓. |
2 participants
Summary
Add support to the Browser SDK for masking sensitive fields in captured HTTP
request/response headers and bodies before any telemetry leaves the
client. Closes HDX-4127 and
addresses upstream hyperdxio/hyperdx-js#199.
API
creditCard.number).intentionally conservative to avoid mangling form-encoded or binary
payloads.
'***'. Masking happens before the valueis set as a span attribute, so sensitive data never leaves the browser.
Why no
maskPlaceholderoption?The original ticket proposed a configurable
maskPlaceholder, but nocomparable browser RUM SDK exposes one (Sentry and Datadog both rely on
beforeSend-style callbacks for redaction) and the OpenTelemetry HTTPsemantic conventions hardcode
REDACTEDfor sensitive query parameters.Shipping with a hardcoded
'***'keeps the API minimal; the option can bereintroduced non-breakingly later if a real need surfaces.
Packages touched
@hyperdx/otel-web— masking utilities (maskBody,shouldMaskHeader,extended
headerCapture) and plumbing intoHyperDXFetchInstrumentation+HyperDXXMLHttpRequestInstrumentation.@hyperdx/browser— exposesmaskFieldsonBrowserSDKConfigandforwards it to the fetch/xhr instrumentations.
Changeset:
@hyperdx/browserminor,@hyperdx/otel-webpatch. The patchbump on otel-web keeps
@hyperdx/otel-web-session-recorder's^0.16.2peer-dep range satisfied so it does not get force-bumped to a new major.
Notes
headerCapture: switched fromfor...ofover theinternal
MaptoMap.prototype.forEachso the function is exercisableby the node-mocha test runner (
test:unit:ci-node), which currentlyresolves an ES5-targeted ts-node config and silently no-ops Map iterators
without
downlevelIteration. Production behaviour is identical.Testing
packages/otel-web/test/masking.test.ts(17 tests)covering header masking, body masking (top-level, nested, dotted paths,
arrays, non-JSON bodies, non-string bodies) and case-insensitive
matching.
yarn workspace @hyperdx/browser ci:lint— passes (warnings only,pre-existing).
yarn workspace @hyperdx/browser ci:unit— passes.yarn workspace @hyperdx/browser build— passes.yarn ci:unit— passes (5 suites, 27 tests).yarn ci:buildandyarn ci:lintsurface pre-existing failures in@hyperdx/instrumentation-exception(Sentry types drift) and@hyperdx/deno(deno lintnpm:imports), reproducible onmainandunrelated to this change.
Follow-up work
Tracked in HDX-4148 —
Unify HTTP capture/mask config across
@hyperdx/browserand@hyperdx/node-opentelemetry. That ticket covers:'***'→ shared'[Filtered]',matching Node and Sentry relay).
Node already applies automatically.
requestHook/responseHookon both SDKs asuser-overridable config (Node has them internally but unsurfaced; Browser
doesn't have them at all).
field-level JSON walk that this PR introduces on Browser, so both SDKs
share one masking algorithm.
Links