Skip to content

fix(ssh/sftp): cap remote file reads on received bytes, not stat() size - #6168

Merged
waleedlatif1 merged 1 commit into
stagingfrom
worktree-fix-sftp-ssh-download-read-cap
Aug 1, 2026
Merged

fix(ssh/sftp): cap remote file reads on received bytes, not stat() size#6168
waleedlatif1 merged 1 commit into
stagingfrom
worktree-fix-sftp-ssh-download-read-cap

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The SFTP/SSH download routes buffered a remote file into memory with the only size guard being sftp.stat().size — a value the caller-supplied SSH server controls. A server that reports a tiny size and then streams endlessly drove unbounded heap growth until OOM.
  • Read through the existing readNodeStreamToBufferWithLimit limiter (the same one the S3/GCS/Blob providers use), which enforces the cap on bytes actually received and destroys the stream on breach, via a small readSftpFileCapped wrapper in sftp/utils.
  • All four SFTP-reading tool routes now share it — sftp/download, ssh/download-file, ssh/read-file-content, and the append path of ssh/write-file-content, which had no cap at all.
  • The wrapper keeps a no-op error listener on the stream for its whole life: ssh2 rejects still-pending SFTP requests when the channel closes, which arrives as a late error event after the limiter has detached its own handlers, and an error event with no listener would take the process down.
  • Too-large responses now return 413, matching how onedrive/download, latex, and v1/files surface PayloadSizeLimitError, and responses report the actual byte count rather than the server-reported stat size.

Type of Change

  • Bug fix

Testing

  • Unit tests in app/api/tools/sftp/utils.test.ts, including a regression test for the late-error-after-abort crash. Verified each goes red when the corresponding guard is removed.
  • Proven end to end against a real ssh2 SFTP server that reports size: 1 for /bomb and then streams forever: the read aborts at ~50MB in ~120ms instead of never terminating, and the process exits cleanly. A legitimate file reads correctly, and a file exactly at the cap still succeeds.
  • 158 tests across app/api/tools, tools/ssh, tools/sftp, lib/core/utils/stream-limits pass; tsc --noEmit clean; check:api-validation:strict, check:boundaries, check:client-boundary, check:utils, check:tool-registry-boundary all pass.

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)

@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:10pm

Request Review

@cursor

cursor Bot commented Aug 1, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Fixes a memory-exhaustion vector on authenticated remote-file tool routes and changes error status codes (400→413) for size limits; scope is security-critical server-side buffering.

Overview
SFTP/SSH tool routes previously trusted sftp.stat().size and buffered the full read stream into memory, so a hostile server could lie about size and stream until OOM. Reads now go through readSftpFileCapped, which enforces limits on bytes actually received via the shared readNodeStreamToBufferWithLimit helper (same pattern as other storage providers).

MAX_SFTP_READ_BYTES (50MB) is centralized in sftp/utils, and sftp/download, ssh/download-file, ssh/read-file-content, and the append path in ssh/write-file-content all use the capped reader. Oversize cases return 413 with PayloadSizeLimitError handling, and response size fields reflect buffered length rather than stat-reported size. The wrapper keeps a no-op error listener on SFTP streams to avoid process crashes from late ssh2 errors after abort.

Unit tests in utils.test.ts cover under-cap success, cap breach with stream destroy, stat-understatement attack, and late-error-after-abort.

Reviewed by Cursor Bugbot for commit fb510d1. Configure here.

The SFTP/SSH download routes buffered a remote file into memory with the
only size guard being sftp.stat().size — a value the caller-supplied SSH
server controls. A server that reports a tiny size and then streams
endlessly drove unbounded heap growth until OOM.

Read through the existing readNodeStreamToBufferWithLimit limiter, which
enforces the cap on bytes actually received and destroys the stream on
breach, via a small readSftpFileCapped wrapper in sftp/utils. All four
SFTP-reading tool routes now share it — download, download-file,
read-file-content, and the append path of write-file-content, which had
no cap at all.

The wrapper keeps a no-op error listener on the stream for its whole
life: ssh2 rejects still-pending SFTP requests when the channel closes,
which arrives as a late error event after the limiter has detached its
own handlers, and an error event with no listener would take the process
down. Too-large responses now return 413 to match how the rest of the
routes surface PayloadSizeLimitError, and responses report the actual
byte count rather than the server-reported stat size.
@waleedlatif1
waleedlatif1 force-pushed the worktree-fix-sftp-ssh-download-read-cap branch from 33cebd7 to fb510d1 Compare August 1, 2026 19:10
@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 fb510d1. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@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 fb510d1. 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 prevents caller-controlled SSH/SFTP servers from causing unbounded buffering by applying a shared limit to bytes actually received.

  • Adds a capped SFTP stream-reading utility with late-error protection.
  • Uses the utility across SFTP download and SSH download, read, and append routes.
  • Returns 413 for limit violations and reports actual received byte counts.
  • Adds utility tests covering valid reads, over-limit destruction, deceptive stat sizes, and late stream errors.

Confidence Score: 5/5

The PR appears safe to merge, with the new limiter consistently preventing unbounded SSH/SFTP file buffering.

The shared reader limits actual received bytes, destroys over-limit streams, safely absorbs documented late ssh2 errors, and all affected routes correctly surface cap violations without changing their successful response contracts.

Important Files Changed

Filename Overview
apps/sim/app/api/tools/sftp/utils.ts Introduces the shared 50MB received-byte cap and preserves an error listener for late ssh2 channel failures.
apps/sim/app/api/tools/sftp/utils.test.ts Covers normal reads, received-byte enforcement, stream destruction, the fixed cap, and late errors after abort.
apps/sim/app/api/tools/sftp/download/route.ts Replaces unbounded chunk accumulation with the capped helper and maps size-limit failures to 413.
apps/sim/app/api/tools/ssh/download-file/route.ts Applies capped buffering, returns actual received sizes, and surfaces limit failures as 413.
apps/sim/app/api/tools/ssh/read-file-content/route.ts Consolidates the existing dynamic cap through the shared helper and reports actual buffered bytes.
apps/sim/app/api/tools/ssh/write-file-content/route.ts Caps the existing-file read used by append mode while preserving prior handling of non-limit read errors.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[SSH/SFTP route] --> B[Remote stat pre-check]
  B --> C[readSftpFileCapped]
  C --> D[readNodeStreamToBufferWithLimit]
  D -->|Within limit| E[Buffered response or append]
  D -->|Received bytes exceed limit| F[Destroy stream]
  F --> G[HTTP 413]
  F -. late ssh2 error .-> H[Persistent no-op error listener]
Loading

Reviews (1): Last reviewed commit: "fix(ssh/sftp): cap remote file reads on ..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 47e8f1e into staging Aug 1, 2026
27 of 28 checks passed
@waleedlatif1
waleedlatif1 deleted the worktree-fix-sftp-ssh-download-read-cap branch August 1, 2026 23:53
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