improvement(sandbox): upgrade pptx/docx/pdf bootstrap with image helpers, MIME guards, and 256 MB isolate limit#4505
Conversation
…ers, MIME guards, and 256 MB isolate limit
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Raises isolated-vm Reviewed by Cursor Bugbot for commit b7db38a. Configure here. |
Greptile SummaryThis PR hardens the PPTX, DOCX, and PDF sandbox bootstraps with unified image-helper globals (
Confidence Score: 5/5Safe to merge; all three sandbox tasks have consistent, well-guarded image helpers and the isolate memory increase is conservative relative to the ECS task limit. The changes are self-contained sandbox bootstrap improvements and a worker config bump. The image helpers add input validation rather than removing it, the MIME guards eliminate previously silent failures, and the memory-limit increase is well-reasoned. The two open notes (pptx MIME validation gap and broad memory limit string match) are non-blocking quality items that do not affect correctness under normal usage. No files require special attention; isolated-vm-worker.cjs carries the OOM detection logic worth a second read if the exact isolated-vm error-message format needs to be confirmed against the library version in use. Important Files Changed
Sequence DiagramsequenceDiagram
participant UserCode as User Sandbox Code
participant Bootstrap as Bootstrap Globals
participant Broker as workspaceFileBroker
participant Lib as PDF/DOCX/PPTX Library
UserCode->>Bootstrap: getFileBase64(fileId)
Bootstrap->>Broker: "workspaceFile({ fileId })"
Broker-->>Bootstrap: "{ dataUri }"
Bootstrap->>Bootstrap: "size check (> 8 MB base64 throws)"
Bootstrap-->>UserCode: dataUri string
UserCode->>Bootstrap: addImage / drawImage / embedImage(...)
Bootstrap->>Bootstrap: validate required opts (x,y,w,h / width,height)
Bootstrap->>Bootstrap: parse comma separator (-1 throws)
Bootstrap->>Bootstrap: MIME type guard (unsupported throws)
Bootstrap->>Lib: embedPng / embedJpg / ImageRun / slide.addImage
Lib-->>UserCode: embedded image object
UserCode->>Bootstrap: finalize (auto-called by executor)
Bootstrap->>Lib: save / write / toBase64String
Lib-->>Bootstrap: bytes / base64
Bootstrap-->>Bootstrap: hand-rolled base64 decode (DOCX only)
Bootstrap-->>UserCode: Uint8Array output
Reviews (9): Last reviewed commit: "fix(sandbox): consistency and cleanup pa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…nt origin placement
|
@greptile |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 925d2ba. Configure here.
- DOCX addImage: upfront width/height validation (matches PDF/PPTX pattern) - PDF embedImage: remove dead Buffer ternary; drop redundant size guard already enforced in getFileBase64 - isolated-vm-worker: add friendly MemoryLimitError branch in both execute paths so OOM produces a clear message instead of a raw V8 error
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 b7db38a. Configure here.
|
@greptile |
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b7db38a. Configure here.
| name: 'MemoryLimitError', | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
OOM check is unreachable due to isDisposed ordering
High Severity
The new MemoryLimitError branches in both executeCode and executeTask are unreachable dead code. The isolate?.isDisposed check on lines 382 and 936 runs before the OOM message check. Since isolated-vm auto-disposes the isolate on OOM, isDisposed is always true when an out-of-memory error occurs, so the AbortError ("Execution cancelled") branch fires first. OOM errors will be misreported as user-initiated cancellations. The OOM check needs to be moved before the isDisposed guard to disambiguate the two cases.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b7db38a. Configure here.
1 participant
Summary
getFileBase64,addImage,drawImage,embedImage)embedImagewith clear error for unsupported types (WebP, GIF, BMP)embedImageto prevent silent empty-buffer errors_MAX_IMG_B64 = 8 * 1024 * 1024constantmemoryLimitfrom 128 → 256 MB at both isolate creation sites (4 workers × 256 MB = 1 GB, safe vs 16 GB ECS task)SLIDE_W,SLIDE_H,MARGIN,CONTENT_W,CONTENT_H)rgb,StandardFonts,LETTER,A4shortcuts to pdf bootstrapPAGE_W,PAGE_H,MARGIN,CONTENT_W) and fulladdSection/docfinalize logicaddImage: upfrontwidth/heightvalidation — now consistent with PDFdrawImageand PPTXaddImageembedImage: removed dead Buffer ternary and redundant size guard already enforced bygetFileBase64MemoryLimitErrorbranch in both execute paths — OOM now surfaces a readable message instead of a raw V8 errorType of Change
Testing
Tested manually — generated PPTX, DOCX, and PDF documents via Mothership. Confirmed image embedding, error messages, and output correctness.
Checklist