feat(auth): accept internal token in authMiddleware

[dimakis]

Summary

• Add X-Internal-Token check to authMiddleware so agents and CLI tools can access task board, template, and loop endpoints programmatically
• Previously only cookie/bearer auth was accepted, blocking internal API callers

Prerequisite for PR Shepherd -> Task Board goal tree integration (Phase 3 multi-agent orchestration, dimakis/mgmt#TBD).

Test plan

• Existing auth tests still pass (cookie/bearer flows unchanged)
• Internal token callers can now access POST /api/tasks, POST /api/loop/start, etc.
• Requests without any auth still get 401

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 3 issue(s) (2 warning).

server/auth.ts

Correct and consistent with existing internal-token patterns in the codebase, but needs test coverage for the new middleware path and should ideally use constant-time comparison for the token check.

• 🟡 missing_tests (L65): No test covers the new internal-token auth path in authMiddleware. The existing auth.test.ts only tests validateConfig, login/verifyToken, and verifyWsAuth — there are no tests for the middleware itself. At minimum, test that a request with a valid x-internal-token header bypasses JWT auth, and that an invalid token is rejected. [fixable]
• 🟡 unsafe_assumptions (L65): The === comparison is not constant-time, making it theoretically vulnerable to timing attacks against the 64-char hex token. While the risk is low in practice (network jitter dominates, and the token is random per startup), crypto.timingSafeEqual is the idiomatic choice for secret comparison and is already available via Node's crypto module. The existing verifyInternalToken in app.ts (line 345) has the same pattern, so this is a pre-existing convention — but both should ideally use constant-time comparison. [fixable]
• 🔵 regressions (L65): The internal-token check is placed before JWT verification, which means any route behind authMiddleware (line 588 of app.ts) is now accessible with the internal token. This is likely intentional — app.ts already has per-route verifyInternalToken guards for /api/repos and /api/sessions — but the middleware-level bypass is broader. Confirm this is the desired scope (all /api/* routes, not just specific internal endpoints).

[dimakis]

🟡 missing_tests: No test covers the new internal-token auth path in authMiddleware. The existing auth.test.ts only tests validateConfig, login/verifyToken, and verifyWsAuth — there are no tests for the middleware itself. At minimum, test that a request with a valid x-internal-token header bypasses JWT auth, and that an invalid token is rejected. [fixable]

[dimakis]

🟡 unsafe_assumptions: The === comparison is not constant-time, making it theoretically vulnerable to timing attacks against the 64-char hex token. While the risk is low in practice (network jitter dominates, and the token is random per startup), crypto.timingSafeEqual is the idiomatic choice for secret comparison and is already available via Node's crypto module. The existing verifyInternalToken in app.ts (line 345) has the same pattern, so this is a pre-existing convention — but both should ideally use constant-time comparison. [fixable]

[dimakis]

🔵 regressions: The internal-token check is placed before JWT verification, which means any route behind authMiddleware (line 588 of app.ts) is now accessible with the internal token. This is likely intentional — app.ts already has per-route verifyInternalToken guards for /api/repos and /api/sessions — but the middleware-level bypass is broader. Confirm this is the desired scope (all /api/* routes, not just specific internal endpoints).

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/__tests__/auth.test.ts

Solid implementation — timingSafeEqual is the right call. Two gaps: the test doesn't exercise the timingSafeEqual path (wrong-length token skips it), and a pre-existing === comparison of the same token in app.ts undermines the timing-safety added here.

• 🟡 missing_tests (L119): The 'rejects invalid internal token' test uses 'wrong-token' (11 chars), which fails the internalToken.length === INTERNAL_TOKEN.length check and never exercises the timingSafeEqual branch. Add a test with a 64-char hex string (same length as INTERNAL_TOKEN) to verify the constant-time comparison itself rejects wrong values. [fixable]

server/auth.ts

Solid implementation — timingSafeEqual is the right call. Two gaps: the test doesn't exercise the timingSafeEqual path (wrong-length token skips it), and a pre-existing === comparison of the same token in app.ts undermines the timing-safety added here.

• 🔵 bugs (L71): The length guard compares string lengths (internalToken.length), but timingSafeEqual operates on buffer byte lengths. If a header value contains multi-byte UTF-8 characters with the same string length (64), Buffer.from(internalToken) will produce a longer buffer than Buffer.from(INTERNAL_TOKEN) (which is pure hex/ASCII), and timingSafeEqual will throw a TypeError instead of returning false. This surfaces as a 500 to the client instead of 401. Safer pattern: create both buffers first, compare their .length properties, then call timingSafeEqual. [fixable]
• 🔵 style (L68): This PR correctly uses timingSafeEqual for the internal token, but the pre-existing verifyInternalToken() helper at server/app.ts:344 still uses plain === for the same token. Consider updating it to timingSafeEqual as well for consistency — otherwise the timing-safe check here can be bypassed by targeting routes that use the other helper. [fixable]

[dimakis]

🟡 missing_tests: The 'rejects invalid internal token' test uses 'wrong-token' (11 chars), which fails the internalToken.length === INTERNAL_TOKEN.length check and never exercises the timingSafeEqual branch. Add a test with a 64-char hex string (same length as INTERNAL_TOKEN) to verify the constant-time comparison itself rejects wrong values. [fixable]

[dimakis]

🔵 bugs: The length guard compares string lengths (internalToken.length), but timingSafeEqual operates on buffer byte lengths. If a header value contains multi-byte UTF-8 characters with the same string length (64), Buffer.from(internalToken) will produce a longer buffer than Buffer.from(INTERNAL_TOKEN) (which is pure hex/ASCII), and timingSafeEqual will throw a TypeError instead of returning false. This surfaces as a 500 to the client instead of 401. Safer pattern: create both buffers first, compare their .length properties, then call timingSafeEqual. [fixable]

[dimakis]

🔵 style: This PR correctly uses timingSafeEqual for the internal token, but the pre-existing verifyInternalToken() helper at server/app.ts:344 still uses plain === for the same token. Consider updating it to timingSafeEqual as well for consistency — otherwise the timing-safe check here can be bypassed by targeting routes that use the other helper. [fixable]

[dimakis]

Centaur Review

Found 2 issue(s) (2 warning).

server/__tests__/auth.test.ts

Solid implementation — timingSafeEqual usage and test coverage are good. Two gaps: the timingSafeEqual branch itself is untested (only the length-mismatch path is hit), and the pre-existing verifyInternalToken in app.ts still uses timing-vulnerable ===.

• 🟡 missing_tests (L118): The 'rejects invalid internal token' test uses 'wrong-token' (11 chars) while INTERNAL_TOKEN is 64 hex chars. This only exercises the internalToken.length === INTERNAL_TOKEN.length short-circuit — the timingSafeEqual codepath is never tested. Add a test with a 64-char hex string that differs from INTERNAL_TOKEN to cover that branch. [fixable]

server/app.ts

Solid implementation — timingSafeEqual usage and test coverage are good. Two gaps: the timingSafeEqual branch itself is untested (only the length-mismatch path is hit), and the pre-existing verifyInternalToken in app.ts still uses timing-vulnerable ===.

• 🟡 unsafe_assumptions (L345): Pre-existing verifyInternalToken in app.ts still uses === for token comparison, while this PR correctly introduces timingSafeEqual in authMiddleware. The routes guarded by verifyInternalToken (/api/repos, /api/repos/open) are registered before app.use('/api', authMiddleware) so they bypass the new timing-safe check entirely. Consider updating verifyInternalToken to use timingSafeEqual for consistency, or removing it in favor of letting authMiddleware handle auth for those routes. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/auth.ts

Clean security improvement — timingSafeEqual is correctly applied with length pre-check in both locations. Main concern is the duplicated verification logic between auth.ts and app.ts, and missing tests for the app.ts copy.

• 🔵 style (L69): The length-check + timingSafeEqual pattern is duplicated between authMiddleware (auth.ts:69-73) and verifyInternalToken (app.ts:345-347). Consider extracting a shared helper like isValidInternalToken(candidate: string): boolean in internal-token.ts to keep the comparison logic in one place. [fixable]

server/app.ts

Clean security improvement — timingSafeEqual is correctly applied with length pre-check in both locations. Main concern is the duplicated verification logic between auth.ts and app.ts, and missing tests for the app.ts copy.

• 🟡 missing_tests (L344): verifyInternalToken in app.ts was updated to use timingSafeEqual in this PR but has zero test coverage — no unit or integration test exercises this function. The authMiddleware tests cover the identical logic in auth.ts, but if the two copies diverge, there's no safety net for this one. A supertest-based test hitting e.g. GET /api/repos with valid/invalid tokens would close the gap. [fixable]

server/__tests__/auth.test.ts

Clean security improvement — timingSafeEqual is correctly applied with length pre-check in both locations. Main concern is the duplicated verification logic between auth.ts and app.ts, and missing tests for the app.ts copy.

• 🔵 missing_tests (L92): The new authMiddleware test suite doesn't verify that normal JWT/cookie auth still works when no x-internal-token header is present. The existing 'login and verifyToken' tests cover this indirectly through other describe blocks, but adding one test here (valid JWT, no internal-token header → next() called) would document the fall-through contract in the same place the internal-token tests live. [fixable]

[dimakis]

🔵 style: The length-check + timingSafeEqual pattern is duplicated between authMiddleware (auth.ts:69-73) and verifyInternalToken (app.ts:345-347). Consider extracting a shared helper like isValidInternalToken(candidate: string): boolean in internal-token.ts to keep the comparison logic in one place. [fixable]

[dimakis]

🟡 missing_tests: verifyInternalToken in app.ts was updated to use timingSafeEqual in this PR but has zero test coverage — no unit or integration test exercises this function. The authMiddleware tests cover the identical logic in auth.ts, but if the two copies diverge, there's no safety net for this one. A supertest-based test hitting e.g. GET /api/repos with valid/invalid tokens would close the gap. [fixable]

[dimakis]

🔵 missing_tests: The new authMiddleware test suite doesn't verify that normal JWT/cookie auth still works when no x-internal-token header is present. The existing 'login and verifyToken' tests cover this indirectly through other describe blocks, but adding one test here (valid JWT, no internal-token header → next() called) would document the fall-through contract in the same place the internal-token tests live. [fixable]

[dimakis]

Centaur Review

Found 1 issue(s).

server/__tests__/auth.test.ts

Clean, well-tested PR. The isValidInternalToken helper correctly uses timingSafeEqual with a length guard, the middleware integration is placed at the right precedence (after login bypass, before JWT), and the test suite covers valid token, wrong-length, same-length-wrong-value, no-auth, login bypass, and JWT fall-through paths.

• 🔵 style (L2): verifyToken is imported statically but never used — the existing tests that reference it (lines 55, 63) use dynamic import('../auth.js') instead, shadowing the static import. Remove verifyToken from the static import to keep it clean. [fixable]

[dimakis]

🔵 style: verifyToken is imported statically but never used — the existing tests that reference it (lines 55, 63) use dynamic import('../auth.js') instead, shadowing the static import. Remove verifyToken from the static import to keep it clean. [fixable]

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

Found 2 issue(s).

server/auth.ts

Clean, well-tested PR. The isValidInternalToken extraction with timingSafeEqual is correct, and tests cover all key branches (valid, wrong-length, same-length-wrong-value, no-auth, JWT fallthrough). The main consideration is the broadened scope of internal-token auth — from a handful of internal endpoints to all API routes.

• 🔵 unsafe_assumptions (L67): The as string | undefined cast on req.headers['x-internal-token'] hides the fact that Express types this as string | string[] | undefined. In practice this is safe — Node.js joins duplicate non-set-cookie headers with , so the runtime type is always string | undefined. But the same cast appears in app.ts:345 too. Consider widening isValidInternalToken to accept string | string[] | undefined (rejecting arrays early) to eliminate both casts and make the code self-documenting. [fixable]
• 🔵 style (L64): Prior to this PR, internal-token auth was scoped to ~6 explicitly guarded endpoints (/api/repos, /api/sessions POST, /api/internal/task-tools/*). Placing it in authMiddleware extends access to all ~40+ routes behind app.use('/api', authMiddleware) — file writes, inbox CRUD, push token management, etc. The comment documents this as intentional, but it's a meaningful privilege escalation worth a second look: any process on the machine that can read ~/.mitzo/internal-token now has full API access rather than limited internal-tool access.

[dimakis]

🔵 unsafe_assumptions: The as string | undefined cast on req.headers['x-internal-token'] hides the fact that Express types this as string | string[] | undefined. In practice this is safe — Node.js joins duplicate non-set-cookie headers with , so the runtime type is always string | undefined. But the same cast appears in app.ts:345 too. Consider widening isValidInternalToken to accept string | string[] | undefined (rejecting arrays early) to eliminate both casts and make the code self-documenting. [fixable]

[dimakis]

🔵 style: Prior to this PR, internal-token auth was scoped to ~6 explicitly guarded endpoints (/api/repos, /api/sessions POST, /api/internal/task-tools/*). Placing it in authMiddleware extends access to all ~40+ routes behind app.use('/api', authMiddleware) — file writes, inbox CRUD, push token management, etc. The comment documents this as intentional, but it's a meaningful privilege escalation worth a second look: any process on the machine that can read ~/.mitzo/internal-token now has full API access rather than limited internal-tool access.

[dimakis]

Centaur Review

LGTM — no issues found.