Repository navigation
fix(workerd): bind bridge access to active plugin credentials - #3051
Conversation
🦋 Changeset detectedLatest commit: 0c6dc2e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
Scope checkThis PR changes 728 lines across 9 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: |
![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 for the reported workerd plugin authorization gap. Binding bridge access to the current signed token (plus an active flag), keying the bridge handler cache by token, and re-checking the token after the request body has arrived all match the architecture of the existing runner/backing-service split. The tests cover version-specific capabilities, stale credentials, admin disable/re-enable, deactivate-hook failures, body-in-flight revocation, and the startup configuration snapshot.
The implementation is mostly clean, but I found one real regression in WorkerdSandboxRunner.ensureRunning(): the new post-startup recursion intended to catch plugins loaded during restart also recurses when restart() throws, turning a transient startup failure into a tight retry loop. There is also a small structural drift between the LoadedPlugin interfaces in runner.ts and capnp.ts now that active was added.
Findings
-
[needs fixing]
packages/workerd/src/sandbox/runner.ts:480-514ensureRunning()now recurses wheneverthis.needsRestartis true after therestart()attempt. That works for the intended case (a plugin was registered while a restart was in flight), but it also loops whenrestart()itself throws – for example, awaitForReadytimeout, a spawn error, or an unusableworkerdbinary. In that paththis.needsRestartis never cleared (it is only assigned on success), so theif (this.needsRestart) { … await this.ensureRunning(); }block spins immediately instead of letting the next invocation retry with backoff.This changes the previous behavior, where a failed start left
needsRestarttrue for the next caller. Add a success guard before recursing:async ensureRunning(): Promise<void> { // If a startup is already in progress, wait for it if (this.startupPromise) { await this.startupPromise; return this.ensureRunning(); } if (!this.needsRestart) return; // Serialize: concurrent callers await the same promise. // Don't clear needsRestart until startup succeeds, so a transient // failure (waitForReady timeout, spawn error) can be retried by // the next invocation. const configurationVersion = this.configurationVersion; this.startupPromise = this.restart(); let restartSucceeded = false; try { await this.startupPromise; this.needsRestart = configurationVersion !== this.configurationVersion; // A repeat load returns the cached instance without marking a // restart, so it never drives the start that clears this. this.gaveUp = false; restartSucceeded = true; } finally { // Always clear startupPromise so a failed start doesn't block // subsequent retries. needsRestart stays true on failure (set above // only after the await succeeds), enabling automatic retry. this.startupPromise = null; } if (this.needsRestart && restartSucceeded) { if (this.plugins.size === 0) { this.needsRestart = false; await this.stopWorkerd(); } else { await this.ensureRunning(); } } } -
[suggestion]
packages/workerd/src/sandbox/capnp.ts:29-34capnp.tsdefines its ownLoadedPlugininterface that now diverges from the one inrunner.ts: it is missing the newactivefield. The snapshot copy passed intogenerateCapnpConfig()has the extra property, and relying on two same-named but different structural types is brittle. Addactive(unused by the generator) so the shapes stay in sync, or better, share a singleLoadedPlugintype between the two modules.interface LoadedPlugin { manifest: PluginManifest; code: string; port: number; token: string; active?: boolean; }
emdashbot
Bot
added
review/awaiting-author
There was a problem hiding this comment.
This is a re-review of the workerd plugin authorization fix. The direction is still right — binding bridge access to the current signed token, keying the handler cache by token, and re-checking the token after the request body arrives are the correct mechanisms for closing the stale-credential gap.
One prior finding is resolved: runner.ts now imports LoadedPlugin from capnp.ts, so the two interfaces are structurally in sync (active is included in the shared type).
The remaining prior finding is not fixed. WorkerdSandboxRunner.ensureRunning() recurses whenever needsRestart is still true after a restart() attempt. That works for the intended case (a plugin was registered while a restart was in flight), but it also loops when restart() itself throws — for example, a waitForReady timeout, a spawn error, or an unusable workerd binary. In that path needsRestart is never cleared (it is only assigned after await this.startupPromise succeeds), so the post-finally block calls ensureRunning() immediately instead of letting the next invocation retry with backoff.
This contradicts the new runner-configuration-snapshot.test.ts test, whose title says a failed startup should be "retried on the next invocation." With the current implementation, the retry happens inside the same call stack. A recurring startup failure therefore becomes a tight loop rather than an orderly backoff.
I also re-checked the authorization-sensitive paths and they look solid: the nonce makes tokens unique per issuance; validateToken rejects inactive or superseded tokens; the backing service re-validates after reading the body; setPluginStatus revokes credentials in a finally block so disable survives a failing deactivate hook; and the cold-start loop applies persisted inactive status. No SQL, locale, query-count, or comment-discipline concerns were introduced.
Fix the ensureRunning recursion guard and this should be good to land.
Findings
-
[needs fixing]
packages/workerd/src/sandbox/runner.ts:488-524ensureRunning()recurses into itself wheneverthis.needsRestartis still true after therestart()attempt, but it does not check whether that restart succeeded. Ifrestart()throws — e.g. awaitForReadytimeout, a spawn error, or a missing workerd binary —needsRestartis never cleared inside thetryblock (it is only assigned afterawait this.startupPromisesucceeds), so the post-finallyblock spins immediately instead of letting the next caller/backoff path retry. This turns a transient startup failure into a tight retry loop and contradicts the newrunner-configuration-snapshottest, which documents that failures should be retried on the next invocation.Add a success guard before recursing:
async ensureRunning(): Promise<void> { // If a startup is already in progress, wait for it if (this.startupPromise) { await this.startupPromise; return this.ensureRunning(); } if (!this.needsRestart) return; // Serialize: concurrent callers await the same promise. // Don't clear needsRestart until startup succeeds, so a transient // failure (waitForReady timeout, spawn error) can be retried by // the next invocation. const configurationVersion = this.configurationVersion; this.startupPromise = this.restart(); let restartSucceeded = false; try { await this.startupPromise; this.needsRestart = configurationVersion !== this.configurationVersion; // A repeat load returns the cached instance without marking a // restart, so it never drives the start that clears this. this.gaveUp = false; restartSucceeded = true; } finally { // Always clear startupPromise so a failed start doesn't block // subsequent retries. needsRestart stays true on failure (set above // only after the await succeeds), enabling automatic retry. this.startupPromise = null; } if (this.needsRestart && restartSucceeded) { if (this.plugins.size === 0) { this.needsRestart = false; await this.stopWorkerd(); } else { await this.ensureRunning(); } } }
There was a problem hiding this comment.
This is a re-review of the workerd plugin authorization fix. The overall direction is still right: binding bridge access to the current signed token, adding a nonce, re-validating the token after the request body arrives, keying the backing-service handler cache by token, and revoking credentials in a finally block during disable all address the stale-credential gap correctly.
The previous LoadedPlugin duplication is fixed — runner.ts now imports LoadedPlugin from capnp.ts, so the active flag lives in a single shared type.
The remaining prior finding is not resolved. WorkerdSandboxRunner.ensureRunning() still recurses into itself whenever this.needsRestart is still true after the restart() attempt, without checking whether that restart succeeded. If restart() throws — for example, a waitForReady timeout, a spawn error, or an unusable workerd binary — needsRestart is never reassigned inside the try block, so the post-finally block spins immediately instead of letting the next invocation or the crash backoff path retry. This contradicts the new runner-configuration-snapshot.test.ts test, which documents that failures should be retried on the next invocation. With a recurring startup failure this becomes a tight async loop rather than an orderly backoff.
The authorization-sensitive paths themselves look solid, and no new SQL, locale, query-count, or comment-discipline issues were introduced. Fix the ensureRunning recursion guard and the rest is ready to land.
| if (this.needsRestart) { | ||
| if (this.plugins.size === 0) { | ||
| this.needsRestart = false; | ||
| await this.stopWorkerd(); | ||
| } else { | ||
| await this.ensureRunning(); | ||
| } |
There was a problem hiding this comment.
[needs fixing] ensureRunning() recurses into itself whenever this.needsRestart is still true after the restart() attempt, but it does not check whether that restart succeeded. If restart() throws — e.g., a waitForReady timeout, a spawn error, or a missing/unusable workerd binary — needsRestart is never reassigned in the try block, so the post-finally block spins immediately instead of letting the next invocation or the crash backoff path retry. This turns a transient startup failure into a tight async retry loop and contradicts runner-configuration-snapshot.test.ts, whose title documents that failures should be retried on the next invocation.
Track whether the restart succeeded before recursing:
| if (this.needsRestart) { | |
| if (this.plugins.size === 0) { | |
| this.needsRestart = false; | |
| await this.stopWorkerd(); | |
| } else { | |
| await this.ensureRunning(); | |
| } | |
| const configurationVersion = this.configurationVersion; | |
| this.startupPromise = this.restart(); | |
| let restartSucceeded = false; | |
| try { | |
| await this.startupPromise; | |
| this.needsRestart = configurationVersion !== this.configurationVersion; | |
| // A repeat load returns the cached instance without marking a | |
| // restart, so it never drives the start that clears this. | |
| this.gaveUp = false; | |
| restartSucceeded = true; | |
| } finally { | |
| // Always clear startupPromise so a failed start doesn't block | |
| // subsequent retries. needsRestart stays true on failure (set above | |
| // only after the await succeeds), enabling automatic retry. | |
| this.startupPromise = null; | |
| } | |
| if (restartSucceeded && this.needsRestart) { | |
| if (this.plugins.size === 0) { | |
| this.needsRestart = false; | |
| await this.stopWorkerd(); | |
| } else { | |
| await this.ensureRunning(); | |
| } | |
| } |
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a sound bug fix that binds workerd bridge access to the currently loaded, active plugin token. The implementation correctly:
- Adds a random nonce to plugin tokens so reload/reactivation produces a fresh credential.
- Validates the token twice in the backing service (before and after body read) to catch disable/unload that arrives while a request is streaming.
- Keys the bridge-handler cache by token as well as plugin/version, so a stale token cannot ride a still-warm handler.
- Revokes credentials in the
finallyblock inEmDashRuntime.setPluginStatus, including whenrunPluginDeactivaterejects. - Uses exact plugin-id matching (strip the last
:${version}segment) so disablingadoes not affecta:b. - Snapshots the plugin map in
restart()and re-runsensureRunning()when registrations change during startup.
The previously unresolved finding in WorkerdSandboxRunner.ensureRunning() is fixed: the new restartSucceeded flag gates the post-restart recursion, so a failed restart no longer spins into a tight retry loop. The restated condition if (this.needsRestart && restartSucceeded) is equivalent to the suggested restartSucceeded && this.needsRestart because of short-circuit evaluation.
I checked the changed files, traced call sites, and verified the new tests cover stale credentials, prefix-id isolation, disable/reactivation, deactivate-hook failures, and startup-with-concurrent-registration. No new SQL, locale, query-count, or comment-discipline issues were introduced. The changeset is user-facing and adequate for a patch.
Approving.
emdashbot
Bot
added
review/approved
github-actions
Bot
removed
the
ci:run
What does this PR do?
A Workerd plugin could keep calling host services after disable or unload because its signed token remained valid.
This PR accepts only the current token for an active registration. Reactivation issues fresh credentials. Requests waiting for their body are checked again before dispatch. Runtime disable revokes access after the deactivate hook settles, including failures; cold-start status is also applied. Status changes match exact plugin IDs, so disabling one plugin does not revoke another whose ID shares its prefix.
Startup uses a configuration snapshot and restarts when registrations change during startup. The lifecycle guarantees cover sequential changes within one runner. They do not coordinate overlapping admin status changes, revoke credentials across processes, or cancel operations already dispatched.
Type of change
Checklist
pnpm typecheckpasses.pnpm lintpasses.AI-generated code disclosure
Screenshots / test output
Not applicable; no UI changes. Coverage includes version-specific permissions, stale credentials, disable/reactivation, lifecycle failures and startup readiness.