feat(voyage): S10 part B — NW2 full bake-off (rich fixture) → verdict POSITIVE
Run the full T2 §5 prose-vs-Workflow /trekreview bake-off (operator GO, choice "a"): 3 runs/arm on a rich-finding JWT-auth fixture, resolving the smoke's 0-finding limitation. Deliverables: - tests/fixtures/bakeoff-rich/ — JWT-auth brief + diff with 5 seeded blatant, brief-traceable issues (varied severity/rule_key, one dual-flaggable). - scripts/bakeoff-armA-merge.mjs — Arm A (prose) validate (NW1) + triplet-dedup, matching Arm B's dedup exactly. - scripts/bakeoff-fidelity.mjs — cross-arm + within-arm + granularity-ladder fidelity analysis over the structured arm outputs. - docs/T2-bakeoff-results.md §Full run — the T2 §5 verdict. Result (3 runs/arm, both arms ran the coordinator): - Verdict fidelity EQUIVALENT — all 6 runs BLOCK, cross-arm verdict-match 1.0. - Finding-set: substrate is fidelity-neutral. Cross-arm jaccard 0.41 (triplet) → 0.71 (file,rule_key) → 1.0 (file); cross-arm ≈ within-arm at every granularity. Issue coverage 5/5 in 6/6 runs. Low triplet jaccard is line-citation noise shared by both arms, not a substrate effect. - Token +4.4% (Arm B vs A; <=+15%). Classifier interference 0 at 9-agent concurrency. JSON-robustness: Arm B schema-forced; Arm A 6/6 valid via NW1. - VERDICT POSITIVE → S11 proceeds with opt-in --workflow flag. Caveat (per plan posture): strict triplet-jaccard>=0.7 flag is 0/9, a metric-calibration artifact (both arms sub-0.7 against themselves), not a regression. Residual: F4 auto/bypass explicit-mode check (mode not settable in-session). Suite green (662/660 pass, 2 skip); plugin validate clean (modulo the pre-existing root-CLAUDE.md warning). No production code changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqBYc8Ltrk7LipyJmGxXiB
This commit is contained in:
parent
869bf318d2
commit
f7c8aa45ab
6 changed files with 537 additions and 4 deletions
61
tests/fixtures/bakeoff-rich/README.md
vendored
Normal file
61
tests/fixtures/bakeoff-rich/README.md
vendored
Normal file
|
|
@ -0,0 +1,61 @@
|
|||
# NW2 bake-off — rich-finding-surface fixture (S10 part B)
|
||||
|
||||
The smoke fixture (`tests/fixtures/bakeoff/`) reviewed a clean, TDD'd NW1 diff
|
||||
and both arms returned **0 findings**, so finding-*set* fidelity was never
|
||||
stressed — only verdict fidelity at zero. This fixture fixes that: a realistic
|
||||
JWT-auth diff + brief seeded with **5 blatant, brief-traceable issues** spanning
|
||||
varied severities and rule_keys, split across both reviewers. Both arms review
|
||||
the **same** `delivered.diff` against the **same** `brief.md`, so any difference
|
||||
in the `{verdict, findings}` is attributable to the orchestration substrate
|
||||
(prose Arm A vs Workflow Arm B), not the input.
|
||||
|
||||
Modeled on the proven determinism scenario in
|
||||
`tests/fixtures/trekreview/review-run-A.md` (same JWT-auth shape, same finding
|
||||
families), but here it is a **real diff + brief pair** the live reviewers read —
|
||||
not a synthetic pre-rendered review.
|
||||
|
||||
## Seeded findings (expected ~5, varied)
|
||||
|
||||
| # | Issue | Where | Likely rule_key | Severity | Owner reviewer |
|
||||
|---|-------|-------|-----------------|----------|----------------|
|
||||
| 1 | `/login` returns **200** (not 401) on invalid credentials | `lib/handlers/login.mjs:17` | `UNIMPLEMENTED_CRITERION` | BLOCKER | conformance (SC2) |
|
||||
| 2 | `verifyToken` reads the verify **algorithm from a request header** | `lib/auth/jwt.mjs:19–20` | `SECURITY_INJECTION` and/or `NON_GOAL_VIOLATED` | BLOCKER | correctness + conformance (NG1) |
|
||||
| 3 | No test covers **concurrent refresh** (no test file in the diff) | `lib/auth/refresh.mjs` (whole) | `MISSING_TEST` | MAJOR | correctness (SC3) |
|
||||
| 4 | Password check uses `crypto.timingSafeEqual` over plaintext, not `bcrypt.compare` per plan | `lib/handlers/login.mjs:13` | `PLAN_EXECUTE_DRIFT` | MAJOR | conformance/correctness (Plan Step 4) |
|
||||
| 5 | `refreshStore` I/O (`get`/`delete`/`set`) is **unwrapped** — backend outage bubbles unhandled | `lib/auth/refresh.mjs:10,16,20` | `MISSING_ERROR_HANDLING` | MINOR | correctness (Constraint) |
|
||||
|
||||
Issue #2 is intentionally **dual-flaggable** (a security defect AND an explicit
|
||||
Non-Goal violation) — it exercises the cross-reviewer overlap that the
|
||||
`(file,line,rule_key)` triplet-dedup and the coordinator must handle. Real LLM
|
||||
reviewers will vary exact line numbers and may surface extra latent issues (e.g.
|
||||
the `user.passwordHash` NPE when the email is unknown); that variance is the
|
||||
**signal** the bake-off measures, not noise to suppress.
|
||||
|
||||
Expected verdict (both arms): **BLOCK** (≥1 BLOCKER present).
|
||||
|
||||
## Triage map (deterministic, pinned — passed to BOTH arms)
|
||||
|
||||
All three files are auth/security surface → `deep-review`:
|
||||
|
||||
```
|
||||
lib/auth/jwt.mjs → deep-review
|
||||
lib/handlers/login.mjs → deep-review
|
||||
lib/auth/refresh.mjs → deep-review
|
||||
```
|
||||
|
||||
## How it's consumed
|
||||
|
||||
The bake-off pins Phases 1–4 (brief + diff + triage above) and passes them to
|
||||
both arms via paths (reviewer agents carry `Read`):
|
||||
|
||||
- **Arm A (prose):** reviewers spawned FOREGROUND via the Agent tool with the
|
||||
prose trailing-`json`-block contract → `validateReviewerOutput` (NW1) →
|
||||
triplet-dedup → `review-coordinator` → `{verdict, findings}`.
|
||||
Harness: `scripts/bakeoff-armA-merge.mjs`.
|
||||
- **Arm B (Workflow):** `scripts/trekreview-armB.workflow.mjs` via the Workflow
|
||||
tool, `args = { briefPath, diffPath, triage }` (StructuredOutput-forced
|
||||
findings → JS triplet-dedup → coordinator verdict schema).
|
||||
|
||||
PRIMARY metric: `fidelityDiffStructured` (`lib/review/fidelity-diff.mjs`) — same
|
||||
verdict + equivalent finding set (IDs / severities / rule_keys), jaccard
|
||||
tolerance 0.7. Analysis harness: `scripts/bakeoff-fidelity.mjs`.
|
||||
69
tests/fixtures/bakeoff-rich/brief.md
vendored
Normal file
69
tests/fixtures/bakeoff-rich/brief.md
vendored
Normal file
|
|
@ -0,0 +1,69 @@
|
|||
---
|
||||
type: trekbrief
|
||||
brief_version: "2.1"
|
||||
slug: jwt-auth-refresh-rotation
|
||||
task: Add JWT authentication with refresh-token rotation to the API
|
||||
research_topics: 0
|
||||
research_status: complete
|
||||
brief_quality: ready
|
||||
created: 2026-06-18
|
||||
---
|
||||
|
||||
# JWT authentication with refresh-token rotation
|
||||
|
||||
## Intent
|
||||
|
||||
Add stateless JWT authentication to the API: a `/login` endpoint that issues a
|
||||
short-lived access token plus a rotating refresh token, and a `/refresh`
|
||||
endpoint that rotates the refresh token on every use. Tokens are signed with a
|
||||
fixed RS256 key pair. This is the security boundary of the service, so the
|
||||
contract below is strict.
|
||||
|
||||
## Plan reference (what the approved plan said to build)
|
||||
|
||||
> **Plan Step 4** — `lib/handlers/login.mjs` verifies the password with
|
||||
> `bcrypt.compare(password, user.passwordHash)`. The stored credential is a
|
||||
> bcrypt hash; no plaintext comparison.
|
||||
>
|
||||
> **Plan Step 6** — `lib/auth/jwt.mjs` hard-codes the verification algorithm to
|
||||
> `['RS256']`. The algorithm is never read from the request.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- **SC1** — `POST /login` with valid credentials returns `200` with both an
|
||||
`accessToken` and a `refreshToken` in the JSON body.
|
||||
- **SC2** — `POST /login` with invalid credentials returns HTTP `401` (not 200)
|
||||
and no tokens. Invalid means the email is unknown OR the password does not
|
||||
match.
|
||||
- **SC3** — Refresh-token rotation is covered by an automated test that
|
||||
exercises the **concurrent-refresh** race window (two refreshes presenting the
|
||||
same refresh token must not both succeed).
|
||||
- **SC4** — Access and refresh tokens are signed and verified with **RS256
|
||||
only**, using the server's fixed key pair.
|
||||
|
||||
## Non-Goals
|
||||
|
||||
- **NG1** — Do NOT accept a caller-supplied signing/verification algorithm. The
|
||||
algorithm must never be read from the request (header, body, or query). A
|
||||
token claiming a different `alg` must be rejected.
|
||||
- **NG2** — Do NOT add a user-registration / sign-up endpoint. Users are
|
||||
provisioned out of band.
|
||||
- **NG3** — Do NOT add password-reset or email flows in this change.
|
||||
|
||||
## Constraints
|
||||
|
||||
- Node stdlib + the already-vendored `jsonwebtoken` and `bcrypt`; no new deps.
|
||||
- Every delivered code path that the SCs describe must have test coverage.
|
||||
- Errors from the refresh-token store (a network resource) must not crash the
|
||||
request handler — degrade to a 5xx, do not let the rejection bubble unhandled.
|
||||
|
||||
## Assumptions
|
||||
|
||||
- `db.getUserByEmail(email)` returns `{ id, email, passwordHash }` or `null`.
|
||||
- A `refreshStore` with `get/set/delete` (async, may throw on backend outage) is
|
||||
injected.
|
||||
|
||||
## NFRs
|
||||
|
||||
- Constant-time password comparison via the bcrypt primitive (no hand-rolled
|
||||
comparison over plaintext-derived buffers).
|
||||
84
tests/fixtures/bakeoff-rich/delivered.diff
vendored
Normal file
84
tests/fixtures/bakeoff-rich/delivered.diff
vendored
Normal file
|
|
@ -0,0 +1,84 @@
|
|||
diff --git a/lib/auth/jwt.mjs b/lib/auth/jwt.mjs
|
||||
new file mode 100644
|
||||
index 0000000..1a2b3c4
|
||||
--- /dev/null
|
||||
+++ b/lib/auth/jwt.mjs
|
||||
@@ -0,0 +1,21 @@
|
||||
+// JWT sign/verify helpers (RS256). Plan Step 6: algorithm hard-coded to RS256.
|
||||
+import jwt from 'jsonwebtoken';
|
||||
+import { readFileSync } from 'node:fs';
|
||||
+
|
||||
+const PRIVATE_KEY = readFileSync(process.env.JWT_PRIVATE_KEY_PATH, 'utf8');
|
||||
+const PUBLIC_KEY = readFileSync(process.env.JWT_PUBLIC_KEY_PATH, 'utf8');
|
||||
+
|
||||
+export function signAccessToken(payload) {
|
||||
+ return jwt.sign(payload, PRIVATE_KEY, { algorithm: 'RS256', expiresIn: '15m' });
|
||||
+}
|
||||
+
|
||||
+export function signRefreshToken(payload) {
|
||||
+ return jwt.sign(payload, PRIVATE_KEY, { algorithm: 'RS256', expiresIn: '7d' });
|
||||
+}
|
||||
+
|
||||
+// Verify a token. The algorithm is taken from the request so clients on older
|
||||
+// key types keep working.
|
||||
+export function verifyToken(token, req) {
|
||||
+ const alg = req.headers['x-jwt-alg'] || 'RS256';
|
||||
+ return jwt.verify(token, PUBLIC_KEY, { algorithms: [alg] });
|
||||
+}
|
||||
diff --git a/lib/handlers/login.mjs b/lib/handlers/login.mjs
|
||||
new file mode 100644
|
||||
index 0000000..2b3c4d5
|
||||
--- /dev/null
|
||||
+++ b/lib/handlers/login.mjs
|
||||
@@ -0,0 +1,23 @@
|
||||
+// POST /login — issue access + refresh tokens. Plan Step 4: bcrypt.compare.
|
||||
+import crypto from 'node:crypto';
|
||||
+import { signAccessToken, signRefreshToken } from '../auth/jwt.mjs';
|
||||
+import { db } from '../db.mjs';
|
||||
+
|
||||
+export async function login(req, res) {
|
||||
+ const { email, password } = req.body;
|
||||
+ const user = await db.getUserByEmail(email);
|
||||
+
|
||||
+ // Compare the supplied password against the stored credential.
|
||||
+ const supplied = Buffer.from(password);
|
||||
+ const stored = Buffer.from(user.passwordHash);
|
||||
+ const ok = supplied.length === stored.length && crypto.timingSafeEqual(supplied, stored);
|
||||
+
|
||||
+ if (!ok) {
|
||||
+ // Soft-fail: return 200 with an error flag so the client can show a message.
|
||||
+ return res.status(200).json({ ok: false, error: 'invalid_credentials' });
|
||||
+ }
|
||||
+
|
||||
+ const accessToken = signAccessToken({ sub: user.id });
|
||||
+ const refreshToken = signRefreshToken({ sub: user.id });
|
||||
+ return res.status(200).json({ ok: true, accessToken, refreshToken });
|
||||
+}
|
||||
diff --git a/lib/auth/refresh.mjs b/lib/auth/refresh.mjs
|
||||
new file mode 100644
|
||||
index 0000000..3c4d5e6
|
||||
--- /dev/null
|
||||
+++ b/lib/auth/refresh.mjs
|
||||
@@ -0,0 +1,22 @@
|
||||
+// POST /refresh — rotate the refresh token. Single-use: the presented token is
|
||||
+// deleted and a new one issued.
|
||||
+import { signAccessToken, signRefreshToken, verifyToken } from './jwt.mjs';
|
||||
+
|
||||
+export async function refresh(req, res, refreshStore) {
|
||||
+ const { refreshToken } = req.body;
|
||||
+ const claims = verifyToken(refreshToken, req);
|
||||
+ const jti = claims.jti;
|
||||
+
|
||||
+ const known = await refreshStore.get(jti);
|
||||
+ if (!known) {
|
||||
+ return res.status(401).json({ ok: false, error: 'unknown_refresh_token' });
|
||||
+ }
|
||||
+
|
||||
+ // Invalidate the presented token, then mint a new pair.
|
||||
+ await refreshStore.delete(jti);
|
||||
+
|
||||
+ const accessToken = signAccessToken({ sub: claims.sub });
|
||||
+ const newRefresh = signRefreshToken({ sub: claims.sub });
|
||||
+ await refreshStore.set(newRefresh.jti, { sub: claims.sub });
|
||||
+ return res.status(200).json({ ok: true, accessToken, refreshToken: newRefresh });
|
||||
+}
|
||||
Loading…
Add table
Add a link
Reference in a new issue