fix: audit quick-wins + security hardening - #17
Merged
Merged
Conversation
From a multi-agent codebase audit. Quick wins: - og: fix undefined `superlogo` (every cast OG / share-preview image 500'd; it was masked by a @ts-ignore) — reuse the existing castoraLogo. - search-suggestions, profile/search: guard a missing `query` param (was a 500 via .replace on null) — return empty results. - notifications: reject an invalid `mode` with 400 instead of a 500. - finish-registration: wrap the 4 account-creation writes in a $transaction so a partial failure can't orphan rows / wedge the unique fid on retry. - dead code: remove orphaned components (banner, PwaInstallPrompt), the dead super-auth routes (create-connection, create-signer, verify-farcaster), and unused deps (lokijs, encoding). Security: - url-preview/other (public, unauthenticated): add an SSRF guard — block private/reserved/link-local/metadata IPs (resolved, not just the literal) and disable redirect-following. (new URL() normalizes alt IP encodings — verified.) - crypto-checkout webhook (Daimo, money path): constant-time token compare; make payment_completed exactly-once via a compare-and-swap claim with revert-on-failure (was: double-provision on every retry); fix a bare `return;` that returned undefined from the handler. Verified: typecheck + build green; adversarial review of the SSRF + webhook changes (alt-IP-encoding & zone-id "bypasses" confirmed non-issues via new URL() normalization; webhook reworked to a CAS claim). Follow-up: make handleSuccessfulPayment idempotent (defense-in-depth) and retire the remaining dead super-auth code (AccountConnectionSession model, signer.ts) — both need a Prisma migration, kept separate.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
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.
Highest-value findings from a multi-agent codebase audit (6 finders → adversarial verification → synthesis). The audit found no critical fires — verification correctly downgraded the scary-sounding findings (a "hardcoded secret" that's just an event tag, an "opt-out enumeration" gated behind a UUID, a null-deref already handled). This PR lands the real, actionable ones.
Quick wins
superlogowas undefined — masked by a@ts-ignore— so every cast share-preview image render 500'd. Now reuses the existingcastoraLogo.search-suggestions+profile/search.replace()'d a possibly-nullquery(→ 500);notificationsaccessedresponsefor an invalidmode(→ 500). Now clean 400s / empty results.finish-registration: wrapped its 4 account-creation writes in a$transaction— a partial failure used to orphan rows and, becausefidis unique, wedge the retry.banner,PwaInstallPrompt), the dead super-auth routes (create-connection,create-signer,verify-farcaster), and unused deps (lokijs~3.5MB,encoding).Security
url-preview/other(public, unauthenticated): blocks private/reserved/link-local/cloud-metadata IPs (resolved via DNS, not just the literal) and disables redirect-following. Verified thatnew URL()already normalizes decimal/octal/hex IP encodings (http://2130706433/→127.0.0.1in both the validated hostname and the fetched URL) and throws on IPv6 zone IDs — so the "encoding bypass" the reviewer raised doesn't apply. Block-if-any-resolved-IP-is-private is kept deliberately (allowing a host because some IP is public would let axios still pick the private one).payment_completedis now exactly-once via a compare-and-swap claim (updateMany where status = previous) with revert-on-failure — previously it double-provisioned membership/storage on every Daimo retry. Also fixed a barereturn;that returnedundefinedfrom the handler.Verification
npm run typecheck✅,npm run build✅ (138 routes; the 3 deleted routes gone).new URL()normalization.Follow-ups (need a Prisma migration — kept separate)
handleSuccessfulPaymentidempotent (defense-in-depth for the webhook).AccountConnectionSessionmodel,signer.ts/APP_MNEMONIC).@@unique([pollId, voterFid]); react-query v3→v5 unification; re-enable ESLint-in-build +reactStrictMode(config debt) — all tracked from the audit.