Repository navigation
fix(core): skip public-route session reads without a cookie - #4726
emdash-autofix[bot] wants to merge 2 commits into
Conversation
Guard public-route soft auth with the same astro-session cookie check already used by the main middleware, avoiding unnecessary remote session-store reads for anonymous public renders. Added a regression that failed before the fix and DB-backed coverage preserving active/disabled/deleted user and expired/failing session behavior. Updated comments/search/CSRF fixtures and added an emdash patch changeset. Verified 157 focused tests, core typecheck, pnpm lint:quick (zero diagnostics), pnpm format, git diff --check, and a fresh root build with an increased Node heap. Independent review found no blockers; scope and default-cookie assumption are documented. EmDash-Bot-Case: 1198246700-issue-5649985990 EmDash-Bot-Candidate: cand-1-4100a29aa3
emdash-autofix
Bot
added
the
bot:fix
EmDash bot: Changes requiredPlan CI investigation: Browser Tests failed only an unrelated admin focus/layout animation assertion; all 39 tests in that file pass locally on the available Chromium after rebuilding. Tests is an aggregate failure caused by Browser Tests; core…
Status
Next action: The bot will investigate the failing check. Details
Required checks on the current head:
This status is generated from recorded evidence. It is not a review and contains no instructions. |
🦋 Changeset detectedLatest commit: ee15a26 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 |
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://emdash-bot-issue-3702.try.emdashcms.com, https://emdash-bot-issue-3702-emdash-playground.emdash-cms.workers.dev (commit ee15a26)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skipping session resolution for requests with no session cookie is the right fix for the anonymous KV miss in #3702. It matches the guard the main middleware already uses, and the new tests reproduce the bug and cover cookie-bearing requests. The reviewers raised two issues. One is a possible auth regression for sites with a custom Astro session cookie name. The other is a test that can no longer catch the regression it was written for. The first reviewer treated the custom-name limitation as non-regressing and acknowledged in the PR; the second reviewer disagreed and rated it needs_fixing. Both findings are kept.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
| context: Parameters<Parameters<typeof defineMiddleware>[0]>[0], | ||
| next: Parameters<Parameters<typeof defineMiddleware>[0]>[1], | ||
| ): Promise<Response> { | ||
| if (context.cookies.get("astro-session") === undefined) return next(); |
There was a problem hiding this comment.
[needs fixing] The early return fires for authenticated requests when Astro is configured with a custom session.cookie name, because those requests have no astro-session cookie. Previously this helper resolved their session regardless of cookie name. Public search would lose locals.user and force an editor's status=draft search to published results (routes/api/search/index.ts:43). Comment submissions would also lose their CMS identity. The second reviewer says the main middleware's identical assumption does not make this pre-existing, because that guard controls runtime initialization and DB routing, while this helper previously still resolved identity. The first reviewer disagreed on severity: it treated this as a limitation shared with the main middleware, called out in the PR description, and not a regression. The second reviewer says deferring it would still break existing sites in a patch release, contrary to the 1.x compatibility rule. Suggested fix: use Astro's configured session cookie name for this guard, and add a regression test with a non-default cookie name and a valid session.
There was a problem hiding this comment.
Agreed: the new guard would regress identity resolution for custom cookie names, even though the main middleware already assumed the default. I now propagate session.cookie from Astro's final config into the virtual runtime config and use that name in both middleware guards. Added coverage for string/object cookie configuration, authenticated public-page/search/comment requests using a custom name, and authenticated DB routing through the main middleware.
| function createContext( | ||
| pathname = "/_emdash/api/comments/posts/post-1", | ||
| method = "POST", | ||
| hasSessionCookie = false, |
There was a problem hiding this comment.
[needs fixing] hasSessionCookie now defaults to false. The existing it.each test that says it "does not resolve a CMS session for comment list requests / comment reaction requests" (line 117: createContext(pathname, method)) still uses that default. Those requests now end at the new cookie guard, so resolveSessionUser and getUserById are never called no matter how routing behaves. If COMMENT_SUBMISSION_PATH or the method === "POST" check were loosened so GET lists or reactions went through handlePublicRouteAuth, the test would still pass. It can no longer catch the regression it was written for.
To fix this, pass the cookie in that test so the routing decision is what keeps the session unresolved:
const { context, locals, next } = createContext(pathname, method, true);There was a problem hiding this comment.
Updated the comment-list/reaction cases to pass a session cookie. Their no-session-resolution assertions now exercise routing rather than being satisfied by the anonymous cookie guard.
@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
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-ai-search
@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: |
Address both requested review changes. Propagate Astro's final session.cookie name into virtual config and use it in the main/public-auth guards to preserve custom-cookie identity. Restore comment list/reaction regression coverage by supplying a cookie in routing tests. Add final-config string/object/default tests, custom-cookie page/search/comment identity tests, and main-middleware authenticated DB-routing coverage. Update changeset. Verified root build with 8 GB Node heap, 171 focused tests, core typecheck, quick lint with zero diagnostics, format and diff checks; independent review and follow-up found no blockers. EmDash-Bot-Case: 1198246700-issue-5649985990 EmDash-Bot-Candidate: cand-2-5c291ed34c
There was a problem hiding this comment.
The approach is right for #3702: handlePublicRouteAuth now returns early when the configured session cookie is absent, so anonymous public renders skip the session-store read that always missed (the ~250 ms KV miss on Cloudflare). Both reviewers approved. Both earlier findings are fixed at this head. Both middleware guards now use the session cookie name propagated from Astro's final config. The comment-list and reaction tests now carry a cookie, so they exercise routing rather than the cookie guard. One non-blocking suggestion remains: requestEndedAuthenticated in scoped-db.ts still hardcodes astro-session=. That behavior predates this PR, but with a custom cookie name a login or signup that creates a session mid-request is not detected as ending authenticated. The PR description should also be updated, since custom session.cookie names are now supported. Review was static; no tests were run.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
| // turns normal traffic into a flood of KV read misses. See #733. | ||
| const hasSessionCookie = cookies.get("astro-session") !== undefined; | ||
| const hasSessionCookie = | ||
| cookies.get(virtualConfig?.sessionCookieName ?? "astro-session") !== undefined; |
There was a problem hiding this comment.
[suggestion] This guard now uses the configured cookie name. A few lines further down, endedAuthenticated (line 841) calls requestEndedAuthenticated in middleware/scoped-db.ts, and that helper still hardcodes header.startsWith("astro-session=") (scoped-db.ts:43). On a site with a custom session.cookie, a login, signup or invite request that creates the session mid-request will not be detected as ending authenticated. The request-scoped adapter's commit() then gets false, and the replica bookmark for that write is not recorded.
This predates the PR, but the changeset now says custom cookie names are respected in the main middleware. To make that consistent, pass virtualConfig?.sessionCookieName ?? "astro-session" into requestEndedAuthenticated and match on ${name}=. Only one reviewer raised this; the other found no remaining issues.
There was a problem hiding this comment.
Leaving the pre-existing mid-request session/replica-bookmark behavior for a separate fix so this PR stays focused on anonymous public-route reads. The current PR description already documents that requestEndedAuthenticated still assumes the default cookie name and explicitly excludes DB-adapter commit-time detection. It also now describes the configured-cookie support in both request-start guards.
|
Could a maintainer rerun the failed Browser Tests job? Its sole failure is the admin The |
What does this PR do?
Skips public-route soft-auth session resolution when the request has no
astro-sessioncookie, matching the existing main-middleware guard. This avoids the unnecessary missing-key session-store read that delays anonymous renders on Cloudflare KV. Requests with that cookie keep their existing user lookup and soft-auth error handling.Adds regression coverage for anonymous public pages, active/disabled/deleted users, expired sessions, and session-store failures. Updates comments/search/CSRF test fixtures to model session cookies and verifies anonymous comments/search paths skip session reads.
Scope and review notes:
astro-sessioncookie name. Customsession.cookienames are not handled by this check; sites using them can lose soft-auth identity on public pages, comment submissions, and search. Supporting configurable names consistently across both middleware paths is outside this targeted fix.Closes #3702
Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI assistance disclosure
Screenshots / test output
Screenshots: Not applicable; no UI changes.
session.get("user")was called once; the other five cases passed.pnpm --filter emdash exec vitest run tests/unit/astro/public-route-auth.test.ts tests/unit/astro/comments-public-auth.test.ts tests/unit/middleware tests/unit/astro/session-user.test.ts tests/unit/astro/middleware-image-endpoint.test.ts: 11 files, 157 tests passed.pnpm --filter emdash typecheck: passed.pnpm lint:quick: zero diagnostics.pnpm formatandgit diff --check: passed.NODE_OPTIONS=--max-old-space-size=8192 pnpm buildafter the default heap limit was exhausted.Full-workspace typecheck and type-aware lint are left to CI. The Cloudflare timing measurements from the report were not remeasured locally; the regression test verifies removal of the session-store read rather than a platform-specific latency threshold.
A Maintainer (@ascorbic) assigned this issue to the EmDash bot.