From d4076d75f2dfd448bc51917104ef018a06cce848 Mon Sep 17 00:00:00 2001 From: vvillait88 Date: Thu, 27 Aug 2026 18:22:01 -0400 Subject: [PATCH] Fail closed when the gate cannot confirm an approval The gate read `const allow = decision === 'allow' || decision == null`, so an assess response carrying no decision was treated as an approval. Paired with the API silently ignoring an unrecognised policy key, a merchant could send a misspelled rule and have both layers agree to let the request through, with a healthy 200 at every step. Only the literal string 'allow' is an approval now, so null, undefined and any decision value added later are not. Two failures that used to look identical are now separated, because they call for opposite handling. A missing decision is an UNREADABLE RESPONSE, indistinguishable from something truncated or mangled by a proxy. It is treated as api_error and honours the merchant's explicit failOpen choice exactly as an unreachable API already does, returning 503 rather than 403 so an agent retries instead of going off to get verified. What changed is that it is no longer a silent allow for merchants who never opted into failing open. An allow whose policy_result did not pass, on a request that DID send a policy, is a COMPLIANCE INTEGRITY failure. We asked for enforcement and the answer does not show it happened, so it denies regardless of failOpen: failing open there would reinstate the exact hole this closes. The check is conditioned on having sent a policy, because a null policy_result is the correct response for every ungated call and denying on it unconditionally would deny every ungated merchant request in production. The two tests that previously asserted this behavior were named "decision null/undefined treated as allow" and pinned the fail-open as correct. They now assert the opposite, alongside cases for an unrecognised decision value, both failOpen directions, and the ungated path that must keep working. --- src/core.ts | 66 +++++++++++++++++-- tests/express.test.ts | 145 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 200 insertions(+), 11 deletions(-) diff --git a/src/core.ts b/src/core.ts index 7c99fd2..1eb75d3 100644 --- a/src/core.ts +++ b/src/core.ts @@ -891,17 +891,71 @@ export function createAgentScoreCore(options: AgentScoreCoreOptions): AgentScore const decision = data.decision as string | null | undefined; const decisionReasons = (data.decision_reasons as string[]) ?? []; - const allow = decision === 'allow' || decision == null; - cache.set(cacheKey, { allow, decision: decision ?? undefined, reasons: decisionReasons, raw: data }); + // FAIL CLOSED on a response we cannot read as an approval. + // + // This previously read `decision === 'allow' || decision == null`, so a + // response carrying no decision at all was treated as an approval. Combined + // with the API silently ignoring an unrecognised policy key, a merchant + // could send a misspelled rule and have both layers agree to let the request + // through. Only the literal string 'allow' is an approval now; null, + // undefined, and any value we add later are not. + // + // The two failures below are deliberately NOT handled the same way, because + // they are not the same kind of failure. + const decisionIsAllow = decision === 'allow'; + const decisionIsUnreadable = decision == null; - // Compose the per-request signer verdicts and ride them on the RETURN VALUE so the adapter can - // stash them on its per-request state (NOT a shared slot — see buildSignerVerdict). guard: - // wallet-only identity (operator-token / AIT win and signer-match is deliberately not enforced). + // Computed here rather than after the allow decision: the fail-closed + // branches below return early and still owe the adapter these verdicts. const signerVerdict = buildSignerVerdict(identity, data); - // Rides the same assess response, so reading it later costs nothing extra. const operatorHandle = readOperatorHandle(data); + // A policy we SENT must come back evaluated and passing. `policy_result` is + // null for every ungated call, which is correct and must stay an allow, so + // this is conditioned on having sent a policy rather than on the field being + // null. Denying on a null policy_result unconditionally would deny every + // ungated merchant request in production. + const policySent = Object.keys(policy).length > 0; + const policyResult = data.policy_result as { all_passed?: boolean } | null | undefined; + const policySatisfied = !policySent || policyResult?.all_passed === true; + + if (decisionIsUnreadable) { + // No decision field. Indistinguishable from a truncated or proxied + // response, so this is an INFRASTRUCTURE failure and honours the + // merchant's explicit `failOpen` choice, exactly as an unreachable API + // does above. What changed is that it is no longer a silent allow for + // merchants who did NOT opt into that. + console.warn('[gate] /v1/assess returned no decision; treating as api_error'); + if (failOpen) return { kind: 'allow', degraded: true, infraReason: 'api_error' }; + return { kind: 'deny', reason: { code: 'api_error' } }; + } + + if (decisionIsAllow && !policySatisfied) { + // An allow carrying no passing policy_result, for a request that DID send + // a policy. Not an availability problem: the gate was asked to enforce + // something and the answer does not show it was enforced. Denies + // regardless of `failOpen`, because failing open here would reinstate the + // exact hole this closes. + console.warn('[gate] /v1/assess allowed a request whose policy was not evaluated; denying'); + return { + kind: 'deny', + reason: { + code: 'api_error', + decision: decision ?? undefined, + reasons: decisionReasons, + data: data as unknown as AssessResult, + }, + ...(signerVerdict !== undefined && { signerVerdict }), + ...(operatorHandle !== undefined && { operatorHandle }), + }; + } + + const allow = decisionIsAllow && policySatisfied; + + cache.set(cacheKey, { allow, decision: decision ?? undefined, reasons: decisionReasons, raw: data }); + + if (allow) { // SDK populates `quota` on the assess response from X-Quota-* headers when the // API emits them. Surface up to the adapter so merchants can monitor approach-to-cap. diff --git a/tests/express.test.ts b/tests/express.test.ts index 9abdbba..6250af1 100644 --- a/tests/express.test.ts +++ b/tests/express.test.ts @@ -377,28 +377,163 @@ describe('agentscoreGate middleware — cache', () => { }); -describe('agentscoreGate middleware — decision null/undefined treated as allow', () => { +describe('agentscoreGate middleware — a response with no decision fails closed', () => { afterEach(() => { vi.restoreAllMocks(); }); - it('allows when decision is null', async () => { + // These two previously asserted the OPPOSITE, under the name "decision + // null/undefined treated as allow": a response carrying no decision was let + // through. A penetration test found that combined with the API silently + // ignoring an unrecognised policy key, so a misspelled rule could pass both + // layers. A missing decision is now indistinguishable-from-broken and is + // treated as an infrastructure failure, which denies unless the merchant has + // explicitly opted into failOpen. + it('denies when decision is null', async () => { mockFetchOk({ ...ALLOW_RESPONSE, decision: null }); const mw = agentscoreGate({ apiKey: API_KEY }); const req = makeReq(WALLET); - const { res } = makeRes(); + const { res, status, json } = makeRes(); const next = makeNext(); await mw(req, res, next); - expect(next).toHaveBeenCalledOnce(); + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + // api_error at 503, not wallet_not_trusted at 403: the response was + // unreadable, which is our failure and retryable, not a compliance verdict + // against the buyer. The distinction matters to an agent choosing whether to + // retry or to go and get verified. + expect(json).toHaveBeenCalledWith(expect.objectContaining({ + error: expect.objectContaining({ code: 'api_error' }), + })); }); - it('allows when decision field is missing (undefined)', async () => { + it('denies when the decision field is missing (undefined)', async () => { const { decision: _, ...noDecision } = ALLOW_RESPONSE; mockFetchOk(noDecision); const mw = agentscoreGate({ apiKey: API_KEY }); const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + // The merchant's explicit availability choice still applies here, exactly as + // it does for an unreachable API: a missing decision is an unreadable + // response, not a compliance verdict. + it('honours failOpen when the merchant opted into it', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: null }); + const mw = agentscoreGate({ apiKey: API_KEY, failOpen: true }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).toHaveBeenCalledOnce(); + }); + + // An unrecognised decision value must not be an allow either, so adding a new + // decision on the API side cannot silently open the gate on old SDKs. This one + // denies at 403 rather than 503: the API returned a READABLE decision that is + // simply not an approval, which is a verdict rather than an infrastructure + // failure, so it is not retryable and must not honour failOpen. + it('denies an unrecognised decision value', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'review' }); + const mw = agentscoreGate({ apiKey: API_KEY }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(403); + }); +}); + +describe('agentscoreGate middleware — a sent policy must come back evaluated', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + // The other half of the same finding. The API used to ignore an unrecognised + // policy key and answer allow with policy_result null, so the gate approved a + // request whose policy never ran. The API rejects that now, but the gate must + // not depend on the API being correct: if we ASKED for enforcement and the + // answer does not show it happened, that is a deny. + it('denies an allow whose policy_result is null when a policy was sent', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + it('denies an allow whose policy_result did not pass', async () => { + mockFetchOk({ + ...ALLOW_RESPONSE, + decision: 'allow', + policy_result: { all_passed: false, checks: [] }, + }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + // failOpen is an AVAILABILITY setting. It must not reinstate the hole: we + // asked for enforcement and did not get it, which is not an outage. + it('denies even under failOpen, because this is not an availability failure', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY, failOpen: true, requireKyc: true }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + }); + + // The guard is conditioned on having SENT a policy. A null policy_result is + // the correct, normal response for every ungated call, and denying on it + // unconditionally would deny every ungated merchant request in production. + it('still allows an ungated request whose policy_result is null', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).toHaveBeenCalledOnce(); + }); + + it('allows when a sent policy came back passing', async () => { + mockFetchOk({ + ...ALLOW_RESPONSE, + decision: 'allow', + policy_result: { all_passed: true, checks: [{ passed: true, rule: 'require_kyc' }] }, + }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); const { res } = makeRes(); const next = makeNext();