fix(sessions): redact connection-URI password in unsupported-URI errors - #602
fix(sessions): redact connection-URI password in unsupported-URI errors#602herdiyana256 wants to merge 4 commits into
Conversation
getSessionServiceFromUri and getConnectionOptionsFromUri interpolated the raw connection URI into their "Unsupported ... URI" error messages. A URI such as postgres://user:password@host/db embeds the password in its userinfo, so an unsupported or mistyped scheme surfaced the password verbatim in a thrown Error, which typically reaches application logs and error-tracking services. Add a redactUriPassword helper that masks the userinfo password (keeping the rest of the URI for debugging, like Go's net/url.URL.Redacted) and use it at both sites.
secretlint's database-connection-string rule flags the literal postgres:// URI in the test as a real credential because "s3cr3t" isn't on its placeholder allowlist. Use "pass", matching the convention already used by the sibling operations_test.ts fixtures.
|
Pushed a follow-up commit to fix the failing @kalenkevich the 4 workflow runs on the new commit are stuck in "awaiting approval" since this is a fork PR could you approve them when you get a chance so CI can finish? |
format:check only surfaced this now that the earlier secretlint failure stopped masking it — the ternary in the catch branch wasn't prettier-formatted.
|
Pushed another follow-up commit — after the secretlint fix let @kalenkevich could you approve the pending workflow runs on this commit too? Should be green after this. |
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
The helper is right and the tests are the ones worth having — the two .rejects.not.toThrow(/hunter2/) assertions test the actual property (the secret is absent) rather than the formatting. new URL() does parse userinfo for non-special schemes like postgres:/oracle:, so the two fixed call sites behave as the tests claim, and the unparseable-input branch fails closed.
One missed call site below — same line, same threat, not covered.
getArtifactServiceFromUri had the same unredacted-URI-in-error-message gap as the two sessions/ call sites fixed earlier in this PR, missed by the original sweep. An artifact store URI (e.g. s3://key:secret@bucket) carries credentials in userinfo just as readily as a session connection string.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Verified the helper's behaviour rather than reading it: I ran the old and new shapes over 300k+ random inputs and the documented cases, and the three exact-equality assertions in the new test hold with no URL.toString() normalization surprises for these schemes. I also grepped for other sites that interpolate a connection URI into an error — the three this PR changes are the complete set of "Unsupported ... URI" throws, so the sweep is done.
One real gap below (query-string credentials), plus a stale description. Neither is a blocker; the PR is a strict improvement as-is.
On CI, so it isn't mistaken for your bug: both red checks are environmental. GitHub Actions Scan flags three unpinned-action findings in .github/workflows/validation.yaml, a file this PR doesn't touch — that job scans the head ref, and this branch is 74 commits behind main, so its stale copy of that workflow shifts the uses: lines and misses the suppressions every up-to-date PR gets. validation failed on macOS only, on app_loader_test.ts timing out at 40000ms; that file doesn't exist on this branch at all (it arrives via the merge ref) and it observed 41.3s and 44.7s against a 40s budget, so it is a borderline-timeout on a slow runner, not anything this change did. Ubuntu ran the identical tree green: 2689 passed, 0 failed, including your redact_uri_test.ts (7 tests). Updating the branch onto main should clear the zizmor one.
| url.password = '***'; | ||
| return url.toString(); | ||
| } | ||
| return uri; |
There was a problem hiding this comment.
Not a nit, though not a blocker either. A credential in the query string is returned verbatim, because url.password is only the userinfo component.
return uri;I checked these against the real helper:
postgres://user:pass@host/db -> postgres://user:***@host/db ok
postgres://user@host/db?password=hunter2 -> unchanged, leaks hunter2
postgres://host/db?password=hunter2 -> unchanged, leaks hunter2
?password= is an accepted form in several of the drivers this function guards, and it reaches exactly the same error path — an unsupported or mistyped scheme with the credential in the query rather than the userinfo. Since the whole point here is that these strings land in logs and error trackers, the narrower coverage is worth closing:
const SECRET_PARAMS = ['password', 'sslpassword', 'passwd', 'pwd'];
let touched = false;
for (const p of SECRET_PARAMS) {
if (url.searchParams.has(p)) {
url.searchParams.set(p, '***');
touched = true;
}
}
if (url.password) {
url.password = '***';
touched = true;
}
return touched ? url.toString() : uri;Caveat I'd want you to weigh rather than take on faith: this makes the function return url.toString() on the query-only path, so a URI with a redacted param gets URL-normalized where today it is passed through byte-for-byte. That is the same normalization the userinfo path already accepts, and I confirmed it is a no-op for the schemes in your tests, but it is a behaviour change on inputs nothing currently covers. Alternatively, scoping the doc comment to "userinfo password" and saying query parameters are out of scope would be an honest, zero-risk resolution.
| const schemeEnd = uri.indexOf('://'); | ||
| return schemeEnd === -1 | ||
| ? '<redacted>' | ||
| : `${uri.slice(0, schemeEnd)}://<redacted>`; |
There was a problem hiding this comment.
Nit, optional — the unparseable fallback is right, and I want to flag one consequence you may already have intended.
: `${uri.slice(0, schemeEnd)}://<redacted>`;This throws away the host and database name for any URI that new URL() rejects, so a genuine typo like postgres//host/db (missing colon) produces <redacted> and the operator loses every clue about what they mistyped. That is the safe direction to err in and I would not change the default, but the error message at the call site now reads Unsupported database URI: <redacted>, which is close to useless for debugging. Worth a sentence in the thrown error telling the user the value was redacted, so they don't think the URI itself was empty.
3 participants
getSessionServiceFromUri (sessions/registry.ts) and getConnectionOptionsFromUri (sessions/db/operations.ts) interpolate the raw connection URI into their "Unsupported ... URI" error messages. A connection URI such as postgres://user:password@host/db carries the password in its userinfo component, so an unsupported or mistyped scheme surfaces the password verbatim in the thrown Error, which typically propagates to application logs and error-tracking services — a different trust boundary from whoever provisioned the connection string.
This adds a redactUriPassword helper that masks the userinfo password while keeping the rest of the URI intact for debugging (mirroring the semantics of Go net/url.URL.Redacted, and the render_as_string(hide_password=True) redaction already used in the adk-python DatabaseSessionService), and applies it at both error sites. Unparseable inputs fall back to returning only the scheme prefix so a credential in a non-URL string is not leaked either. Adds unit and regression tests covering both call sites.