Repository navigation
feat(plugins): add sandboxed redirect access - #3184
Conversation
🦋 Changeset detectedLatest commit: 83ab759 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 2,918 lines across 70 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 | 83ab759 | Sep 19 2026, 01:41 PM |
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. |
|
Review of 1. The new loop trigger aborts legitimate slug renames, rolling back the whole content save
await repository.collapseChains(oldUrl, newUrl, fence);
// The new URL serves live content again — any redirect from it would shadow the page.
... .deleteFrom("_emdash_redirects").where("source", "=", newUrl)
Repro against SQLite 3.51 with the trigger body copied verbatim from the migration: INSERT INTO _emdash_redirects VALUES ('r1','/blog/b','/promo',1);
INSERT INTO _emdash_redirects VALUES ('r2','/promo','/blog/a',1);
-- editor renames a post's slug a -> b; step 1 of createAutoRedirect:
UPDATE _emdash_redirects SET destination='/blog/b' WHERE destination='/blog/a' AND enabled=1;Two ordinary manual redirects that coexist fine today ( This is not confined to the redirects admin. Fix: run the 2. Migration 080 fails closed against previous-version code, breaking content saves mid-deployWHEN NEW.write_generation = 0 AND (
NEW.is_pattern = 1 OR EXISTS (SELECT 1 FROM _emdash_redirects WHERE enabled = 1 AND is_pattern = 1))
BEGIN SELECT RAISE(ABORT, 'pattern redirect writes require the current runtime'); END
The code comment frames this as protecting pattern writes, but the That's the case AGENTS.md § Migrations rules out: "old application code must tolerate the expanded schema during a rolling deploy". The migration test asserts the abort as intended behaviour and the changeset doesn't mention it. Fix: drop Checked and clean: ~ 🤖 Codex |
Cross-PR blocker: three open PRs all add migration
|
| PR | migration file | sorts |
|---|---|---|
| #3182 | 080_content_translation_locale_unique.ts |
1st |
| #3184 | 080_redirect_write_guards.ts |
2nd |
| #3169 | 080_taxonomy_translation_locale_unique.ts |
3rd |
Individually each is fine. Together they can hard-break upgrades, because the runner builds a plain new Migrator({ provider: new StaticMigrationProvider() }) (packages/core/src/database/migrations/runner.ts:227-229) with no allowUnorderedMigrations, so Kysely enforces strict alphabetical ordering:
throw new Error(`corrupted migrations: expected previously executed migration ${executedMigrations[i]} to be at index ${i} but ${migrations[i].name} was found in its place. New migrations must always have a name that comes alphabetically after the last executed migration.`);kysely@0.29.2/dist/migration/migrator.js:503, #ensureMigrationsInOrder
Failure scenario: #3184 merges and ships, and a site upgrades and runs 080_redirect_write_guards. #3182 then merges and ships 080_content_translation_locale_unique, which sorts before the already-executed 080. On the next upgrade migrateToLatest() throws — and because migrations run from runtime init, that site can't boot into a migrated state at all. Recovery means hand-editing the migration table on production.
This only bites when the 080s land in different releases. If they all ship in one release a fresh run executes them in sorted order and nothing notices, which is exactly why this will slip through CI — each PR is green on its own, and main never sees the conflict until the second one merges.
Fix: keep 080 for whichever merges first and renumber the others to 081/082 (file name, the static import and the getMigrations() key in runner.ts) before they merge. If the intent is instead to allow out-of-order migrations, that's a deliberate migrator-level decision and needs its own change, not three coincidental 080s.
For this PR: 080_redirect_write_guards sorts after #3182's migration. If #3182 ships first, this one is fine as-is; if this one ships first, #3182 must renumber. Coordinate before merging either.
This is separate from, and in addition to, the two blockers in my earlier comment.
~ 🤖 Codex
|
Re-reviewed at FixedSlug-rename abort. The delete now precedes Pattern guards. Migration numbering. Still open: the loop trigger aborts the previous release's rename sequenceBlocker 2 was about old binaries failing against the new schema. Removing the pattern guards fixed the path I named, but the loop trigger reaches the same outcome, because the reorder that fixes blocker 1 only exists in the new code.
// Collapse chains: update any existing redirects pointing to the old URL
await this.collapseChains(oldUrl, newUrl);
...
await this.db.deleteFrom("_emdash_redirects").where("source", "=", newUrl).execute();
That is exactly the order
The fence triggers just gained a generation-aware opt-out; the loop triggers need the equivalent, or they need to not fire on a write that an old binary can produce. Changeset
For completeness, I checked the fix commit for new regressions and found none beyond this: the ~ 🤖 Codex (claude-opus-5) |
# Conflicts: # packages/cloudflare/src/sandbox/bridge.ts
# Conflicts: # apps/release-action/dist/index.js # 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/wrapper.ts # packages/core/src/database/migrations/runner.ts # packages/core/src/index.ts # packages/core/src/plugin-test-runtime.ts # packages/core/src/plugins/context.ts # packages/core/src/plugins/host.ts # packages/core/src/plugins/index.ts # packages/core/tests/integration/plugins/capabilities.test.ts # packages/plugin-cli/src/init/templates.ts # packages/plugin-test/src/runtime-host.ts # packages/plugin-test/test/fixture/emdash-plugin.jsonc # packages/plugin-test/test/fixture/src/plugin.ts # packages/plugin-types/tests/capabilities.test.ts # packages/registry-lexicons/src/generated/types/com/emdashcms/experimental/package/releaseExtension.ts # packages/workerd/src/sandbox/bridge-handler.ts # packages/workerd/src/sandbox/wrapper.ts
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds capability-gated, versioned redirect CRUD for sandboxed plugins across native, Cloudflare Worker Loader, and Node/workerd runtimes. It threads the new redirects:read/redirects:write capabilities through the manifest schema, registry lexicon, plugin context, all three sandbox bridges, the plugin test host, and the CLI bundle path, plus a migration that adds serialization and version columns.
The approach is sound and fits EmDash's architecture: it reuses the existing redirect handlers/validation, adds per-dialect serialized write guards, surfaces an opaque _rev for optimistic concurrency, and keeps capability enforcement at the bridge so all runtimes share the same policy. The changeset, docs, and tests are comprehensive.
I reviewed the implementation statically: major files (migration 081, redirect repository/handler, plugin context, both bridges, wrapper generators, manifest/lexicon changes, test fixture) and traced cross-cutting concerns (capability implication, revision round-trip, bridge parity, RBAC permission existence, index discipline, SQL safety, logged-out query impact). I found no logic bug, regression, authorization gap, or AGENTS.md convention violation. The code is clean enough to sign off.
One minor suggestion for clarity is noted below.
github-actions
Bot
added
review/approved
What does this PR do?
Adds capability-gated, versioned redirect CRUD for sandboxed plugins across native, Worker Loader, and Node/workerd runtimes. It reuses host validation and cache invalidation, adds serialized database guards, and extends the runtime-backed test host. Related foundation: #3162.
Merge order: #3182 is merged as migration 080; this PR uses 081. #3169 follows with 082.
Type of change
This is a maintainer-directed capability slice following #3162; there is no separate Discussion URL.
Checklist
CONTRIBUTING.mdAI-generated code disclosure
Screenshots / test output
Not applicable; no interface component or layout change.
Validated at
83ab759b3: full build, format check, typecheck, type-aware lint, 144 focused SQLite/PostgreSQL migration and redirect tests, 13 D1/workerd migration tests, both sandbox bridges and the combined Stack A authoring/manifest/test-host consumers, Worker startup closure (18 modules / 1,728,969 bytes), changeset validation,git diff --check, and independent adversarial conflict review.