apiKeyAuth fails open when no key is configured and compares keys with !==, a non-constant-time comparison #222

Priority: High  ·  Area: Authentication  ·  Est. effort: 8–12 h

📌 Problem

src/middleware/apiKeyAuth.ts is 28 lines and has three distinct weaknesses.

1. Fails open by default. Its own doc comment states it: "if no key is configured the middleware is a no-op (open access)".

if (!apiKey || !MUTATING_METHODS.has(req.method)) {
  next();
  return;
}

A missing or empty apiKey — a typo in an env var name, a secret that failed to load, a config path that silently returned undefined — makes every mutating endpoint publicly writable. The service does not refuse to start; it starts wide open, and nothing distinguishes that from a deliberate open-access deployment.

2. Non-constant-time comparison.

if (req.header("x-api-key") !== apiKey) {

JavaScript's !== on strings short-circuits at the first differing byte. That leaks a timing signal proportional to the length of the correct prefix, which is the standard precondition for a byte-at-a-time key-recovery attack.

3. A single static shared key. One key for all callers means no per-client identity, no revocation of an individual consumer, and no rotation without coordinating every client at once.

🎯 Design decision required

State and defend:

1. Fail-closed policy. Should a missing key be a startup failure rather than open access? Argue it, including how local development stays workable — an explicit AUTH_DISABLED=true that logs loudly is very different from an unset variable that silently disables auth.
2. Read-only exposure. The middleware only guards mutating methods; GET is always open. Confirm whether that is intended for every read endpoint, and say so — an intentional public read API is fine, an accidental one is not.
3. Key model. Single shared key, or per-client keys with identity? Per-client enables revocation and audit attribution, at the cost of storage — and this service currently has no persistence layer (separate issue). Argue what is achievable now versus what should follow.

🧩 Requirements and context

• The comparison must become constant-time. crypto.timingSafeEqual is in the Node standard library; note it throws on length mismatch, so handle that without reintroducing a length oracle.
• If you move to stored keys, store a hash, never the raw key.
• Changing the default to fail-closed is a breaking change for any deployment relying on the current behaviour. State that plainly in the PR.
• All 42 existing test files must pass. src/middleware/apiKeyAuth.test.ts exists — extend it.
• src/config.ts is where startup validation belongs.

🛠️ Suggested execution

1. Write tests pinning current behaviour: unset key ⇒ mutating request succeeds; wrong key ⇒ 401.
2. Add a constant-time comparison with a test for the length-mismatch path.
3. Implement fail-closed startup validation with the explicit opt-out.
4. Add a test asserting the service refuses to start (or refuses mutations) with no key and no explicit opt-out.
5. Document the breaking change and the migration for operators.

✅ Acceptance criteria

• Key comparison is constant-time, with a test covering mismatched lengths.
• A missing key no longer results in silent open access; the behaviour matches your stated policy and is tested.
• Any opt-out is explicit and logs prominently at startup.
• The PR states the read-only exposure decision.
• If keys are stored, they are hashed — never raw.
• The breaking change and operator migration are documented.
• npm run lint, npm run build and npm test pass.

🚫 Out of scope

• Building a full OAuth/JWT system.
• The persistence layer — separate issue, though per-client keys may depend on it.
• Rate limiting — separate issue.

🧪 Verification

npm ci
npm test src/middleware/apiKeyAuth.test.ts
npm run lint && npm run build && npm test

📤 What your PR must include

1. Tests pinning the original fail-open behaviour.
2. Your fail-closed policy and how local development stays usable.
3. Your read-only exposure decision.
4. Your key-model decision and what you deferred.
5. Closes #<n>.

🔒 Security notes

This is the only authentication on the service's mutating endpoints — the ones that create anchors, move liquidity and record settlements. The fail-open default is the more serious of the two defects: it converts an ordinary configuration mistake into an unauthenticated write API, with no error and no signal that it has happened. The timing leak is a genuine secondary concern but requires an attacker already probing the endpoint; the fail-open path requires only a deployment mistake.

📋 Guidelines

• Minimum 95% test coverage on changed lines
• Clear documentation
• Timeframe: 96 hours from assignment
• One logical change per commit; no merge commits

💬 Join our community

Working on this, or want to sanity-check your approach before you start? Come and ask — the maintainers are there and happy to help.

Telegram: https://t.me/Grainlify