Fix audit-log hooks skipped for missing capabilities and pass item id to content:beforeSave - #2897
Fix audit-log hooks skipped for missing capabilities and pass item id to content:beforeSave#2897ascorbic wants to merge 2 commits into
Conversation
On an update the runtime hands content:beforeSave only the submitted field values, which never include the item id, so a hook cannot look up the stored item it is about to change. Set an optional `id` on the event for updates, in both the trusted pipeline and the sandboxed path, resolved to the real id when the caller updates by slug. Creates leave it unset.
The manifest declared only content:read while the sandbox entry registers content:beforeSave and media:afterUpload, which the hook pipeline gates on content:write and media:read. Every boot logged a skip warning for each, media uploads were never audited, and update entries had no before/after diff. Declare content:write and media:read. Registering a beforeSave hook is write access whatever the handler does, so the declaration describes the plugin's real power on the write path; media:read is exactly what the media event exposes. Read the item id from `event.id` so the before-state cache is populated on updates.
🦋 Changeset detectedLatest commit: ee5a304 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 |
github-actions
Bot
added
review/needs-review
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://claude-admiring-wing-483524.try.emdashcms.com, https://claude-admiring-wing-483524-emdash-playground.emdash-cms.workers.dev (commit ee5a304)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
@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-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 | ee5a304 | Sep 03 2026, 04:35 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is the right fix for the right problem: the audit-log manifest was under-declared relative to the hooks its sandbox entry actually registers, and the runtime didn’t give content:beforeSave handlers the existing item ID on updates, so the plugin never had enough information to record the before-state. The chosen approach—declare the capabilities the pipeline already requires and add an optional id to the update event—is additive, honest about the trust contract, and avoids a much larger core refactor.
I checked the diff and the full files, tracing how the event field moves through the trusted pipeline (HookPipeline.runContentBeforeSave), the manager delegator, and the sandboxed path (EmDashRuntime.runSandboxedBeforeSave), and confirmed both:
handleContentUpdateresolves slug→ID once and passesresolvedItem?.id.- That ID is only attached to the event on updates; the create path never sets it.
The new tests cover the regression: the audit-log test fails before the manifest and event-ID fixes, and the event-ID test covers trusted and sandboxed updates. The two changesets are user-facing and note the upgrade implication (marketplace capability approval) clearly.
Only one small coverage gap stood out.
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[suggestion] The sandboxed plugin block tests update-by-id but not update-by-slug, while the trusted block tests both. Since handleContentUpdate resolves the slug to the real item id before invoking the sandboxed hook, adding a slug case here would protect the two paths from diverging in future refactors.
it("passes the resolved item id when updating by slug", async () => {
const item = await repo.create({
type: "post",
slug: "hello-world",
data: { title: "Original" },
});
const result = await runtime.handleContentUpdate("post", "hello-world", {
data: { title: "Changed" },
});
expect(result.success).toBe(true);
expect(invokeHook).toHaveBeenCalledWith("content:beforeSave", {
content: { title: "Changed" },
collection: "post",
isNew: false,
id: item.id,
});
});
github-actions
Bot
added
review/awaiting-author
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. |
1 participant
What does this PR do?
The audit-log manifest declared only
content:read, but its sandbox entry registerscontent:beforeSave(gated oncontent:writebyHookPipeline.HOOK_REQUIRED_CAPABILITY) andmedia:afterUpload(gated onmedia:read). Every site bundling the plugin, including the blog template, logged both[hooks] Plugin "audit-log" declares … hook without … capability — skippinglines on boot, media uploads were never audited, and update entries had no before/after diff.Fix: declare the capabilities the hooks need (option 1)
The manifest now declares
content:read,content:writeandmedia:read, with a comment explaining why a plugin that never writes content still needscontent:write.Why not a read-only pre-save hook variant in core (option 2):
content:beforeSavehandler's return value replaces the draft, so registering that hook is write access, regardless of what the handler does today. Declaringcontent:writekeeps the trust contract honest about what the plugin can do on the write path.media:readis not an over-grant:media:afterUploadhands the plugin media metadata, which is exactly whatmedia:readgates.event.contentin place whatever flag it declares, so a genuine read-only guarantee would mean cloning the draft for every such hook on every save. The change would touchHookConfig/ResolvedHook,definePlugin,adaptSandboxEntry, the manifest hook-entry schema and plugin-cli translation, the registry lexicons, admin plugin views and docs, plus a minoremdashrelease, for one first-party plugin, and older hosts would still silently skip such a hook. If other plugin authors hit this, it is a good Discussion topic.The published manifest schema (
packages/plugin-cli/schemas/emdash-plugin.schema.json) needs no change: capability entries are free-form strings there, and the CLI validates them against core's current list, which includes both names.emdash-plugin validatepasses on the updated manifest.Also:
content:beforeSavereceives the item id on updatesThe capability fix alone did not restore the diff. On updates the runtime passes
content:beforeSaveonly the submitted field values (body.data), which never containid(a reserved field slug), so the plugin'sevent.content.idlookup could never populate its before-state cache. This PR adds an optionalidtoContentHookEvent, set for updates by both the trusted pipeline and the sandboxed path and resolved to the real ID when the caller updates by slug. The plugin readsevent.id ?? event.content.id. The change is additive: on an olderemdashthe plugin behaves as it does today and records the update without the previous state. It is a separate commit with its ownemdashchangeset (minor) and docs updates so it can be reviewed or dropped on its own.Tests
packages/core/tests/integration/plugins/audit-log-plugin.test.tsloads the real manifest and sandbox entry throughadaptSandboxEntryand drives the runtime. Before the fix all three cases failed: the two hooks were not registered (skip warnings logged), the update entry had nochanges.before, and no media entry was written.packages/core/tests/integration/runtime/before-save-event-id.test.tscovers the new event field for trusted and sandboxed plugins on create (absent), update by id, and update by slug.Verification
templates/blog(astro dev) before and after. With the previous build both[hooks] … skippinglines printed on the first request; with the rebuilt core and plugin there is no[hooks]output while the runtime serves pages.pnpm lint:jsonreports 0 diagnostics after a workspace build.pnpm --filter emdash typecheckandpnpm --filter @emdash-cms/plugin-audit-log typecheckpass.pnpm formathas been run. Full core vitest run: 525 files passed, 6314 tests passed (2 files / 9 tests skipped, pre-existing).Follow-up worth a Discussion, not included here: have
emdash-plugin buildwarn when a declared hook lacks its required capability, so this class of mismatch fails at build time instead of at every boot.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. — n/a, no admin UI strings changedAI-generated code disclosure
Screenshots / test output
Not applicable (no UI change).