Kjell Tore Guttormsen
d88245c06e
Ordre 5 (wave 1, P11 + P5), review_version 1.1. - K7: `verdict: ALLOW | WARN | BLOCK` is required in review.md (strict error, soft warning for 1.0 reviews). New CLI lib/review/verdict-gate.mjs: 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide (never green); a verdict greener than the file's own BLOCKERs or JSON block is exit 2. Realpath main guard (the old file:// guard exits 0 under a symlink - measured). - No finding is struck for length: succinctness trims only MINOR/SUGGESTION text (catalogue tier), keeps BLOCKER/MAJOR whole, counts trims; the coordinator lists what it declined to judge. - Claude Code's /code-review and /security-review run as the engine via the Skill tool (never --fix/--comment/--post/ultra); lib/review/engine-findings.mjs turns their findings into a reviewer payload; two catalogue keys (CODE_REVIEW_DEFECT, SECURITY_REVIEW_VULNERABILITY), catalogue 12 -> 14. An engine that did not run forbids ALLOW. - Every BLOCKER is written as a numbered `### Step R<n>` with its source finding (strict error REVIEW_MISSING_REMEDIATION_STEP). - P5: lib/review/recurrence.mjs - a finding class in two review.md files (current + git history, no new store) becomes one proposal, routed by the catalogue's memory_route; written only between Voyage markers in .claude/rules/voyage-review-lessons.md under a size budget, CLAUDE.md gets a pointer only; the operator approves every line (Phase 8.7). - Workflow substrate: catalogue enum in step; a missing reviewer or engine forbids ALLOW there too (it returned ALLOW on zero findings before). - Fixtures review-run-A/B said WARN over a BLOCKER; now BLOCK. Borrowed and credited in code: Superpowers (declined-to-judge), ai-native-sdlc (cap only nits; mistake/fix/check), Spec Kit (managed block; converge steps), Pocock (retro routing), Kiro (verdict file + gate; evidence per lesson), Cherny (engine -> brief control order; review -> memory; budget). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
117 lines
5 KiB
Markdown
117 lines
5 KiB
Markdown
---
|
|
type: trekreview
|
|
review_version: "1.0"
|
|
created: 2026-05-01
|
|
task: "Add JWT authentication with refresh-token rotation"
|
|
slug: jwt-auth
|
|
project_dir: .claude/projects/2026-05-01-jwt-auth/
|
|
brief_path: .claude/projects/2026-05-01-jwt-auth/brief.md
|
|
scope_sha_start: 0123456789abcdef0123456789abcdef01234567
|
|
scope_sha_end: fedcba9876543210fedcba9876543210fedcba98
|
|
reviewed_files_count: 3
|
|
verdict: BLOCK
|
|
findings:
|
|
- d2d0e27875ae9ef0d818cb08bb6f14e6d33c4232
|
|
- 7861519c326c207aabf17072db51c469bebc217b
|
|
- 400dfcff81e0e219eb04a7123c68ae870696f121
|
|
- 763d174e6c519fafbadcba5d1706708479e36e61
|
|
- 7a3d7d0a668f6431ef3877ceeb106023b0f6295e
|
|
- bf3e8b347cf4269ad005a9cf64dab6f601345704
|
|
---
|
|
|
|
# Review: Add JWT authentication with refresh-token rotation (Run B)
|
|
|
|
## Executive Summary
|
|
|
|
Same diff as Run A, re-reviewed independently to test determinism. This run found
|
|
the same 5 findings plus one extra (a placeholder TODO in logout.mjs that Run A
|
|
missed). Verdict: **BLOCK** — same as Run A; the extra finding is MAJOR but does
|
|
not change the merge gate.
|
|
|
|
This is a SYNTHETIC v1.0 fixture for testing the Jaccard determinism pipeline.
|
|
Run A's set is a strict subset of Run B's set, giving Jaccard = 5/6 = 0.833.
|
|
|
|
## Coverage
|
|
|
|
| File | Treatment | Reason |
|
|
|---|---|---|
|
|
| `lib/auth/jwt.mjs` | deep-review | Security-critical (token signing/verification) |
|
|
| `lib/handlers/login.mjs` | deep-review | Auth surface |
|
|
| `lib/handlers/logout.mjs` | deep-review | Auth surface |
|
|
| `package-lock.json` | skip | Lockfile |
|
|
| `dist/**` | skip | Build output |
|
|
|
|
## Findings (BLOCKER)
|
|
|
|
### 763d174e6c519fafbadcba5d1706708479e36e61
|
|
|
|
- **Location:** `lib/handlers/login.mjs:23`
|
|
- **Rule:** `UNIMPLEMENTED_CRITERION`
|
|
- **Brief ref:** SC-2 ("login endpoint MUST return 401 on invalid credentials")
|
|
- **Evidence:** Handler returns 200 with empty body when password mismatch occurs.
|
|
- **Fix:** Return 401 with WWW-Authenticate header per brief SC-2.
|
|
|
|
## Findings (MAJOR)
|
|
|
|
### d2d0e27875ae9ef0d818cb08bb6f14e6d33c4232
|
|
|
|
- **Location:** `lib/auth/jwt.mjs:42`
|
|
- **Rule:** `SECURITY_INJECTION`
|
|
- **Brief ref:** Non-Goal #3 ("must not accept user-supplied algorithm header")
|
|
- **Evidence:** `jwt.verify(token, secret, { algorithms: req.body.alg })` — algorithm taken from request body.
|
|
- **Fix:** Hard-code `algorithms: ['RS256']`; reject any token claiming a different alg.
|
|
|
|
### 7861519c326c207aabf17072db51c469bebc217b
|
|
|
|
- **Location:** `lib/auth/jwt.mjs:88`
|
|
- **Rule:** `MISSING_TEST`
|
|
- **Brief ref:** SC-4 ("refresh-token rotation must be tested under concurrent refresh")
|
|
- **Evidence:** No test in `tests/` covers the concurrent-refresh path; only happy-path is exercised.
|
|
- **Fix:** Add `tests/auth/concurrent-refresh.test.mjs` covering the race window.
|
|
|
|
### 7a3d7d0a668f6431ef3877ceeb106023b0f6295e
|
|
|
|
- **Location:** `lib/handlers/login.mjs:56`
|
|
- **Rule:** `PLAN_EXECUTE_DRIFT`
|
|
- **Brief ref:** Plan Step 4 ("login.mjs uses bcrypt.compare()")
|
|
- **Evidence:** Plan said `bcrypt.compare`; implementation uses `crypto.timingSafeEqual` over plaintext-derived buffers.
|
|
- **Fix:** Either update plan + brief to record the deviation or refactor to bcrypt.compare per plan.
|
|
|
|
### bf3e8b347cf4269ad005a9cf64dab6f601345704
|
|
|
|
- **Location:** `lib/handlers/logout.mjs:14`
|
|
- **Rule:** `PLACEHOLDER_IN_CODE`
|
|
- **Brief ref:** none (Rule 7a violation)
|
|
- **Evidence:** `// TODO: invalidate refresh-token cookie before responding` — left in committed code.
|
|
- **Fix:** Implement the cookie invalidation or remove the comment with an issue link.
|
|
|
|
## Findings (MINOR)
|
|
|
|
### 400dfcff81e0e219eb04a7123c68ae870696f121
|
|
|
|
- **Location:** `lib/auth/jwt.mjs:117`
|
|
- **Rule:** `MISSING_ERROR_HANDLING`
|
|
- **Brief ref:** none (engineering hygiene)
|
|
- **Evidence:** `await refreshTokenStore.delete(jti)` is not wrapped — store-down throws bubble to top-level handler.
|
|
- **Fix:** Wrap in try/catch; log + 503 on store failure.
|
|
|
|
## Remediation Summary
|
|
|
|
6 findings total: 1 BLOCKER, 4 MAJOR, 1 MINOR. Same merge gate as Run A; one
|
|
extra MAJOR (PLACEHOLDER_IN_CODE) that Run A missed. Run a remediation plan via
|
|
`/trekplan --brief review.md`.
|
|
|
|
```json
|
|
{
|
|
"fixture_kind": "synthetic-v1.0",
|
|
"jaccard_with_run_A": "5/6 = 0.833",
|
|
"findings": [
|
|
{"id": "763d174e6c519fafbadcba5d1706708479e36e61", "severity": "BLOCKER", "rule": "UNIMPLEMENTED_CRITERION", "file": "lib/handlers/login.mjs", "line": 23},
|
|
{"id": "d2d0e27875ae9ef0d818cb08bb6f14e6d33c4232", "severity": "MAJOR", "rule": "SECURITY_INJECTION", "file": "lib/auth/jwt.mjs", "line": 42},
|
|
{"id": "7861519c326c207aabf17072db51c469bebc217b", "severity": "MAJOR", "rule": "MISSING_TEST", "file": "lib/auth/jwt.mjs", "line": 88},
|
|
{"id": "7a3d7d0a668f6431ef3877ceeb106023b0f6295e", "severity": "MAJOR", "rule": "PLAN_EXECUTE_DRIFT", "file": "lib/handlers/login.mjs", "line": 56},
|
|
{"id": "bf3e8b347cf4269ad005a9cf64dab6f601345704", "severity": "MAJOR", "rule": "PLACEHOLDER_IN_CODE", "file": "lib/handlers/logout.mjs", "line": 14},
|
|
{"id": "400dfcff81e0e219eb04a7123c68ae870696f121", "severity": "MINOR", "rule": "MISSING_ERROR_HANDLING", "file": "lib/auth/jwt.mjs", "line": 117}
|
|
]
|
|
}
|
|
```
|