fix(proxy): degrade instead of refusing when no bundle applies - #70
Conversation
A request carrying a bundle placeholder that no bundle covers was answered with 403. That looked protective and was not. A placeholder is synthetic, so forwarding it grants nothing and the upstream answers as it would for any credential it does not accept. What refusing did do was turn every route a scope does not name into a hard client failure. Observed against Codex: its built-in codex_apps connector opens an MCP session at https://chatgpt.com/backend-api/MCP carrying the same placeholder as the Codex backend. A bundle scoped to /backend-api/codex made that a 403, so Codex started with "MCP client for codex_apps failed to start: HTTP 403: credential bundle rejected" — which reads as a proxy fault rather than as the absence of a credential the connector was never granted. Codex itself worked throughout; only the unscoped connector broke, and it broke loudly. Such a request is now forwarded untouched with nothing injected, and the near miss is recorded on the policy log so a bundle whose scope disagrees with where its client actually goes stays visible instead of silent. The boundary is unchanged and is what it always should have been: what gets injected, not what gets through. Scope, origin, method, canonical path, and RequireAll all still decide injection exactly as before — verified by neutering bundleScopeMatches and confirming the off-scope tests fail because the credential leaks, not because a status code changed. The MCP relay follows the same rule: it still never injects a bundle, since it resolves its target from the registered server list rather than from a scope, but it records rather than refuses.
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 3m 4s —— View job Review: fix(proxy): degrade instead of refusing when no bundle applies
Summary: The core change is sound. Two things worth addressing before merge, plus one lower-priority note:
Nothing here is a security leak or a race — it's an observability/documentation gap introduced by moving from "block" to "degrade," which is exactly the kind of thing that's easy to miss when the fix itself is correct. |
Review catch on the degrade change. Every PolicyLogData reaching gatekeeper.go's sink was warned as "policy denial", given a policy.denial span event, and counted by RecordPolicyDenial. That held while every caller of logPolicy was a real refusal — routing the four bundle-skip sites through it made a harmless out-of-scope forward page whoever alerts on the denial rate. The same "looks like a proxy fault" outcome the degrade change fixed for clients, relocated to the operator. PolicyLogData gains Blocking. logPolicy sets it true, so every existing caller is unchanged; a new logPolicyObservation sets it false and the bundle skips use that. The sink logs an observation at info as "policy observation" with a policy.observation span event, and only counts RecordPolicyDenial for blocking entries. Skips stay visible without polluting the alert channel. Also updates a comment that still said a mismatch is denied before an upstream request is constructed; it is now forwarded like any other request. Mutation-checked both directions: marking observations blocking, and marking denials non-blocking, each fail their own test.
|
All three addressed in 31e6b30. 1.
Mutation-checked in both directions, since a single-sided test here would pass on a proxy that had simply stopped flagging anything: 2. Stale comment — fixed. It now reads that bundles are evaluated and applied before 3. CHANGELOG — deliberately not in this PR. This repo adds changelog entries in a separate On the v0.23.0 entry specifically: I'd leave it as written rather than amend it. It accurately describes v0.23.0, which shipped with the 403 — editing it would make the changelog wrong about a released version. The v0.23.1 entry will carry the correction, including that scope mismatches no longer increment |
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 6m 23s —— View job Code review: PR #70
SummaryThe core behavioral change — degrading to "skip injection, forward unchanged" instead of a hard 403 when a bundle placeholder is out of scope — is sound and well-tested:
One real bug found, posted as an inline comment on This PR adds a No other security, race-condition, or error-handling issues found in the diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // request was stopped; false means policy noticed something and let it | ||
| // through. Consumers alert and count on denials, so an observation logged | ||
| // as one would page an operator for traffic that was never blocked. | ||
| Blocking bool |
There was a problem hiding this comment.
This PR introduces Blocking and gates both the log level and RecordPolicyDenial on it (gatekeeper.go:439-441,460-462), with logPolicy/logPolicyObservation as the only two call-sites that set it correctly.
But proxy/postgres.go:849 (untouched by this PR) builds a PolicyLogData{...} literal directly for the Postgres-plane network-policy denial, right before it calls deny(403, ...) and actually closes the connection:
p.proxy.policyLogger(PolicyLogData{
RunID: runID,
Scope: "network",
Operation: "postgres.connect",
Message: "Host not in allow list: " + sniHost,
})Since Blocking zero-values to false, this real, connection-terminating denial now gets logged as "policy observation" at INFO instead of "policy denial" at WARN, and skips proxy.RecordPolicyDenial entirely. Every Postgres-plane network-policy denial becomes invisible to denial-rate alerting — the same class of regression this PR's own tests (TestNetworkPolicyDenialIsStillBlocking, TestSkippedBundleIsNotReportedAsADenial) guard against on the HTTP plane, but with no equivalent coverage for postgres.go.
Suggest setting Blocking: true at postgres.go:849 (or routing it through a shared logPolicy-style helper) as part of this PR, since it's this PR's own new field/semantics that broke it.
There was a problem hiding this comment.
Confirmed and fixed in e0a5e36 — this was a real regression from my own new field, and a good catch on a file the PR never touched.
I went further than setting Blocking: true at that one site, because the site isn't really the bug — the default is. A flag meaning "this is a denial" that defaults to false means every direct PolicyLogData literal, present and future, silently drops out of alerting unless its author knows the field exists. That failure mode is silence in an alerting path, which is strictly worse than noise.
So the field is inverted: it's now NonBlocking, the zero value is a denial, and only logPolicyObservation opts out. postgres.go:849 needs no change and is correct as written; so is any literal someone adds later. Getting it wrong now over-reports rather than going quiet.
Two guards, since you noted there was no coverage on that plane:
TestZeroValuePolicyLogDataCountsAsADenialpins the invariant that protects every literal.TestPostgresNetworkDenialIsCountedAsADenialmirrors the deny-site literal and asserts it survives as a denial.
Mutation-checked: forcing the zero value to NonBlocking fails the invariant test. go vet clean, go test -race ./... green.
Review catch: proxy/postgres.go builds PolicyLogData as a literal for its network-policy denial, immediately before closing the connection. With Blocking defaulting to false, that real refusal was logged as an observation at info and skipped RecordPolicyDenial entirely — every Postgres-plane denial silently left the alert channel. Rather than set the flag at that one site, invert the sense. The field is now NonBlocking, so the zero value is a denial and a literal is correct without its author knowing the field exists. Only logPolicyObservation opts out. A flag that defaults to "observation" fails by going quiet in an alerting path; this one fails by over-reporting, which is the direction to err. Guards added for both halves: the zero value must count as a denial, and the Postgres deny-site literal must survive as one. Mutation-checked — forcing the zero value to NonBlocking fails the invariant test.
|
Claude finished @dpup's task in 1m 7s —— View job Review: fix(proxy): degrade instead of refusing when no bundle applies
The flagged issue is fixed, and fixed well. Commit e0a5e36 inverts the field from Coverage for the fix is real, not just described:
One minor observation, not blocking: Rest of the diff ( I was not able to run Nothing further to fix. This addresses the one open item from the previous review; ready to merge as far as this pass is concerned. |
1 participant
Follow-up to #67, found by running a real Codex session through a scoped bundle.
The problem
A request carrying a bundle placeholder that no bundle covers was answered with
403 credential bundle rejected. That reads as fail-closed but isn't buying anything: the placeholder is synthetic, so forwarding it grants nothing and the upstream rejects it like any bad credential. The cost was real, though — every route a scope doesn't name became a hard client failure.Codex's built-in
codex_appsconnector opens an MCP session athttps://chatgpt.com/backend-api/MCP, carrying the sameAuthorizationplaceholder as the Codex backend. With a bundle scoped to/backend-api/codex, that became:Codex itself worked fine — the bundle matched
/backend-api/codexand real subscription data flowed. Only the unscoped connector broke, and it broke in a way that looks like a proxy bug rather than "this connector was never granted a credential."The change
Out-of-scope requests are forwarded untouched with nothing injected, and the near miss goes to the policy log.
Denied/Reasonon the bundle result becomesSkipped/Reason.The boundary is unchanged. Scope, origin, method, canonical path, and
RequireAllall still decide injection exactly as before — the boundary is what gets injected, not what gets through. This is also what the original design specified: "if any placeholder is absent or different, inject nothing from the bundle." The 403 was an implementation choice beyond that.The MCP relay follows the same rule: still never injects a bundle (it resolves its target from the registered server list, not from a scope), but records rather than refuses.
Verification
The
codex_appssymptom was reproduced red before the fix. Scope enforcement is mutation-checked: neuteringbundleScopeMatchesfails the off-scope tests on all three paths because the credential leaks, not because a status code changed.go vetclean,go test -race ./...green.