fix(security): make the job routes' API-key gate fail closed - #405
Merged
Merged
Conversation
The six API_KEY checks in src/routes/jobs.ts were written as
`if (requiredApiKey) { ...verify... }`, which skipped authentication
entirely whenever API_KEY was unset. A deployment that forgot to set it
served these endpoints to anyone, with nothing in the logs to say so:
GET /api/jobs/by-wallet/:address
GET /api/jobs/:contractId
GET /api/jobs/:contractId/whitelist
POST /api/jobs/:contractId/whitelist/update
POST /api/jobs/:contractId/milestones/:index/partial-release
GET /api/jobs/:contractId/milestones/:index/time-remaining
requireAdmin() in middleware/adminAuth.ts already failed closed, so the
two halves of the codebase disagreed on what a missing key means.
Replace all six copies with one ensureApiKey() helper that rejects when
API_KEY is unset, logging at error level so a misconfigured deployment is
visible. Six duplicated copies are what let this drift in the first place.
Also add a production startup assertion to src/index.ts. It runs directly
after dotenv.config(), before migrations, the poller and listen, so a
misconfigured container exits instead of booting into a state that passes
/health while serving job endpoints unauthenticated (API_KEY unset),
rejecting the real frontend origin (ALLOWED_ORIGINS defaulting to
localhost:3000), or indexing nothing against testnet (CONTRACT_ID unset).
Tests: closing the gate invalidated 113 existing tests that reached their
handlers only because the gate was open. They are not weakened — each now
authenticates through a test-only autoAuth middleware that injects the key
only while API_KEY still holds jest.setup's default, so the dedicated auth
suites keep full control of the header and their "no key"/"wrong key"
assertions are untouched. Two tests that asserted the old behaviour
("returns 200 (no gate) when API_KEY is not set") now assert 401.
Adds one fail-closed test per route plus two covering the startup
assertion, which imports the real entry point rather than re-implementing
the check. Both verified by mutation: reverting ensureApiKey to fail-open
fails exactly six tests, one per route; removing the startup assertion
fails exactly two.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
The
API_KEYchecks insrc/routes/jobs.tswere written so that a missing key disabled authentication rather than denying it:A deployment that forgot to set
API_KEYserved six endpoints to anyone, and logged nothing to say so:/api/jobs/by-wallet/:address/api/jobs/:contractId/api/jobs/:contractId/whitelist/api/jobs/:contractId/whitelist/update/api/jobs/:contractId/milestones/:index/partial-release/api/jobs/:contractId/milestones/:index/time-remainingrequireAdmin()inmiddleware/adminAuth.tsalready failed closed on the same condition, so the two halves of the codebase disagreed about what a missing key means.The fix
All six copies collapse into one
ensureApiKey()helper that rejects whenAPI_KEYis unset, logging aterrorlevel so a misconfigured deployment is visible:Six duplicated copies are what allowed this to drift — one implementation is also easier to audit.
Plus a production startup assertion in
src/index.ts, placed directly afterdotenv.config()and before migrations, the poller andlisten, so a misconfigured container exits rather than booting into a state that passes/healthwhile serving job endpoints unauthenticated, rejecting the real frontend origin (ALLOWED_ORIGINSdefaults tolocalhost:3000), or indexing nothing against testnet.About the test changes
Closing the gate invalidated 113 existing tests that reached their handlers only because the gate was open. They are not weakened to accommodate the fix.
Each now authenticates through a test-only
autoAuthmiddleware that injects the key only whileAPI_KEYstill holds jest.setup's default value. The dedicated auth suites set their own value ("secret-key","secret-test-key") in abeforeEach, so for those the middleware does nothing and their "no key provided" / "wrong key" assertions test exactly what they did before. An explicitx-api-keyheader is never overwritten.Two tests asserted the old behaviour outright —
returns 200 (no gate) when API_KEY is not set— and now assert 401.Verification
Baseline before this change was 91 suites / 1612 tests, so this adds 6: one fail-closed test per route, plus two for the startup assertion. That test imports the real
src/index.tsrather than re-implementing the check, so deleting the assertion or moving it below a side effect fails the test.Mutation-verified. Reverting
ensureApiKeyto fail-open fails exactly six tests, one per hardened route:Removing the startup assertion fails exactly two:
Both restored; suite green after each.
Deployment impact — read before merging
API_KEYandADMIN_API_KEYmust be set in production before this ships, or all six endpoints return 401 and the process refuses to boot. That is the intent, but it makes this a coordinated change rather than a drop-in.ALLOWED_ORIGINSandCONTRACT_IDbecome mandatory in production too.Not in scope
Deploy scaffolding (workflow,
deploy/,.nvmrc,engines) lands in a separate PR. The indexer's DB-over-envCONTRACT_IDprecedence is unchanged.🤖 Generated with Claude Code