fix(security): close SSRF guard bypasses in liveness-browser#917
Conversation
The PRIVATE_HOST_PATTERNS check in rejectPrivateOrInvalid runs against
`new URL(url).hostname`, which the WHATWG URL parser serializes in ways
that the original regex list doesn't cover. The guard is meant to keep
`scan.mjs --verify` and `check-liveness.mjs` from pointing headless
Chromium at internal infrastructure, but several Chromium-routable hosts
slip through it today:
- IPv6 hostnames keep their brackets (`new URL('http://[::1]/').hostname`
is `'[::1]'`), so `^::1$`, `^fc[0-9a-f]{2}:`, and `^fe80:` never match
any IPv6 URL — the loopback / ULA / link-local guards were effectively
dead.
- IPv4-mapped IPv6 (`::ffff:127.0.0.1` and the hex form `::ffff:7f00:1`)
routes to the embedded IPv4 in Chromium but wasn't decoded before the
IPv4 block list ran.
- `0.0.0.0` and the all-zeros IPv6 `::` both reach loopback on Linux and
were missing from the list.
- FQDN trailing dot (`localhost.`) bypassed the anchored `^localhost$`
pattern while still routing to localhost in the resolver.
The fix:
- Normalize `parsed.hostname` (lowercase, strip IPv6 brackets, strip
trailing dot) before the regex pass.
- Detect IPv4-mapped IPv6 in both dotted and hex form and check the
embedded IPv4 against the IPv4 patterns.
- Add explicit entries for `0.0.0.0`, `::`, and `localhost.localdomain`.
- Export `rejectPrivateOrInvalid` so it can be unit-tested directly.
`test-all.mjs` gains a block of cases that exercise every bypass above
plus a sanity allow-list for legitimate ATS hostnames so the guard
doesn't false-positive on Greenhouse / Lever / Pracuj.
Detected by Aeon + manual review of the SSRF surface documented in
SECURITY.md. CWE-918 (SSRF) / CWE-1390 (Weak Authentication via
Bypassable Identifier) — defense-in-depth weakening.
|
Welcome to career-ops, @aaronjmars! Thanks for your first PR. A few things to know:
We'll review your PR soon. Join our Discord if you have questions. |
📝 WalkthroughWalkthroughThis PR strengthens SSRF protection in the liveness browser module by expanding private-host detection patterns, normalizing hostnames (including IPv6 and IPv4-mapped addresses), exporting the blocking function for testing, and validating the changes with comprehensive regression tests covering known bypass vectors. ChangesSSRF Protection Enhancement
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
liveness-browser.mjs (1)
107-123:⚠️ Potential issue | 🟠 MajorEnforce private-host checks on redirects and all Playwright subrequests
liveness-browser.mjsappliesrejectPrivateOrInvalid(url)only to the initial navigation URL beforepage.goto(...), but it never re-validatespage.url()after redirects and there’s no Playwright request routing/interception to applyPRIVATE_HOST_PATTERNSto subsequent requests/subresources. Implement per-request enforcement (e.g.,context.route/page.routeor request interception) and treat redirect-to-private/blocked request URLs asuncertain/blocked rather than letting liveness classification proceed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@liveness-browser.mjs` around lines 107 - 123, The current flow only calls rejectPrivateOrInvalid(url) before page.goto and misses redirects and subresource requests; add a Playwright request route handler (using context.route or page.route) that calls rejectPrivateOrInvalid(req.url()) for every request and aborts or responds with a blocked/uncertain outcome when PRIVATE_HOST_PATTERNS match, and after navigation re-check page.url() (or the navigation response/request chain) with rejectPrivateOrInvalid to detect redirect-to-private targets before proceeding with liveness classification; update logic around page.goto and any liveness decision points to treat any blocked/uncertain result from the route handler or post-navigation check as an overall uncertain/blocked outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@liveness-browser.mjs`:
- Around line 59-72: The IPv6 ULA range fc00::/7 is only partially covered by
the /^fc[0-9a-f]{2}:/ pattern in PRIVATE_HOST_PATTERNS, leaving fd00::/8
addresses allowed; update the pattern for PRIVATE_HOST_PATTERNS (the entry
currently /^fc[0-9a-f]{2}:/) to match both fc and fd prefixes (e.g., use a regex
that matches f followed by c or d like /^f[cd][0-9a-f]{2}:/ with
case-insensitive matching) so fc00::/7 ULA addresses are fully blocked by the
SSRF guard.
---
Outside diff comments:
In `@liveness-browser.mjs`:
- Around line 107-123: The current flow only calls rejectPrivateOrInvalid(url)
before page.goto and misses redirects and subresource requests; add a Playwright
request route handler (using context.route or page.route) that calls
rejectPrivateOrInvalid(req.url()) for every request and aborts or responds with
a blocked/uncertain outcome when PRIVATE_HOST_PATTERNS match, and after
navigation re-check page.url() (or the navigation response/request chain) with
rejectPrivateOrInvalid to detect redirect-to-private targets before proceeding
with liveness classification; update logic around page.goto and any liveness
decision points to treat any blocked/uncertain result from the route handler or
post-navigation check as an overall uncertain/blocked outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5289b3a0-6927-45c8-b303-fc4218b2e1ad
📒 Files selected for processing (2)
liveness-browser.mjstest-all.mjs
| const PRIVATE_HOST_PATTERNS = [ | ||
| /^localhost$/i, | ||
| /^localhost$/, | ||
| /^localhost\.localdomain$/, | ||
| /^0\.0\.0\.0$/, | ||
| /^127\./, | ||
| /^10\./, | ||
| /^192\.168\./, | ||
| /^172\.(1[6-9]|2\d|3[01])\./, | ||
| /^169\.254\./, | ||
| /^::1$/, | ||
| /^fc[0-9a-f]{2}:/i, | ||
| /^fe80:/i, | ||
| /^::$/, | ||
| /^fc[0-9a-f]{2}:/, | ||
| /^fe80:/, | ||
| ]; |
There was a problem hiding this comment.
fd00::/8 remains unblocked.
fc00::/7 spans both fc... and fd..., but Line 70 only matches fc. That still allows http://[fd00::1]/-style ULA hosts through the SSRF guard, which defeats the private-IPv6 goal.
Suggested fix
- /^fc[0-9a-f]{2}:/,
+ /^f[cd][0-9a-f]{2}:/,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@liveness-browser.mjs` around lines 59 - 72, The IPv6 ULA range fc00::/7 is
only partially covered by the /^fc[0-9a-f]{2}:/ pattern in
PRIVATE_HOST_PATTERNS, leaving fd00::/8 addresses allowed; update the pattern
for PRIVATE_HOST_PATTERNS (the entry currently /^fc[0-9a-f]{2}:/) to match both
fc and fd prefixes (e.g., use a regex that matches f followed by c or d like
/^f[cd][0-9a-f]{2}:/ with case-insensitive matching) so fc00::/7 ULA addresses
are fully blocked by the SSRF guard.
There was a problem hiding this comment.
How much u have to chat about the day
There was a problem hiding this comment.
@williamjacksonw919-design, just enough to keep your code secure! 😄
To bring us back on topic — there's still an open security finding in this PR worth addressing: the fd00::/8 ULA range is not blocked by the current PRIVATE_HOST_PATTERNS. The fix is a one-liner:
- /^fc[0-9a-f]{2}:/,
+ /^f[cd][0-9a-f]{2}:/,This ensures the full fc00::/7 private IPv6 range is covered, not just the fc half of it.
|
Welcome to career-ops, @aaronjmars — this was a genuinely good catch. The bracketed-hostname detail meant every IPv6 guard in the file was dead, and your normalize-first fix closes all of it; I also threw percent-encoding, expanded mapped forms, hex/octal/decimal literals and zone-IDs at it and nothing got through. Tests are exactly the right shape (asserting blocked_host, not just running). Merged — your first contribution here. 🚀 One tiny note for the record: |
3 participants
Summary
rejectPrivateOrInvalidinliveness-browser.mjsis supposed to keepscan.mjs --verifyandcheck-liveness.mjsfrom pointing headlessChromium at internal infrastructure. SECURITY.md lists SSRF in the
*.mjsscripts as in-scope, and the existing block list (127.x,10.x,192.168.x,172.16-31.x,169.254.x,::1,fcXX:,fe80:)shows the intent. But the check runs against
new URL(url).hostname,which the WHATWG URL parser serializes in ways the original regex list
doesn't cover, so several Chromium-routable hosts slip through.
Impact
A misconfigured
portals.ymlentry, a compromised ATS feed, or a job URLcopy-pasted from a hostile source can drive headless Chromium at:
http://[::1]/) —parsed.hostnameis'[::1]', so/^::1$/never matches. The same hole affects every IPv6 guard:^fc[0-9a-f]{2}:,^fe80:, etc. The IPv6 block list is effectivelydead.
http://[::ffff:127.0.0.1]/and the hex formhttp://[::ffff:7f00:1]/) — routes to the embedded IPv4 in Chromiumbut the embedded address isn't decoded before the IPv4 list runs.
http://0.0.0.0/,http://[::]/) — reach loopback onLinux and aren't in the list at all.
http://localhost./) — anchored/^localhost$/doesn't match, but the resolver still treats it as
localhost.The full bypass set covers AWS / GCP metadata (
[::ffff:169.254.169.254]),arbitrary RFC1918 services, and any HTTP endpoint reachable on the host
running scan/liveness. Real impact depends on the running host:
(databases, dev servers, admin consoles).
IPv4-mapped IPv6 bypass even when the literal IPv4 form is blocked.
page.evaluate), andany GET-side-effect endpoint on the local host gets triggered.
Location
liveness-browser.mjs:42-76—PRIVATE_HOST_PATTERNS+rejectPrivateOrInvalid.Called by both entry points:
scan.mjs:298-381— the--verifypathcheck-liveness.mjs:62-71— the standalone liveness CLIFix
parsed.hostname(lowercase, strip IPv6 brackets, strip FQDNtrailing dot) before the regex pass.
::ffff:127.0.0.1) and hex(
::ffff:7f00:1) form and check the embedded IPv4 against the IPv4block list.
0.0.0.0,::, andlocalhost.localdomain.rejectPrivateOrInvalidso it can be unit-tested directly.The patterns themselves stay regex-driven and the existing code path
(stable
blocked_hostcode for routing inscan.mjs) is unchanged —nothing downstream needs to move.
Verification
test-all.mjsgains a block of cases exercising every bypass above plusa small allow-list to keep the guard from false-positiving on legitimate
ATS hostnames:
The existing
classifyLiveness/checkUrlLivenessWithFallback/jitteredDelayMs/isChallengeResulttests are untouched.Manually verified the bypass mechanics against Node 20+ WHATWG URL
behavior:
new URL('http://[::1]/').hostname === '[::1]'(brackets preserved)new URL('http://[::ffff:127.0.0.1]/').hostname === '[::ffff:127.0.0.1]'new URL('http://0.0.0.0/').hostname === '0.0.0.0'Run locally:
Detected by
Aeon + manual review of the SSRF surface documented in
SECURITY.md.explicitly relies on; reachable when feed data or
portals.ymlishostile)
Identifier)
abhigyanpatwari/GitNexus #1148
(IPv4-mapped IPv6) and Doorman11991/smallcode #39
(IPv4-mapped IPv6 + browser redirects). Pre-merge code uses regex on
parsed.hostnamewithout bracket/normalization.Filed by Aeon. Happy to
iterate on the patch if you'd rather take a different shape (e.g. a
post-DNS-resolution check via
dns.lookup, or a tighter allow-list keyedoff
tracked_companies). The block above is the smallest change thatcloses the documented bypass set without altering the existing call
sites.
Summary by CodeRabbit
Bug Fixes
Tests