fix!: an introspection URL carries no credential, and a raw one is redacted (#140), with the release audit's findings - #141
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Security-sensitive configuration and logging changes warrant final human review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Hardens introspection URL validation and credential redaction, fixes synchronous single-flight failures, and updates provider behavior documentation.
Changes:
- Rejects unsafe introspection URLs at startup.
- Improves URL redaction and error-field allowlisting.
- Fixes single-flight cleanup and updates tests/documentation.
- Minor grammar nit remains in a test comment.
| File | Description |
|---|---|
src/single-flight.mts |
Handles synchronous fetcher failures. |
src/README.md |
Documents logging and timeout behavior. |
src/modes/injection/README.md |
Documents response-read failures. |
src/modes/injection/jwt-bearer-client.mts |
Updates response classification documentation. |
src/logger.mts |
Improves error serialization and URL redaction. |
src/__tests__/single-flight.test.mts |
Tests synchronous failure cleanup. |
src/__tests__/logger.test.mts |
Tests redaction and error allowlisting. |
src/__tests__/config.test.mts |
Tests URL validation. |
README.md |
Updates configuration and provider documentation. |
README.ja.md |
Mirrors documentation updates. |
config/application.schema.mts |
Validates introspection URLs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… checked at boot (#140) `auth.validation.introspect.url` was a bare `z.string()`, and two kinds of value passed it that could never introspect a token, and leaked: - A URL with userinfo. `fetch` refuses to build a request from one, on every call ("Request cannot be constructed from a URL that includes credentials"), and the refusal quotes the URL exactly as configured — not normalised, so a space or an `@` in the password stays as written. The validation path logs that message. The logger's URL redaction did not recognise a raw password with a space or an interior `@`, so such a password reached the log on every request. That takes one misconfiguration, not two: the URL need not be unparseable. - A string that is not a URL, which `fetch` fails to parse, with the same raw quote in its message. The URL must now be an absolute http(s) URL without userinfo, as `auth.injection.providerOrigin` always had to be; anything else stops the process at boot, naming the key. The check is a refine rather than `.url()`, so the message is a fixed one and never quotes the value it refused; the tests pin both that it names the key and that it does not carry the credential. **What changes for an operator:** a deployment that booted with a URL with userinfo, or a string that is not a URL, served only what validation passes through without introspecting — requests with no `Authorization` — and failed every other. It now does not boot. A non-http(s) scheme is refused too, and one of them was worse than failing: `fetch` answers a POST to a `data:` URL with the body it encodes, so `data:application/json,{"active":true}` admitted every token. A deployment configured that way stops at boot as well. Because a deployment that booted before may not boot after, this is marked breaking. The URL carries no credential in any configuration that works: the introspection client authenticates with `auth.validation.client`, or presents the inbound token when that is unset. The tests failed before the schema change: every refusal was accepted. BREAKING CHANGE: a deployment whose INTROSPECT_URL (auth.validation.introspect.url) carries userinfo, is not a URL, or uses a scheme other than http(s) now stops at boot, naming the key, where it used to boot. Such a deployment served only requests with no Authorization and failed every other one — or, with a data: URL, admitted every token. Set INTROSPECT_URL to the endpoint's http(s) URL with no credential in it, and configure CLIENT_ID / CLIENT_SECRET if the introspection client should authenticate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…@` in it (#140) `redactUrlCredentials` matched `[^\s/@]*@` after the scheme. That is right for a URL in its WHATWG spelling, where a space is `%20` and an interior `@` is `%40`, and wrong for the spelling error messages actually carry: `fetch` quotes a URL as it was given, both when it refuses one with credentials and when it fails to parse one. A space in the password stopped the match before the `@`, so nothing was redacted; an interior `@` ended it early and left the rest of the password in the log. The first holds for a perfectly parseable URL: `https://proxy:my pw@auth.test/x` was logged whole on every request. It now takes everything after `://` up to the last `@` before the next `/` or the end of the line. Userinfo may carry a space, an `@`, a `?` or a `#` unencoded and is redacted whole; the match never reaches past a line of a stack, and a URL with a path keeps a query or fragment `@` intact. Known limit, stated in the helper: a `/` inside a raw password cannot be told from the start of a path, and what follows it is not redacted — as before. The configuration change before this one keeps any credential-bearing introspection URL out of the process; this keeps the helper sound for any other raw URL that reaches a message. It errs towards redacting after a bare origin with no path, where an `@` later on the line takes the text before it. Three new cases failed before the change: a raw password with a space (parseable and not) and one with an interior `@`. The `?` and `#` cases were redacted by the old pattern and are pinned so this change keeps them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
F48's allowlist was installed as the serialiser for the `error` key only.
`shutdown.mts` logs its failures under `err`, where pino's own serialiser
applied — and that one copies every enumerable property, which is the
behaviour the allowlist exists to prevent (undici's `HTTPParserError.data`
is the provider's unparsed response). Nothing on the shutdown path carries
a provider body today, so no line leaked; but the guarantee depended on
which name a log line chose, and nothing stopped a future provider-path line
from choosing `err`.
Both keys now go through `serializeLoggedError`. For the one shutdown line
reachable today (a failed `server.close`, no cause) the output is the same
`{ type, message, stack, code }` pino produced. What pino's `err` serialiser
kept and the allowlist drops: `aggregateErrors` of an `AggregateError`, and
the concatenated cause messages — neither of which the proxy produces. The logger header and
`src/README.md` say which keys are covered, and that only an `Error`
instance is recognised.
The new test failed before the change: a `data` field on an Error logged
under `err` reached the line.
Found by the release audit of `v0.6.0..2ba68c0`.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`run` started the leader's fetcher inside an async wrapper whose `finally` cleared the key. A fetcher that threw before returning a promise made the wrapper reject synchronously, so `finally` ran — and deleted nothing — before `pending.set` stored the already-rejected promise. The key stayed pinned to that rejection: every later call for it got the old error without running its own fetcher, until the process restarted. Unreachable from the bundled callers, which all pass `async` functions and so cannot throw synchronously. But `SingleFlight` has been shared by both modes since F5/F23, and a supplied fetcher is anyone's; the header said the slot is cleared "settled either way", which was not true for this case. The fetcher is still started within `run`, before it yields, and a synchronous throw now becomes the flight's rejection; `finally` hangs off the promise the waiters await, so the key is cleared before any of them resumes. A test pins the synchronous start, so the fix did not trade one ordering for another. The new test failed before the change: the entry was still there after the throw. Found by the release audit of `v0.6.0..2ba68c0`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`src/README.md` said both modes share one log vocabulary. They share `requestId` and a per-mode `event` (#134), but not the shape of `error`: validation logs the Error itself, which the allowlist serialises into an object with `message`, `stack` and `cause`; injection logs a string — `err.message`, or `String(err)` for a throw it could not classify (`src/modes/injection/decision.mts`, `exchange.mts`). A query on `error.message` therefore matches validation lines only. The paragraph now says so. Found by the release audit of `v0.6.0..2ba68c0`: #134 listed this difference and was closed with it still in place. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`src/README.md` listed "a timeout" among the proxy's own failures, all logged at `error`. On the session grant a provider `401`'s body is read, bounded, to tell `invalid_client` from an expired session (#95 F47). A timeout during that read leaves `data` null, which falls through to the expired session: `401 session_required`, logged `injection.session_unauthorized` at `info` (`src/modes/injection/session-grant-client.mts`, and the level table in `src/modes/injection/decision.mts`). `session-grant-client.mts` already documents the residual; the directory README now does too, so an operator alerting on error-level timeouts knows this one is not among them. Found by the release audit of `v0.6.0..2ba68c0`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…available one The exchange's error table, the injection README and the jwt-bearer client's header all listed "network error" and "timeout" under `provider_unavailable` without qualification. That holds before the response arrives, where `fetch` rejects. Once a `200` has arrived its body is read through `readBoundedJsonObject` (`src/response-body.mts`), which answers `null` for a body it cannot read — an aborted read or a dropped connection alike — and the jwt-bearer client answers a `null` 200 body as `provider_invalid_response` (`src/modes/injection/jwt-bearer-client.mts`). The two rows in `README.md` and `README.ja.md`, the injection directory's README (`src/modes/injection/README.md`), and the error-code list in the client's header now say which failure is which. The header follows the wording `session-grant-client.mts` already had; per AGENTS.md the file's header is the source of truth, so it had to change with the READMEs. Found by the release audit of `v0.6.0..2ba68c0`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
y1o1
force-pushed
the
fix/140-introspect-url-and-audit
branch
from
September 24, 2026 02:07
3441018 to
9e075e4
Compare
y1o1
changed the title
Copilot on #141: "both names an Error is logged under" read as incomplete. It now says "both keys under which an Error is logged". Comment only. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
On the Copilot overview ("Needs a closer look — security-sensitive configuration and logging changes warrant final human review"): not a code finding, and nothing in this branch answers it. It is a request for the owner's review before merge, which is where it is left. Its one finding (the test comment) is fixed in 4e01f76. For that review, the parts that carry the risk are the boot-time refusal in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #140. Also carries four findings from the release audit of
v0.6.0..2ba68c0(#95). One commit per item; each fix commit is test-first and its test failed against its parent.#140 — a credential in
INTROSPECT_URLreached the logSeverity, corrected (see the correction on #140):
fetchquotes the configured URL as given in both of its refusals, not only when the URL fails to parse. Ondevelop, anINTROSPECT_URLwhose password contains a space or an@— a perfectly parseable URL — wrote that password into the validation path's log on every request. One misconfiguration was enough.fix(config)auth.validation.introspect.urlmust be an absolute http(s) URL without userinfo, refused at boot with a fixed message that never quotes the value.fix(logger)redactUrlCredentialstakes everything up to the last@before the next/or end of line, so a raw password with a space,@,?or#is redacted whole. A/inside a raw password remains a stated limit, as before.The config commit is the real fix: no credential-bearing introspection URL reaches
fetchany more. The logger commit keeps the helper sound for any other raw URL that reaches a message.Release-audit findings
fix(logger)erras well aserror.shutdown.mtslogs undererr, where pino's own serializer copied every enumerable property.fixSingleFlight.run: a fetcher that throws synchronously no longer pins its rejection under the key. Unreachable from the bundled callers (allasync); the fetcher still starts beforerunyields.docssrc/README.md: the log vocabulary is shared, but theerrorfield is an object in validation and a string in injection.docssrc/README.md: one timeout is logged atinfo— a session401whose body read times out.docs200body — timeout or dropped connection — isprovider_invalid_response, notprovider_unavailable:README.md,README.ja.md,src/modes/injection/README.mdand the jwt-bearer client's header.For the cut — breaking
The config commit is marked breaking (
fix(config)!, with aBREAKING CHANGE:footer) on the release owner's decision. A deployment that booted with anINTROSPECT_URLcarrying userinfo, a string that is not a URL, or a non-http(s) scheme will no longer boot. Such a deployment served only requests with noAuthorizationand failed every other — and adata:URL was worse than failing:fetchanswers a POST to one with its encoded body, sodata:application/json,{"active":true}admitted every token.Operator action: set
INTROSPECT_URLto the endpoint's http(s) URL with no credential in it, and configureCLIENT_ID/CLIENT_SECRETif the introspection client should authenticate.This brings the proxy cut's breaking list to ten: F7, F8, F29, F38, F42, F43, F47, F30 and #139 (the last two carry the footer without
!), and this one.Suggested CHANGELOG lines: Changed — BREAKING — the boot-time refusal above. Security — the #140 leak with the corrected severity, the
data:case, and theerrallowlist. Fixed — the single-flight pinning. The three docs commits need no entry.None of the other six commits is breaking: the package is private, so
SingleFlightandserializeLoggedErrorare not public API, and the shutdown line reachable today serializes exactly as before.Checks
?and#, that the severity above was understated, and that thedata:case had been described wrongly; all were folded into the commits they concern.🤖 Generated with Claude Code