fix(orchestrator): constant-time bearer-token check + rotation slot (#76)

[chrisleekr-bot]

Summary

Closes a timing-attack surface on the orchestrator's daemon WebSocket handshake (#76). The previous authHeader !== \Bearer ${authToken}`short-circuited on the first mismatched byte, leaking the matching prefix ofDAEMON_AUTH_TOKEN via response latency and allowing a network-adjacent attacker to recover the secret byte-by-byte. A recovered token would let the attacker register a malicious daemon and harvest per-job GitHub App installation tokens (contents:write/issues:write`).

The fix swaps the comparison for a crypto.timingSafeEqual-based comparator over length-padded buffers and adds an optional DAEMON_AUTH_TOKEN_PREVIOUS rotation slot so operators can rotate the secret without a synchronised fleet restart.

Changes

• Constant-time bearer-token comparator in src/orchestrator/ws-server.ts. New internal isAuthHeaderValid() helper pads buffers to a fixed length, calls crypto.timingSafeEqual for both primary and previous tokens unconditionally, and combines results with bitwise | (no JS short-circuit). Wired into the upgrade handler in place of !==. Preserves the existing 401 response shape and logger.warn payload so log shippers and the daemon reconnect path are unaffected.
• Rotation slot. New optional DAEMON_AUTH_TOKEN_PREVIOUS env var (src/config.ts). Orchestrator accepts either the primary or the previous token; daemons keep sending the primary. validateDataLayerConfig continues to require only the primary, so existing deployments are unaffected.
• Regression tests. Six new cases in test/orchestrator/ws-server.test.ts exercising every header shape (missing, shorter, equal-length-different-bytes, longer prefix-collision, primary accept, previous-token accept) through the real Bun.serve fetch handler.
• Docs. docs/operate/configuration.md adds the new env var; docs/operate/runbooks/daemon-fleet.md adds a "Rotating DAEMON_AUTH_TOKEN" runbook section with a sequence diagram and 5-step overlap-window procedure (90-day cadence per OWASP Secrets Management).

Files changed

• src/orchestrator/ws-server.ts · adds the constant-time comparator and replaces the vulnerable !== check.
• src/config.ts · adds daemonAuthTokenPrevious zod field + DAEMON_AUTH_TOKEN_PREVIOUS env mapping.
• test/orchestrator/ws-server.test.ts · six new regression cases under describe("WebSocket auth (constant-time bearer comparator, #76)").
• docs/operate/configuration.md · DAEMON_AUTH_TOKEN_PREVIOUS row + constant-time note + runbook link.
• docs/operate/runbooks/daemon-fleet.md · new "Rotating DAEMON_AUTH_TOKEN" section with mermaid sequence diagram.
• .env.example · documents the new env var.
• IMPLEMENT.md · tracking summary for the bot workflow.

Commits

• cdc4834 · fix(orchestrator): constant-time bearer-token check + rotation slot (#76)
• 329233d · docs(implement): add tracking summary for issue #76

Tests run

• bun run typecheck · pass (clean exit)
• bunx eslint src/orchestrator/ws-server.ts src/config.ts test/orchestrator/ws-server.test.ts · 0 errors / 10 pre-existing warnings
• bun run format · all files pass Prettier
• bun test test/orchestrator/ws-server.test.ts · 17 pass / 0 fail (11 pre-existing + 6 new); src/orchestrator/ws-server.ts reports 100% line coverage
• bun test test/config.test.ts · 40 pass / 0 fail
• bun run scripts/check-docs-versions.ts · OK
• bun run scripts/check-docs-citations.ts · OK
• bun test (full suite) · 535 pass / 153 skip / 194 fail — verified by git stash + re-run that all 194 failures are pre-existing on main (Postgres / Valkey / removed-test-imports infra) and not introduced by this PR
• bun run docs:build · not run locally (mkdocs/Python not installed in the bot sandbox); the two project-specific gates (check:docs-versions, check:docs-citations) that run ahead of mkdocs build --strict in CI both pass

Verification

• Vulnerable line gone. grep -n "Bearer" src/orchestrator/ws-server.ts shows only the two Buffer.from(\Bearer ${...}`, "utf8")lines insideisAuthHeaderValid— no!==` Bearer comparison remains.
• Constant-time primitive in use. grep -rn "timingSafeEqual" src/ now returns 3 hits inside src/orchestrator/ws-server.ts (was 0 before — same negative grep cited in the issue).
• Comparator covers all four header shapes by working over length-padded buffers and combining timingSafeEqual results with an explicit length-equality check via bitwise AND, so the work is constant regardless of whether the header is missing / shorter / equal-but-wrong / longer-with-correct-prefix. The longer-with-correct-prefix case is the one a buggy length-prefix comparator would have authenticated; the explicit length-equality guard rejects it.
• No timing leak between primary and previous. Both timingSafeEqual calls run unconditionally when expectedPrevious !== null, and the results are combined with bitwise | (no JS || short-circuit), so an attacker cannot distinguish which slot rejected them via timing.
• Rotation slot is orchestrator-only. The previous-token slot is read in src/orchestrator/ws-server.ts; daemons (src/daemon/ws-client.ts) keep sending the primary daemonAuthToken value. validateDataLayerConfig still only requires the primary, so existing deployments without _PREVIOUS continue to start.
• Plan tasks T1–T6 all satisfied — see IMPLEMENT.md for the per-task evidence map.
• Out of scope (intentionally not done in this PR). The issue's suggested-next-steps fix(deps): upgrade zod to v4, prepar e npm publish, fix CI peer-dep conflict #3 (Origin allowlist), chore(speckit): setup speckit tooling, constitution, and unified check script #5 (escalate ws:// to a hard refusal in production), and chore: project housekeeping — 90% coverage, ESLint v8, CI security scans #6 (per-IP rate limit / jittered 401 delay) are deliberately deferred — they are separate hardening surfaces and the plan explicitly bounded this PR to T1–T6 (constant-time fix + rotation slot + tests + docs). Each is its own follow-up issue candidate.

Related Issues

• Closes security(orchestrator): non-constant-time bearer-token check exposes DAEMON_AUTH_TOKEN to timing attacks #76

Test plan

• Tests added/updated where the change introduces new behaviour
• bun run typecheck clean
• bun run lint no new errors
• Existing tests still pass (or pre-existing failures noted above)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Support for seamless authentication-token rotation via an optional previous token for rolling daemon updates.

• Documentation

  • New token-rotation runbook and updated configuration docs with step-by-step procedures and operational guidance.

• Improvements

  • Authentication validation hardened using constant-time comparisons to reduce timing-attack risk.
  • Logging redaction extended to cover the new token field.

• Tests

  • Added integration and redaction tests for token validation and logging.

[chrisleekr-bot]

bot workflow review — succeeded

🔍 Code review complete — 7 files, +286/-48.

Review — PR #103 (fix/daemon-auth-timing-safe-76)

Reviewed at HEAD 329233d.

Summary

Reviewed end-to-end: the comparator at src/orchestrator/ws-server.ts:42-83 is correct, the test coverage at test/orchestrator/ws-server.test.ts:290-403 exercises every adversarial header shape that matters, and the rotation slot is wired in without breaking existing single-token deployments. One minor defense-in-depth gap surfaced — the new daemonAuthTokenPrevious config field is missing from the Pino redaction list — but no blocker or major issue. The fix lands as advertised and is otherwise ready for human approval.

What was checked

Files read in full:

• src/orchestrator/ws-server.ts — comparator + upgrade handler.
• src/config.ts (relevant sections: schema at L270–290, validateDataLayerConfig at L629–664, env mapping at L760–795).
• src/logger.ts — full file, including REDACT_PATHS (L17–44) and SENSITIVE_FIELD_NAMES_LC (L55–69).
• test/orchestrator/ws-server.test.ts — full file, focusing on the new withServer helper (L291–323) and six new cases (L325–402).
• docs/operate/configuration.md, docs/operate/runbooks/daemon-fleet.md, .env.example, IMPLEMENT.md — diff in full.

Cross-references performed:

• grep daemonAuthToken across src/ to confirm only webhook/router.ts, daemon/main.ts, logger.ts, config.ts, orchestrator/ws-server.ts consume the field; daemon side (daemon/main.ts:390) keeps sending the primary as the PR claims.
• grep DAEMON_AUTH_TOKEN_PREVIOUS / daemonAuthTokenPrevious across the repo: only orchestrator + config + tests + docs reference it; no daemon-side path picks it up.
• Walked every logger.info({ config… }) / JSON.stringify(config) / console.log(config) call site (src/app.ts:191, src/app.ts:391, src/orchestrator/valkey.ts:33, src/orchestrator/ws-server.ts:183) to confirm the redaction gap is currently latent, not active.
• Verified validateDataLayerConfig still requires only the primary so existing single-token deployments do not regress.
• Confirmed Bearer is not case-normalised (PR keeps the existing case-sensitive scheme behaviour from !==; not a regression).

Validation runs:

• bun run typecheck — pass.
• bun test test/orchestrator/ws-server.test.ts — 17 pass / 0 fail; per-file coverage shows src/orchestrator/ws-server.ts at 100% line / 100% function.
• bun test test/config.test.ts — 40 pass / 0 fail.
• bun run scripts/check-docs-citations.ts and bun run scripts/check-docs-versions.ts — both OK.

Findings

[minor] src/config.ts:288 — redaction symmetry for the new previous-token slot

daemonAuthTokenPrevious carries the same secrecy obligations as daemonAuthToken but is absent from src/logger.ts:39 (REDACT_PATHS) and src/logger.ts:63 (SENSITIVE_FIELD_NAMES_LC for the err-walker). No call site currently logs the whole config object, so this is not actively leaking — but the PR introduces an asymmetry the existing redaction list relies on. A future logger.info({ config }, …) line, or an Octokit error that ends up carrying both fields under request.headers.*, would emit the previous token in cleartext while the primary continues to be censored.

Recommended fix: add "daemonAuthTokenPrevious" to REDACT_PATHS and "daemonauthtokenprevious" to SENSITIVE_FIELD_NAMES_LC (lowercase per existing convention) in src/logger.ts.

Inline comment: #103 (comment)

Reasoning

Things I considered carefully and did NOT flag:

• Comparator correctness. The padded-buffer construction at src/orchestrator/ws-server.ts:59-77 is provably correct against every adversarial header shape:
  • Missing / empty header → actualRaw.length = 0 ≠ expectedPrimary.length, length-eq check fails.
  • Shorter / longer header → length-eq check fails (the explicit actualRaw.length === expectedPrimary.length guard is the load-bearing reject path against the prefix-collision attack the test on L353–363 simulates).
  • Equal-length-different-bytes → timingSafeEqual returns false on the padded buffers.
    The bitwise & / | combinators are over precomputed booleans coerced via Number(...); || was correctly avoided to dodge JS short-circuit. The two timingSafeEqual calls run unconditionally inside the expectedPrevious !== null branch, so primary-vs-previous timing parity holds within a given configuration.
• No timing channel between secrets. Within a single configuration (primary present, previous either set or unset), comparator runtime depends only on padLength, not on which slot a candidate matches. The "is _PREVIOUS configured" attribute is observable through latency, but that bit is not sensitive — it is part of the deployment posture, not the secret.
• Length-difference micro-leak. padLength = max(expectedPrimary.length, expectedPrevious?.length ?? 0) exposes a tiny side channel about whether the previous token is longer than the primary. The threat model already assumes the operator sets these values; it does not leak the secret bytes. Not worth flagging.
• Rotation-slot scope is correct. Daemons (src/daemon/main.ts:390) read only daemonAuthToken; the _PREVIOUS value never travels over the wire. validateDataLayerConfig (src/config.ts:629-664) still requires only the primary, so existing deployments without the new env var continue to start.
• Test setup. withServer in test/orchestrator/ws-server.test.ts:291-323 mutates the singleton config via type-cast and restores in a finally, matching the pre-existing pattern used by the surrounding tests. The inner try/finally ensures stopWebSocketServer runs even if fn throws, and the outer finally restores config even if stop hangs (stopWebSocketServer itself has a 2 s timeout race in ws-server.ts:198-210). No leakage across cases.
• Logger-redaction surface today. The four call sites that touch config and a logger only log named subfields (port, wsPort, valkeyUrl-after-redact). So the redaction gap is latent, not active — but worth fixing while the change is fresh. (See finding above.)
• No new dependencies. node:crypto is built-in; the import on ws-server.ts:1 is the only addition.

Verdict: ship-ready. The single Minor finding is genuinely defense-in-depth and can either land in this PR or be addressed in a follow-up.

cost: $4.3547 · turns: 53 · duration: 539s

[chrisleekr-bot]

bot workflow resolve — succeeded

🔎 Resolve iteration complete — 0 failing checks, 1 open comment threads (some may already be resolved).

Summary

PR #103 (fix/daemon-auth-timing-safe-76 → main) is ready for human approval. This resolve iteration addressed the single open review thread — a Minor finding flagging asymmetric Pino redaction coverage between the new daemonAuthTokenPrevious rotation slot and the primary daemonAuthToken. The branch was already up-to-date with main and CI was already all-green at the start of this iteration; the only remaining gate is human review.

CI status

• Pre-fix state on 329233d: all checks green (Analyze ×2, CI/Lint & Test, CodeQL, Docker build/merge/Trivy ×10, Gitleaks ×2, Label PR, Lint & Test, Semantic Release dev, build).
• Failing checks during iteration: none — no fix attempts spent on CI (0/3 used).
• Final post-fix state on 45956b9: verified all checks reached terminal success via gh pr checks 103 polling loop. No failure / cancelled / timed_out / action_required results.

Review comments

#	File:line	Classification	Action	Commit / Reply	Thread
1	src/config.ts:288 — daemonAuthTokenPrevious missing from Pino redaction list	Valid	Added "daemonAuthTokenPrevious" to REDACT_PATHS (src/logger.ts:39) and "daemonauthtokenprevious" to SENSITIVE_FIELD_NAMES_LC (src/logger.ts:64). Added two regression tests in test/utils/logger.test.ts:264-308 — one for the path-based redactor (top-level field) and one for the structural walker (nested under err.response.data). All 14 logger tests pass; bun run typecheck clean.	commit 45956b9 · reply discussion_r3182083222	✅ resolved

Commits pushed

• 45956b9 · fix(logger): redact daemonAuthTokenPrevious symmetrically with primary (#76)

Outstanding

Nothing blocking from the resolve workflow's perspective. The PR is awaiting human review/approval (reviewDecision empty — no formal approve/request-changes yet). Per FR-017 the bot does not merge.

cost: $3.2415 · turns: 5 · duration: 26s

[chrisleekr-bot]

bot workflow review — succeeded

🔍 Code review complete — 9 files, +331/-48.

Review — PR #103 (fix/daemon-auth-timing-safe-76)

Reviewed at HEAD 45956b9 (3 commits ahead of main, 0 behind — no rebase needed).

Summary

End-to-end re-review after the resolve iteration. The constant-time bearer-token comparator, rotation slot, regression tests, doc updates, and the follow-up redaction-symmetry fix all stand up to scrutiny: the comparator at src/orchestrator/ws-server.ts:42-83 is provably correct against all four adversarial header shapes; the logger redaction at src/logger.ts:40 + src/logger.ts:65 now covers both halves of the rotation pair; and the daemon side keeps sending only the primary token. Locally re-ran typecheck, the targeted unit suites, and both doc gates — all clean. No new findings, and the prior review's single Minor finding has been correctly addressed.

What was checked

Files read in full at 45956b9:

• src/orchestrator/ws-server.ts — comparator (isAuthHeaderValid) + upgrade handler.
• src/config.ts (relevant sections L270–290 schema, L629–664 validateDataLayerConfig, L760–780 env mapping).
• src/logger.ts — full file: REDACT_PATHS (L17–45) and SENSITIVE_FIELD_NAMES_LC (L56–71).
• test/orchestrator/ws-server.test.ts — full file, focusing on the withServer helper (L291–323) and the six new cases (L325–402).
• test/utils/logger.test.ts — focused on the two new #76 follow-up cases (L264–305).
• docs/operate/configuration.md, docs/operate/runbooks/daemon-fleet.md, .env.example, IMPLEMENT.md — diff in full.

Cross-references performed:

• grep daemonAuthToken across src/: only webhook/router.ts:189 (ephemeral-spawn injection, primary only), daemon/main.ts:390 (daemon outbound, primary only), logger.ts, config.ts, orchestrator/ws-server.ts consume the field. Daemon side never reads _PREVIOUS, as advertised.
• grep daemonAuthTokenPrevious / DAEMON_AUTH_TOKEN_PREVIOUS across the repo: orchestrator + config + logger + tests + docs only. No daemon-side path picks it up.
• validateDataLayerConfig (src/config.ts:629-664) still requires only the primary, so single-token deployments do not regress.
• Newly-spawned ephemeral daemons receive only DAEMON_AUTH_TOKEN (the primary) via the spawner — confirmed by reading src/webhook/router.ts:189-216.

Validation runs at 45956b9:

• bun run typecheck — pass (clean exit).
• bun test test/orchestrator/ws-server.test.ts test/utils/logger.test.ts — 31 pass / 0 fail; per-file coverage: src/orchestrator/ws-server.ts 100% line / 100% function.
• bun test test/config.test.ts — 40 pass / 0 fail.
• bun run scripts/check-docs-versions.ts — OK.
• bun run scripts/check-docs-citations.ts — OK.

Findings

No findings. Posting no inline comments. The single Minor finding from the prior review (daemonAuthTokenPrevious missing from the redaction list) was correctly addressed in 45956b9 with two regression tests in test/utils/logger.test.ts:264-305 — one for the path-based redactor (top-level field), one for the structural walker (nested under err.response.data). Both new tests pass locally.

Reasoning

Things I considered carefully and explicitly chose NOT to flag:

• Comparator correctness against all four header shapes. Walked the math for each case at src/orchestrator/ws-server.ts:59-82:
  • Missing / empty → actualRaw.length = 0, primaryLenEq = false, matchPrimary = 0.
  • Shorter → length-eq false; Buffer.alloc(padLength) zero-pads actual so timingSafeEqual returns false anyway (zero-bytes ≠ secret-bytes); both terms zero.
  • Equal-length but different bytes → timingSafeEqual returns false on padded buffers; reject.
  • Longer-with-correct-prefix (the load-bearing case) → actual = Buffer.alloc(padLength) truncates actualRaw to first padLength bytes, so timingSafeEqual(actual, primaryPadded) = true. The explicit actualRaw.length === expectedPrimary.length guard is what rejects it. The new test at test/orchestrator/ws-server.test.ts:353-363 exercises exactly this path with Bearer <primary>-EXTRA.
• No timing channel between secrets within a configured deployment. When expectedPrevious !== null, both timingSafeEqual calls run unconditionally and the results are combined with bitwise | over Number(...)-coerced booleans (no JS || short-circuit). Comparator runtime depends only on padLength, not on which slot a candidate matches, so primary-vs-previous timing parity holds. The presence/absence of _PREVIOUS is observable through latency, but that's deployment posture, not the secret.
• Length-difference micro-leak between the two configured tokens. padLength = max(expectedPrimary.length, expectedPrevious?.length ?? 0) exposes a tiny side channel about whether the previous token is longer than the primary. Threat model already assumes the operator chooses these values; it does not leak the secret bytes. Not worth flagging.
• Empty-string _PREVIOUS parity. process.env["DAEMON_AUTH_TOKEN_PREVIOUS"] = "" is accepted by z.string().optional() but then filtered at src/orchestrator/ws-server.ts:48-51 (previousToken.length > 0). So an explicit empty value is treated as "not set" — sane and consistent with how DAEMON_AUTH_TOKEN itself rejects empty in validateDataLayerConfig. No bug.
• Buffer allocation cost for malicious input. Buffer.from(authHeader ?? "", "utf8") allocates O(authHeader.length) bytes per request. Bun's Bun.serve enforces an HTTP header size cap, so this is bounded — not a DoS vector.
• Case sensitivity of the Bearer scheme name. RFC 7235 says scheme names are case-insensitive, but the original !== was case-sensitive too — daemon and orchestrator both use the literal Bearer token, so interop is preserved. Not a regression introduced by this PR; not flagged.
• webhook/router.ts:189 ephemeral-spawn injection. The router reads only the primary token to inject into spawned ephemeral daemon Pods, which is correct: a freshly-spawned Pod should receive the new primary, never the old previous slot. Confirms the rotation-slot is intentionally and correctly scoped to orchestrator-only.
• Test setup hygiene. withServer in test/orchestrator/ws-server.test.ts:291-323 mutates the singleton config via type-cast and restores in nested try/finally blocks — the inner finally ensures stopWebSocketServer runs even if the test body throws, the outer finally restores config even if stop hangs (stopWebSocketServer itself has a 2 s timeout race at src/orchestrator/ws-server.ts:204-207). No leakage across cases.
• Logger-redaction surface today. Walked every site that touches config and a logger (src/app.ts, src/orchestrator/valkey.ts, src/orchestrator/ws-server.ts) — none log the whole config object, so the redaction gap was latent rather than active even before the fix. The two new tests in test/utils/logger.test.ts:264-305 lock in the symmetric redaction in case a future call site adds a logger.info({ config }, ...) line.

Verdict: ship-ready. Awaiting human approval per FR-017.

cost: $3.6298 · turns: 45 · duration: 435s

[chrisleekr]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 62fc9f47-adbb-4721-b84f-bf422f95b7d8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

This PR implements issue #76 by replacing a non-constant-time bearer-token comparison with a crypto.timingSafeEqual validator that supports both a primary and optional previous token, enabling rotation without synchronized fleet restarts. The changes span configuration schema, WebSocket authentication logic, logger redaction, comprehensive test coverage, and operational documentation.

Changes

Daemon WebSocket Bearer Token Security & Rotation

Layer / File(s)	Summary
Data Shape src/config.ts	Config schema adds optional daemonAuthTokenPrevious: string | undefined for rotation overlap; env loading maps DAEMON_AUTH_TOKEN_PREVIOUS to the config field.
Core Implementation src/orchestrator/ws-server.ts	Introduces isAuthHeaderValid(authHeader, primaryToken, previousToken) using crypto.timingSafeEqual with length-precheck and buffer padding to prevent timing-based token-prefix leakage; WebSocket upgrade handler calls the validator instead of direct string equality.
Integration & Redaction src/logger.ts	Extends REDACT_PATHS and error-serializer SENSITIVE_FIELD_NAMES_LC to redact both daemonAuthToken and daemonAuthTokenPrevious in logs.
Tests test/orchestrator/ws-server.test.ts, test/utils/logger.test.ts	New WebSocket auth test suite with withServer helper validates rejection of missing/short/mismatched/prefix-collision tokens and acceptance of primary and previous tokens; logger tests verify redaction of the new daemonAuthTokenPrevious field.
Configuration & Operational Documentation .env.example, docs/operate/configuration.md, docs/operate/runbooks/daemon-fleet.md	.env.example documents optional DAEMON_AUTH_TOKEN_PREVIOUS; configuration table is reformatted; new runbook section provides a 2-phase rotation procedure (orchestrator accepts OLD+NEW, then NEW-only) with step-by-step instructions and verification steps.
Implementation Record IMPLEMENT.md	Implementation tracking document updated to record issue #76 (bearer-token timing-attack fix and rotation support), listing all changed files, test/lint results, and verification checklist.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• chrisleekr/github-app-playground#89: Extends logger redaction configuration for sensitive tokens; this PR adds daemonAuthTokenPrevious to the same redaction paths.

Suggested labels

released, bot:resolve, type: docs 📋

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'fix(orchestrator): constant-time bearer-token check + rotation slot (#76)' clearly and concisely summarizes the main changes: introducing constant-time token comparison for security and adding a rotation slot for operational flexibility.
Linked Issues check	✅ Passed	The PR fully addresses all primary coding objectives from issue #76: constant-time comparison using crypto.timingSafeEqual [#76], comprehensive test coverage [#76], optional DAEMON_AUTH_TOKEN_PREVIOUS field [#76], and documentation of rotation procedures [#76].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #76 objectives. No out-of-scope modifications detected; deferred follow-ups (Origin allowlist, ws:// enforcement, rate limiting) are correctly excluded.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR hardens the orchestrator’s daemon WebSocket authentication by replacing a timing-leaky bearer token string comparison with a constant-time timingSafeEqual-based comparator, and adds an optional previous-token rotation slot to support safe secret rotation with an overlap window.

Changes:

• Implement constant-time bearer-token validation for the /ws upgrade path, accepting either the primary token or an optional previous token for rotation.
• Extend configuration and docs to support DAEMON_AUTH_TOKEN_PREVIOUS, including an operational runbook for rotating the daemon fleet.
• Add regression tests for auth header shapes and extend log redaction coverage to include the new previous-token field.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/orchestrator/ws-server.ts	Adds isAuthHeaderValid() using timingSafeEqual and wires it into the WebSocket upgrade auth check.
src/config.ts	Adds optional daemonAuthTokenPrevious and maps DAEMON_AUTH_TOKEN_PREVIOUS from env.
src/logger.ts	Adds redaction coverage for daemonAuthTokenPrevious in both path-based and structural redaction.
test/orchestrator/ws-server.test.ts	Adds regression tests covering missing/short/wrong/long auth headers and previous-token acceptance.
test/utils/logger.test.ts	Adds tests ensuring both daemon auth tokens are redacted, including nested error payload cases.
docs/operate/configuration.md	Documents DAEMON_AUTH_TOKEN_PREVIOUS and links to the rotation runbook section.
docs/operate/runbooks/daemon-fleet.md	Adds a rotation procedure (with sequence diagram) for DAEMON_AUTH_TOKEN.
.env.example	Documents the new optional DAEMON_AUTH_TOKEN_PREVIOUS env var.
IMPLEMENT.md	Updates implementation tracking summary/evidence for issue #76.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@IMPLEMENT.md`:
- Line 7: Update the inline code span on the documented line so it has no
leading or trailing spaces inside the backticks (fix MD038); specifically
normalize the inline span that references authHeader !== `Bearer ${authToken}`
and DAEMON_AUTH_TOKEN so there are no internal edge spaces, save the markdown,
and re-run markdownlint to verify the MD038 warning is gone.

In `@src/config.ts`:
- Around line 282-288: The daemonAuthTokenPrevious schema currently allows
whitespace-only strings; update the zod schema for daemonAuthTokenPrevious so
whitespace-only values become undefined (treated as unset). Replace the simple
z.string().optional() with a transform/refinement that trims the value and
returns undefined for empty results (e.g. z.string().optional().transform(v => {
if (typeof v !== 'string') return undefined; const t = v.trim(); return t === ''
? undefined : t; })). This ensures downstream logic (e.g. isAuthHeaderValid)
never sees a whitespace-only previous token.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cab21761-d84f-4971-bc6f-3ae4aa1f02ce

📥 Commits

Reviewing files that changed from the base of the PR and between a151d68 and 45956b9.

📒 Files selected for processing (9)

• .env.example
• IMPLEMENT.md
• docs/operate/configuration.md
• docs/operate/runbooks/daemon-fleet.md
• src/config.ts
• src/logger.ts
• src/orchestrator/ws-server.ts
• test/orchestrator/ws-server.test.ts
• test/utils/logger.test.ts

[chrisleekr-bot]

bot workflow resolve — succeeded

🔎 Resolve iteration complete — 0 failing checks, 5 open comment threads (some may already be resolved).

Resolve iteration — PR #103 (fix/daemon-auth-timing-safe-76 → main)

Iteration date: 2026-05-04 (post-review iteration triggered by @chrisleekr-bot after CodeRabbit + Copilot review pass).

Summary

Took PR #103 from "1 prior iteration done, all-green, awaiting human review with 4 newly-opened review threads" through a full resolve pass. All four open review threads classified as actionable (3 Valid, 1 Partially Valid) and addressed in a single follow-up commit 953fd7f. The branch is now 4 commits ahead of main, 0 behind. CI returned to all-green after the push (final state captured below). No outstanding blockers from the resolve workflow's perspective — only human approval remains (reviewDecision empty — per FR-017 the bot does not merge).

CI status

• Pre-fix state on 45956b9: all checks green (Analyze ×2, CI/Lint & Test, CodeQL, CodeRabbit, Docker build/merge/Trivy ×10, Gitleaks ×2, Label PR, Lint & Test, Semantic Release dev, build, Trivy skipping). 0 failing checks at start; 0 fix attempts spent on CI (cap 3/iteration).
• Final post-fix state on 953fd7f: verified via gh pr checks 103 polling loop until every check reached a terminal status. All-green — no failure / cancelled / timed_out / action_required results.

Review comments

#	File:line	Author	Classification	Action taken	Commit / Reply	Resolved?
1	src/orchestrator/ws-server.ts:51	copilot-pull-request-reviewer	Valid	Hoisted the precomputed Bearer … padded buffers, original byte lengths, and shared padLength out of the per-request comparator into a new buildAuthExpectations() builder called once at server start. The fetch handler now passes a frozen AuthExpectations object into isAuthHeaderValid(), eliminating two Buffer.from(\Bearer …`, "utf8")` allocations per upgrade attempt. Reduces auth-flood amplification.	commit 953fd7f · reply r3184820977	✅ resolved (PRRT_kwDORS-R1M5_ekVi)
2	src/orchestrator/ws-server.ts:63	copilot-pull-request-reviewer	Valid	Bounded per-request comparator work to O(padLength). The previous Buffer.from(authHeader ?? "", "utf8") allocated proportional to the (attacker-controlled) header length and undermined the doc-comment "work bounded to expected token length" claim. Replaced with Buffer.byteLength(headerStr, "utf8") for the original-length check (no buffer alloc) and actual.write(headerStr, 0, padLength, "utf8") to copy at most padLength bytes into a fixed-size padded buffer. The longer-with-correct-prefix attack is still rejected by the explicit length-equality guard.	commit 953fd7f · reply r3184821764	✅ resolved (PRRT_kwDORS-R1M5_ekWF)
3	IMPLEMENT.md:7	coderabbitai	Partially Valid	Confirmed the readability problem (backtick-adjacent text running together) but the literal-space patch was clobbered on save by prettier --write, which reflows the line back to the no-space form because the surrounding \authHeader !== \`Bearer ${authToken}\``nested escaped-backtick code span confuses prettier's CommonMark walker. Rewrote the sentence to drop the verbatim-comparison code span, refer to the secret as "the daemon auth token" in prose, and splitcontents:write/issues:writewith anandso both code spans have plain whitespace on either side.bunx prettier --check IMPLEMENT.md` now passes.	commit 953fd7f · reply r3184822797	✅ resolved (PRRT_kwDORS-R1M5_emeW)
4	src/config.ts:288	coderabbitai	Valid	Switched daemonAuthTokenPrevious schema from z.string().optional() to the existing nonEmptyOptionalString helper. A whitespace-only env value (e.g. an empty Secret key whose decrypted value is "" or " ") is now coerced to undefined at parse time rather than surviving as a "valid" but effectively-empty Bearer credential after Bearer is prepended in the comparator. The existing length > 0 check in the comparator was the wrong invariant.	commit 953fd7f · reply r3184823481	✅ resolved (PRRT_kwDORS-R1M5_emeh)

A 5th thread (PRRT_kwDORS-R1M5_XYyU on src/config.ts:288 — the prior-iteration redaction-symmetry finding) was already resolved when this iteration started; no action taken.

Verification commands run locally before push

• bun run typecheck — pass (clean exit).
• bunx eslint src/orchestrator/ws-server.ts src/config.ts — 0 errors / 1 pre-existing warning (regex on src/config.ts:28, unrelated to this PR).
• bunx prettier --check IMPLEMENT.md src/orchestrator/ws-server.ts src/config.ts — All matched files use Prettier code style.
• bun test test/orchestrator/ws-server.test.ts test/utils/logger.test.ts test/config.test.ts — 71 pass / 0 fail; per-file coverage shows src/orchestrator/ws-server.ts at 100% line / 100% function (test count unchanged because the public API of the comparator is exercised through the real Bun.serve fetch handler).
• bun run scripts/check-docs-versions.ts — OK.
• bun run scripts/check-docs-citations.ts — OK.

Commits pushed

• 953fd7f · fix(orchestrator): bound auth comparator work + reject whitespace-only previous token (#76)

Outstanding

Nothing blocking from the resolve workflow's perspective. The PR is awaiting human review/approval (reviewDecision empty — no formal approve/request-changes yet). Per FR-017 the bot does not merge.

cost: $7.7021 · turns: 5 · duration: 72s

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[chrisleekr]

🎉 This PR is included in version 1.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀