fix(core): content API answers 500 NOT_CONFIGURED, not 401, when the runtime failed to init#2129
Conversation
… routes When the runtime fails to initialize, locals.emdash is never set and the auth middleware never resolves a user — even for a valid token. Content routes then answered 401 UNAUTHORIZED out of requirePerm before ever reaching their NOT_CONFIGURED check, blaming the caller's credentials for a server fault (and making clients treat it as non-retryable). Move the initialization check ahead of the permission check in the 13 content routes that had the order inverted, mirroring the schema routes which already do this correctly. Add regression tests pinning the 500 NOT_CONFIGURED response, and give the authz test mock a db so the permission paths stay exercised. Closes emdash-cms#2094
🦋 Changeset detectedLatest commit: edc2ad7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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
area/core
size/M
review/needs-review
@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: |
There was a problem hiding this comment.
Pull request overview
Fixes a correctness/diagnostics bug in the core content API: when the EmDash runtime fails to initialize (so locals.emdash and/or emdash.db are missing), affected content routes now return 500 NOT_CONFIGURED instead of incorrectly short-circuiting on auth and returning 401 UNAUTHORIZED.
Changes:
- Reordered checks in 13 content route handlers so the “runtime initialized / DB present” guard runs before
requirePerm(...). - Updated the authz unit-test runtime stub to include a
dbfield so existing authorization assertions still exercise the intended code path. - Added a regression-focused unit test block that pins the
NOT_CONFIGURED500 behavior for list/get/trash when the runtime is uninitialized.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/astro/routes/api/content/[collection]/index.ts | Moves runtime/handler presence checks ahead of permission checks for list/create. |
| packages/core/src/astro/routes/api/content/[collection]/[id].ts | Moves handleContentGet initialization check ahead of requirePerm for single-item GET. |
| packages/core/src/astro/routes/api/content/[collection]/trash.ts | Ensures handleContentListTrashed NOT_CONFIGURED check runs before authz. |
| packages/core/src/astro/routes/api/content/[collection]/authors.ts | Ensures authors endpoint returns NOT_CONFIGURED before authz when runtime missing. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/translations.ts | Moves translations handler presence check ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/revisions.ts | Moves revision list handler presence check ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/compare.ts | Moves compare handler presence check ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/duplicate.ts | Moves duplicate/get handler presence check ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/permanent.ts | Moves permanent-delete handler presence check ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/preview-url.ts | Moves DB presence check (emdash?.db) ahead of requirePerm. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/references/[relation]/children.ts | Moves requireDb ahead of requirePerm for GET/POST reference-children endpoints. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/references/[relation]/parents.ts | Moves requireDb ahead of requirePerm for reference-parents endpoint. |
| packages/core/src/astro/routes/api/content/[collection]/[id]/terms/[taxonomy].ts | Moves requireDb ahead of requirePerm for terms GET/POST to avoid unreachable NOT_CONFIGURED. |
| packages/core/tests/unit/astro/content-routes-authz.test.ts | Updates runtime stub to include db and adds regression tests for uninitialized runtime returning 500 NOT_CONFIGURED. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR addresses a real bug (#2094): when EmDash runtime initialization fails, locals.emdash is unset and the auth middleware leaves locals.user null. Content routes used to call requirePerm first, so they answered 401 UNAUTHORIZED instead of 500 NOT_CONFIGURED, misattributing a server fault to the caller and breaking retry semantics.
The approach is correct and minimal: move each route's existing init check (emdash?.handleXxx or requireDb(emdash?.db)) ahead of the permission check, matching the already-correct schema route ordering. No new concepts, no behavior change when the runtime is healthy, and the 13 files listed in the description are all updated consistently (I read each changed route and the test file).
The updated unit test keeps the existing 403 assertions valid by giving the mock a db, and adds a regression block that pins 500 NOT_CONFIGURED for list / get / trash, including with an authenticated admin.
The only AGENTS.md violation is a missing changeset. packages/core is the published emdash package, and AGENTS.md requires a changeset whenever a published package changes. Add a patch-level changeset for emdash describing the user-visible fix ( callers now receive 500 NOT_CONFIGURED instead of 401 UNAUTHORIZED when the runtime failed to initialize).
Headline: code changes are clean and the fix is right; add the required changeset before merge.
|
|
||
| export const GET: APIRoute = async ({ params, url, locals }) => { | ||
| const { emdash, user } = locals; | ||
| if (!emdash?.handleContentList) { |
There was a problem hiding this comment.
[needs fixing] This change — and the 12 sibling content route files changed by this PR — modifies the published emdash package (packages/core), but no changeset was added under .changeset/. AGENTS.md requires a changeset whenever a published package changes.
Add a patch-level changeset describing the observable fix, e.g. .changeset/content-api-init-500.md:
| if (!emdash?.handleContentList) { | |
| --- | |
| "emdash": patch | |
| --- | |
| Fixes the content API returning 401 UNAUTHORIZED instead of 500 NOT_CONFIGURED when EmDash failed to initialize. Clients can now distinguish a server-side initialization fault from an authentication failure and retry appropriately. |
| // Uninitialized runtime (#2094) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("uninitialized runtime returns NOT_CONFIGURED, not UNAUTHORIZED", () => { |
There was a problem hiding this comment.
[suggestion] The new regression block covers list / get / trash, but the same reordering also applies to routes that use requireDb (references/children, references/parents, terms/[taxonomy]) and other handler-presence routes (compare, duplicate, permanent, preview-url, revisions, translations, authors). The pattern is mechanical, so the existing tests already guard the main public surfaces; adding at least one requireDb-style route and one handler-presence write route to this block would fully pin the behavior across all changed paths.
| describe("uninitialized runtime returns NOT_CONFIGURED, not UNAUTHORIZED", () => { | |
| it("GET /content/:collection/:id/references/:relation/children → 500 NOT_CONFIGURED", async () => { | |
| const res = await getReferenceChildren( | |
| ctx({ | |
| user: null, | |
| emdash: uninitialized, | |
| params: { collection: "post", id: "p1", relation: "related" }, | |
| }), | |
| ); | |
| expect(res.status).toBe(500); | |
| }); | |
| it("POST /content/:collection/:id/duplicate → 500 NOT_CONFIGURED", async () => { | |
| const res = await postDuplicate( | |
| ctx({ | |
| user: null, | |
| emdash: uninitialized, | |
| params: { collection: "post", id: "p1" }, | |
| }), | |
| ); | |
| expect(res.status).toBe(500); | |
| }); |
github-actions
Bot
added
review/awaiting-author
- .changeset/content-api-init-500.md: patch changeset for the published emdash package, per AGENTS.md. - content-routes-authz.test.ts: extend the uninitialized-runtime block with a requireDb-gated route (references/parents) and a handler-presence write route (permanent DELETE), as suggested in review.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of PR #2129.
Approach: Still the right fix for #2094. When EmDash fails to initialize, the middleware leaves locals.emdash unset and locals.user null. Returning 500 NOT_CONFIGURED before the permission check is the correct behavior and matches the already-correct schema route ordering. Keeping the change mechanical—moving each route’s existing init guard earlier rather than introducing a new abstraction—is minimal and safe.
What changed since the last review: The only outstanding item was the missing changeset; that is now addressed by .changeset/content-api-init-500.md, a patch-level changeset for the emdash package. The route reordering and new regression tests are unchanged.
What I checked: I re-read the unified diff, the full changed route files, the authorization and requireDb/requirePerm helpers, the untouched sibling routes (publish, unpublish, restore, schedule, discard-draft) to confirm they already check init first, and the updated unit tests. All 13 listed routes now place the existing emdash?.handleXxx or requireDb(emdash?.db) guard ahead of any authz check. The mock runtime now carries db: {} so existing 403 assertions still exercise the authz path, and the new regression block pins 500 NOT_CONFIGURED for list / get / trash / a requireDb-gated route / a handler-presence write route, including for an authenticated admin.
Conclusion: The changeset requirement from the prior review is resolved. This is a clean, focused bug fix with solid regression coverage. LGTM.
github-actions
Bot
added
review/approved
What does this PR do?
When the EmDash runtime fails to initialize (broken DB binding, downstream failure),
locals.emdashis never set and the auth middleware never resolves a user — even for a valid token. The content routes ranrequirePerm(user, …)first, so they answered 401 UNAUTHORIZED and theNOT_CONFIGURED500 a few lines below was unreachable. A server fault got blamed on the caller's credentials, and clients correctly treating 4xx as non-retryable aborted instead of retrying.This PR moves each route's existing initialization check (handler-presence check or
requireDb) ahead of the permission check in the 13 content route files that had the order inverted, mirroring the schema routes which already do this correctly (requireDbbeforerequirePerminsrc/astro/routes/api/schema/index.ts). Routes that already checked init first (publish,unpublish,restore,schedule,discard-draft) are untouched. No new concepts — each route keeps its own check, just earlier.Tests: the authz mock runtime now carries a
db(a configured runtime) so all existing 403 assertions still exercise the authz path, and a new describe block pins the regression — uninitialized runtime → 500NOT_CONFIGUREDon list / get / trash / arequireDb-gated route (references/parents) / a handler-presence write route (permanent DELETE), including for an authenticated admin. A patch changeset is included.Closes #2094
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
vitest runoncontent-route-permissions,content-routes-authz(26 tests incl. 6 new), andcontent-route-locale: all pass. Prettier clean.