Conversation
|
Warning Review limit reached
Next review available in: 4 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRemoves jsdom and replaces HTML-parsing Cloudflare detection with a ChangesDNS Pinning, SSRF Hardening, and jsdom Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
|
Preview deployed to: https://ThunLights.github.io/distopia/storybook/pr-preview-106 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.changeset/light-lies-shave.md (1)
1-6: 📐 Maintainability & Code Quality | 🔵 TrivialThe changeset description under-represents the PR scope.
The current description—"remove unused dependencies and improve URL parsing in invite handling"—omits the substantial security hardening in this PR: DNS pinning to prevent DNS rebinding/SSRF, the new
resolveHostnameToSafeIp/validateSafeUrl/resolveToPinnedUrlAPIs, header-based Cloudflare detection replacing jsdom HTML parsing, and CGNAT range coverage inisLocalIPv4. Consumers reading this changeset will not understand the security implications or new capabilities.Expand the description to accurately reflect the DNS pinning, SSRF hardening, and jsdom removal. For example:
-fix: remove unused dependencies and improve URL parsing in invite handling +fix: remove jsdom, add DNS pinning and SSRF hardening for HTTP fetches + +- Replaces jsdom-based Cloudflare detection with `cf-mitigated` header check +- Adds `resolveHostnameToSafeIp`, `validateSafeUrl`, and `resolveToPinnedUrl` for DNS-rebinding protection +- Extends `isLocalIPv4` to cover CGNAT (`100.64.0.0/10`) +- Pins fetch URLs to resolved IPs while preserving `Host` header and TLS SNIThe
minorbump level is appropriate given the new exports and behavioral changes.🤖 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 @.changeset/light-lies-shave.md around lines 1 - 6, The changeset summary understates the scope of the PR and should be expanded to mention the security-hardening work in addition to the dependency cleanup and invite URL parsing tweaks. Update the description in the changeset to reflect the new exports and behavioral changes around DNS pinning/SSRF protection, including resolveHostnameToSafeIp, validateSafeUrl, resolveToPinnedUrl, Cloudflare header-based detection, and the broader CGNAT coverage in isLocalIPv4, so consumers can understand the security impact.src/infrastructure/http/src/safefetch.test.ts (1)
301-305: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual pinned request, too.
This test verifies the proxied
response.url, but it would still pass iffetchwere called with the original hostname instead of the pinned IP.Suggested test assertions
it("exposes the original hostname URL via response.url (not the pinned IP)", async () => { fetchMock.mockResolvedValueOnce(ok()); const result = await safeFetch(url("https://example.com/path")); expect((result as Response).url).toBe("https://example.com/path"); + expect(fetchMock).toHaveBeenCalledWith( + "https://1.2.3.4/path", + expect.objectContaining({ redirect: "manual" }), + ); + + const init = fetchMock.mock.calls[0]?.[1] as RequestInit & { + tls?: { serverName?: string }; + }; + expect(new Headers(init.headers).get("Host")).toBe("example.com"); + expect(init.tls?.serverName).toBe("example.com"); });🤖 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 `@src/infrastructure/http/src/safefetch.test.ts` around lines 301 - 305, The safefetch test only checks Response.url and could still pass if safeFetch calls fetch with the original hostname, so update the test to also assert the actual request passed to fetchMock. In the safeFetch test case around response.url, verify the mocked fetch was invoked with the pinned IP (while preserving the original Host header or equivalent hostname mapping expected by safeFetch) using the existing safeFetch, fetchMock, and ok helpers so the test proves both the outbound request target and the proxied response URL behavior.
🤖 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 `@src/infrastructure/http/src/invite.test.ts`:
- Around line 49-69: The invite tests are still using the local helper instead
of the production implementation, so they won’t catch regressions in invite.ts.
Update the test block in invite.test.ts to import and call the exported isUsedCf
from the invite module, and remove or rename the test-local helper so these
assertions exercise the real implementation rather than duplicated logic.
In `@src/infrastructure/http/src/safefetch.ts`:
- Line 86: Disable automatic redirects in safeFetchForDiscord by updating the
fetch call in safeFetchForDiscord to use redirect handling consistent with
safeFetch. The current return await fetch(pinned.url, ...) lets native fetch
follow redirects automatically, which bypasses the DNS pinning and header
stripping protections. Set fetch to not follow redirects and rely on the
existing safeFetch-style manual redirect handling so cross-origin redirects
remove authorization/cookie headers before the next request.
- Around line 58-60: The URL pinning logic in safefetch is not handling IPv6
literals correctly, so the hostname assignment can fail to replace the original
host. Update the safe fetch flow around resolveHostnameToSafeIp() and the
pinnedUrlObj.hostname assignment to detect raw IPv6 results and wrap them in
bracket notation before setting hostname. Keep the change localized to the URL
construction in safefetch so IPv4 behavior remains unchanged and IPv6-only hosts
are properly pinned.
In `@src/infrastructure/http/src/safeurl.ts`:
- Around line 21-25: The validateSafeUrl function is still doing manual parsing
and branding inline, so move the URL validation logic into a Zod 4 schema and
have validateSafeUrl use that schema for parsing. Keep the protocol check inside
the schema definition, brand the validated result there as SafeUrl, and make
validateSafeUrl return the schema’s parsed output or null so it follows the
existing TypeScript validation pattern.
In `@src/infrastructure/http/src/url.test.ts`:
- Line 15: The url.test.ts CGNAT lower-bound case is using the wrong address for
the labeled boundary. Update the test case in the URL IP boundary list so the
entry for the CGNAT lower bound uses the exact lower-edge address of the
100.64.0.0/10 range, keeping the existing test structure and label aligned with
that boundary value.
---
Nitpick comments:
In @.changeset/light-lies-shave.md:
- Around line 1-6: The changeset summary understates the scope of the PR and
should be expanded to mention the security-hardening work in addition to the
dependency cleanup and invite URL parsing tweaks. Update the description in the
changeset to reflect the new exports and behavioral changes around DNS
pinning/SSRF protection, including resolveHostnameToSafeIp, validateSafeUrl,
resolveToPinnedUrl, Cloudflare header-based detection, and the broader CGNAT
coverage in isLocalIPv4, so consumers can understand the security impact.
In `@src/infrastructure/http/src/safefetch.test.ts`:
- Around line 301-305: The safefetch test only checks Response.url and could
still pass if safeFetch calls fetch with the original hostname, so update the
test to also assert the actual request passed to fetchMock. In the safeFetch
test case around response.url, verify the mocked fetch was invoked with the
pinned IP (while preserving the original Host header or equivalent hostname
mapping expected by safeFetch) using the existing safeFetch, fetchMock, and ok
helpers so the test proves both the outbound request target and the proxied
response URL behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7638219-18bb-4b70-8d06-ab95d6feee55
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.changeset/light-lies-shave.mdlib/distopia/jsr.jsonlib/distopia/package.jsonpackage.jsonsrc/infrastructure/http/package.jsonsrc/infrastructure/http/src/dns.tssrc/infrastructure/http/src/invite.test.tssrc/infrastructure/http/src/invite.tssrc/infrastructure/http/src/safefetch.test.tssrc/infrastructure/http/src/safefetch.tssrc/infrastructure/http/src/safeurl.tssrc/infrastructure/http/src/url.test.tssrc/infrastructure/http/src/url.ts
💤 Files with no reviewable changes (4)
- lib/distopia/jsr.json
- lib/distopia/package.json
- package.json
- src/infrastructure/http/package.json
| it("returns true when cf-mitigated: challenge header is present", () => { | ||
| const res = fakeResponse("https://example.com", 403, "", { "cf-mitigated": "challenge" }); | ||
| expect(isUsedCf(res)).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false for a 200 with a Cloudflare-like title", async () => { | ||
| const res = fakeResponse( | ||
| "https://example.com", | ||
| 200, | ||
| "<html><title>Just a moment...</title></html>", | ||
| ); | ||
| expect(await isUsedCf(res)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns true for a 403 with the exact Cloudflare title", async () => { | ||
| const html = | ||
| "<html><head><title>Just a moment...</title></head><body>CF challenge</body></html>"; | ||
| const res = fakeResponse("https://example.com", 403, html); | ||
| expect(await isUsedCf(res)).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false for a 403 with a different title", async () => { | ||
| const html = "<html><head><title>Access Denied</title></head><body>Forbidden</body></html>"; | ||
| const res = fakeResponse("https://example.com", 403, html); | ||
| expect(await isUsedCf(res)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false for a 403 with no <title> element", async () => { | ||
| const html = "<html><body>Forbidden</body></html>"; | ||
| const res = fakeResponse("https://example.com", 403, html); | ||
| expect(await isUsedCf(res)).toBe(false); | ||
| it("returns true for any status code when cf-mitigated: challenge is set", () => { | ||
| // CF challenges can be served with 200, 403, 429, or 503 | ||
| for (const status of [200, 403, 429, 503]) { | ||
| const res = fakeResponse("https://example.com", status, "", { "cf-mitigated": "challenge" }); | ||
| expect(isUsedCf(res)).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false for a 403 with a title that partially matches", async () => { | ||
| const html = "<html><head><title>Just a moment</title></head></html>"; | ||
| const res = fakeResponse("https://example.com", 403, html); | ||
| expect(await isUsedCf(res)).toBe(false); | ||
| it("returns false when cf-mitigated header is absent", () => { | ||
| const res = fakeResponse("https://example.com", 403, ""); | ||
| expect(isUsedCf(res)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false for a 403 with an empty title", async () => { | ||
| const html = "<html><head><title></title></head></html>"; | ||
| const res = fakeResponse("https://example.com", 403, html); | ||
| expect(await isUsedCf(res)).toBe(false); | ||
| it("returns false when cf-mitigated has a value other than 'challenge'", () => { | ||
| const res = fakeResponse("https://example.com", 403, "", { "cf-mitigated": "other" }); | ||
| expect(isUsedCf(res)).toBe(false); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import the production isUsedCf into this test block.
These assertions are currently calling the test-local helper defined at Lines 32-34, so they still pass if src/infrastructure/http/src/invite.ts regresses. Please exercise the exported implementation here instead of mirroring its logic.
🤖 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 `@src/infrastructure/http/src/invite.test.ts` around lines 49 - 69, The invite
tests are still using the local helper instead of the production implementation,
so they won’t catch regressions in invite.ts. Update the test block in
invite.test.ts to import and call the exported isUsedCf from the invite module,
and remove or rename the test-local helper so these assertions exercise the real
implementation rather than duplicated logic.
|
Preview deployed to: https://ThunLights.github.io/distopia/storybook/pr-preview-106 |
|
Preview deployed to: https://ThunLights.github.io/distopia/storybook/pr-preview-106 |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes