Add stdin support to exec() - #362
Conversation
Enables passing arbitrary user input to commands without shell injection risks. Input is written to a temp file and redirected to the command's stdin, then cleaned up after execution. Closes #357
🦋 Changeset detectedLatest commit: a32e460 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
OpenCode Review
This is a well-implemented feature that follows the Cloudflare security best practice of writing input to a file instead of interpolating into commands. The implementation correctly flows stdin through all three layers of the architecture.
Security approach is sound: Writing stdin to a temp file and using shell redirection (< file) prevents shell injection, which aligns with Cloudflare's documented security patterns.
Architecture compliance: The changes properly follow the three-layer pattern - SDK → Durable Object → Container - with appropriate type definitions in shared package.
Implementation quality: Good test coverage at the session level, proper parameter threading through all layers, defensive programming with optional chaining, and appropriate file cleanup in finally blocks.
Minor gap: The E2E test infrastructure doesn't support the stdin parameter yet, so there's no end-to-end validation through the full HTTP API stack. Consider updating the test worker to enable comprehensive integration testing.
The changeset follows project conventions. Overall this looks good to merge.
commit: |
🐳 Docker Images PublishedDefault: FROM cloudflare/sandbox:0.0.0-pr-362-b25fad1With Python: FROM cloudflare/sandbox:0.0.0-pr-362-b25fad1-pythonWith OpenCode: FROM cloudflare/sandbox:0.0.0-pr-362-b25fad1-opencodeVersion: Use the 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-362-b25fad1 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 21335195099 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-362-b25fad1 cat /container-server/sandbox > sandbox && chmod +x sandbox |
|
/bonk review this PR |
|
I've now thoroughly reviewed all 12 changed files. Here's my review: PR Review: Add stdin support to exec()Overall AssessmentThis is a well-structured feature that correctly threads the What's Good
Issues1.
2. No size limit on stdin data (Low-Medium) There's no validation on the size of the
Consider adding a reasonable size limit (e.g., 10MB) with a clear error message, or at minimum documenting the practical limit. 3. Stdin file race with timeout (Low) If a command times out ( 4. Missing The stdin tests only cover the foreground Nits
VerdictThis is solid work. The architecture follows established patterns, security is handled correctly, and the implementation is clean. The issues above are improvement suggestions rather than blockers. The |
|
As discussed on chat a few weeks ago, closing this in favour of doing a proper PTY API implementation that will be far more flexible and handle all sorts of ways to pass and read from stdin rather than the specific approach this implementation takes. |
…am(), and startProcess() (cloudflare#27850)" This reverts merge commit b56f39e. Remove stale documentation claiming that `exec()`, `execStream()`, and `startProcess()` accept `stdin` input. The documentation was added in cloudflare-docs#27850 to match cloudflare/sandbox-sdk#362, but the SDK change was closed without merging in favor of a future PTY-based design. The released SDK does not expose this option, so the current examples direct users toward an unsupported API. Keep the command reference aligned with the released implementation while cloudflare/sandbox-sdk#357 remains unresolved.
Summary
Adds
stdinoption toexec()method, enabling users to pass arbitrary input to commands without shell injection risks.Usage
Closes #357