feat(plugins): read-only taxonomy access via new taxonomies:read capability#1719
Conversation
🦋 Changeset detectedLatest commit: 93522c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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
Scope checkThis PR changes 593 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/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 PR implements read-only taxonomy access for plugins through the existing content:read capability, as approved in Discussion #1718. The approach is sound: it reuses TaxonomyRepository, adds the three methods to ContentAccess, and mirrors them in both sandbox bridges/wrappers with consistent shapes and locale filtering. The changeset and docs are in place, and the in-process integration tests cover the core behavior well.
I found no logic bugs, regressions, or security issues in the new code. SQL is parameterized in both bridges, capability gating follows each bridge's established convention, and the i18n semantics (locale filtering + translation group exposure) match the repository design.
Two things to note:
- The new doc comment in
ContentAccessuses the legacy capability nameread:content; the canonical/runtime name iscontent:read. Minor cleanup. - The bridge/wrapper wiring in
@emdash-cms/cloudflareand@emdash-cms/sandbox-workerdis not covered by new tests — only the in-process core path is. Given that the bridge methods include their own row mapping and (in Cloudflare) raw SQL, bridge-level tests would strengthen the PR.
One broader architectural context: the workerd bridge still enforces legacy capability strings (read:content, read:media, etc.) because the workerd runner passes manifest capabilities through without normalizing them to canonical names. The new taxonomy cases follow that existing pattern, so they do not introduce a fresh inconsistency, but full parity with the Cloudflare bridge for canonical-manifest plugins remains blocked by that pre-existing runner behavior.
| get(collection: string, id: string): Promise<ContentItem | null>; | ||
| list(collection: string, options?: ContentListOptions): Promise<PaginatedResult<ContentItem>>; | ||
|
|
||
| // Taxonomy read operations (requires read:content). Terms classify content |
There was a problem hiding this comment.
[suggestion] The new comment uses the deprecated capability name read:content. Capabilities are normalized to the canonical content:read name before reaching PluginContextFactory, so the docstring should match the runtime vocabulary.
| // Taxonomy read operations (requires read:content). Terms classify content | |
| // Taxonomy read operations (requires content:read). Terms classify content |
There was a problem hiding this comment.
Resolved by the switch to the dedicated capability: the docstring now reads "capability-gated on taxonomies:read" — the read:content wording is gone.
| // ── Taxonomies (read-only) ────────────────────────────────────── | ||
| case "taxonomy/list": | ||
| requireCapability(opts, "read:content"); |
There was a problem hiding this comment.
[suggestion] The workerd bridge taxonomy wiring is currently only exercised by the in-process core tests indirectly. Consider adding bridge-handler-level tests (similar to the existing content capability enforcement cases in packages/workerd/test/bridge-handler.test.ts and plugin-integration.test.ts) that assert the taxonomy/list, taxonomy/terms, and taxonomy/entryTerms cases enforce capabilities, respect locale filtering, and return the expected term shapes.
There was a problem hiding this comment.
Covered since the taxonomies:read rework: packages/workerd/test/bridge-handler.test.ts now has explicit cases asserting taxonomy/list is rejected without taxonomies:read (including that content:read does not grant it) and allowed with it.
| > { | ||
| const { capabilities } = this.ctx.props; | ||
| if (!capabilities.includes("content:read")) { |
There was a problem hiding this comment.
[suggestion] The Cloudflare bridge taxonomy methods add their own row parsing and raw SQL paths, but they are not covered by package-specific tests. Adding tests for taxonomyList, taxonomyTerms, and taxonomyEntryTerms would guard regressions in the D1 row mapping, JSON parsing, and locale/taxonomy filtering logic.
There was a problem hiding this comment.
Added in 49220f9 — packages/cloudflare/tests/sandbox/bridge-taxonomy.test.ts covers all three methods: capability enforcement (content:read doesn't grant access), the locale/taxonomy filter SQL + bound params, the pivot join on translation_group, and the row mapping (JSON collections/data parsing incl. malformed JSON, hierarchical int→bool, nullable label_singular/parent_id). 8 cases, all green.
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. |
github-actions
Bot
added
review/awaiting-author
78d9e63 to
203dc15
Compare
github-actions
Bot
added
review/needs-rereview
swissky
changed the title
Review follow-up: capability enforcement, locale/taxonomy filter SQL wiring, and D1 row mapping (JSON parsing, int→bool, nullable columns) for taxonomyList/taxonomyTerms/taxonomyEntryTerms.
ascorbic
added
bot:review
There was a problem hiding this comment.
This PR implements the new taxonomies:read plugin capability as approved in Discussion #1718, switching from the earlier approach of piggybacking on content:read. The new trust-contract surface is wired consistently across core, @emdash-cms/plugin-types, @emdash-cms/plugin-cli, @emdash-cms/registry-lexicons, @emdash-cms/admin, @emdash-cms/cloudflare, and @emdash-cms/sandbox-workerd, including the isomorphic DeclaredAccess round-trip and manifest schema changes.
What I checked:
- Capability gating in
PluginContextFactory, both sandbox bridges, and the workerd bridge handler — all correctly require the canonicaltaxonomies:read, with no legacy alias (as intended for a post-rename capability). - SQL safety in both bridges: Cloudflare uses parameterized
?placeholders and hardcoded table names; workerd uses Kysely with parameterized identifiers/values. No injection vector. - Locale filtering:
getAll,getTerms, andgetEntryTermsall accept and apply an optional locale, matching the repository semantics. - Trust-contract consistency:
plugin-typesconverters, coremanifest-schema,plugin-clischema,marketplaceauthor routes,adminlabels, and registry lexicons all includetaxonomies:read. - Tests: core integration tests cover the in-process API and capability separation; the Cloudflare bridge now has dedicated tests for row mapping, parameter binding, and gating.
No logic bugs, security issues, or regressions. Two non-blocking follow-ups would strengthen it: (1) the in-process createTaxonomyAccess.getAll should parse collections defensively to match the graceful handling in both bridges, and (2) the workerd bridge conformance suite only tests taxonomy/list — adding cases for taxonomy/terms and taxonomy/entryTerms would complete the bridge parity this PR otherwise achieves.
| label: row.label, | ||
| labelSingular: row.label_singular, | ||
| hierarchical: row.hierarchical === 1, | ||
| collections: row.collections ? (JSON.parse(row.collections) as string[]) : [], |
There was a problem hiding this comment.
[suggestion] JSON.parse(row.collections) will throw if the definition row somehow holds malformed JSON. The Cloudflare and workerd bridges already guard this with try/catch and fall back to [], so an in-process plugin could crash on the same data that a sandboxed plugin would degrade from. Wrap the parse the same way to keep the three surfaces consistent.
| collections: row.collections ? (JSON.parse(row.collections) as string[]) : [], | |
| collections: row.collections ? parseStringArray(row.collections) : [], |
(Sharing the existing parseCollectionsColumn-style helper from the bridges would work.)
There was a problem hiding this comment.
Fixed in 93522c7 — the parse now goes through parseCollectionsColumn(), which try/catches JSON.parse, returns [] for anything that isn't an array, and filters non-string items — mirroring the guards in the Cloudflare/workerd bridges.
| }) | ||
| .execute(); | ||
|
|
||
| const handler = makeHandler({ capabilities: ["taxonomies:read"] }); |
There was a problem hiding this comment.
[suggestion] The workerd conformance suite only exercises taxonomy/list. Because taxonomy/terms and taxonomy/entryTerms include their own join logic and row mapping (especially the content_taxonomies.taxonomy_id → taxonomies.translation_group join), add analogous tests for those two handlers to ensure workerd parity with the Cloudflare bridge.
There was a problem hiding this comment.
Added in 93522c7 — the workerd conformance suite now exercises all three methods: capability rejection for taxonomy/terms and taxonomy/entryTerms, plus happy-path cases including the locale-filtered entryTerms join.
github-actions
Bot
added
review/approved
Guard the in-process collections JSON parse like both bridges (an in-process plugin no longer crashes on malformed definition data), and extend the workerd conformance suite to taxonomy/terms and taxonomy/entryTerms: capability gating, locale filtering, data JSON parsing, and the pivot join on translation_group.
github-actions
Bot
added
review/needs-rereview
|
Both follow-ups from the approving review are in (93522c7): the in-process |
…bility (emdash-cms#1719) * feat(plugins): read-only taxonomy access via new taxonomies:read capability * test(cloudflare): cover PluginBridge taxonomy methods Review follow-up: capability enforcement, locale/taxonomy filter SQL wiring, and D1 row mapping (JSON parsing, int→bool, nullable columns) for taxonomyList/taxonomyTerms/taxonomyEntryTerms. * test/fix: review follow-ups for taxonomies:read Guard the in-process collections JSON parse like both bridges (an in-process plugin no longer crashes on malformed definition data), and extend the workerd conformance suite to taxonomy/terms and taxonomy/entryTerms: capability gating, locale filtering, data JSON parsing, and the pivot join on translation_group.
2 participants
What does this PR do?
Implements the proposal in Discussion #1718, updated per @ascorbic's feedback there: taxonomy reads are now gated behind a new, separate
taxonomies:readcapability instead of piggybacking oncontent:read. Plugins that declare it get a read-onlyctx.taxonomiesaccessor, available in-process and in both sandbox runners with identical shapes and gating:ctx.taxonomies.getAll(opts?)— taxonomy definitions (name, label,labelSingular,hierarchical,collections,locale)ctx.taxonomies.getTerms(taxonomy, opts?)— all terms of a taxonomy, ordered by label (flat; hierarchy reconstructable viaparentId, which carries the parent's locale-agnostic translation group)ctx.taxonomies.getEntryTerms(collection, entryId, opts?)— terms assigned to a content entry, optionally scoped to one taxonomyAll three accept an optional
locale; terms include theirdata(description etc.) andtranslationGroupfor i18n-aware plugins. There is no implication fromcontent:read/content:write— a plugin that reads content and its classification declares both. Taxonomy writes stay out of scope (admin-side invariants around translation groups and the pivot table shouldn't be re-exposed).Naming note:
taxonomies:read(plural) matches the existingtaxonomies:read/taxonomies:manageadmin permissions inrbac.ts. Happy to rename to the singulartaxonomy:readif preferred.Wiring (the full trust-contract surface for a new capability):
createTaxonomyAccessinplugins/context.ts(on top ofTaxonomyRepository), gated in the context factory;taxonomiesadded toCURRENT_PLUGIN_CAPABILITIESand the manifest Zod schema; newTaxonomyAccess/TaxonomyDefInfo/TaxonomyTermInfo/TaxonomyReadOptionstypes exported.taxonomies:readin thePluginCapabilityunion,taxonomies.readinDeclaredAccess, both converters (capabilitiesToDeclaredAccess/declaredAccessToCapabilities) stay total and isomorphic.taxonomiescategory inreleaseExtension.json(+ regenerated types), so registry records can carry the declaration.PluginBridgemethodstaxonomyList/taxonomyTerms/taxonomyEntryTerms, gated ontaxonomies:read, exposed asctx.taxonomiesin the sandbox wrapper.taxonomy/list|terms|entryTermsbridge-handler cases + wrapper accessor, behavior-parity with the Cloudflare bridge (canonical name checked — the new capability has no legacy alias).Discussion: #1718
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
Integration tests in
packages/core/tests/integration/plugins/capabilities.test.tscover the read surface plus the gating in both directions (content:readalone does NOT exposectx.taxonomies, andtaxonomies:readalone does not exposectx.content). The workerd bridge conformance suite asserts the same at the bridge level, and the plugin-types round-trip test now enumerates 2880 implication-closed capability states including the new one.pnpm typecheckandpnpm lint(0 diagnostics) pass across the workspace.