add entrypoint-based custom export api - #197
Conversation
|
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! |
- implement design proposed in #114
- harden remote browser request handling - apply size limits to exported html and screenshots
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! |
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.