From ec46fee81280ab6af40dbe21b4b867b322e88513 Mon Sep 17 00:00:00 2001 From: Nicholas Carter Date: Sun, 13 Sep 2026 09:57:26 +0000 Subject: [PATCH 1/2] fix(handler): fail closed after Core 503 refusal Stop nextApiHandler from calling proxy after middleware denial. Add refusal/bypass regression tests and bump contracts to 0.2.0 so envelope validation can fail closed. Keep README disclosure until merge. --- package-lock.json | 8 +- package.json | 2 +- .../checklists/fail-closed.md | 71 ++++++ .../checklists/requirements.md | 35 +++ .../contracts/handler-continuation.md | 27 +++ .../data-model.md | 39 +++ specs/002-fail-closed-next-handler/plan.md | 74 ++++++ .../quickstart.md | 29 +++ .../002-fail-closed-next-handler/research.md | 31 +++ specs/002-fail-closed-next-handler/spec.md | 106 +++++++++ specs/002-fail-closed-next-handler/tasks.md | 151 ++++++++++++ src/index.ts | 71 ++++-- tests/router.test.ts | 224 ++++++++++++++++++ 13 files changed, 849 insertions(+), 19 deletions(-) create mode 100644 specs/002-fail-closed-next-handler/checklists/fail-closed.md create mode 100644 specs/002-fail-closed-next-handler/checklists/requirements.md create mode 100644 specs/002-fail-closed-next-handler/contracts/handler-continuation.md create mode 100644 specs/002-fail-closed-next-handler/data-model.md create mode 100644 specs/002-fail-closed-next-handler/plan.md create mode 100644 specs/002-fail-closed-next-handler/quickstart.md create mode 100644 specs/002-fail-closed-next-handler/research.md create mode 100644 specs/002-fail-closed-next-handler/spec.md create mode 100644 specs/002-fail-closed-next-handler/tasks.md diff --git a/package-lock.json b/package-lock.json index 7e8b100..bc184e8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "0.1.0", "license": "MIT", "dependencies": { - "@bodanglin/verdict-contracts": "^0.1.0", + "@bodanglin/verdict-contracts": "^0.2.0", "@types/express": "^5.0.6", "zod": "^3.25.76" }, @@ -529,9 +529,9 @@ "license": "MIT" }, "node_modules/@bodanglin/verdict-contracts": { - "version": "0.1.0", - "resolved": "https://registry.npmjs.org/@bodanglin/verdict-contracts/-/verdict-contracts-0.1.0.tgz", - "integrity": "sha512-N7ILa9OvnaXPi/rqRwq4K1e3dJuWUDCb7nkudgbX8oJk/DEJeTekbyV4BDTKzEEj8lzr3YehWI8iAZ83+aWgyg==", + "version": "0.2.0", + "resolved": "https://registry.npmjs.org/@bodanglin/verdict-contracts/-/verdict-contracts-0.2.0.tgz", + "integrity": "sha512-AGc4FGic3+xDFiOBfSrWCIObZWTNJ9AyDMF+W2wtRVDlmd/DI2syCvarbgpsJQ+sJZWlTklBIRH9uvA8TBKYNA==", "license": "MIT", "engines": { "node": ">=18" diff --git a/package.json b/package.json index 76a9c1e..2bb00f2 100644 --- a/package.json +++ b/package.json @@ -58,7 +58,7 @@ "zod" ], "dependencies": { - "@bodanglin/verdict-contracts": "^0.1.0", + "@bodanglin/verdict-contracts": "^0.2.0", "@types/express": "^5.0.6", "zod": "^3.25.76" }, diff --git a/specs/002-fail-closed-next-handler/checklists/fail-closed.md b/specs/002-fail-closed-next-handler/checklists/fail-closed.md new file mode 100644 index 0000000..7eab9c7 --- /dev/null +++ b/specs/002-fail-closed-next-handler/checklists/fail-closed.md @@ -0,0 +1,71 @@ +# Fail-closed Requirements Checklist: Fail-closed after policy refusal + +**Purpose**: Reviewer-owned unit tests for the written requirements of refuse-then-stop, remaining bypasses, and README disclosure timing +**Created**: 2026-09-06 +**Feature**: [spec.md](../spec.md) + +**Note**: This custom checklist is generated by the `/speckit-checklist` command based on feature context and requirements. +**Review Ownership**: This checklist is a reviewer-owned requirements-quality review artifact. Mark an item `[x]` only when the reviewer determines the requirements-quality criterion is satisfied. +**Marker Semantics**: `[x]` means the criterion has been reviewed and satisfied for requirements quality. It does not mean implementation work is complete. + +## Requirement Completeness + +- [x] CHK001 Are all five policy-refusal cases named (no decision, deny, timeout, invalid payload, no endpoint)? [Completeness, Spec §FR-002] +- [x] CHK002 Is “do not forward after refusal” specified as a hard stop, not a best-effort? [Completeness, Spec §FR-001] +- [x] CHK003 Are missing-envelope, un-rechecked local model, and missing integrity evidence listed as bypasses that must close? [Completeness, Spec §FR-006] +- [x] CHK004 Is README disclosure required to remain until the fix and tests are proven? [Completeness, Spec §FR-004] +- [x] CHK005 Is this lane specified as separate from the already-shipped docs-accuracy work? [Completeness, Spec §FR-005] + +## Requirement Clarity + +- [x] CHK006 Is “refusal” defined as the response already written when policy cannot allow the request? [Clarity, Spec §Key Entities] +- [x] CHK007 Is “every bypass found during the fix” bounded enough that “done” means none remain open? [Clarity, Spec §FR-006, Spec §Clarifications] +- [x] CHK008 Is the compatibility opt-out distinguished from the default policy-gated path? [Clarity, Spec §US1, Plan contracts] + +## Requirement Consistency + +- [x] CHK009 Do FR-004 / US2 and SC-003 agree that the warning is not removed before green required checks? [Consistency, Spec §FR-004, Spec §SC-003] +- [x] CHK010 Do FR-006 / US3 and SC-004 agree that newly found bypasses are in this lane, not deferred? [Consistency, Spec §FR-006, Spec §SC-004] +- [x] CHK011 Does “allow path unchanged” conflict with requiring an envelope on allow (default path)? [Consistency, Spec §Edge Cases, Plan data-model] + +## Acceptance Criteria Quality + +- [x] CHK012 Can “zero upstream forwards” be judged without implementation internals? [Measurability, Spec §SC-001] +- [x] CHK013 Can “100% of found bypasses have a test” be counted from an audit list plus any new finds? [Measurability, Spec §SC-004] +- [x] CHK014 Can README presence of the warning be judged by reading the primary README? [Measurability, Spec §SC-003] + +## Scenario Coverage + +- [x] CHK015 Are primary-path requirements complete for a true allow that still forwards? [Coverage, Spec §SC-002] +- [x] CHK016 Are exception-path requirements complete for all five refusal cases? [Coverage, Spec §US1] +- [x] CHK017 Are recovery/rollback requirements needed, or is revert of this change enough? [Coverage, Spec §Assumptions] + +## Edge Case Coverage + +- [x] CHK018 Is a refusal that was written while continuation would still run specified as must-stop? [Edge Case, Spec §Edge Cases] +- [x] CHK019 Is a missing envelope on the default path specified as refuse, not forward? [Edge Case, Spec §FR-006] +- [x] CHK020 Is an un-rechecked local model swap specified as refuse or recheck? [Edge Case, Spec §FR-006] + +## Non-Functional / Policy Requirements + +- [x] CHK021 Is fail-closed specified without inventing latency or throughput numbers this lane does not need? [Non-Functional, Plan Technical Context] +- [x] CHK022 Does the spec keep the product framed as an enforcement client of the decision service? [Non-Functional, Spec §Assumptions, Constitution I] + +## Dependencies & Assumptions + +- [x] CHK023 Is issue #51 named as the tracking issue for this defect? [Dependency, Spec §FR-005] +- [x] CHK024 Is the isolated-worktree / do-not-write-dirty-checkout constraint documented? [Assumption, Spec §Assumptions] + +## Ambiguities & Conflicts + +- [x] CHK025 Does “allow path does not change” need a note that default-path envelope enforcement is a close of a bypass, not a change to a true allow? [Conflict, Spec §Edge Cases, Spec §FR-006] +- [x] CHK026 Are plan-level details (file paths, test runner) kept out of the spec and only in the plan? [Clarity, Spec vs Plan] + +## Notes + +- Mark items `[x]` only after review confirms the requirement-quality criterion is satisfied +- Leave items unchecked when they still require clarification, correction, or reviewer evaluation +- `/speckit-implement` reads checklist checkbox state as a gate and must not modify markers +- `checklists/requirements.md` has a separate built-in lifecycle maintained by `/speckit-specify` and `/speckit-clarify` +- This run used defaults (depth: standard; audience: PR reviewer; focus: refuse-then-stop, bypass closure, README timing) because `/speckit-checklist` had no extra arguments +- Reviewer evaluation 2026-09-06: all 26 items pass after spec edits (true allow vs envelope bypass, FR-004 same-PR rewrite, rollback=revert, compatibility opt-out named) plus tasks T007/T016 diff --git a/specs/002-fail-closed-next-handler/checklists/requirements.md b/specs/002-fail-closed-next-handler/checklists/requirements.md new file mode 100644 index 0000000..1a2ff1a --- /dev/null +++ b/specs/002-fail-closed-next-handler/checklists/requirements.md @@ -0,0 +1,35 @@ +# Specification Quality Checklist: Fail-closed after policy refusal + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-09-06 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Clarify 2026-09-06: Q1 → C. This lane closes every audit bypass found, including new ones during the fix. +- Next: `/speckit-plan`. GitHub issue #51. README disclosure stays until implement+CI. diff --git a/specs/002-fail-closed-next-handler/contracts/handler-continuation.md b/specs/002-fail-closed-next-handler/contracts/handler-continuation.md new file mode 100644 index 0000000..2983f84 --- /dev/null +++ b/specs/002-fail-closed-next-handler/contracts/handler-continuation.md @@ -0,0 +1,27 @@ +# Contract: Handler continuation after policy check + +Public factory: `createNextApiHandler` (higher-level HTTP handler). + +## Refusal (must not forward) + +When the policy check cannot allow the request, the handler: + +- Writes a refusal to the caller (existing 503 body for unavailable/denied). +- Does **not** call the upstream model endpoint. +- Applies for: no decision, explicit deny, timeout, invalid payload, no decision endpoint. + +## Allow (default path) + +When the policy check allows: + +- An envelope MUST be present and enforced before upstream fetch. +- Any locally chosen fallback model MUST be rechecked against that envelope. +- Integrity evidence MUST be independent or the request is refused. + +## Compatibility opt-out + +Explicit opt-out of policy (`requireCoreDecision: false`) is not a policy-gated path. It must stay labeled as compatibility, not as Core-authorized execution. + +## Tests + +See [quickstart.md](../quickstart.md). Spies on upstream `/chat/completions` must stay at zero on every refusal and every closed bypass. diff --git a/specs/002-fail-closed-next-handler/data-model.md b/specs/002-fail-closed-next-handler/data-model.md new file mode 100644 index 0000000..d362154 --- /dev/null +++ b/specs/002-fail-closed-next-handler/data-model.md @@ -0,0 +1,39 @@ +# Data Model: 002-fail-closed-next-handler + +## Entities + +### Policy check result + +| Field | Meaning | +|-------|---------| +| allowed | True only when a valid allow decision is present | +| refusal | Already-written caller response when not allowed | +| envelope | Authorization record attached only on allow | + +### Continuation decision + +| State | Rule | +|-------|------| +| refuse | Return the refusal. Zero upstream forwards. | +| allow | Forward only after envelope checks (default path). | +| compatibility opt-out | Explicit non-default path; not represented as policy-gated. | + +### Bypass (must be closed on default path) + +| Bypass | Required close | +|--------|----------------| +| Continue after refusal | Stop; do not forward | +| Missing envelope | Refuse; do not forward | +| Local model swap | Recheck against envelope or refuse | +| Missing integrity evidence | Refuse or attach independent digest evidence | + +## State transitions + +```text +request + -> policy check + -> refusal written -> STOP (no upstream) + -> allow + envelope ok -> forward + -> allow + envelope missing/invalid (default path) -> STOP + -> new bypass found -> STOP until closed +``` diff --git a/specs/002-fail-closed-next-handler/plan.md b/specs/002-fail-closed-next-handler/plan.md new file mode 100644 index 0000000..54623f8 --- /dev/null +++ b/specs/002-fail-closed-next-handler/plan.md @@ -0,0 +1,74 @@ +# Implementation Plan: Fail-closed after policy refusal + +**Branch**: `002-fail-closed-next-handler` | **Date**: 2026-09-06 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/002-fail-closed-next-handler/spec.md` + +## Summary + +Stop the higher-level handler from forwarding after a policy refusal, and close every remaining default-path bypass (missing envelope, un-rechecked local model, missing integrity evidence, plus any found during the fix). Tests first. README disclosure last. + +## Technical Context + +**Language/Version**: TypeScript, Node.js >= 18 + +**Primary Dependencies**: Existing `@bodanglin/verdict-node` library (`src/index.ts` handler + `src/middleware/forwarder.ts` envelope checks) + +**Storage**: N/A + +**Testing**: Jest (`npm test -- tests/router.test.ts`) + +**Target Platform**: Node HTTP adapter (Express and Next-style handler) + +**Project Type**: TypeScript library + +**Performance Goals**: N/A — correctness of refuse vs forward + +**Constraints**: Fail-closed; constitution I/III/IV; clarify C (every found bypass); isolated worktree; no dirty `feat/nod002-parity-gate` writes + +**Scale/Scope**: One public handler factory, proxy continuation, envelope/ladder checks, README paragraphs after green tests + +## Constitution Check + +| Principle | Status | Evidence | +|-----------|--------|----------| +| I. Downstream enforcement client | Pass | Handler enforces Core decisions; does not become a peer router | +| II. Verifiable claims | Pass | README rewrite only after tests prove fail-closed | +| III. Disclosed defects stay disclosed | Pass | Warning remains until FR-001/FR-003/FR-006 tests green | +| IV. Alpha until declared | Pass | Alpha label stays | + +Post-Phase 1: still pass. Design does not hide the defect in docs first. + +## Project Structure + +### Documentation (this feature) + +```text +specs/002-fail-closed-next-handler/ +├── plan.md +├── research.md +├── data-model.md +├── quickstart.md +├── contracts/handler-continuation.md +└── tasks.md # /speckit-tasks — not this command +``` + +### Source Code (repository root) + +```text +src/index.ts # nextApiHandler, middleware, proxy +src/middleware/forwarder.ts # envelope helpers to reuse +tests/router.test.ts # extend Next.js /api compatibility +README.md # last, after tests +docs/adr/ADR-001-*.md # align defect paragraph after tests +``` + +**Structure Decision**: No new packages. Fix continuation and default-path enforcement in the existing handler. + +## Complexity Tracking + +> No constitution violations. + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| — | — | — | diff --git a/specs/002-fail-closed-next-handler/quickstart.md b/specs/002-fail-closed-next-handler/quickstart.md new file mode 100644 index 0000000..ced7c33 --- /dev/null +++ b/specs/002-fail-closed-next-handler/quickstart.md @@ -0,0 +1,29 @@ +# Quickstart: 002-fail-closed-next-handler + +Work only in this worktree. Do not edit the dirty `feat/nod002-parity-gate` checkout. Do not rewrite README until tests pass. + +## 1. Tests first + +From the worktree root: + +```bash +npm test -- tests/router.test.ts +``` + +Add cases for timeout, invalid decision payload, and no decision endpoint. Each must be 503 (or the existing refusal) with **no** `/chat/completions` fetch. + +## 2. Bypass cases + +Same file (or adjacent): missing envelope on the default path does not forward; substituted ladder model is rechecked or refused. + +## 3. Allow path still works + +Existing compatibility and allow tests stay green. + +## 4. README last + +Only after the suite is green, replace the "Critical limitation / Critical defect" paragraphs so they match fail-closed behavior. Keep alpha and envelope-reconciliation statements. + +## 5. CI + +`npm test` and lint as in `package.json`. diff --git a/specs/002-fail-closed-next-handler/research.md b/specs/002-fail-closed-next-handler/research.md new file mode 100644 index 0000000..aeaaf15 --- /dev/null +++ b/specs/002-fail-closed-next-handler/research.md @@ -0,0 +1,31 @@ +# Research: 002-fail-closed-next-handler + +**Date**: 2026-09-06 + +## Decision: Guard continuation, not only the boolean return + +- **Decision**: After the policy middleware runs, the higher-level handler must not call the forwarder if the middleware returned false **or** if a refusal status was already written. +- **Rationale**: `origin/master` already returns false after a 503 in some cases and has two tests (unavailable, denied). The README still documents unconditional forwarding. Remaining gaps are missing refusal cases and other bypasses (clarification C). A second check on "response already written" catches a true/false mismatch. +- **Alternatives considered**: Trust only the boolean — rejected; that is the current claimed fix and still disagrees with the README/ADR. Remove the handler — rejected; it is the public Next-style API. + +## Decision: Close remaining bypasses on this path in the same lane + +- **Decision**: On the default (policy required) path: do not forward without an envelope; do not swap in a local ladder model without rechecking it against the envelope; supply independent policy-digest evidence or refuse. Any new bypass found while fixing is closed here (spec C). +- **Rationale**: FR-006 / User Story 3. Standalone Express forwarder already fail-closes missing envelopes; the higher-level proxy still skips when envelope is absent. +- **Alternatives considered**: Track envelope/ladder as later issues — rejected by clarify C. + +## Decision: README warning stays until tests for all FR-002 cases and found bypasses pass + +- **Decision**: Do not rewrite the critical-limitation paragraphs until the regression suite is green in this worktree. Then rewrite to match fail-closed behavior and keep alpha + envelope-reconciliation language. +- **Rationale**: Constitution III. +- **Alternatives considered**: Update README first — forbidden. + +## Decision: Tests live next to the existing Next.js handler tests + +- **Decision**: Extend `tests/router.test.ts` "Next.js /api compatibility" with timeout, invalid payload, no endpoint, envelope-required, and ladder-recheck cases. Spy on `fetch` for `/chat/completions`. +- **Rationale**: Same harness as the two existing 503 tests. +- **Alternatives considered**: New file only — extra split without need. + +## Live `origin/master` note + +`src/index.ts` `nextApiHandler` already has `if (!authorized) return`. Middleware returns false after 503 when policy is required. Tests cover unavailable and denied only. README/ADR still describe the old unconditional `proxy()` call. This plan treats that as incomplete, not done. diff --git a/specs/002-fail-closed-next-handler/spec.md b/specs/002-fail-closed-next-handler/spec.md new file mode 100644 index 0000000..58b8a8f --- /dev/null +++ b/specs/002-fail-closed-next-handler/spec.md @@ -0,0 +1,106 @@ +# Feature Specification: Fail-closed after policy refusal + +**Feature Branch**: `002-fail-closed-next-handler` + +**Created**: 2026-09-06 + +**Status**: Draft + +**Input**: User description: "When the higher-level request handler's policy check has already refused a request (no decision, denied, error, timeout, or invalid payload), the handler MUST return that refusal and MUST NOT forward the request upstream. Add a regression test covering those refusal cases. Do not remove the public disclosure of this defect until the fix and test are merged and CI is green. GitHub issue #51." + +## Clarifications + +### Session 2026-09-06 + +- Q: Should this feature only stop forwarding after a policy refusal, or also close the other known bypasses (missing envelope, locally substituted models not rechecked)? → A: This lane closes every audit bypass found, even if new ones appear during the fix. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Refused requests stay refused (Priority: P1) + +An operator using the higher-level HTTP handler expects that if the policy check has already told the caller "no" (or "cannot decide"), the request is not also sent to the model provider. + +**Why this priority**: This is a safety defect. A refused request that still runs is fail-open. + +**Independent Test**: Submit a request in each refusal case below. Confirm the caller only sees the refusal, and the upstream provider is not contacted. + +**Acceptance Scenarios**: + +1. **Given** the policy service is unreachable or returns a server error, **When** a chat request arrives, **Then** the handler returns the refusal and does not forward upstream. +2. **Given** the policy decision is explicitly denied, **When** a chat request arrives, **Then** the handler returns the refusal and does not forward upstream. +3. **Given** the policy decision times out, **When** a chat request arrives, **Then** the handler returns the refusal and does not forward upstream. +4. **Given** the policy payload is invalid, **When** a chat request arrives, **Then** the handler returns the refusal and does not forward upstream. +5. **Given** no policy endpoint is configured, **When** a chat request arrives, **Then** the handler returns the refusal and does not forward upstream. + +--- + +### User Story 2 - Disclosure stays until the fix is proven (Priority: P1) + +A visitor reading the project README still sees a clear warning about this defect until the code fix and its regression test are merged with green required checks. + +**Why this priority**: The constitution forbids hiding a known safety gap with documentation. + +**Independent Test**: The README warning remains until this feature's merge; after merge and green checks it may be rewritten to match the new behavior. + +**Acceptance Scenarios**: + +1. **Given** this feature is not yet merged, **When** someone reads the primary README, **Then** the existing critical-limitation warning is still present and not softened. +2. **Given** this feature is merged and required checks are green, **When** the README is updated, **Then** the warning is replaced with accurate fail-closed language, not deleted silently before the proof exists. + +--- + +### User Story 3 - Every found bypass is closed in this lane (Priority: P1) + +An operator must not have a second path that still sends a request after policy has refused it, or that skips policy by omitting an envelope or by swapping in a local model that was not rechecked. + +**Why this priority**: Chosen clarification C. Leaving a discovered bypass unfixed would keep the handler fail-open. + +**Independent Test**: For each bypass found in the audit or during this fix, a test shows the request is refused or rechecked, not forwarded around policy. + +**Acceptance Scenarios**: + +1. **Given** a known audit bypass (missing envelope, locally substituted model not rechecked, or missing integrity evidence on this path), **When** this feature is delivered, **Then** that bypass is closed with a test, not deferred. +2. **Given** a new bypass is found while fixing this handler, **When** the feature is declared done, **Then** that bypass is also closed and tested in this lane. + +--- + +### Edge Cases + +- Policy check writes a refusal and then the handler would have continued into the forwarder: must stop. +- Missing request envelope: must not forward around policy. +- A locally substituted model that was not rechecked against policy: must not forward. +- A true allow (policy allowed, and on the default path envelope plus integrity evidence present) continues to forward. Requiring an envelope on that path closes a bypass; it does not change a true allow. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: After the policy check has already written a refusal, the handler MUST return that response and MUST NOT contact the upstream provider. +- **FR-002**: FR-001 applies for: no decision, explicit deny, timeout, invalid decision payload, and no decision endpoint configured. +- **FR-003**: A regression test MUST cover every case in FR-002 and MUST fail if the upstream forwarder is invoked after a refusal. +- **FR-004**: The public README disclosure of this defect MUST remain until FR-001, FR-003, and FR-006 have passing tests. The warning MAY be rewritten in the same change as those tests. It MUST NOT be removed in a change that lacks them. Merge ships the fix and the rewrite together. +- **FR-005**: This work MUST NOT silently fold into the already-shipped docs-accuracy feature. It is a separate code-fix lane (issue #51). +- **FR-006**: Every policy bypass found in the audit or during this fix MUST be closed in this lane, with a test. That includes missing-envelope forwarding, locally substituted models not rechecked, missing integrity evidence on this path, and any new bypass discovered while implementing. This feature is not done while a found bypass remains open. + +### Key Entities + +- **Policy check**: The step that asks the decision service whether a request may run. +- **Refusal**: The response already written when the check cannot allow the request. +- **Upstream forward**: Sending the same request to the model provider after that check. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: 100% of the five refusal cases return the refusal and record zero upstream forwards. +- **SC-002**: The allow path still forwards when the policy check allows the request. +- **SC-003**: The README warning is still present on this branch until the required checks for the fix are green; it is not removed in the same change that lacks a passing regression test. +- **SC-004**: 100% of bypasses found in the audit or during this fix have a passing test that they no longer forward around policy. + +## Assumptions + +- Issue #51 and constitution v1.0.0 (Principles I, III, IV) govern this work. +- Docs-accuracy (`001-docs-pitch-accuracy`) already shipped and must not be rewritten as a substitute for this fix. +- Isolated worktree from `origin/master`; do not write the dirty `feat/nod002-parity-gate` checkout. +- Rollback is revert of this feature’s change. No extra recovery path. +- Compatibility opt-out (`requireCoreDecision` off) is not a policy-gated path and is out of scope except that it must not be described as Core-authorized. diff --git a/specs/002-fail-closed-next-handler/tasks.md b/specs/002-fail-closed-next-handler/tasks.md new file mode 100644 index 0000000..65fe84a --- /dev/null +++ b/specs/002-fail-closed-next-handler/tasks.md @@ -0,0 +1,151 @@ +--- +description: "Task list for 002-fail-closed-next-handler" +--- + +# Tasks: Fail-closed after policy refusal + +**Input**: Design documents from `/specs/002-fail-closed-next-handler/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/, quickstart.md + +**Tests**: Spec FR-003/FR-006 require regression tests. Write them first and confirm they fail where the bypass still exists. + +**Organization**: One writer in this worktree. Do not edit `/home/nick/dev/verdict-node` dirty `feat/nod002-parity-gate`. Do not rewrite README until US1+US3 tests are green. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies) +- **[Story]**: Which user story this task belongs to +- Include exact file paths in descriptions + +## Path Conventions + +- Single project: `src/index.ts`, `src/middleware/forwarder.ts`, `tests/router.test.ts`, `README.md` + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Confirm isolation and current tests + +- [ ] T001 Record HEAD and confirm this worktree is `002-fail-closed-next-handler` tracking `origin/master` in `specs/002-fail-closed-next-handler/quickstart.md` (do not write the dirty main checkout) +- [ ] T002 Run `npm test -- tests/router.test.ts` in this worktree and note which Next.js 503 cases already pass + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Shared spy pattern for “zero `/chat/completions` fetches” + +- [ ] T003 Add a shared fetch-spy helper in `tests/router.test.ts` that records `/chat/completions` calls for all later refusal/bypass tests + +**Checkpoint**: Helper exists. User stories can start. + +--- + +## Phase 3: User Story 1 - Refused requests stay refused (Priority: P1) 🎯 MVP + +**Goal**: Five refusal cases return the refusal and never forward upstream. + +**Independent Test**: `npm test -- tests/router.test.ts` — timeout, invalid payload, and no endpoint cases 503 (or existing refusal) with zero `/chat/completions` fetches. Unavailable and denied stay green. + +### Tests for User Story 1 + +> **NOTE: Write these tests FIRST, ensure they FAIL before implementation if the bypass still exists** + +- [ ] T004 [US1] Add failing or documenting tests in `tests/router.test.ts` for policy timeout, invalid decision payload, and no decision endpoint (zero `/chat/completions`) +- [ ] T005 [US1] Confirm existing unavailable and denied tests in `tests/router.test.ts` still assert zero `/chat/completions` + +### Implementation for User Story 1 + +- [ ] T006 [US1] Stop continuation after a refusal in `src/index.ts` (`nextApiHandler` must not call `proxy` if middleware returned false or a refusal was already written) +- [ ] T007 [US1] Re-run `npm test -- tests/router.test.ts` and confirm T004–T005 pass and the existing allow-path test still forwards (HTTP 200 to `/chat/completions` when policy allows) + +**Checkpoint**: All five FR-002 cases refuse with zero upstream forwards. + +--- + +## Phase 4: User Story 2 - Disclosure stays until the fix is proven (Priority: P1) + +**Goal**: README warning remains until US1+US3 tests are green; then rewrite to match fail-closed. + +**Independent Test**: README still contains the critical-limitation language until T012. After T012, warning is replaced, alpha and envelope-reconciliation remain. + +### Implementation for User Story 2 + +- [ ] T008 [US2] Confirm `README.md` still contains the `createNextApiHandler` critical-limitation / critical-defect paragraphs (do not edit yet) +- [ ] T012 [US2] After T007 and T011 are green, rewrite those paragraphs in `README.md` and the matching paragraph in `docs/adr/ADR-001-execution-envelope-enforcement.md` to fail-closed language; keep alpha and “still being reconciled” + +**Checkpoint**: Docs match proven behavior. Warning was not removed early. + +--- + +## Phase 5: User Story 3 - Every found bypass is closed (Priority: P1) + +**Goal**: Missing envelope, un-rechecked local model, missing integrity evidence, and any new bypass found while fixing are closed with tests. + +**Independent Test**: Default-path tests refuse or recheck; `/chat/completions` is not called around policy. + +### Tests for User Story 3 + +- [ ] T009 [US3] Add tests in `tests/router.test.ts` that the default path does not forward without an envelope +- [ ] T010 [US3] Add tests in `tests/router.test.ts` that a locally substituted ladder model is rechecked against the envelope or refused +- [ ] T016 [US3] Add a test in `tests/router.test.ts` that missing independent integrity / policy-digest evidence on the default path refuses and does not call `/chat/completions` + +### Implementation for User Story 3 + +- [ ] T011 [US3] Close envelope, ladder-recheck, and integrity-evidence bypasses in `src/index.ts` (reuse helpers from `src/middleware/forwarder.ts` where they already exist); if a new bypass appears, add a test in `tests/router.test.ts` and close it in this lane. Depends on T009, T010, T016. + +**Checkpoint**: FR-006 satisfied. Then T012 (README). + +--- + +## Phase 6: Polish & Cross-Cutting Concerns + +- [ ] T013 Run `npm test` in this worktree +- [ ] T014 Follow `specs/002-fail-closed-next-handler/quickstart.md` end to end +- [ ] T015 Confirm `specs/001-docs-pitch-accuracy/` was not rewritten as a substitute for this fix + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- Setup → Foundational → US1 (MVP) → US3 → US2 (T012 last) → Polish +- T008 (read-only README check) can run anytime after Setup +- T012 depends on T007 and T011 +- T016 before T011 (integrity-evidence test first) + +### User Story Dependencies + +- **US1**: After T003. MVP. +- **US3**: After T003; can overlap T004 writing but `src/index.ts` is one writer +- **US2 T012**: After US1+US3 tests green + +### Parallel Opportunities + +- T004 and T009 on the same file `tests/router.test.ts` — sequential, not [P] +- One writer. Do not parallelize two edits to `src/index.ts` + +--- + +## Implementation Strategy + +### MVP First (User Story 1 Only) + +1. T001–T003 +2. T004–T007 five refusal cases +3. Stop and confirm zero `/chat/completions` on refuse + +### Incremental Delivery + +1. US1 refuse-then-stop +2. US3 remaining bypasses +3. US2 README last +4. Full `npm test` + +--- + +## Notes + +- [P] unused: same-file conflicts +- Next command: `/speckit-analyze`, then `/speckit-implement` (custom checklist `fail-closed.md` is still unchecked until a reviewer ticks it) diff --git a/src/index.ts b/src/index.ts index c4c36e8..814634d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,7 +3,10 @@ import * as http from 'http'; import * as https from 'https'; import type { RoutingDecision as CanonicalRoutingDecision } from '@bodanglin/verdict-contracts'; import { adaptRoutingDecision } from './adapters/contract-to-middleware.js'; -import { enforceExecutionEnvelope } from './middleware/forwarder.js'; +import { + ExecutionEnvelopeError, + enforceExecutionEnvelope, +} from './middleware/forwarder.js'; const UNSAFE_OBJECT_KEYS = new Set(['__proto__', 'prototype', 'constructor']); @@ -328,7 +331,11 @@ export type OpenAIChatCompletionChunk = z.infer; - llmRouter?: { decision?: Partial; executionEnvelope?: unknown }; + llmRouter?: { + decision?: Partial; + executionEnvelope?: unknown; + expectedPolicyDigest?: string; + }; } export interface ProxyResponseLike { @@ -338,6 +345,7 @@ export interface ProxyResponseLike { write(chunk: Uint8Array | string): boolean; end(chunk?: Uint8Array | string): void; headersSent?: boolean; + statusCode?: number; flushHeaders?: () => void; } @@ -808,9 +816,11 @@ export class LlmGateNode { } else { const adapted = adaptRoutingDecision(canonical); const envelope = (canonical as Record).execution_envelope as unknown; + const digest = (canonical as Record).policy_digest; req.llmRouter = { decision: { ...adapted, latencyMs: Date.now() - start }, executionEnvelope: envelope, + expectedPolicyDigest: typeof digest === 'string' ? digest : undefined, }; next(); return true; @@ -1004,8 +1014,11 @@ export class LlmGateNode { throw error; } }); - if (!authorized) { - return; // Response already sent by middleware (e.g., 503) + const refusalAlreadyWritten = + res.headersSent === true || + (typeof res.statusCode === 'number' && res.statusCode >= 400); + if (!authorized || refusalAlreadyWritten) { + return; } await proxy(req, res, (error: unknown) => { if (error) { @@ -1033,20 +1046,34 @@ export class LlmGateNode { }); } - // Enforce Core execution envelope before any upstream fetch. - // Only enforced when a Core decision (and thus envelope) is available. - // When requireCoreDecision=false (compatibility path), no envelope exists and enforcement is skipped. const envelope = req.llmRouter?.executionEnvelope; - if (envelope !== undefined) { + const expectedPolicyDigest = req.llmRouter?.expectedPolicyDigest; + const denyEnvelope = (err: unknown) => { + if (err instanceof ExecutionEnvelopeError) { + return res.status(403).json({ error: err.message, code: err.code }); + } + if (err instanceof Error && 'code' in err) { + return res.status(403).json({ error: err.message, code: (err as { code: string }).code }); + } + return res + .status(403) + .json({ error: 'Execution envelope validation failed', code: 'envelope_invalid' }); + }; + + if (this.requireCoreDecision) { + try { + enforceExecutionEnvelope(envelope, parsedRequest.data, { + required: true, + expectedPolicyDigest, + }); + } catch (err) { + return denyEnvelope(err); + } + } else if (envelope !== undefined) { try { enforceExecutionEnvelope(envelope, parsedRequest.data, { required: true }); } catch (err) { - if (err instanceof Error && 'code' in err) { - return res.status(403).json({ error: err.message, code: (err as any).code }); - } - return res - .status(403) - .json({ error: 'Execution envelope validation failed', code: 'envelope_invalid' }); + return denyEnvelope(err); } } @@ -1054,8 +1081,21 @@ export class LlmGateNode { const requestBody = parsedRequest.data; const isStream = requestBody.stream === true; let lastError = null; + let envelopeDenied = false; for (const candidateModel of ladder) { + if (this.requireCoreDecision) { + try { + enforceExecutionEnvelope(envelope, { ...requestBody, model: candidateModel }, { + required: true, + expectedPolicyDigest, + }); + } catch (err) { + envelopeDenied = true; + lastError = err; + continue; + } + } const fetchStart = Date.now(); try { const payload = { ...requestBody, model: candidateModel }; @@ -1096,6 +1136,9 @@ export class LlmGateNode { } } + if (envelopeDenied && lastError instanceof ExecutionEnvelopeError) { + return res.status(403).json({ error: lastError.message, code: lastError.code }); + } res.status(502).json({ error: 'All downstream dynamic routing targets failed/exhausted.', details: String(lastError), diff --git a/tests/router.test.ts b/tests/router.test.ts index f6a7177..057b437 100644 --- a/tests/router.test.ts +++ b/tests/router.test.ts @@ -1,4 +1,7 @@ import { afterEach, describe, expect, it, jest } from '@jest/globals'; +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; import express, { Request, Response as ExpressResponse } from 'express'; import request from 'supertest'; import { @@ -173,6 +176,7 @@ describe('LlmGateNode', () => { delete process.env.OMNIROUTE_API_BASE_URL; delete process.env.OMNIROUTE_API_KEY; delete process.env.OPENAI_API_KEY; + delete process.env.VERDICT_CORE_DECISION_ENDPOINT; }); afterEach(() => { @@ -209,6 +213,12 @@ describe('LlmGateNode', () => { jsonPayload = payload; return payload; }, + get statusCode() { + return statusCode ?? undefined; + }, + get headersSent() { + return statusCode !== null || jsonPayload !== undefined || ended; + }, setHeader(name: string, value: string | string[]) { headers.set(name, value); }, @@ -923,6 +933,220 @@ describe('LlmGateNode', () => { // Verify NO upstream fetch to /chat/completions was made expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); }); + + const recordChatCompletions = () => { + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async (url: string | URL | Request) => { + const urlStr = String(url); + fetchCalls.push(urlStr); + if (urlStr.endsWith('/chat/completions')) { + return new Response(JSON.stringify(validResponse), { status: 200 }); + } + return new Response(JSON.stringify({ error: 'no decision' }), { status: 500 }); + }); + return fetchCalls; + }; + + const postHandler = async ( + handler: ReturnType, + extraHeaders: Record = {} + ) => { + const recorder = createProxyResponseRecorder(); + await handler( + { + method: 'POST', + body: validRequest, + headers: { accept: 'application/json', ...extraHeaders }, + }, + recorder.res + ); + return recorder; + }; + + it('returns 503 and does NOT call upstream when Core decision times out', async () => { + const handler = createNextApiHandler({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + decisionTimeoutMs: 5, + }); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + fetchCalls.push(String(_url)); + return await new Promise((_, reject) => { + const signal = (init as RequestInit | undefined)?.signal; + if (signal) { + signal.addEventListener('abort', () => { + const err = new Error('The operation was aborted.'); + err.name = 'TimeoutError'; + reject(err); + }); + } + }); + }); + const recorder = await postHandler(handler); + expect(recorder.statusCode).toBe(503); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + it('returns 503 and does NOT call upstream when Core decision payload is invalid', async () => { + const handler = createNextApiHandler({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + }); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async url => { + const urlStr = String(url); + fetchCalls.push(urlStr); + return new Response(JSON.stringify({ not: 'a-decision' }), { status: 200 }); + }); + const recorder = await postHandler(handler); + expect(recorder.statusCode).toBe(503); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + it('returns 503 and does NOT call upstream when no decision endpoint is configured', async () => { + delete process.env.VERDICT_CORE_DECISION_ENDPOINT; + const handler = createNextApiHandler({ requireCoreDecision: true }); + const fetchCalls = recordChatCompletions(); + const recorder = await postHandler(handler); + expect(recorder.statusCode).toBe(503); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + const allowDecision = (envelope: unknown, digest?: string) => ({ + schema_version: '1', + selected_route: { + runtime_id: 'test/gpt-4o-mini', + model: 'gpt-4o-mini', + provider: 'test', + decision: 'allowed', + availability: 'available', + latency_ms: 1, + }, + exclusions: [], + execution_envelope: envelope, + ...(digest ? { policy_digest: digest } : {}), + }); + + const testEnvelope = { + ...(JSON.parse( + fs.readFileSync( + path.join(path.dirname(fileURLToPath(import.meta.url)), '../test_fixtures/envelopes/valid_envelope.json'), + 'utf-8' + ) + ) as Record), + execution_constraints: { + allowed_models: ['gpt-4o-mini'], + allowed_tools: ['lookup_weather'], + max_request_usd: 1.0, + }, + }; + + it('does not forward on the default path without an envelope', async () => { + const handler = createNextApiHandler({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + apiKey: 'secret-token', + }); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async url => { + const urlStr = String(url); + fetchCalls.push(urlStr); + if (urlStr.includes('decision.test')) { + return new Response(JSON.stringify(allowDecision(undefined)), { status: 200 }); + } + return new Response(JSON.stringify(validResponse), { status: 200 }); + }); + const recorder = await postHandler(handler); + expect(recorder.statusCode).toBe(403); + expect((recorder.jsonPayload as { code?: string }).code).toBe('envelope_missing'); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + it('does not forward a locally substituted ladder model outside the envelope', async () => { + const handler = createNextApiHandler({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + apiKey: 'secret-token', + }); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async url => { + const urlStr = String(url); + fetchCalls.push(urlStr); + if (urlStr.includes('decision.test')) { + return new Response(JSON.stringify(allowDecision(testEnvelope)), { status: 200 }); + } + return new Response(JSON.stringify(validResponse), { status: 200 }); + }); + const node = new LlmGateNode({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + apiKey: 'secret-token', + }); + jest.spyOn(node as any, 'buildDynamicLadder').mockResolvedValue(['other-model']); + const recorder = createProxyResponseRecorder(); + await node.nextApiHandler()( + { method: 'POST', body: validRequest, headers: { accept: 'application/json' } }, + recorder.res + ); + expect(recorder.statusCode).toBe(403); + expect((recorder.jsonPayload as { code?: string }).code).toBe('model_disallowed'); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + it('does not forward when envelope policy digest does not match independent evidence', async () => { + const handler = createNextApiHandler({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + apiKey: 'secret-token', + }); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async url => { + const urlStr = String(url); + fetchCalls.push(urlStr); + if (urlStr.includes('decision.test')) { + return new Response( + JSON.stringify( + allowDecision( + testEnvelope, + 'sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + ) + ), + { status: 200 } + ); + } + return new Response(JSON.stringify(validResponse), { status: 200 }); + }); + const recorder = await postHandler(handler); + expect(recorder.statusCode).toBe(403); + expect((recorder.jsonPayload as { code?: string }).code).toBe('envelope_tampered'); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(false); + }); + + it('forwards when policy allows and envelope matches (true allow path)', async () => { + const node = new LlmGateNode({ + requireCoreDecision: true, + decisionEndpoint: 'http://decision.test/route', + apiKey: 'secret-token', + }); + jest.spyOn(node as any, 'buildDynamicLadder').mockResolvedValue(['gpt-4o-mini']); + const fetchCalls: string[] = []; + jest.spyOn(globalThis, 'fetch').mockImplementation(async url => { + const urlStr = String(url); + fetchCalls.push(urlStr); + if (urlStr.includes('decision.test')) { + return new Response(JSON.stringify(allowDecision(testEnvelope)), { status: 200 }); + } + return new Response(JSON.stringify(validResponse), { status: 200 }); + }); + const recorder = createProxyResponseRecorder(); + await node.nextApiHandler()( + { method: 'POST', body: validRequest, headers: { accept: 'application/json' } }, + recorder.res + ); + expect(recorder.statusCode).toBe(200); + expect(fetchCalls.some(c => c.endsWith('/chat/completions'))).toBe(true); + }); }); describe('OpenAI chat completion request parser', () => { From d82cb0002f61d86da549cdefc7ae27132f4af844 Mon Sep 17 00:00:00 2001 From: Nicholas Carter Date: Sun, 13 Sep 2026 10:26:48 +0000 Subject: [PATCH 2/2] style(handler): format fail-closed handler for CI prettier --- src/index.ts | 20 ++++++++++---------- tests/router.test.ts | 5 ++++- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/index.ts b/src/index.ts index 814634d..eb5274a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,10 +3,7 @@ import * as http from 'http'; import * as https from 'https'; import type { RoutingDecision as CanonicalRoutingDecision } from '@bodanglin/verdict-contracts'; import { adaptRoutingDecision } from './adapters/contract-to-middleware.js'; -import { - ExecutionEnvelopeError, - enforceExecutionEnvelope, -} from './middleware/forwarder.js'; +import { ExecutionEnvelopeError, enforceExecutionEnvelope } from './middleware/forwarder.js'; const UNSAFE_OBJECT_KEYS = new Set(['__proto__', 'prototype', 'constructor']); @@ -1015,8 +1012,7 @@ export class LlmGateNode { } }); const refusalAlreadyWritten = - res.headersSent === true || - (typeof res.statusCode === 'number' && res.statusCode >= 400); + res.headersSent === true || (typeof res.statusCode === 'number' && res.statusCode >= 400); if (!authorized || refusalAlreadyWritten) { return; } @@ -1086,10 +1082,14 @@ export class LlmGateNode { for (const candidateModel of ladder) { if (this.requireCoreDecision) { try { - enforceExecutionEnvelope(envelope, { ...requestBody, model: candidateModel }, { - required: true, - expectedPolicyDigest, - }); + enforceExecutionEnvelope( + envelope, + { ...requestBody, model: candidateModel }, + { + required: true, + expectedPolicyDigest, + } + ); } catch (err) { envelopeDenied = true; lastError = err; diff --git a/tests/router.test.ts b/tests/router.test.ts index 057b437..ee49eaa 100644 --- a/tests/router.test.ts +++ b/tests/router.test.ts @@ -1031,7 +1031,10 @@ describe('LlmGateNode', () => { const testEnvelope = { ...(JSON.parse( fs.readFileSync( - path.join(path.dirname(fileURLToPath(import.meta.url)), '../test_fixtures/envelopes/valid_envelope.json'), + path.join( + path.dirname(fileURLToPath(import.meta.url)), + '../test_fixtures/envelopes/valid_envelope.json' + ), 'utf-8' ) ) as Record),