fix: require dedicated secrets for download signing and API key encryption

[bmersereau]

Summary

• Remove SUPABASE_SECRET_KEY fallback from downloadTokens.getSecret() — DOWNLOAD_SIGNING_SECRET is now required
• Remove three-way fallback from userApiKeys.encryptionKey() — USER_API_KEYS_ENCRYPTION_SECRET is now required
• Add assertSecretIsolation() called at server startup to enforce presence, uniqueness, and cross-secret isolation of each secret
• Updates JSDoc comment in downloadTokens.ts to reflect the TTL-based expiration added in PR fix: add expiration to download tokens (30-day TTL) #77

Closes #66
Closes #82
Closes #83
Closes #88

Changes

• backend/src/lib/downloadTokens.ts — getSecret() reads only DOWNLOAD_SIGNING_SECRET; JSDoc updated to reference configurable TTL
• backend/src/lib/userApiKeys.ts — encryptionKey() reads only USER_API_KEYS_ENCRYPTION_SECRET
• backend/src/lib/startup.ts — new assertSecretIsolation() validates at boot: missing vars throw, secrets matching SUPABASE_SECRET_KEY throw, and DOWNLOAD_SIGNING_SECRET === USER_API_KEYS_ENCRYPTION_SECRET throws
• backend/src/index.ts — call assertSecretIsolation() before the server starts
• backend/vitest.config.ts — vitest config excluding dist/
• backend/src/lib/__tests__/secretIsolation.test.ts — 11 vitest unit tests covering all paths

Test plan

• npm test passes (11/11) in backend/
• npm run build passes with no TypeScript errors
• .env must be updated before restarting backend (see migration note below)

⚠️ Migration note

Before restarting the backend after this change, ensure your environment has:

DOWNLOAD_SIGNING_SECRET=<dedicated random value>
USER_API_KEYS_ENCRYPTION_SECRET=<dedicated random value>

Generate fresh values with openssl rand -hex 32.

If you previously had API_KEYS_ENCRYPTION_SECRET set (an undocumented legacy fallback that was removed in this PR): set USER_API_KEYS_ENCRYPTION_SECRET to that same value. Do not generate a new value — doing so will change the AES key and make all stored user API keys in the database unreadable.

[bmersereau]

PR Review: fix: require dedicated secrets for download signing and API key encryption

Summary

This PR eliminates the SUPABASE_SECRET_KEY fallback from both downloadTokens.getSecret() and userApiKeys.encryptionKey(), adds a dedicated startup.ts with assertSecretIsolation() that enforces presence and uniqueness of each secret at boot, and wires it into index.ts. The intent and implementation are correct; the main gap is a missing migration note for anyone using the now-silently-dropped API_KEYS_ENCRYPTION_SECRET fallback name.

Risk Assessment

• Risk Level: Medium
• Blast Radius: Any deployment relying on API_KEYS_ENCRYPTION_SECRET (the middle fallback that was silently removed) will: (1) fail to start until USER_API_KEYS_ENCRYPTION_SECRET is set, and (2) lose all stored user API keys if a new random value is used instead of copying the old secret
• Rollback Plan: git revert this commit; server restarts with the previous fallback chain intact and encrypted data readable again

---

Review by Category

Correctness

• [severity:major] Silent removal of the API_KEYS_ENCRYPTION_SECRET middle fallback without a migration note

  Before:

  const secret =
      process.env.USER_API_KEYS_ENCRYPTION_SECRET ||
      process.env.API_KEYS_ENCRYPTION_SECRET ||
      process.env.SUPABASE_SECRET_KEY;

  After:

  const secret = process.env.USER_API_KEYS_ENCRYPTION_SECRET;

  The API_KEYS_ENCRYPTION_SECRET name doesn't appear in .env.example, but it existed in the fallback chain and could have been set in production. The startup guard will block the server from starting (good), but the PR description only says "add USER_API_KEYS_ENCRYPTION_SECRET" — not "if you had API_KEYS_ENCRYPTION_SECRET set, copy that value." Setting a fresh random value would silently change the AES key and make all stored user API key rows unreadable (decrypt() swallows AES-GCM auth-tag failures and returns null). Please add to the PR description:

  ⚠️ Migration note: If API_KEYS_ENCRYPTION_SECRET was previously set in your environment, copy its value to USER_API_KEYS_ENCRYPTION_SECRET. Do not generate a new value — doing so will invalidate all stored user API keys in the database.

Security

• [severity:praise] The startup fail-fast pattern is exactly right. Throwing before the Express server binds means a misconfigured deploy never accepts traffic with a wrong or shared encryption key. Error messages include openssl rand -hex 32 remediation instructions — good operational hygiene.

• [severity:minor] assertSecretIsolation checks that neither application secret equals SUPABASE_SECRET_KEY, but does not check whether the two application secrets equal each other. Reusing the same value for HMAC signing and AES-GCM encryption is inadvisable (key separation). Simple addition:

  if (downloadSecret === encryptionSecret) {
      throw new Error(
          "DOWNLOAD_SIGNING_SECRET and USER_API_KEYS_ENCRYPTION_SECRET must not be the same value.",
      );
  }

Test Coverage

• [severity:nit] mockDb as never suppresses the type checker rather than satisfying it. Prefer:

  mockDb as Parameters<typeof saveUserApiKey>[3]

• [severity:minor] No test for the cross-secret equality check (once added): assertSecretIsolation should throw when DOWNLOAD_SIGNING_SECRET === USER_API_KEYS_ENCRYPTION_SECRET.

---

Test Coverage Assessment

• Lines added (non-test): 31
• Test lines added: 114
• Missing test cases: cross-secret equality (DOWNLOAD_SIGNING_SECRET === USER_API_KEYS_ENCRYPTION_SECRET); decrypt-path test with correct key (would catch KDF regressions from issue [Security][Critical] Weak AES key derivation for user API key encryption (SHA-256, no KDF) #67 fix)

---

Questions

1. Were any known production deployments using API_KEYS_ENCRYPTION_SECRET? Even if not, calling it out in the PR description is worth doing for posterity.

---

Verdict

• Approve - Ship it
• Approve with nits — Add the API_KEYS_ENCRYPTION_SECRET migration note to the PR description before merge; the cross-secret check and test are good additions but not blocking
• Request changes - Blocking issues must be addressed
• Needs discussion - Architecture/approach concerns

What I Verified

• Read the full diff
• Ran tests locally (10/10 pass)
• Checked .env.example on main — API_KEYS_ENCRYPTION_SECRET is absent from documented vars
• Traced decrypt() error-swallowing path to confirm data-loss scenario is real
• Reviewed linked issue [Security][Critical] SUPABASE_SECRET_KEY triple-reuse — single key leak = total system compromise #66

[bmersereau]

PR Review (follow-up): fix: require dedicated secrets for download signing and API key encryption

All issues from the previous review have been addressed. This is a clean pass.

What changed since last review

• Cross-secret equality check added to assertSecretIsolation: DOWNLOAD_SIGNING_SECRET === USER_API_KEYS_ENCRYPTION_SECRET now throws
• Matching test added (11th test) — asserts regex /DOWNLOAD_SIGNING_SECRET.*USER_API_KEYS_ENCRYPTION_SECRET/ for specificity
• mockDb as never → as unknown as SupabaseClient — proper typed cast
• vitest.config.ts added to exclude dist/ from test discovery
• PR description updated with API_KEYS_ENCRYPTION_SECRET migration note (clear failure mode + exact remediation)

Remaining (non-blocking)

• [severity:nit] API_KEYS_ENCRYPTION_SECRET is still listed in ENV_KEYS in the test snapshot array but is no longer used by any production code — can be removed.
• [severity:nit] The cross-secret equality check fires only after both missing-secret checks pass. This is acceptable UX (fix one thing at a time), no change needed.

Verdict

• Approve — Ship it

What I Verified

• Read the full diff
• Ran tests locally — 11/11 pass
• Confirmed assertSecretIsolation covers all pairwise secret combinations
• Verified migration note in PR description is accurate