Skip to content

fix(security): bound response bodies in secureFetchWithPinnedIP by default - #6169

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/unbounded-response-buffering-secure-fetch
Aug 1, 2026
Merged

fix(security): bound response bodies in secureFetchWithPinnedIP by default#6169
waleedlatif1 merged 1 commit into
stagingfrom
fix/unbounded-response-buffering-secure-fetch

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • secureFetchWithPinnedIP only bounded a response body when the caller passed maxResponseBytes — with the option absent the body streamed into an unbounded Buffer.concat. Tool proxies whose target host is user-supplied (Jupyter, ClickHouse, Grafana, 1Password Connect) and the RSS poller called it without a cap, so an attacker-controlled server answering with an endless chunked body grows the shared process heap until it is OOM-killed. One ~200-byte tool invocation is enough; an RSS trigger repeats it unattended on a schedule.
  • Made the cap fail-safe: maxResponseBytes now defaults to 100MB, and a non-positive value falls back to the default — there is no unlimited mode left.
  • Passed tighter explicit caps at the user-supplied-host sites: 10MB for the JSON proxies (Jupyter proxy/upload, ClickHouse, Grafana update_dashboard/update_alert_rule/update_folder, 1Password Connect), MAX_FILE_SIZE for 1Password file content, 10MB for the RSS poller (which also bounds the XML handed to rss-parser, which has no input-size limit of its own).
  • Exempted bodyless responses (HEAD, 204, 304) from the content-length pre-check — they advertise the resource size as metadata, so activating the check by default would otherwise have spuriously failed the Buffer media HEAD probe of any file >100MB and the RSS conditional-GET 304.

Type of Change

  • Bug fix

Testing

Audited all 47 secureFetchWithPinnedIP call sites for false positives before defaulting the cap:

  • 20 already pass an explicit cap — unchanged.
  • Every /api/tools/** route's response is already read back by the executor through readToolResponseBody, capped at MAX_TOOL_RESPONSE_BODY_BYTES (10MB) on the internal-fetch branch too — so anything above 10MB already fails today and the new caps cannot block a path that currently works.
  • The streaming-to-storage paths (Drive/OneDrive/SharePoint/Slack/STT/uploads) already cap at MAX_FILE_SIZE = 100MB = the new default — identical behavior.
  • The HEAD/304 cases above were the only two real false positives found; both are fixed and covered by tests.

New secure-fetch-response-cap.server.test.ts drives a real loopback server (explicit cap on a chunked body, default cap via content-length, an under-cap read, HEAD, 304). Verified the tests can fail: reverted input-validation.server.ts and 3 of the 5 went red.

Full apps/sim suite: 17,294 passing; the only 2 failures are pre-existing and environmental (a test shelling out to a missing rg binary, and a 30s-timeout flake under parallel load) and reproduce independently of this change. tsc clean on apps/sim and packages/testing, biome clean, check:api-validation passed.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…fault

secureFetchWithPinnedIP only capped a response body when the caller passed
maxResponseBytes; with the option absent the body streamed into an unbounded
Buffer.concat. Several tool proxies whose target host is user-supplied
(Jupyter, ClickHouse, Grafana, 1Password Connect) and the RSS poller called it
without that option, so an attacker-controlled server answering with an endless
chunked body could grow the shared process heap until it was OOM-killed.

Make the cap fail-safe: default to 100MB (and treat a non-positive value as the
default) so there is no unlimited mode, then pass tighter explicit caps at the
user-supplied-host sites.

Responses that carry no body (HEAD, 204, 304) are exempted from the
content-length pre-check — they advertise the resource size as metadata, which
would otherwise spuriously fail a HEAD probe of a large file or an RSS
conditional-GET 304.
@vercel

vercel Bot commented Aug 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 1, 2026 7:04pm

Request Review

@cursor

cursor Bot commented Aug 1, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Security-critical change to outbound fetch behavior across many tool routes and the RSS poller; mis-tuned caps could break legitimate large responses, while the fix closes a DoS/OOM vector on user-controlled URLs.

Overview
secureFetchWithPinnedIP now always caps the response body — omitting maxResponseBytes used to buffer without limit, which let attacker-controlled hosts (endless chunked responses) OOM the shared process. The default is 100MB (DEFAULT_MAX_RESPONSE_BYTES); there is no unlimited mode.

User-supplied-host callers pass tighter limits: 10MB (MAX_JSON_API_RESPONSE_BYTES) for JSON/control-plane proxies (ClickHouse, Grafana updates, Jupyter proxy/upload, 1Password Connect JSON), MAX_FILE_SIZE for 1Password file downloads, and a 10MB RSS feed cap (also bounds XML fed to rss-parser). HEAD, 204, and 304 skip the Content-Length pre-check so large-file metadata probes and conditional GETs do not fail spuriously.

New loopback tests cover explicit cap, default cap, bodyless responses, and streaming rejection; shared test mocks export the new constants.

Reviewed by Cursor Bugbot for commit cfe18d4. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cfe18d4. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cfe18d4. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes response-size limiting fail-safe in secureFetchWithPinnedIP and applies tighter limits to user-configurable tool and RSS endpoints.

  • Defaults uncapped or non-positive response limits to 100MB.
  • Applies 10MB limits to JSON/control-plane requests and RSS feeds, while allowing up to the file-size limit for 1Password file content.
  • Exempts HEAD, 204, and 304 responses from content-length rejection.
  • Adds loopback-server coverage for explicit/default caps and bodyless responses.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable regressions identified in the changed response-cap behavior.

The default and endpoint-specific limits consistently bound buffered bodies, bodyless HTTP responses avoid metadata false positives, and the changed paths retain appropriate stream error and socket cleanup behavior.

Important Files Changed

Filename Overview
apps/sim/lib/core/security/input-validation.server.ts Introduces the default 100MB response ceiling, a shared 10MB JSON ceiling, and bodyless-response handling while preserving stream-level enforcement and cleanup.
apps/sim/lib/core/security/secure-fetch-response-cap.server.test.ts Covers chunked overflow, default content-length rejection, successful bounded reads, HEAD metadata, and 304 metadata using a local HTTP server.
apps/sim/app/api/tools/onepassword/utils.ts Defaults 1Password Connect JSON responses to 10MB while allowing file callers to request a larger explicit bound.
apps/sim/lib/webhooks/polling/rss.ts Caps attacker-controlled RSS response bodies at 10MB before XML parsing.
apps/sim/app/api/tools/jupyter/proxy/route.ts Adds a 10MB upstream response limit to the user-configurable Jupyter proxy.
apps/sim/app/api/tools/clickhouse/utils.ts Adds a 10MB limit to ClickHouse response buffering.

Reviews (1): Last reviewed commit: "fix(security): bound response bodies in ..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit d89ab4a into staging Aug 1, 2026
27 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/unbounded-response-buffering-secure-fetch branch August 1, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant