fix(security): hash ApiKey/ApiMaster tokens at rest#74
Merged
Conversation
Critical finding from the architect audit (#73): both ApiKey.akKEY and ApiMaster.amKEY columns were UUID-typed and stored the raw token verbatim. A DB leak, replica snapshot, or backup-tarball compromise meant every active operator's credential was immediately replayable. Migration 20260518000000-hash-api-keys: - ALTER COLUMN akKEY / amKEY: UUID → TEXT (USING ::text cast). - JS-side UPDATE each row to SHA-256(value). Idempotent — rows that look already-hashed (64 lowercase hex chars) are skipped, so a partial-failed migration can be re-run safely. - Lookup index on the hashed column (non-unique because archived rows from key rotation may share a hash with their non-archived replacement until physically deleted; uniqueness was never constraint-enforced pre-migration). auth.isMaster + auth.getCompanyId hash the incoming `authKey` header via the new `hashKey()` helper before the SQL lookup, so operator-held tokens issued pre-migration keep working without re-issue. The hash is unsalted SHA-256 (vs. bcrypt/argon2id) because API tokens are high-entropy (UUID v4 = 122 bits); brute force against a leaked hash table is impractical, and bcrypt's per-request work-factor cost doesn't earn anything against the actual threat (DB-leak replay prevention). Same pattern Stripe / GitHub / most well-known API services use. Down migration: reverts column type to UUID. Any rows still in SHA-256 hex form will fail the cast — operator must rotate to fresh UUIDs first. Documented in the migration header. Verified locally against the compose Postgres: migration applies in 28 ms after the column-type change, 8 ApiKey rows + matching ApiMaster rows now hashed. Tests: 7 unit cases for `hashKey` + the no-DB short-circuit paths of isMaster/getCompanyId. Behavioral test of the hash-on-lookup flow needs the integration suite (vi.mock on db.config.js doesn't intercept the nested CJS require chain — documented limitation in audit #73 P5-M). Closes #73 P1-A. Suite: 276 / 276 + 4 integration skipped (was 269 / 269). Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Closes #73 (P1-A — the most critical audit finding).
Problem
ApiKey.akKEYandApiMaster.amKEYwere UUID-typed columns storing the raw token verbatim. Any DB leak / backup / replica snapshot meant every operator's credential was immediately usable.Fix
Migration
20260518000000-hash-api-keysconverts the columns to TEXT and replaces each row withSHA-256(value).auth.isMaster+auth.getCompanyIdhash the incoming header via a newhashKey()helper before the SQL lookup — operator tokens issued before the migration keep working without re-issue.Unsalted SHA-256 chosen over bcrypt/argon2id: API tokens are high-entropy (UUID v4 = 122 bits), so brute force against a leaked hash table is impractical; per-request bcrypt cost doesn't earn anything against the actual threat (DB-leak replay). Same approach Stripe / GitHub use.
Test plan
hashKey+ short-circuit casesauthKey:header is hashed server-side, matches the migrated row, request succeedsMigration safety
up: ALTER COLUMN UUID → TEXT, hash each row, add lookup index. Idempotent.down: drops the index, attempts UUID cast. Any post-up row still in 64-hex form fails the cast — operator must rotate to fresh UUIDs first. Documented in the migration header.Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/