Harden the security layer (DNS pinning, rate-limit key, body cap, scan budget) and add a test suite - #2
Merged
Conversation
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
Closes the gap between what
guardUrl()validates and what the scanners actually connect to, plus a handful of smaller correctness fixes, and adds the repo's first tests.DNS pinning (the main one).
guardUrl()resolved a hostname and checked every answer, then handed the hostname tofetch()/tls.connect(), which resolved it again — so a second DNS answer could point anywhere (classic rebinding TOCTOU).guardUrlnow returns every validated address and there's a shared lookup that replays only those:safeFetchhad to move offfetch()tonode:http/node:httpsrequest()to acceptlookup. The public contract is unchanged (SafeFetchResult, manual redirects with the guard re-applied per hop,Headerswith everyset-cookieappended separately); only the init type narrowed fromRequestInitto a localSafeFetchInit. Noaccept-encodingis sent, so nothing needs decompressing.lib/tls/scan.tskeepshost/servernameas the hostname and passes the same pinned lookup to both the strict and lenient handshakes, which also stops the two sockets from landing on different endpoints.IPv6 literals.
url.hostnamereturns IPv6 literals bracketed, sonet.isIP("[::1]") === 0and every IPv6-literal target fell through to the hostname path and died indns.lookup— fail-closed, but also unusable. Brackets are now stripped before literal detection,::ffff:mapped addresses are decoded from their normalised hex form (::ffff:127.0.0.1→::ffff:7f00:1) before the IPv4 blocklist is applied, and2001:db8::/32joins the blocklist for parity with the IPv4 documentation ranges.Rate-limit key. The leftmost
x-forwarded-forentry is client-controlled, so it was trivially spoofable. New precedence:x-vercel-forwarded-for→x-real-ip→ rightmost XFF entry →"unknown". Expired buckets are now swept (time-based, or when the map exceeds 1000 entries) so the map can't grow unbounded.Body cap.
readLimitedTextappended a chunk then compared against 256 KiB, so one large chunk overshot the cap. It now slices the final chunk so the cap is a hard limit.Per-scan time budget. The 6s timeout was per hop while misconfig/CORS make 6–9 requests against 15s/20s
maxDuration.safeFetchtakes an optional absolutedeadlineMsand clamps each hop's timeout to the remaining budget (recomputed after DNS); misconfig and CORS set one deadline per scan (13s / 18s) and thread it through the initial fetch and every probe.Smaller fixes
lib/tls/der.ts: real bounds validation (truncated input, indefinite lengths, over-long length bytes, lengths past the buffer, OID ending on a continuation bit) and correct OID first-arc decoding —first = min(floor(b/40), 2), so0x55 04 03decodes as2.5.4.3, not1.45.4.3.lib/cors/scan.ts: the "suffix bypass" probe generatedhttps://attacker<host-with-dots-stripped>.example, which tests nothing the label claims. Suffix now ends with the target host (example.com→https://notexample.com, catching naiveendsWith); prefix stayshttps://example.com.attacker.example(naivestartsWith).Vary: Originis checked on every reflected-origin probe instead of only the arbitrary-origin one.lib/tls/types.ts: dropped theed25519/ed448/dsakeyTypevariants —detectKey()cannot produce them, so the Edwards-key finding was dead code.components/ToolCard.tsx: removedWrapper: anyin favour of explicitLink/divbranches.Tests
vitest(only new dep, dev-only) withnpm testwired into CI. 39 tests over the pure logic that carries the risk: the SSRF blocklists (v4, v6, mapped, documentation ranges) and DNS paths (mockednode:dns/promises), the DER reader incl. every new malformed case, JWT parse/analyze finding ids and severities, the misconfig header/cookie/disclosure graders, rate-limit key precedence and the 12/minute trip, and the CORS probe construction + reflection-with-credentials grading.The IPv6 literal bug above was caught by these tests on the first CI run, not by hand.
Network paths of
safeFetchandtls/scan.tsare not covered — worth a follow-up with a local server.npm test,npm run lint,npm run typecheckandnpm run buildall pass.Link to Devin session: https://app.devin.ai/sessions/c13b64ef84ed489f8e564ba16c6ffe96
Requested by: @tinkthemaker