Repository navigation
Conversation
forwardCookies called c.header once per cookie. Hono keeps only the last value of a repeated header, so the three clearing cookies from signOut collapsed into better-auth.dont_remember and the browser kept the stale session cookies. Pass append: true so each Set-Cookie survives as its own header. The new integration test asserts that all three arrive with Max-Age=0, Path=/, HttpOnly and SameSite=Lax.
readSetCookie returned only the first Set-Cookie entry when headers arrived in array shape, and magicLinkHeaders returned an undefined headers object that surfaced later as a generic assertion failure. Both are test-only diagnostics.
Collaborator
|
There's no code in better-auth itself to do this? Or constants to re-use? |
logout copied Set-Cookie values from a Headers object onto the Hono context through a hand-rolled helper. Hono's c.header replaces rather than appends, which is how two of the three clearing cookies were dropped before #103. Ask for the signOut response itself with asResponse: true and append its set-cookie values to the redirect Response. No code in this repo then decides how cookies are forwarded, so a future handler cannot lose headers the same way. forwardCookies and its module are gone.
asResponse makes better-auth return APIErrors as error responses instead of throwing them (dispatch.mjs:193,249,258). logout never read the status, so a failed signOut redirected to /?message=Bye! while the session still stood. Return the signOut response and log the status when it is not ok.
mroderick
added a commit
that referenced
this pull request
Nov 12, 2026
logout copied Set-Cookie values from a Headers object onto the Hono context through a hand-rolled helper. Hono's c.header replaces rather than appends, which is how two of the three clearing cookies were dropped before #103. Ask for the signOut response itself with asResponse: true and append its set-cookie values to the redirect Response. No code in this repo then decides how cookies are forwarded, so a future handler cannot lose headers the same way. forwardCookies and its module are gone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Logout cleared only one of the three Better Auth cookies.
forwardCookiescalledc.header("Set-Cookie", cookie)once per cookie, and Hono keeps only the last value of a repeated header, so the three clearing cookies fromsignOutcollapsed intobetter-auth.dont_remember. The browser keptsession_tokenandsession_data. The helper now passes{ append: true }.The PR also adds
test/integration/cookie-attributes.test.js, which covers the cookie path that had no tests: the session cookie's attributes on magic-link verification, the authorize endpoint setting no cookie, and logout forwarding all three clearing cookies with their attributes. That coverage is the baseline the dependency review of #102 was blocked on.Review notes
Focus on the one-line production change in
src/app/utils/cookies.js. The risk to check is whether appending can duplicate aSet-Cookiethat something else on the response already emitted.forwardCookieshas one caller,src/app/handlers/logout.js, which forwards headers fromauth.api.signOutbefore returningc.redirect, so nothing else sets them on that response. The new test asserts that exactly three cookies arrive.An earlier draft of these tests assumed a
better-auth.statecookie exists on/oauth2/authorize. It does not: with the seeded planner client, every authorize path answers with a redirect and no cookie, because OAuth state travels in a signed query string. That test is replaced by one that pins the real contract.