fix(frontend): pin backend DNS to block SSRF - #1184
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds trusted backend fetching with DNS resolution validation, destination pinning, Undici dispatchers, and timeout handling. Proxy and session requests use the new pipeline, while tests cover public, loopback, Docker-internal, metadata, and pinned-address behavior. ChangesTrusted backend protection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProxyRoute
participant fetchTrustedBackend
participant DNSLookup
participant UndiciAgent
ProxyRoute->>fetchTrustedBackend: Submit trusted backend request
fetchTrustedBackend->>DNSLookup: Resolve hostname
DNSLookup-->>fetchTrustedBackend: Return validated addresses
fetchTrustedBackend->>UndiciAgent: Dispatch using pinned lookup
UndiciAgent-->>fetchTrustedBackend: Return backend response
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
PR governance metadata gate update for PR governance metadata gate is ready; all current-head requirements passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/lib/backend-request.ts (1)
89-100: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFragile string-prefix check for IPv4-mapped IPv6.
isIpv4MappedIpv6only matches two exact textual prefixes (::ffff:and0:0:0:0:0:ffff:). This works for the canonical forms Node's DNS resolver typically returns, but it's not a robust IP-address-aware check — any other valid textual representation of the same address (e.g. partial zero-compression, mixed-case, or an all-hex form without dotted-decimal) would silently bypass this guard rather than fail closed. Since this function exists specifically to close the IPv4-mapped-IPv6 SSRF bypass, prefernet.BlockList, which normalizes addresses before comparing, over string prefix matching — consistent with how the rest of the file already validates addresses.🔒 Proposed fix using BlockList instead of string prefixes
+const IPV4_MAPPED_IPV6_ADDRESSES = new BlockList(); +IPV4_MAPPED_IPV6_ADDRESSES.addSubnet("::ffff:0:0", 96, "ipv6"); + function isIpv4MappedIpv6(address: string): boolean { - const normalized = address.toLowerCase(); - return ( - normalized.startsWith("::ffff:") || - normalized.startsWith("0:0:0:0:0:ffff:") - ); + return IPV4_MAPPED_IPV6_ADDRESSES.check(address, "ipv6"); }Please confirm with the web whether
node:net'sdns.lookup/getaddrinfoon all supported platforms always normalizes IPv4-mapped IPv6 addresses to one of these two textual forms, to gauge real-world bypass risk of the current implementation.🤖 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 `@frontend/src/lib/backend-request.ts` around lines 89 - 100, Replace the string-prefix logic in isIpv4MappedIpv6 with a net.BlockList-based address check so IPv4-mapped IPv6 representations are normalized and detected consistently. Reuse the existing node:net validation approach in the file, preserving the boolean contract and fail-closed behavior for all equivalent textual forms.frontend/src/lib/backend-request.test.ts (1)
1-185: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftGood coverage of
resolveBackendAddresses/createPinnedBackendLookup, butfetchTrustedBackenditself is never exercised.The suite thoroughly unit-tests destination classification, DNS validation/dedup, and the pinned lookup's hostname-rejection logic in isolation. However,
fetchTrustedBackend— the function that actually wiresresolveBackendAddresses's output intocreatePinnedBackendLookup'sexpectedHostname/addressesand hands theAgenttofetch()— has no direct test here. The route-level tests (route.test.ts,route.test.tsinauth/session, etc.) all stubglobal.fetch, so they never exercise the realconnect.lookupinvocation either. This means a mismatch between the hostname format used for validation and the format undici's connector actually passes tolookup(see the bracket/IPv6-literal concern raised inbackend-request.ts) would not be caught by any test in this PR.🤖 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 `@frontend/src/lib/backend-request.test.ts` around lines 1 - 185, Add direct coverage for fetchTrustedBackend that mocks DNS resolution and fetch, then verifies the real Agent uses createPinnedBackendLookup with the validated hostname and addresses when handling a request. Exercise the connect.lookup path with the hostname format used by the HTTP client, including an IPv6-literal or bracketed hostname case, and assert that fetch receives the configured Agent while unexpected hostname formats are rejected.
🤖 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.
Nitpick comments:
In `@frontend/src/lib/backend-request.test.ts`:
- Around line 1-185: Add direct coverage for fetchTrustedBackend that mocks DNS
resolution and fetch, then verifies the real Agent uses
createPinnedBackendLookup with the validated hostname and addresses when
handling a request. Exercise the connect.lookup path with the hostname format
used by the HTTP client, including an IPv6-literal or bracketed hostname case,
and assert that fetch receives the configured Agent while unexpected hostname
formats are rejected.
In `@frontend/src/lib/backend-request.ts`:
- Around line 89-100: Replace the string-prefix logic in isIpv4MappedIpv6 with a
net.BlockList-based address check so IPv4-mapped IPv6 representations are
normalized and detected consistently. Reuse the existing node:net validation
approach in the file, preserving the boolean contract and fail-closed behavior
for all equivalent textual forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aaea24b-abf4-4e25-a8a2-f56f0a6b3107
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
frontend/package.jsonfrontend/src/app/api/[...path]/route.test.tsfrontend/src/app/api/[...path]/route.tsfrontend/src/app/auth/oidc/callback/route.test.tsfrontend/src/app/auth/session/route.test.tsfrontend/src/lib/backend-request.test.tsfrontend/src/lib/backend-request.tsfrontend/src/lib/backend-session-probe.ts
|
Addressed both current security review notes in 6e2f112:
@coderabbitai review |
|
✅ Action performedReview finished.
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head6e2f11297a8d6abe2d0167ea924677ce7b27124b. -
Head SHA:
6e2f11297a8d6abe2d0167ea924677ce7b27124b -
Workflow run: 30502031874
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Frontend (9 files)"]
S1 --> I1["browser runtime and bundle"]
I1 --> R1["Review risk: Frontend (9 files)"]
R1 --> V1["frontend tests"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Frontend (8 files)"]
S1 --> I1["browser runtime and bundle"]
I1 --> R1["Review risk: Frontend (8 files)"]
R1 --> V1["frontend tests"]
|
Central coverage infrastructure failure fixed and merged in ContextualWisdomLab/.github#661 (main 68d3e75bb0ae482f66e4432052b20c95843ae3f3). This automated REQUEST_CHANGES cited only failed run 30502031874, so dismiss it to require a fresh same-head review of 6e2f112 under the repaired workflow.
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head6e2f11297a8d6abe2d0167ea924677ce7b27124b. -
Head SHA:
6e2f11297a8d6abe2d0167ea924677ce7b27124b -
Workflow run: 30507507768
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Frontend (9 files)"]
S1 --> I1["browser runtime and bundle"]
I1 --> R1["Review risk: Frontend (9 files)"]
R1 --> V1["frontend tests"]
Stale prior-head review: current HEAD 0ada486 removes the pnpm lock delta that caused coverage failure; same-head coverage-evidence, dependency review, OSV, Trivy, Semgrep, Bandit, and CodeQL analysis now pass. A fresh current-head OpenCode dispatch is required.
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head0ada486fa29d35482607b942f97ac902509853fc. -
Head SHA:
0ada486fa29d35482607b942f97ac902509853fc -
Workflow run: 30509059490
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Frontend (8 files)"]
S1 --> I1["browser runtime and bundle"]
I1 --> R1["Review risk: Frontend (8 files)"]
R1 --> V1["frontend tests"]
Dismissed after fixing the central coverage sandbox Git trust bug in ContextualWisdomLab/.github#665 (merged as ddaf14e91d348fdde49ce6dd7d4367b2c0fa2263). This review contained no Naruon code finding; its only blocker was the now-repaired coverage-evidence infrastructure failure. The exact current head will be re-reviewed.
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head0ada486fa29d35482607b942f97ac902509853fc. -
Head SHA:
0ada486fa29d35482607b942f97ac902509853fc -
Workflow run: 30512267763
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Frontend (8 files)"]
S1 --> I1["browser runtime and bundle"]
I1 --> R1["Review risk: Frontend (8 files)"]
R1 --> V1["frontend tests"]
|
@coderabbitai review Please review current head |
|
✅ Action performedReview finished.
|
|
Closing after Loop drain: permanently blocked — branch was updated onto develop for mergeability, which cleared prior APPROVED robot evidence; re-review (CodeRabbit/OpenCode) and/or central gate jobs (metadata-only gate evaluation, coverage-evidence) remained pending/stuck without a re-runnable workflow handle. Not force-merging (merge-gate policy). Re-open a focused PR when robot capacity is available. Related product security fixes that reimplemented cleanly remain on branch goal/carddav-path-traversal-decode (#1206) for relaunch. |
Pull request was closed
2 participants
Security boundary
CodeQL Critical alert #318 traces operator-configured
BACKEND_INTERNAL_URLinto the Node API proxy request sink. URL syntax and literal-IP checks did not prevent a public hostname from resolving or rebinding to metadata/private addresses between validation and connection.Fix
http/httpsrequests whose lookup returns only prevalidated addresses;agent: falseprevents reuse of sockets opened outside the policyundicidependency sofrontend/package.jsonandfrontend/pnpm-lock.yamlexactly matchorigin/developVerification
Current HEAD:
0ada486fa29d35482607b942f97ac902509853fcpnpm typecheck— passedpnpm lint— passedpnpm build— passedpnpm audit --audit-level=moderate— no known vulnerabilitiesgit diff origin/develop --exit-code -- frontend/package.json frontend/pnpm-lock.yaml— passedgit diff --check— passedfetchremainsThe regression suite proves that mixed public/private DNS answers and metadata, link-local, mapped-IPv6, unexpected-host, and automatic-redirect cases fail before a socket is opened. It also covers legitimate public IPv4/IPv6, development loopback, opted-in Compose private answers, Host/SNI preservation, request-body forwarding, and streamed responses.