fix(core): redact URL userinfo credentials in shared redactors - #4597
fix(core): redact URL userinfo credentials in shared redactors#4597Rangsh wants to merge 2 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Astro-Han
left a comment
There was a problem hiding this comment.
Right shape of fix, wrong character class: the userinfo pattern also matches whitespace and newlines, so it deletes benign log text and can wipe an entire PTY screen. Details inline on redaction.ts.
The structural approach is correct and it lands in the right place. It extends both existing shared redactors rather than adding a third, and it respects the documented split between them (display-redaction.ts:26-31). Coverage of the reachable paths is complete without touching any caller: redactUrlUserinfoSecrets runs first in redactTextSecrets, and redactJsonValue routes every string value through redactTextSecrets, so the JSON/persistence path is covered too. That reaches shell-run-manager, pty-screen-collector, shell-tools, tool-runtime, pipe-tail-collector, agent-run, provider-error-classification and the display consumers in pi-transcript-tools, session-todo and tool-quiet-preview.
Edge cases I ran against the head regex, all correct: IPv6 authority (https://user:pass@[::1]:8080/x keeps the host, bare https://[::1]:8080/x untouched), @ in the path (/@user/repo), in the query (?to=a@b.com) and in the fragment (#a@b) all left alone, percent-encoded userinfo masked, uppercase scheme masked, git+https:// masked, two URLs on one line masked independently ([^/?#] cannot cross //), idempotent, and the pre-existing ghp_ case produces the same output as before.
P2: the rule only covers https?://. postgres://user:pass@db:5432/app, mongodb+srv://admin:s3cret@cluster0.mongodb.net/db and redis://:pw@127.0.0.1:6379 still pass through both redactors unchanged and reach persisted logs. That is not a regression from this PR, but the PR's own argument ("any authority that contains @ is credential-bearing, so this does not depend on a provider prefix list") applies to every scheme, and the scheme restriction is the one list still left. Widening to any scheme:// would over-redact ssh://git@github.com/o/r.git, where the username is not a secret, so a reasonable middle ground is an explicit set of credential-bearing schemes (postgres, postgresql, mysql, mongodb, mongodb+srv, redis, rediss, amqp, amqps, ftp), or requiring a : in the userinfo for non-http schemes. If the intent is to keep the scope #4593 defined, a follow-up issue is fine.
P3: during streaming, a partial userinfo is shown in the clear until the @ arrives. The pattern requires @, so https://alice:hunter2 matches nothing and redactStableStreamingSuffix returns no suffix, unlike the authorization header rule whose value group matches a partial token. Detecting it earlier would mean pre-emptively masking everything after any https://, which is worse. Worth a comment noting the window; no behavior change needed.
One observation, not a finding: provider-endpoint-presentation.ts:126-151 (#3639) is the local workaround #4593 names, and its userinfo branch now overlaps this rule. It is not redundant though, since it also masks query values under arbitrary key names while keeping the key names, and it goes through URL parsing so it covers every scheme. Leaving it is right.
| function redactUrlUserinfoSecrets(value: string): string { | ||
| // Authority runs through the first `/`, `?`, or `#`. If it contains `@`, | ||
| // everything from the host-start through the last `@` is userinfo. | ||
| return value.replace(/(https?:\/\/)[^/?#]*@/gi, '$1[redacted]@'); |
There was a problem hiding this comment.
P1: [^/?#]* excludes only /, ? and #, so it also matches spaces, quotes and newlines. Any bare https://host followed later by an @, with no /, ? or # in between, swallows everything between them.
I ran this through the real redactSecrets chain (built dist/redaction.js with this same patch applied):
in: see https://example.com and mail bob@corp.com
out: see https://[redacted]@corp.com
in: Fetching https://registry.example.com\nContact: support@example.com for help
out: Fetching https://[redacted]@example.com for help
Because it crosses newlines, it also trips the whole-screen suppression test in pty-screen-collector.ts:497. With this completely benign npm output:
$ npm install
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org
npm ERR! 404 '@acme-internal' is not in this registry.
the per-line pass at :472 finds all four lines clean, but redactSecrets(complete) !== complete on the joined text is true, so screenText becomes REDACTED_MARKER and scrollbackText is dropped. The whole screen plus scrollback disappears for both the user and the model, with no indication why.
Smallest fix: exclude whitespace from the userinfo class here, /(https?:\/\/)[^\s/?#]*@/gi, and make the display pattern's regex match the terminator set it already declares. Please also add negative assertions for the two cases above, since every test in this PR is a positive case.
| // rule so only the userinfo is replaced and host/path survive. | ||
| { | ||
| label: 'url userinfo', | ||
| regex: /(https?:\/\/)([^/?#]*@)/gi, |
There was a problem hiding this comment.
P1 (same defect as redaction.ts): this pattern already declares streamingTerminator: /[/?#\s"'<>]/ on the next line, but the regex class is only [^/?#]. The two disagree, and the regex is the loose one, so a benign https://host plus a later @ on the same or a following line over-redacts the text in between. Bringing the regex in line with the terminator fixes both the over-redaction and the inconsistency: /(https?:\/\/)([^\s"'<>/?#]*@)/gi.
Summary
Shared redactors in
@maka/corealready masked URL query secrets (?token=…) but left URL userinfo credentials (https://user:pass@host/…) intact unless the secret happened to match a fixed provider prefix such asghp_. That leak showed up in ordinary shell output (git remote -v, push/fetch errors) viashell-run-managerandpty-screen-collector.This PR adds a structural userinfo step to both
redaction.tsanddisplay-redaction.ts(before the query-secret step): for eachhttps?://authority that contains@, replace everything through the last@with the redaction marker while preserving host and path. No provider-prefix list is required. The display pattern also declaresstreamingTerminator/streamingValueGroupso streaming-suffix redaction keeps its contract.Fixes #4593
Verification
npm --workspace @maka/core run buildnode --test packages/core/dist/__tests__/redaction.test.js packages/core/dist/__tests__/display-redaction.test.js— passnpm --workspace @maka/core run test:dist— 798 pass / 0 failnpm --workspace @maka/ui run buildnode --test packages/ui/dist/__tests__/streaming-display-redaction.test.js— 15 pass / 0 failNot run: full monorepo
npm test, lint/format/typecheck across unrelated workspaces.AI use
Select exactly one:
Tool(s) and scope:
Cursor (Composer) drafted the userinfo redaction changes in
packages/core/src/redaction.tsandpackages/core/src/display-redaction.ts, added the covering tests, and prepared the commit/push. Human review and PR submission remain with the author.Checklist
Does this PR entail a change in behavior?