Skip to content

add entrypoint-based custom export api - #197

Open
maxwellpeterson wants to merge 6 commits into
mainfrom
mpeterson/export-api
Open

add entrypoint-based custom export api#197
maxwellpeterson wants to merge 6 commits into
mainfrom
mpeterson/export-api

Conversation

@maxwellpeterson

@maxwellpeterson maxwellpeterson commented Aug 13, 2026

Copy link
Copy Markdown
Member

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 new packages/workshop-backend/browser/browser-export-page.ts file 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.

@github-actions github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel workshop/shared Changes to shared Workshop APIs labels Aug 13, 2026
@ask-bonk

ask-bonk Bot commented Aug 13, 2026

Copy link
Copy Markdown

Confirmed — OverseerImpl is not an RpcTarget, so its public methods aren't RPC-exposed. Fine.

Now let me reconsider the HTML export CSP more carefully as it's the main new security surface. The static HTML CSP allows style-src data: 'unsafe-inline' and img-src data:. DOMPurify sanitizes. But there's a subtle issue: DOMPurify by default allows <style> with 'unsafe-inline' CSS. CSS can contain url() with data: only (img-src/font-src restricted to data:). That's fine. External resource loads are blocked by default-src 'none'. Reasonable.

Let me verify the sanitizer runs in the isolated realm and operates on the main world's DOM string. In createStaticHtmlSnapshot, document.documentElement.outerHTML — this runs in the isolated realm, but document in the isolated realm shares the same DOM as the main world (Puppeteer isolated realms share the DOM). Good. So it captures the rendered gadget DOM and sanitizes it. Sensible.

I've completed a thorough review. Let me check one last thing — whether getGadgetUiBundle for the use role now leaks proposed-chat content. The UseGadgetClientInterface.getUiBundle calls getGadgetUiBundle(this.id) with no chatId — so no chat changes. And its export/getExportFormats deny chatId and call impl without chatId. Consistent with prior deny behavior. Good.

Let me verify the Fetcher<GadgetExportEntrypoint> type is right — getEntrypoint returns a service stub. Let me confirm the type checks by checking how readCustomExportFormats handler param matches.

The handler param type in readCustomExportFormats is structural {getExportFormats(gadget): Promise<unknown>}, and it's called with the entrypoint stub. This compiles under tsc which the CI runs. I'll trust the type check.

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):

  • gadget-export.ts:118-120 detects an absent ExportHandler entrypoint by exact-matching the string Worker has no such entrypoint: ExportHandler. If workerd ever reworks that message, custom-export detection silently breaks and every gadget without a handler would throw instead of falling back to defaults. A test pins the current string, so a regression would be caught by the workerd bump rather than in prod, but the coupling is fragile. This mirrors no existing helper, so it's worth a comment noting the dependency.

  • overseer.ts:2594 obtains the ExportHandler entrypoint stub via loadGadgetWorker(...).getEntrypoint(...) and never disposes it (the formats-only path discards it entirely). This matches the existing getGadgetHookEntrypoint pattern (overseer.ts:2615), so it's consistent, but if worker-loader entrypoint stubs do hold resources it's a latent leak in both places.

  • GadgetEditor.tsx drops the disabled={activeTab !== 'app' || previewMode} gate on the export control, so export is now offered from the code tab and in preview mode. Export reads from the ydoc/chat regardless of the visible tab, so it's functionally correct — just confirm the always-available affordance is the intended UX.

Kernel-review points I specifically checked and found clean: getGadgetUiBundle/prepareGadgetChat are new public (non-#) methods on OverseerImpl, but OverseerImpl is not an RpcTarget (overseer.ts:1097), so they aren't RPC-reachable; access stays mediated by GadgetClientImpl/UseGadgetClientInterface. The use role continues to deny chatId and never exposes proposed-chat content. The workshop-shared API additions are all doc-commented. The facet is derived from the real type via getGadgetFacet with a documented as NativeRpcStub cast rather than a hand-mirrored interface.

LGTM!

github run

@ask-bonk

ask-bonk Bot commented Aug 13, 2026

Copy link
Copy Markdown

allowJs: true is set at the root, so the generated .js is type-checked and its exported function signatures are inferred. Good — that's how page.evaluate(sendToBrowser, message) etc. remain typed. And the separate tsc --project tsconfig.browser.json step type-checks the actual browser/*.ts sources with DOM lib. Clean design.

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:

  • CSP/sanitization: Static HTML export uses default-src 'none' + DOMPurify with WHOLE_DOCUMENT; render CSP is strict with only data:/blob: for resources and connect-src 'none'. Reasonable.
  • Resource cleanup: renderGadgetInBrowser disposes gadget stub and closes browser on all paths (launch failure, setup failure, timeout, stream settle/cancel). limitExportStream enforces byte + wall-clock limits and releases on settle.
  • Server-mode: using gadget kept alive through the full exportServerFormat await; documented contract that handler must not retain gadget past export() return.
  • Validation: zod schema bounds all format fields, dedupes ids, restricts browser content types, and the extension regex now aligns with the file picker's constraints.
  • Capability security: use role continues to deny chatId; new OverseerImpl public methods aren't RPC-reachable (not an RpcTarget); API additions are doc-commented.
  • Screenshot: pixel-area validated with overflow-safe width > Math.floor(maxPixels / height).
  • DOMPurify correctly a devDependency (bundled at build time into a string, never runtime-imported by the worker).

The prior review already surfaced the minor fragility notes (workerd error-string coupling in readCustomExportFormats, undisposed entrypoint stub matching the existing hook pattern, and the export affordance now always-available). Those remain accurate but non-blocking, and I don't have new actionable findings beyond them. I inspected existing reviews and will not repeat published findings or submit a redundant review.

LGTM!

github run

@maxwellpeterson
maxwellpeterson marked this pull request as ready for review August 14, 2026 00:14
@ask-bonk

ask-bonk Bot commented Aug 14, 2026

Copy link
Copy Markdown

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:

  • CSP/sanitization: Static HTML export uses default-src 'none'; script-src 'none' with DOMPurify (WHOLE_DOCUMENT); the CSP <meta> is injected after sanitization so it's preserved. Render CSP restricts resources to data:/blob: with connect-src 'none'. Sanitizer runs in the isolated realm over the main-world DOM (document.documentElement.outerHTML). Sound.
  • Resource lifetime: limitExportStream enforces byte + wall-clock caps and releases the browser on every settle path (done/error/cancel/expire); cancellation is treated as advisory so a hostile stream can't keep the browser alive. Deadline expiry uses non-awaiting release() to avoid deadlock.
  • Server-mode gadget lifetime: using gadget is disposed when exportServerFormat returns, and the handler contract (documented in gadget-export.ts and the agent prompt) requires materializing state before returning the stream — an intentional, documented design choice, not a bug.
  • Validation: zod bounds all format fields, dedupes ids, restricts browser content types to the four supported types, and the extension regex aligns with the showSaveFilePicker accept map. zod ^4.4.3 matches the two other consumers in the repo.
  • Screenshot area check: width > Math.floor(maxPixels / height) is overflow-safe.
  • Capability security (kernel): The use role still denies chatId on getExportFormats/export and never exposes proposed-chat content. New getGadgetUiBundle/prepareGadgetChat/exportGadget are public methods on OverseerImpl, which is not an RpcTarget, so they aren't RPC-reachable. All workshop-shared API additions are doc-commented. The stub-type mismatch is handled with a documented as unknown as NativeRpcStub cast rather than a hand-mirrored interface.
  • Build/codegen: generated files (html-sanitizer-runtime.txt, browser-export-page.js) are gitignored and not committed; tsconfig.browser.json type-checks the DOM-targeted browser/ sources separately; DOMPurify is correctly a devDependency (bundled to a string at build time, never runtime-imported by the worker).

The prior reviews' minor non-blocking notes remain accurate (workerd error-string coupling in readCustomExportFormats, the undisposed getEntrypoint stub matching the existing hook pattern, and the export affordance now always-available regardless of tab/preview). I have nothing new to add and won't repeat them as inline comments.

LGTM!

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Changes to the Workshop kernel workshop/frontend Changes to the Workshop frontend workshop/shared Changes to shared Workshop APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant