add entrypoint-based custom export api - #197
Conversation
github-actions
Bot
added
workshop/frontend
|
Confirmed — Now let me reconsider the HTML export CSP more carefully as it's the main new security surface. The static HTML CSP allows Let me verify the sanitizer runs in the isolated realm and operates on the main world's DOM string. In I've completed a thorough review. Let me check one last thing — whether Let me verify the The handler param type in I've reviewed all 29 files. Let me summarize my findings. The overall implementation is careful and well-tested, with strong attention to resource cleanup, timeouts, CSP, and sanitization. I found no blocking issues. Let me note a couple of minor observations. I reviewed the full PR. This is a well-constructed change that extends the existing PDF export into a general export API (HTML/PNG/JPG/PDF browser modes plus server-mode custom handlers). The implementation is careful about the things that matter most here — resource cleanup, wall-clock deadlines, size caps, CSP, and HTML sanitization — and it's backed by thorough unit tests on both the backend and frontend. Observations (none blocking):
Kernel-review points I specifically checked and found clean: LGTM! |
2d84940 to
791b03c
Compare
|
I've completed a thorough review. This PR was already reviewed by ask-bonk (concluded LGTM), and the subsequent commits addressed prior findings (screenshot pixel caps, byte limits, extension regex alignment, TOCTOU fix). Let me do a final consideration of the key security surfaces and correctness. Key things verified:
The prior review already surfaced the minor fragility notes (workerd error-string coupling in LGTM! |
|
I have completed a thorough independent review of all 29 changed files. This PR was already reviewed twice by ask-bonk (both concluding LGTM), and the subsequent commits addressed the earlier local AI review findings (screenshot pixel caps, byte limits, extension regex alignment, TOCTOU fix, print-media-only-for-PDF). My independent pass confirms those conclusions and I found no new actionable issues. Points I verified independently:
The prior reviews' minor non-blocking notes remain accurate (workerd error-string coupling in LGTM! |
0b2401e to
7d0cb6c
Compare
|
@maxwellpeterson Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
bonk break glass |
- implement design proposed in #114
- harden remote browser request handling - apply size limits to exported html and screenshots
- split out file export system prompt into its own section - use top-level await for signaling client export readiness - update built-in blueprints to use top-level awat - update overseer methods to create facet stub exactly once per method
7d0cb6c to
6569646
Compare
Preview:
|
- exportGadget incorrectly used .move() instead of .dup() - restore 250ms DOM settle window to avoid breaking PDF export for existing gadgets that don't implement top-level await
- exportGadget incorrectly used .move() instead of .dup() - restore 250ms DOM settle window to avoid breaking PDF export for existing gadgets that don't implement top-level await
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let bundle = this.getGadgetUiBundle(gadgetId, chatId); | ||
| if (!bundle) throw new Error("This Gadget does not have a UI to export."); | ||
| let title = this.getGadgetRecord(gadgetId).title; | ||
| return renderGadgetInBrowser(browser, bundle.jsCode, title, exportGadget.move(), format); |
There was a problem hiding this comment.
🔴 Browser-based exports lose their connection to the Gadget's server side
The Gadget's server connection is released instead of handed over (exportGadget.move() at packages/workshop-backend/src/overseer.ts:2579) before the browser renders the page, so HTML, PDF and image exports come out empty or fail.
Impact: Users downloading a PDF/HTML/PNG export get a broken or blank file whenever the Gadget's UI needs data from its server.
Why `move()` is not an ownership transfer on a Workers RPC stub
gadget here is a native Workers RPC stub (new NativeRpcStub(proxy) in getGadgetFacet, packages/workshop-backend/src/overseer.ts:2536), and workerd's JsRpcStub registers only dup and Symbol.dispose as real members (verified against the bundled workerd binary's JsRpcStub::registerMembers symbols); neither @cloudflare/workers-types/worker-configuration.d.ts (StubBase declares only dup(): this) nor capnweb 0.11 defines move(). Any other property access on a stub is turned into a remote method call, so exportGadget.move() issues an RPC named move to the Gadget facet (the wrapper Proxy forwards every property, packages/workshop-backend/src/overseer.ts:2490-2518) and returns a rejected JsRpcPromise rather than a stub. Two consequences follow: renderGadgetInBrowser receives a promise where it expects an RpcStub and installs it as the Cap'n Web session main (packages/workshop-backend/src/browser-export.ts:229-232), and the using exportGadget = gadget declaration disposes the real facet stub as soon as exportGadget() returns, while the browser export is still running. The type checker cannot catch this because the stub is typed NativeRpcStub<any>. Passing exportGadget.dup() (or dropping the using and transferring the stub directly, as the previous exportPdf() did) is the correct transfer.
| return renderGadgetInBrowser(browser, bundle.jsCode, title, exportGadget.move(), format); | |
| return renderGadgetInBrowser(browser, bundle.jsCode, title, exportGadget.dup(), format); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| case "text/html": { | ||
| await isolatedRealm.evaluate(HTML_SANITIZER_RUNTIME); | ||
| const html = await isolatedRealm.evaluate( | ||
| createStaticHtmlSnapshot, | ||
| STATIC_HTML_CSP, | ||
| MAX_EXPORT_BYTES, | ||
| ); | ||
| return streamBytes(new TextEncoder().encode(html)); |
There was a problem hiding this comment.
🟡 A very large HTML or image export is held entirely in server memory
The whole exported file is collected in memory before any of it is sent (streamBytes(new TextEncoder().encode(html)) at packages/workshop-backend/src/browser-export.ts:251) with a 100 MB allowance, so a big export can exhaust the server's memory instead of failing cleanly.
Impact: Exporting a very large page can crash the workspace's server instead of returning a clear error.
Buffering replaces the streaming path the PDF export used
createStaticHtmlSnapshot only rejects a snapshot above MAX_EXPORT_BYTES (100 MB) in the browser (packages/workshop-backend/browser/browser-export-page.ts:60-62), so anything under that is transferred through CDP into the Worker as a JS string (UTF-16, i.e. up to ~200 MB) and then re-encoded into a Uint8Array before limitExportStream ever sees a byte. Workers/Durable Objects have a far smaller memory budget than that, so the limit cannot actually be reached without an OOM. The screenshot paths have the same shape: page.screenshot({clip, captureBeyondViewport: true}) fully buffers a capture bounded only by MAX_SCREENSHOT_PIXELS (25 M pixels). The PDF path avoids this by returning createPDFStream(). Consider a much smaller in-memory cap for the buffered formats (or streaming the snapshot/screenshot in chunks).
Was this helpful? React with 👍 or 👎 to provide feedback.
- exportGadget incorrectly used .move() instead of .dup() - restore 250ms DOM settle window to avoid breaking PDF export for existing gadgets that don't implement top-level await
3 participants
Implements the Gadget file export API design proposed in #114, which I would read first (it's small). The API interface is unchanged in this PR.
The implementation is an extension of the existing PDF export infrastructure. The same remote browser setup is extended to support HTML, PNG, and JPG exports in addition to PDF. DOMPurify is added as a dependency to assist with HTML sanitization on export. This is slightly unfortunate since newer browsers support the HTML Sanitizer API which we could use instead, but the older version of Chromium used by Cloudflare's remote browsers does not support this API. I also refactored functions passed to
page.evaluate()out into a newpackages/workshop-backend/browser/browser-export-page.tsfile so they can be properly typed for the browser environment that they run in.This PR doesn't attempt to detect or prevent Gadget code changes during the export process. It's possible that Gadget code will change between the user selecting an export format and the exported file being produced. This came up repeatedly in local AI code review, but I'm not sure it's a big problem in practice. Exports may fail or produce mismatched file types if Gadget code changes at specific points in the process. We could detect these changes and abort the export, but this feels harsh since most code changes are unlikely to be problematic. Gadget export code (if any exists) should be infrequently updated and cover a small fraction of all total Gadget code. More sophisticated change detection and revision pinning across export operations both add complexity that I wasn't sure we needed. If we did want to revisit this in the future, we could add stronger guardrails without changing the current Gadget API interface.