fix(workerd): accept current plugin capability names - #2576
Conversation
🦋 Changeset detectedLatest commit: 636050f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 640 lines across 12 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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix, in the right place, and implemented consistently.
The bug is that the workerd bridge and wrapper were checking for legacy capability names (read:content, network:fetch, etc.) and rejecting the current canonical names that emdash already uses elsewhere. The correct boundary fix is to normalize legacy aliases to current names at the bridge/wrapper entry points, which is exactly what the PR does. This mirrors the existing Cloudflare runner behavior (which already calls normalizeCapabilities), so the two sandbox runners now agree.
What I checked:
- The full diff and every changed source file.
- The capability rename map in
@emdash-cms/plugin-types(CAPABILITY_RENAMES) to confirmnetwork:fetch:any → network:request:unrestricted,read:users → users:read, etc. - The flow through
WorkerdSandboxRunner.generatePluginToken→ backing service →createBridgeHandler, and throughgeneratePluginWrapper, to confirm both paths now normalize consistently. - The new regression tests cover current names, legacy aliases, the unrestricted network supersede, wrapper
ctx.usersexposure, and end-to-end capability behavior through a spawned workerd isolate. - CI change is correctly scoped to deterministic tests that do not need the workerd binary or Miniflare.
Headline conclusion: the implementation is sound and the regression coverage is solid. I have two minor test-quality suggestions — one clearly tautological assertion and one brittle generated-source check — but nothing that blocks the fix.
| ]; | ||
|
|
||
| it("declares no capability that publish rejects", () => { | ||
| expect(MANIFEST_CAPABILITIES.filter(isDeprecatedCapability)).toEqual([]); |
There was a problem hiding this comment.
[suggestion] This assertion checks a test-local literal against itself via isDeprecatedCapability. It cannot fail on a real regression; it only fails if someone intentionally edits MANIFEST_CAPABILITIES. The neighboring test already verifies that the fixture authorizes every declared operation, so this assertion is redundant and inflates coverage.
| expect(MANIFEST_CAPABILITIES.filter(isDeprecatedCapability)).toEqual([]); | |
| it("authorizes every operation the manifest declares", async () => { |
Remove the declares no capability that publish rejects test block entirely (and then drop the unused isDeprecatedCapability import).
| }); | ||
|
|
||
| describe("wrapper API surface", () => { | ||
| function usersExposed(capabilities: string[]) { |
There was a problem hiding this comment.
[suggestion] usersExposed tests the generated wrapper by looking for the exact emitted string const users = true ?. That pattern is an implementation detail of the generator; a behavior-preserving refactor (e.g., emitting a named boolean) would break this test without breaking users. The new workerd-integration.test.ts already observes typeof ctx.users through a real isolate, so consider relying on that integration coverage or evaluating the generated module instead of asserting on source shape.
github-actions
Bot
added
review/needs-rereview
The Workerd bridge and wrapper were added after the capability rename but still checked the pre-rename names. As a result, manifests using current content, media, users and network capabilities could be denied, and users:read did not expose ctx.users. Normalize capabilities at the bridge handler and wrapper boundaries, keep supported legacy aliases working, and report current names in permission errors. Add deterministic Workerd capability tests to the existing Tests job because pnpm test:unit does not run this package's tests.
ac36b14 to
636050f
Compare
github-actions
Bot
removed
the
review/needs-rereview
1 participant
What does this PR do?
Fixes the Workerd sandbox rejecting manifests that declare current capability names for content, media, users, and network access.
The bridge and generated wrapper now normalize manifest capabilities before checking them. Current names authorize the matching APIs, supported legacy aliases remain accepted, and permission errors report the current names. This also exposes
ctx.userswhen a manifest declaresusers:read.Regression tests cover current and legacy names across the bridge, generated wrapper, and a real Workerd process. CI now runs the deterministic Workerd capability tests for this package.
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. Not applicable: this PR does not change the admin UI.AI-generated code disclosure
Screenshots / test output
pnpm format:check, andgit diff --checkpassed.git diff --checkpassed.pnpm format:checkreported the same 10 symlink-checkout files on the pristine base and candidate because the Windows checkout hadcore.symlinks=false; this was not a candidate regression.