fix(auth): normalize INTERNAL_JOB_TOKEN whitespace like the three private secrets - #9772
Conversation
…vate secrets
`authenticateInternalToken` compared the incoming bearer against the raw
`env.INTERNAL_JOB_TOKEN`, while its sibling `authenticatePrivateToken` wraps each
of its three secrets in `nonBlank()` (trim → undefined-if-empty) and early-exits
on a falsy token. `extractBearerToken` already trims the incoming token, so an
`INTERNAL_JOB_TOKEN` secret carrying a trailing newline — the shape a
`wrangler secret put` piped from a file produces — could never match any caller
and returned a blanket 401 for every `/v1/internal/*` route, while the same
whitespace on `LOOPOVER_API_TOKEN` was tolerated.
Apply `nonBlank()` and the `if (!token) return null` early exit, mirroring
`authenticatePrivateToken` exactly. This is a normalization gap, not a fail-open
one: `timingSafeEqual` already returns false for an empty/undefined expected
value, and `nonBlank(" ")` is `undefined`, so a whitespace-only secret still
authenticates nobody.
Closes JSONbored#9713
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 08:06:45 UTC
Review summary Nits — 3 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9772 +/- ##
==========================================
+ Coverage 76.58% 76.63% +0.04%
==========================================
Files 282 283 +1
Lines 59464 59575 +111
Branches 6555 6595 +40
==========================================
+ Hits 45543 45653 +110
Misses 13639 13639
- Partials 282 283 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Held for manual review: the gate and required CI are green, but GitHub reports this pull request's mergeable state as |
What & why
src/auth/security.tshas two token authenticators side by side.authenticatePrivateTokenearly-returnsnullfor a falsy token, then compares againstnonBlank(env.LOOPOVER_API_TOKEN),nonBlank(env.LOOPOVER_MCP_ADMIN_TOKEN), andnonBlank(env.LOOPOVER_MCP_TOKEN).authenticateInternalTokenwas the only one of the four comparing against the rawenv.INTERNAL_JOB_TOKEN, with nononBlank()and no early exit.extractBearerTokenalready trims the incoming bearer, so a configuredINTERNAL_JOB_TOKENcarrying leading/trailing whitespace — the exact shape a secret file or awrangler secret putpiped from a file produces when it ends in a newline — could never be matched by any caller, while the same whitespace onLOOPOVER_API_TOKENis tolerated. The failure mode is a blanket401 unauthorizedfrom the/v1/internal/*middleware for every internal job route and read endpoint, with nothing pointing at whitespace as the cause.The fix
authenticateInternalTokennow appliesnonBlank()toenv.INTERNAL_JOB_TOKENand early-returnsnullfor an absent/emptytoken, mirroringauthenticatePrivateTokenexactly (the existing module-localnonBlankhelper and the existingtimingSafeEqualcall shape — no new helper, no change totimingSafeEqualsemantics, no change at the middleware call site or to the incoming bearer).This is a normalization gap, not a fail-open one:
timingSafeEqualalready returnsfalsefor an empty/undefined expected value, andnonBlank(" ")isundefined, so a whitespace-only secret still authenticates nobody.Tests (
test/unit/auth-security-helpers.test.ts)New
#9713suite covering every DoD case:INTERNAL_JOB_TOKEN = " secret ", bearer"secret"→{ kind: "static", actor: "internal" }(named regression; fails on currentmain)."secret"/"secret") → internal identity (unchanged behavior).INTERNAL_JOB_TOKEN = " "(whitespace-only) →authenticateInternalToken(env, " ")and(env, "")bothnull.authenticateInternalToken(env, undefined)→nullwithout consulting the secret.null.Both arms of the new
if (!token)guard and both arms of thenonBlankresult are covered.Validation
npm run typecheckgreen; full auth suite green.src/auth/security.tsis 100% (every added line and branch); the file's whole-file branch coverage is well above the gate floor.git diff --checkclean; no route/schema/wrangler/migration change, so nothing to regenerate.src/auth/security.ts+ its unit test.Closes #9713