Fix/sr 131 134 security hardening - #1412
Merged
GoodnessJohn merged 5 commits intoAug 28, 2026
Merged
Conversation
…vices (SR-131) scopedApiKeyMiddleware and initApiKeyMiddleware existed fully implemented in api-key-rate-limit.ts but were never wired into api.ts, leaving /api/admin/*, /api/aml/*, /api/compliance/* and /api/devices/* protected only by a flat 20 req/min rate limiter with no identity check — GET /api/compliance/report served up to 10,000 joined remittance/transaction rows (sender_address, amounts, currency) to any caller. - Mount scopedApiKeyMiddleware ahead of every router and call initApiKeyMiddleware(pool) at startup; the middleware now rejects unauthenticated requests to any scope-mapped route with 401 instead of silently letting them through. - Extend ROUTE_SCOPES to cover /api/aml, /api/compliance and /api/devices, which previously had no entry so requiredScopeForRoute silently returned null for them. - Derive audit-log/officer attribution (logAdminAction in api.ts/admin.ts, requireOfficer in aml.ts, requiredActor in compliance.ts) from the verified API-key owner instead of the unverified x-user-id/x-officer-id headers; compliance.ts now rejects with 401 instead of defaulting to 'anonymous'. - Add backend/AUTH_MATRIX.md and a drift test asserting each of the three route groups rejects unauthenticated requests, mirroring api/AUTH_MATRIX.md.
…rasure/purge (SR-131) privacy.ts implemented consent/erasure/retention entirely against in-process Map objects that no other request or the database ever saw, and privacyRouter was never app.use()'d anywhere — every SAR/erasure/rectify/consent endpoint was unreachable. Separately, POST /purge-expired called purgeExpiredPersonalData() with no pool argument, which returns an all-zero report immediately, so the endpoint always reported success while deleting nothing and was not on any scheduler. - Mount privacyRouter at /api/v1/privacy in api.ts; every handler enforces ownership (the request's own identity or an admin:* scoped key) before acting on a given user_id. - Replace the Map-backed stores with queries against user_consents, notification_preferences, user_kyc_status, kyc_uploads and privacy_requests, using encryptColumn/decryptObject for email/phone and IP columns at rest. - Pass the real pg.Pool into purgeExpiredPersonalData(pool) from the route handler (admin:* only) and add the same purge to the nightly scheduler (03:45 UTC) alongside the existing AML RetentionService job, so audit-log IP anonymization, transient KYC purge and revoked-consent purge actually run automatically. - Add a regression test asserting the purge report's counts match the rowCount each underlying query reports, and that per-category failures don't abort the remaining purges.
… (SR-131) encryption.ts's getMasterKey() silently fell back to DEFAULT_KEY_HEX — a literal, publicly-visible constant committed to the repo — whenever ENCRYPTION_KEY was unset, in every environment including production, with no guard at all. encryptColumn/decryptColumn/encryptObject/ decryptObject back the GDPR privacy routes' "encryption" of full_name, email, phone_number and address; a misconfigured deploy would produce ciphertext anyone who had read this source file already had the key for, while application code and logs kept treating it as protected. This also bypassed the shared SecretsManager that already centralizes sourcing/ rotation checks for the JWT secret, DB URL and admin key. - Remove DEFAULT_KEY_HEX. getMasterKey() now throws immediately outside NODE_ENV=test when ENCRYPTION_KEY is absent, mirroring env-guard.ts's fail-fast treatment of other required secrets. - Add getEncryptionKey() to the shared SecretsManager (optional secret, same rotation/validation posture as FX_API_KEY) and resolve it in backend/src/index.ts's loadSecrets(), writing it into process.env so the synchronous encryption module keeps working without an invasive async refactor of every call site. - Add a test asserting encryptColumn/decryptColumn throw rather than silently encrypting with a known key when ENCRYPTION_KEY is unset outside test, and that a configured key still works in production.
…webhook (SR-131)
verifyAnchorSignature existed but was never called anywhere, and even if it
had been, it only checked the caller-controlled anchor_id URL parameter
against an allowlist — no HMAC/signature validation at all. Anyone on the
internet could POST {"user_id": "...", "status": "APPROVED"} to
/webhooks/kyc/:anchor_id and flip a user's KYC status, which feeds
setKycApprovedOnChain (stellar-kyc.ts): an unauthenticated path to on-chain
KYC approval. SR-045 hardened ramp-webhook-handler.ts but never touched this
SEP-12 path.
- Rewrite verifyAnchorSignature to compute HMAC-SHA256 over
"${timestamp}.${rawBody}" using a per-anchor WEBHOOK_SECRET_{ANCHOR_ID}
secret (already documented in .env.example but unused), compared with
crypto.timingSafeEqual, mirroring ramp-provider.ts's verifyWebhook.
- Enforce a 5-minute timestamp window and an in-memory nonce replay cache
(bounded, self-pruning), consistent with SR-045's treatment of ramp
provider webhooks.
- handleKycWebhook now verifies the signature before doing anything else and
rejects with 401 (with a machine-readable reason) on any failure —
missing/invalid signature, missing/stale timestamp, wrong anchor secret,
or replayed nonce — never reaching saveUserKycStatus.
- Add a `verify` callback to the global express.json() in api.ts so
handlers can sign/verify against the exact request bytes rather than a
re-serialization of the parsed body.
- Rewrite the webhook handler test suite: valid-signature happy path plus
regression tests asserting unsigned, mis-signed, wrong-anchor, and replayed
requests are all rejected with 401 and never call saveUserKycStatus.
- Update .env.example: document the WEBHOOK_SECRET_<ANCHOR_ID> requirement
and remove the now-dead TRUSTED_ANCHOR_IDS-only guidance.
|
@james2177 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@james2177 is attempting to deploy a commit to the Harold's projects Team on Vercel. A member of the Team first needs to authorize it. |
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
2 participants
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
Linked Issue
Closes #1285
Closes #1286
Closes #1287
Closes #1288
Type of Change
Checklist
sdk/) if this touches a published packageBreaking Change Notes