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.
Three paths had no coverage: the GET verb on /logout, a Set-Cookie already present on the response before forwardCookies appends to it, and a signOut that returns no headers at all. Each fails when its target is removed.
till
reviewed
Nov 11, 2026
| import { forwardCookies } from "../../src/app/utils/cookies.js"; | ||
| import { authorizeParams } from "../helpers/oauth-flow.js"; | ||
|
|
||
| const SESSION_COOKIE = "better-auth.session_token"; |
Collaborator
There was a problem hiding this comment.
I had a q about this in the other PR, I think if we really have to do this, then we should probably unify this?
olleolleolle
approved these changes
Nov 11, 2026
olleolleolle
left a comment
olleolleolle
left a comment
Collaborator
There was a problem hiding this comment.
Good to get coverage!
Collaborator
Author
|
Not needed in this form. Two of the three tests pin code #107 deleted, and the other is salvaged into #108.
#107 merged into |
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.
Three logout paths had no coverage. This adds a test for each, stacked on #103 because it extends the test file that PR introduces.
GET /logoutclears the same three cookies asPOST /logout. The handler is registered for both verbs, and only POST was covered.forwardCookieskeeps aSet-Cookiethat is already on the response instead of replacing it. This pins the append behaviour that fix: forward every set-cookie header on logout #103 relies on, and documents the one residual risk the security review raised: appending preserves anything already set.logoutredirects whensignOutreturns no headers, covering theif (status.headers)guard insrc/app/handlers/logout.js.Each test fails when its target is removed. Dropping
{ append: true }fails the first two; removing thestatus.headersguard fails the third.Review notes
Nothing risky here: test-only, no production change. The third test builds a minimal
authobject withgetSessionandsignOutand passes it tocreateApp, which already accepts an injected instance, so no module mocking is involved.Base is
test/cookie-attributes, so this merges after #103.