fix(proxy): block unsafe redirects and local targets#840
Open
harlan-zw wants to merge 5 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change hardens first-party proxying by validating public HTTPS hosts and redirect chains, filtering proxy credentials, rejecting unsafe response types, and adding security headers. Instagram embeds now use dedicated URL checks and HTML/CSS sanitization. Client-side routing and configured domains accept only HTTP(S), alias handling rejects dot segments, and generated secrets replace empty persisted entries. Privacy transformations support arrays, safer object containers, and expanded IPv6 anonymization, with regression coverage across these behaviors. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 8
🧹 Nitpick comments (1)
test/unit/proxy-alias-generators.test.ts (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest generated interceptor behavior, not only source text.
This assertion can pass even if the generated
proxyUrl()behavior is ineffective. Execute the generated path withdata:/javascript:and same-origin URLs to verify those requests retain native handling.🤖 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 `@test/unit/proxy-alias-generators.test.ts` at line 43, Update the test around the generated proxy alias code assertion to execute the generated proxyUrl() behavior rather than only checking its source text. Cover data:, javascript:, and same-origin URLs, and assert each retains native handling while preserving the existing generated-code coverage.
🤖 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 `@packages/script/src/module.ts`:
- Around line 173-178: Update the PROXY_SECRET_ENV_VALUE_RE match handling in
the dotenv secret-loading flow to trim the captured value before checking
whether it is non-empty. Return the normalized secret only when the trimmed
value has content; otherwise continue to the writeFileSync replacement branch so
whitespace-only declarations receive and persist the generated secret.
In `@packages/script/src/plugins/intercept.ts`:
- Around line 30-32: Build the serialized alias maps with null prototypes before
bracket lookups, preventing special __proto__ handling and inherited keys such
as constructor or toString from affecting resolution. Apply this to the alias
lookup in packages/script/src/plugins/intercept.ts at lines 30-32 and the
corresponding alias-map construction in packages/script/src/registry.ts at lines
885-888.
In `@packages/script/src/runtime/server/proxy-handler.ts`:
- Around line 466-475: Update the response handling around setResponseStatus and
sendStream to clear timeoutId immediately after headers are set, before
streaming begins. Remove the later timeout cleanup from the sendStream.finally
path while preserving the existing no-body cleanup and stream return behavior.
In `@packages/script/src/runtime/server/utils/cached-upstream.ts`:
- Around line 136-137: Update the error message in the redirect check within the
cached upstream request flow to reference the existing allowUrl configuration
property instead of the nonexistent allowRedirect field, while preserving the
current condition and thrown-error behavior.
- Around line 119-155: Update the $fetch.raw calls in createCachedBinaryFetch
and createCachedJsonFetch so manual redirect probes always set
ignoreResponseError to true, allowing resolveRedirect to inspect real 3xx
responses and Location headers. After the redirect loop, explicitly handle
non-redirect 4xx/5xx responses using the existing response-error behavior rather
than allowing the probe to throw prematurely.
In `@packages/script/src/runtime/server/utils/privacy.ts`:
- Around line 443-446: Update the payload privacy bucketing logic for the sw/sh
entries so array values remain intact and each sh value uses the screen-width
class at the same index, rather than always using sw[0]. Preserve scalar
handling and add a regression case covering sw [375, 1280] with sh [667, 720],
expecting paired buckets [640, 1080].
- Around line 192-198: Update the IP handling around anonymizeIP to strip the
zone identifier once before both IPv4-mapped matching and IPv6 expansion. Use
the normalized address for the mapped-IPv4 regex and the expandIPv6 path, while
preserving the existing anonymization behavior and fallback.
- Line 533: Update the hardware branch in the result-mapping logic so non-string
dv values are replaced with an empty value rather than passed through unchanged;
preserve mapString(value, anonymizeDeviceInfo) for string values and the
existing non-hardware behavior.
---
Nitpick comments:
In `@test/unit/proxy-alias-generators.test.ts`:
- Line 43: Update the test around the generated proxy alias code assertion to
execute the generated proxyUrl() behavior rather than only checking its source
text. Cover data:, javascript:, and same-origin URLs, and assert each retains
native handling while preserving the existing generated-code coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 191d8c65-1db3-4bb9-a371-2713c97a6075
📒 Files selected for processing (30)
docs/content/docs/1.guides/2.first-party.mdpackages/script/src/module.tspackages/script/src/plugins/intercept.tspackages/script/src/proxy-alias.tspackages/script/src/registry.tspackages/script/src/runtime/server/bluesky-embed.tspackages/script/src/runtime/server/google-maps-geocode-proxy.tspackages/script/src/runtime/server/google-static-maps-proxy.tspackages/script/src/runtime/server/gravatar-proxy.tspackages/script/src/runtime/server/instagram-embed-asset.tspackages/script/src/runtime/server/instagram-embed.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/server/utils/image-proxy.tspackages/script/src/runtime/server/utils/instagram-embed.tspackages/script/src/runtime/server/utils/privacy.tspackages/script/src/runtime/server/utils/proxy-query.tspackages/script/src/runtime/server/x-embed.tstest/unit/cached-upstream.test.tstest/unit/google-geocode-proxy.test.tstest/unit/google-static-map-proxy.test.tstest/unit/gravatar-proxy-handler.test.tstest/unit/image-proxy-security.test.tstest/unit/instagram-embed.test.tstest/unit/proxy-alias-generators.test.tstest/unit/proxy-alias.test.tstest/unit/proxy-handler-body.test.tstest/unit/proxy-privacy.test.tstest/unit/resolve-proxy-secret.test.tstest/unit/setup.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/script/src/runtime/server/utils/network-host.ts`:
- Around line 65-69: Update the hostname validation flow around the visible
suffix check and its callers in proxy-handler.ts to resolve permitted hostnames
and reject any A/AAAA result that is not globally routable, including loopback
and private ranges. Reuse the validated resolution for the subsequent fetch
connection so DNS is not resolved again without validation, preserving existing
literal-IP and local-suffix rejection 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bee5ca34-de2e-458f-abf1-b6f3525bb048
📒 Files selected for processing (10)
docs/content/docs/1.guides/2.first-party.mdpackages/script/src/module.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/server/utils/image-proxy.tspackages/script/src/runtime/server/utils/network-host.tstest/unit/image-proxy-security.test.tstest/unit/network-host.test.tstest/unit/proxy-handler-body.test.tstest/unit/setup.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/unit/setup.test.ts
- packages/script/src/module.ts
- packages/script/src/runtime/server/utils/image-proxy.ts
- packages/script/src/runtime/server/utils/cached-upstream.ts
- test/unit/proxy-handler-body.test.ts
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
harlan-zw
changed the title
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 (2)
packages/script/src/module.ts (1)
174-183: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize
.envsecret creation across processes.
contentsis read once before this branch. Two dev processes can both observe no usable secret, then append/replace independently and each retain a different generated value. Signed URLs can consequently fail when served by the other process. Use an exclusive lock, then re-read and reuse or persist the secret inside that critical section.🤖 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 `@packages/script/src/module.ts` around lines 174 - 183, Serialize generated secret creation around the `.env` update by acquiring an exclusive lock before checking or modifying the file. Within that critical section, re-read the current contents, reuse any usable value matched by PROXY_SECRET_ENV_VALUE_RE, or replace/append the generated secret via the existing write paths; release the lock afterward so concurrent processes cannot retain different secrets.packages/script/src/runtime/server/proxy-handler.ts (1)
429-490: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNo timeout/idle-guard covers the body-streaming phase.
clearTimeout(timeoutId)fires as soon as headers arrive (line 429), and streaming now happens viasendStream(event, response.body)with no further deadline. Previously, this section's aborted-timeout risk was the concern (now fixed); the flip side is that an upstream that responds with headers instantly but drips the body arbitrarily slowly can hold the proxy connection (and itsnetworkdispatcher/socket) open indefinitely — a slow-body DoS vector on a public-facing proxy endpoint. Buffering previously bounded this implicitly; streaming removes that bound.Consider wrapping
response.bodyin a stream that resets/enforces an idle timer per chunk (or reintroducingcontroller.signal-driven cancellation keyed off inactivity) before handing it tosendStream.🤖 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 `@packages/script/src/runtime/server/proxy-handler.ts` around lines 429 - 490, Extend the timeout protection in the proxy handler’s response-body path: do not leave the upstream stream unbounded after headers arrive. Wrap response.body or otherwise integrate its cancellation with an idle timer that resets whenever a chunk is received, aborts the upstream, closes network, and causes the request to fail when no body data arrives within the configured limit, then pass the guarded stream to sendStream.
🤖 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 `@test/unit/proxy-alias-generators.test.ts`:
- Line 34: Update both generated-code evaluation harnesses in
test/unit/proxy-alias-generators.test.ts at lines 34-34 and 79-79: replace the
eval calls used by generatePartytownResolveUrl and the generated plugin test
with an accepted isolated test helper, or apply the repository-approved Biome
noGlobalEval suppression with a test-only justification.
---
Outside diff comments:
In `@packages/script/src/module.ts`:
- Around line 174-183: Serialize generated secret creation around the `.env`
update by acquiring an exclusive lock before checking or modifying the file.
Within that critical section, re-read the current contents, reuse any usable
value matched by PROXY_SECRET_ENV_VALUE_RE, or replace/append the generated
secret via the existing write paths; release the lock afterward so concurrent
processes cannot retain different secrets.
In `@packages/script/src/runtime/server/proxy-handler.ts`:
- Around line 429-490: Extend the timeout protection in the proxy handler’s
response-body path: do not leave the upstream stream unbounded after headers
arrive. Wrap response.body or otherwise integrate its cancellation with an idle
timer that resets whenever a chunk is received, aborts the upstream, closes
network, and causes the request to fail when no body data arrives within the
configured limit, then pass the guarded stream to sendStream.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d38944f7-e864-4220-8f12-2dd8a79a5da4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
packages/script/package.jsonpackages/script/src/module.tspackages/script/src/plugins/intercept.tspackages/script/src/registry.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/server/utils/network-host.tspackages/script/src/runtime/server/utils/privacy.tspnpm-workspace.yamltest/unit/cached-upstream.test.tstest/unit/google-geocode-proxy.test.tstest/unit/google-static-map-proxy.test.tstest/unit/gravatar-proxy-handler.test.tstest/unit/image-proxy-security.test.tstest/unit/network-host.test.tstest/unit/proxy-alias-generators.test.tstest/unit/proxy-handler-alias.test.tstest/unit/proxy-handler-body.test.tstest/unit/proxy-handler-hop-by-hop.test.tstest/unit/proxy-privacy.test.tstest/unit/resolve-proxy-secret.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/script/src/registry.ts
- packages/script/src/plugins/intercept.ts
- test/unit/gravatar-proxy-handler.test.ts
- test/unit/google-static-map-proxy.test.ts
- test/unit/image-proxy-security.test.ts
- test/unit/cached-upstream.test.ts
- test/unit/proxy-privacy.test.ts
- packages/script/src/runtime/server/utils/privacy.ts
- packages/script/src/runtime/server/utils/cached-upstream.ts
- test/unit/proxy-handler-body.test.ts
Collaborator
Author
|
Addressed the completed review in 1783639:
Verified with lint, typecheck, build, and the full suite: 1,020 tests passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/script/src/module.ts`:
- Around line 131-164: Update withProxySecretFileLock to recover stale lock
files by checking the existing lock’s mtime against PROXY_SECRET_LOCK_TIMEOUT_MS
and removing it before retrying when expired. Make cleanup errors in the finally
block non-masking: preserve the value or error from effect() and only ignore
expected missing-lock cleanup, rather than allowing unlink failures to replace a
successful result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 495ee5c5-e89a-4012-9b2b-d237267a354e
📒 Files selected for processing (5)
packages/script/src/module.tspackages/script/src/runtime/server/proxy-handler.tstest/unit/proxy-alias-generators.test.tstest/unit/proxy-handler-body.test.tstest/unit/resolve-proxy-secret.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/proxy-handler-body.test.ts
- test/unit/proxy-alias-generators.test.ts
- packages/script/src/runtime/server/proxy-handler.ts
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
1 participant
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.
🔗 Linked issue
None.
❓ Type of change
📚 Description
Proxy routes could follow redirects outside the configured host or resolve an allowed hostname to a private address. Each redirect is now checked, and Node DNS results are pinned into the socket connection after public-address validation. The generic relay streams response bodies; image responses reject active content, Instagram HTML is sanitized, and Google proxy credentials are removed before forwarding.