fix(server): redact the URL on a driven NET_DETAIL, and merge on what was requested - #780
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
a44a976 to
cf77e32
Compare
… was requested
`buildNetworkDetail` takes three fields raw from the driver: the headers,
the request body, and the URL. It redacts the first two and passes the
third through. A URL is the field an app is most likely to put a credential
in without meaning to, so a presigned upload, an OAuth callback or a
password-reset link the agent was asked to follow reached the transcript and
the on-disk journal in cleartext, on the drive path only. The in-page
observer redacts the same URL through `redactUrl`.
The second half follows from the first. `mergeNetworkDetail` keys on the URL,
and the two sides disagreed about it exactly when it mattered: a NET_REQUEST
reported `?X-Amz-Signature=[REDACTED]` and the detail reported the signature,
so they never matched. The requests carrying a credential were precisely the
ones whose authoritative response headers and wire body were dropped, and a
duplicate of each survived into the timeline with the credential in it.
So:
- `redactUrl` and `netUrlFields` move to `@reticlehq/core`, beside the key
rule they already call. That is where the wire contract lives, and it is
the only way both sides can run one copy of a heuristic that must not
drift. `packages/browser/src/observers/network-redact.ts` re-exports them,
so every existing import is unchanged. Both take the sensitivity predicate
as an argument now, defaulting to the ambient rule: the browser keeps the
page's own policy, and the daemon passes the session policy it already
resolves per response, because it deliberately has no ambient rule.
- A NET_DETAIL now carries `url` and `urlRaw` on the same contract a
NET_REQUEST has carried them on all along, `urlRaw` omitted when nothing
was rewritten. `withoutUrlRaw` already strips it before an event is
rendered to an agent.
- The merge keys on `urlForMatch`, so the comparison is over what was
actually requested rather than over what each side was willing to display.
That is robust to the two policies differing, which they can: the page may
declare extra keys or exempt one, and the daemon never honours an exemption.
One hardening that came with the move. The fragment sweep matched its key with
an unanchored `([A-Za-z0-9_.-]+)=`, which can begin at every index of a long
run of key-legal characters and rescan the rest of it from each one, quadratic
in the length of the fragment. That was harmless while it only ran in the page
over the app's own URLs. It runs in the daemon now, over URLs read off the
network stack, so the input is no longer the app's to vouch for, and CodeQL
flags it as polynomial on uncontrolled data. The key is now anchored to the
delimiter that must precede it, which costs nothing: `#`, `&`, `?` and `/` are
already outside the key character class, so every key this matched before began
right after one of them. Covered by a 60,000-character delimiter-free fragment
that returns unchanged, plus the OAuth and plain-anchor cases either side of it.
Signed-off-by: Abhijeet Sharma <meetabhijeet05@gmail.com>
cf77e32 to
4a96da1
Compare
|
The failing scaffold moves between runs. Run 1 and run 2: What run 3 actually shows is a port from the previous scaffold. Reading the per-scaffold lines in order:
Why I could not reproduce it locally. Two mechanisms I checked and ruled out, in case they are worth knowing:
|
e845ca2 to
4a96da1
Compare
3 participants
What & why
buildNetworkDetailtakes three fields raw from the driver: the response headers, the request body, and the URL. It redacts the first two, with a comment explaining that this is the one payload reaching the journal and the agent's context without passing through the SDK sanitizer. It passes the third through untouched.A URL is the field an app is most likely to put a credential in without meaning to. On the drive path, all of these reached the agent transcript and the on-disk journal in cleartext, while the in-page observer redacted the identical URL through
redactUrl:isSensitiveKeyalready recognises every one of those parameter names, andredaction.presigned.test.tsexists specifically to pin the presigned-URL case. The rule was there; this path never called it.The second half follows from the first
mergeNetworkDetailkeys on the URL, and the two sides disagreed about it exactly when it mattered. The NET_REQUEST reported?X-Amz-Signature=%5BREDACTED%5Dand the NET_DETAIL reported the signature, so they never matched. Which means the requests that carried a credential were precisely the ones that:The leak and the enrichment miss are the same bug seen from two sides, and one fix closes both.
No issue filed; found while reading the redaction path.
How it was verified
packages/server/src/input/network-detail.url.test.ts, written RED first. Six cases on the builder (query parameter, presigned upload, path-embedded token, userinfo, an ordinary URL returned byte for byte with nourlRaw, andurlRawkept when redaction did rewrite) and one on the merge, which fails onmainwith two events in the timeline instead of one enriched request.The existing
network-detail.test.tspasses unchanged, and so does the fullpackages/browsersuite, which is what pinsredactUrl's behaviour and now exercises it through the core implementation.On the shape of the fix
redactUrlandnetUrlFieldsmove to@reticlehq/core, beside the key rule they already call. The header ofredaction.tssays these live in core because they are a property of the wire rather than of one side of it, and that they moved when the driven path began capturing bodies raw. This is the same migration, one function later.packages/browser/src/observers/network-redact.tsre-exports both, so every existing import is untouched.urlandurlRawon the same contract a NET_REQUEST has carried them on all along,urlRawomitted when nothing was rewritten.withoutUrlRawalready strips it before an event is rendered to an agent, so no new field reaches a transcript.urlForMatch. That compares what was actually requested rather than what each side was willing to display, which matters because the two policies genuinely can differ: a page may declare extra keys or exempt one, andwireRedactionKeysnever carries an exemption to the daemon.One hardening that came with the move
CodeQL flagged the fragment sweep on the first CI run, and it is right. The key was matched with an unanchored
([A-Za-z0-9_.-]+)=, which can begin at every index of a long run of key-legal characters and rescan the rest of it from each one, so it is quadratic in the length of the fragment.That was harmless while it only ran in the page over the app's own URLs. It runs in the daemon now, over URLs read off the network stack, so the input is no longer the app's to vouch for. In other words this PR is what turned a latent shape into a reachable one, so fixing it belongs here rather than in a follow-up.
The key is now anchored to the delimiter that must precede it, which costs nothing:
#,&,?and/are already outside the key character class, so every key this matched before began right after one of them. Covered by a 60,000-character delimiter-free fragment that has to come back unchanged, with the OAuth implicit-flow and plain-anchor cases either side of it so the anchoring cannot quietly stop redacting.A note on
install-gate (windows-latest)This job is red on the branch and green on
main, so I investigated rather than asking for a re-run. I do not believe this change causes it, and here is the work either way.It is not the SDK change. I pushed a revision with
packages/browser/src/observers/network-redact.tsbyte-identical tomain, so the SDK module was untouched by the PR. The gate failed anyway, in the same shape. That revision is reverted, because the duplication it cost bought nothing. Worth noting the isolation was imperfect:@reticlehq/corestill differed, and the SDK bundles core.Two mechanisms measured and excluded.
NET_DETAIL's payload schema is.passthrough(), so the addedurlRawkey is accepted rather than rejected. AndredactUrlcosts 0.55 microseconds per call on the built core over representative Vite dev URLs, so redacting every response URL on the drive path is about half a millisecond per thousand responses.What the logs show is a dev-server port that is not the app's. Across four runs the failing scaffold moves (
vite-vue,vite-vue, thennext-app-routerandmonorepo-subdirwithvite-vuepassing 9/0, thenvite-vueagain), and in every case the only failing assertion isinit exits 0, becauseinitverified against a port the app was not on. In run 3,initfornext-app-routerprobedhttp://localhost:5175, which wasvite-react's dev server from the scaffold before it, while its own app was on:3000and the gate's own next assertion found the session there. That is the third causeinit's message lists, "a bridge port that differs on the two sides", reported accurately.I could not reproduce it locally:
--only vite-vuepasses on this branch, and one scaffold means there is no preceding scaffold to inherit a port from.Four-for-four red against a
mainthat is four-for-four green is a correlation I cannot fully explain, and I am not going to claim flake as if I had proved it. But nothing left in this diff has a path toinit's dev-server port resolution:packages/coregains two functions and one leaf import (net.tsimports nothing, so there is no cycle or evaluation-order change), andpackages/serverchanges only CDP response capture. If this is a known shape on that runner, say so; if you would rather I split thepackages/browserre-export into its own PR so this one carries only the server fix, I will do that.Gates run
pnpm lint && pnpm typecheck && pnpm test:unitpnpm test:e2epnpm gate:installpnpm test:e2e:desktopThis touches
packages/coreand the observed event shape, sotest:e2eis the tier and I could not run it on this machine, so I have left the box unticked. CI rane2e,desktop-e2e,install-gateandverifyon the first push of this branch and all passed; nothing in the fragment-anchoring follow-up changes an event shape.Two unit tests fail on my machine on a clean
mainas well, so they are not from this change: the@reticlehq/nextloader and the@reticlehq/vite-pluginCJS require probe, both 5s timeouts under parallel load on Windows, plusformatGeneratedSource's Prettier sync resolution.Checklist
any, no free strings, no non-null!console.logor internal tracking codesCHANGELOG.mdupdated under[Unreleased]urlRawis the existing grader-only field with the existing strip on the read path, and the localhost-only, nothing-leaves-the-machine posture is untouched.