fix: resolve security and correctness issues (#1099 #1100 #1101 #1102)#1124
Merged
ogazboiz merged 1 commit intoJun 27, 2026
Conversation
…#1100 LabsCrypt#1101 LabsCrypt#1102 - (LabsCrypt#1102) Remove duplicate POST /:loanId/mark-defaulted route registration in loanRoutes.ts; Express only ever dispatches to the first handler so the second block was unreachable dead code. - (LabsCrypt#1101) Add requireScopes('read:score') to the GET /:userId/breakdown route in scoreRoutes.ts, matching the guard applied to every sibling score route. Adds a test asserting a token without that scope gets 403. - (LabsCrypt#1100) Replace string inequality (k.secret !== keyStr) in requireApiKey with crypto.timingSafeEqual to prevent per-byte timing side-channels on the INTERNAL_API_KEY. Length is checked first; timingSafeEqual is used only on equal-length buffers. Adds a unit test for wrong-key-same-length. - (LabsCrypt#1099) Fix ensure-core-tables migration: replace the pg_tables guard (which only covers ordinary tables) with to_regclass(), which resolves views too. Prevents the migration from attempting to CREATE TABLE loan_events when the unified-contract-events migration has already created a VIEW of that name. Adds a migration-check CI job that runs the full migration set against a fresh Postgres 16 instance.
3 tasks
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.
Summary
This PR addresses four issues filed against the backend:
#1102 — Remove duplicate
mark-defaultedroute registrationloanRoutes.tsregisteredPOST /:loanId/mark-defaultedtwice inside the sameNODE_ENVguard (lines 62–69 and 72–79). Express only dispatches to the first matching handler, so the second block was unreachable dead code and a maintenance hazard.Change: Removed the duplicate registration block.
#1101 — Add missing
requireScopes('read:score')to/:userId/breakdownEvery sibling score route (
GET /:userId,/:walletAddress/history,/:walletAddress/nft) enforcesrequireScopes('read:score')betweenrequireJwtAuthand the param check. The breakdown route was missing it, allowing any JWT—regardless of scopes—to read score breakdown data.Changes:
requireScopes('read:score')to theGET /:userId/breakdownroute inscoreRoutes.tsscoreBreakdown.test.tsasserting that a token lackingread:scorereceives 403#1100 — Replace
!==withcrypto.timingSafeEqualinrequireApiKeyauth.tscompared API key secrets withk.secret !== keyStrinsideArray.find. String inequality short-circuits on the first differing byte, leaking per-character timing information that can be exploited to recoverINTERNAL_API_KEYover many requests.Changes:
import crypto from "node:crypto"crypto.timingSafeEqualon fixed-lengthBufferinstances (length checked first; keys of different lengths are rejected immediately without callingtimingSafeEqual)apiKeyScopes.test.tsverifying a wrong key of identical length is rejected#1099 — Fix
ensure-core-tablesmigration colliding with theloan_eventsviewMigration
1788000000018_unified-contract-events.jsrenames theloan_eventstable tocontract_eventsand creates a view namedloan_events. The subsequent migration1789000000000_ensure-core-tables.jsguarded itsCREATE TABLE loan_eventswithIF NOT EXISTS (SELECT FROM pg_tables WHERE tablename='loan_events').pg_tablesonly enumerates ordinary tables (relkind r/p) and excludes views, so the guard evaluatedTRUEand attempted to create the table, failing withrelation loan_events already existson any clean sequential migration run.Changes:
pg_tablesguard withto_regclass('public.loan_events') IS NULL, which resolves any relation type including viewsmigration-checkjob in.github/workflows/ci.ymlthat spins up a fresh Postgres 16 service and runsnpm run migrate:upagainst it end-to-endTest plan
backendCI job: lint, build, typecheck, and existing test suite passmigration-checkCI job: all migrations apply cleanly against a fresh empty databaseGET /api/score/:userId/breakdownwith a scopeless token → 403requireApiKeywith a wrong key of the same byte-length as the configured key → throws (rejected)Closes #1099
Closes #1100
Closes #1101
Closes #1102