feat(plugins): add sandbox media access capabilities - #3172
Conversation
🦋 Changeset detectedLatest commit: fafcb80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
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 |
Scope checkThis PR changes 1,766 lines across 49 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | d65daba | Sep 19 2026, 05:20 PM |
74a469a to
3798860
Compare
ascorbic
added
bot:review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a well-scoped, capability-separated sandbox media-access slice. The three authorities (media:read, media:bytes:read, media:metadata:write) are kept properly isolated in the plugin context factory, both sandbox bridges, and the manifest/lexicon layers. Byte reads are bounded against the consumed stream rather than the stored size, metadata patches are constrained to alt/caption/focal points, and the new authenticated asset route hides storage keys as intended. Tests cover the bridges, runtime host, and auth boundary.
I found two concrete issues that should be fixed before merge:
-
Authenticated asset route uses
publiccache headers. The new/_emdash/api/media/asset/[id]/[filename]route requiresmedia:read, but it reuses thepubliccache constants designed for the unauthenticated, storage-key-based file route.publiclets shared caches store authenticated responses, andimmutablefor non-image types is unsafe because the URL is ID-based (the underlying file can change while the URL stays the same). It should useprivatecaching at most, orprivate, no-store. -
Cloudflare bridge
mediaReadBytesdoes not alignmaxBytesvalidation with the workerd bridge.packages/cloudflare/src/sandbox/bridge.tspassesmaxBytesstraight toreadPluginMediaBytes, which yields a different error message than the workerd bridge's explicit type guard. The bridge files explicitly promise "same error messages," so this is a parity gap.
Neither issue blocks the conceptual approach; both are localized fixes.
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
The three sandbox media authorities are implemented consistently across core, the manifest/lexicon layers, the plugin CLI, admin consent labels, both sandbox bridges (Cloudflare Worker Loader and workerd), and the runtime-backed plugin test host. The authenticated asset route hides storage keys and rejects logged-out requests before touching the media table; byte reads bound the consumed stream instead of the stored size; and metadata patches are constrained to alt, caption, and complete focal-point pairs.
Both reviewed-head blockers from the previous review are fixed:
- The opaque asset route now uses
Cache-Control: private, max-age=0, must-revalidate, replacing the unsafe public/immutable cache headers. - The Cloudflare bridge now validates
maxByteswith the samemedia/readBytes: maxBytes must be a numbererror as the workerd bridge.
I read the new plugin-media module, the route, the repository change, both bridges and their wrappers, the runtime wiring, the registry changes, the docs, and the changeset. I did not find any new logic bugs, regressions, or AGENTS.md convention violations. The implementation matches the documented contract, tests cover the capability separation and byte-limit paths, and the documentation claims align with the code.
This is a clean, well-scoped slice. LGTM.
|
Review of 1. The new capabilities can never be published — the registry rejects them/** Must stay in sync with PLUGIN_CAPABILITIES in packages/core/src/plugins/manifest-schema.ts */
const VALID_CAPABILITIES = [ ... "media:read", "media:write", ... ] as const;
...
capabilities: z.array(z.enum(VALID_CAPABILITIES)),
This PR adds Failure scenario: the marketplace Worker is the live distribution path ( This is the established contract rather than a guess: the last capability-adding PR updated this exact list ( Fix: add 2.
|
|
Fixed both blockers in
Validated with root build, full typecheck/lint, marketplace tests (45), workerd bridge tests (52), Cloudflare bridge tests, dialect-backed repository tests, and a real-workerd media integration run. The final adversarial re-review found no bugs. ~ 🤖 Codex (gpt-5.6-sol) |
github-actions
Bot
added
review/needs-rereview
Merge-order hazard with the sibling marketplace fix#3170 and #3172 both change The two took different approaches to the same bug:
The conflict is loud, so nothing slips through silently at merge time. The risk is in the resolution: keeping #3172's hardcoded block would discard the shared import and re-break Correct resolution: take #3170's shared Worth noting this affects more than these two — #3185 ( ~ 🤖 Codex (claude-opus-5) |
…cess # Conflicts: # packages/cloudflare/src/sandbox/bridge.ts
ascorbic
added
bot:review
There was a problem hiding this comment.
This PR is a well-scoped, architecture-fitting addition: it splits media authority into three separate capabilities (media:read, media:bytes:read, media:metadata:write) and threads them through the runtime, both sandbox bridges, CLI/lexicon validation, admin consent labels, and docs. The implementation generally follows EmDash conventions — no raw SQL, capability gating before storage access, bounded byte reads enforced on the consumed stream, and the authenticated asset route rejects anonymous requests before touching D1.
I read the core media helpers, the asset route, RBAC, the Cloudflare and workerd bridge changes, plugin-type round-tripping, the admin marketplace labels, the test host, and the changed docs/changesets. The test coverage is thorough across runtimes and boundaries.
Two real issues survive:
updatePluginMediaMetadata()invalidates the site-settings cache, which has nothing to do with media metadata and will cause unnecessary cache churn.- The public docs, skill, and changeset all say the authenticated asset route is for administrator sessions / API tokens with
media:read, but the route only checks the RBACmedia:readpermission, whose minimum role isSUBSCRIBER. That claim is false and misleading about the access-control model.
Everything else is clean. I did not run tests, lint, or builds (static review only).
github-actions
Bot
added
review/approved
…cess # Conflicts: # docs/src/content/docs/plugins/creating-plugins/api-routes.mdx # docs/src/content/docs/plugins/creating-plugins/capabilities.mdx # docs/src/content/docs/plugins/creating-plugins/testing.mdx # docs/src/content/docs/plugins/installing.mdx # packages/cloudflare/src/sandbox/bridge-runtime.ts # packages/cloudflare/src/sandbox/bridge.ts # packages/cloudflare/src/sandbox/runner.ts # packages/core/src/index.ts # packages/core/src/plugins/host.ts # packages/marketplace/src/routes/author.ts # packages/plugin-cli/src/init/templates.ts # packages/plugin-cli/tests/init-templates.test.ts # packages/plugin-cli/tests/manifest-trust-contract.test.ts # packages/plugin-test/src/runtime-host.ts # packages/plugin-test/test/fixture/src/plugin.ts # packages/plugin-test/test/host.test.ts # packages/plugin-types/src/index.ts # packages/plugin-types/tests/capabilities.test.ts # packages/workerd/src/sandbox/bridge-handler.ts # skills/creating-plugins/SKILL.md # skills/creating-plugins/references/sandbox-boundaries.md
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This remains a well-scoped, architecture-fitting addition: it splits media authority into three separate capabilities (media:read, media:bytes:read, media:metadata:write) and threads them through the runtime, both sandbox bridges, CLI/lexicon validation, admin consent labels, and docs. The implementation follows EmDash conventions—capability gating before storage access, bounded byte reads enforced on the consumed stream, and the authenticated asset route rejecting anonymous requests before touching D1.
Prior review issues are both resolved.
updatePluginMediaMetadata()no longer invalidates the site-settings cache; that responsibility is correctly left withEmDashRuntime.handleMediaUpdate(where width/height can affect resolved site-setting URLs) and withcreateMediaAccessWithWrite().delete().- The public docs, skill, and changeset now accurately describe the authenticated asset route as requiring the RBAC
media:readpermission, whose minimum role isSUBSCRIBER. The newmedia-asset-auth.test.tscovers the logged-out 401 and Subscriber+ token authorization paths.
I re-read the core media helpers, the asset route and auth tests, the RBAC mapping, the Cloudflare and workerd bridge changes, the plugin-type round-tripping, the admin marketplace labels, the runtime test host, and the docs/changesets. Test coverage is thorough across runtimes and boundaries.
One small piece of documentation still lags the implementation: the PluginContext.media docstring in packages/core/src/plugins/types.ts still claims the field exists only for the legacy read:media/write:media capabilities, ignoring media:bytes:read and media:metadata:write and using deprecated capability names.
Everything else is clean. I did not run tests, lint, or builds (static review only).
Findings
-
[suggestion]
packages/core/src/plugins/types.ts:877The
PluginContext.mediadocstring still says the field exists "only ifread:mediaorwrite:mediacapability". That is no longer true:PluginContextFactory.createContextpopulatesmediawhenever any media capability is declared, including the newmedia:bytes:readandmedia:metadata:write. The capability names in the comment are also the deprecated legacy forms rather than the canonicalmedia:read/media:write./** Media access - only if any media capability is declared */
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Adds the independent sandboxed-plugin media-access vertical slice from the maintainer-provided capability expansion plan.
It keeps three authorities separate:
media:readreturns expanded ready-media metadata without storage keys, author identity, content hashes, or bytes. Its asset URL uses an authenticated media-ID route, so logged-out requests stop before the route queries D1.media:bytes:readaddsctx.media.readBytes(). Reads use the configured storage adapter, enforce the actual stream, default to 10 MiB, and cannot request more than 16 MiB. Content hashes are visible only with this capability.media:metadata:writeaddsctx.media.updateMetadata()for non-empty alt, caption, and complete focal-point patches. It cannot upload, replace, move, or delete media.The capabilities round-trip through
declaredAccess, shared/core/CLI manifest validation, the registry lexicon and generated types, install/update consent, native context, Cloudflare Worker Loader, Node/workerd, and the runtime-backed plugin test host. Public docs, canonical and generated authoring skills, and package changesets describe the same contract.The mandatory independent adversarial review found and verified fixes for four issues before PR creation: storage-key leakage in adapter errors, a non-routable redacted URL, trusting stored size instead of the consumed stream, and empty metadata patches becoming reads. A second boundary review required and verified authenticated middleware/RBAC semantics for the opaque asset route. The final reviewed head is
74a469a32.Upload, replacement, movement, and deletion changes are out of scope.
Prior feature approval: maintainer-directed implementation of the untracked sandbox capability expansion plan. The planning file is intentionally not included in this PR.
Closes # — not applicable.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Screenshots are not applicable. This PR adds localized labels to the existing consent-dialog component without changing its layout or styling. The rendered component test exercises the real update-escalation flow and verifies both new labels.
Validated at
74a469a32:pnpm buildpnpm typecheckpnpm lintpnpm --dir docs build~ 🤖 Codex