Skip to content

fix[faustwp-core]: (#2313) add security flags to removeCookie()#2314

Open
latenighthackathon wants to merge 2 commits intowpengine:canaryfrom
latenighthackathon:fix/faustwp-core-cookie-removal
Open

fix[faustwp-core]: (#2313) add security flags to removeCookie()#2314
latenighthackathon wants to merge 2 commits intowpengine:canaryfrom
latenighthackathon:fix/faustwp-core-cookie-removal

Conversation

@latenighthackathon
Copy link
Copy Markdown
Contributor

@latenighthackathon latenighthackathon commented Apr 3, 2026

Description

removeCookie() in packages/faustwp-core/src/server/auth/cookie.ts expires the refresh token cookie with only expires: new Date(0), missing the path, sameSite, secure, and httpOnly flags that setCookie() uses when setting it.

Per RFC 6265, a Set-Cookie header without a path attribute defaults to the current request path. If the logout request comes from a different path than /, the browser won't match and delete the original cookie — it persists after what the application considers a successful logout.

Before

cookie.serialize(key, '', {
    expires: new Date(0),
})

After

cookie.serialize(key, '', {
    path: '/',
    sameSite: 'strict',
    secure: true,
    httpOnly: true,
    expires: new Date(0),
})

These flags match the attributes used in setRefreshToken() (token.ts:51-58).

Related Issues

Closes #2313

Testing

Confirm the issue (before the fix)

1. Verify removeCookie() is missing security flags:

sed -n '67,76p' packages/faustwp-core/src/server/auth/cookie.ts
# Expected on canary: only "expires: new Date(0)" — no path, sameSite, secure, or httpOnly

2. Compare with setCookie() which has the correct flags:

grep -A 7 'this.cookies.setCookie' packages/faustwp-core/src/server/auth/token.ts
# Expected: path: '/', sameSite: 'strict', secure: true, httpOnly: true
# These are the flags that removeCookie() should also have

Confirm the fix

3. Verify removeCookie() now includes all security flags:

sed -n '67,80p' packages/faustwp-core/src/server/auth/cookie.ts
# Expected: path, sameSite, secure, httpOnly all present before expires

4. Verify flags match the setCookie() call in token.ts:

# Both should use: path: '/', sameSite: 'strict', secure: true, httpOnly: true
diff <(grep -A 4 "path:" packages/faustwp-core/src/server/auth/cookie.ts) \
     <(grep -A 4 "path:" packages/faustwp-core/src/server/auth/token.ts)
# Expected: identical flag values (only expires/maxAge will differ)

Run the test suite

JS tests — confirms no regressions:

npm install && npm run build && npm run test

removeCookie() expires the refresh token cookie with only the
expires attribute, missing the path, sameSite, secure, and httpOnly
flags that setCookie() uses when setting it. Without a matching
path: '/', the browser may not delete the correct cookie on logout.

Add the same security attributes used in setRefreshToken() so the
browser correctly identifies and expires the target cookie.

Closes wpengine#2313
@latenighthackathon latenighthackathon requested a review from a team as a code owner April 3, 2026 04:39
@headless-platform-by-wp-engine
Copy link
Copy Markdown

Currently, we do not support the creation of preview deployments based on changes coming from forked repositories.
Learn more about preview environments in our documentation.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 3, 2026

🦋 Changeset detected

Latest commit: 8c2f67b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@faustwp/core Patch

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-project-automation github-project-automation bot moved this to 🆕 Backlog in Headless OSS Apr 3, 2026
@ahuseyn ahuseyn moved this from 🆕 Backlog to 👀 In review in Headless OSS Apr 3, 2026
@Fran-A-Dev Fran-A-Dev self-requested a review April 4, 2026 19:07
Copy link
Copy Markdown
Contributor

@Fran-A-Dev Fran-A-Dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I don't think the original cookie is ever set with domain attribute anywhere else. This looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

fix[faustwp-core]: removeCookie() missing path, sameSite, secure, and httpOnly flags

3 participants