fix: Make sure we clear the cookie properly when session is invalid #669
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f667562 The changes in this PR will be included in the next version bump. 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 |
WalkthroughUpdates the auth module’s user-session service to clear the session cookie using configured COOKIE_OPTIONS when an InvalidSessionError occurs. Adds a changeset entry for a patch release of @baseplate-dev/plugin-auth. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant UserSessionService as UserSessionService
participant Cookie as Cookie (session)
participant Reply as Reply
Client->>Server: HTTP request with session cookie
Server->>UserSessionService: getSessionInfoFromRequest(req, reply)
UserSessionService->>Cookie: Read & validate session
alt Valid session
UserSessionService-->>Server: Session info
Server-->>Client: Response
else InvalidSessionError
Note over UserSessionService,Reply: Updated behavior
UserSessionService->>Reply: clearCookie(cookieName, COOKIE_OPTIONS)
UserSessionService-->>Server: Propagate/handle invalid session
Server-->>Client: Response (no session cookie)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
kingston
deleted the
kingston/eng-878-fix-cookie-clearing-logic-for-host-cookies
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts (1)
153-155: Set maxAge: 0 when clearing session cookieCOOKIE_OPTIONS contains a positive maxAge; override it to maxAge: 0 when clearing so the cookie is actually removed.
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts (lines ~153–155)
- context.cookieStore.clear(cookieName, COOKIE_OPTIONS); + context.cookieStore.clear(cookieName, { ...COOKIE_OPTIONS, maxAge: 0 });plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts (lines ~239–243)
- reply.clearCookie(cookieName, COOKIE_OPTIONS); + reply.clearCookie(cookieName, { ...COOKIE_OPTIONS, maxAge: 0 });examples/blog-with-auth/packages/backend/src/modules/accounts/services/user-session.service.ts (lines ~156–158)
- context.cookieStore.clear(cookieName, COOKIE_OPTIONS); + context.cookieStore.clear(cookieName, { ...COOKIE_OPTIONS, maxAge: 0 });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/violet-lemons-say.md(1 hunks)plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx}: TypeScript with strict type checking
Always include return types on top-level functions including React components (React.ReactElement)
Include absolute paths in import statements via tsconfig paths (@src/is the alias forsrc/)
If a particular interface or type is not exported, change the file so it is exportedIf a particular interface or type is not exported in a TypeScript file, modify the file to export it
Files:
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Node 16 module resolution - include file extensions in imports (
.js)
Files:
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx,js}: Sort imports by group: external libs first, then local imports
Use camelCase for variables/functions, PascalCase for types/classes
Order functions such that functions are placed below the variables/functions they use
We use the prefer using nullish coalescing operator (??) ESLint rule instead of a logical or (||), as it is a safer operator
Use console.info/warn/error instead of console.log
Files:
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Use kebab-case for file names
Files:
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When adding or changing a feature, add a new Changeset in
.changeset/with patch bump frontmatter and a description
Files:
.changeset/violet-lemons-say.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Verify Sync (blog-with-auth)
- GitHub Check: Lint
- GitHub Check: Test E2E
- GitHub Check: test
🔇 Additional comments (1)
.changeset/violet-lemons-say.md (1)
1-6: Changeset looks correct (patch bump + clear description).Good scope and message for a patch release.
| // clear the cookie if it's invalid | ||
| if (err instanceof InvalidSessionError && reply) { | ||
| reply.clearCookie(cookieName); | ||
| reply.clearCookie(cookieName, COOKIE_OPTIONS); | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
Bug: Passing COOKIE_OPTIONS to clearCookie likely prevents deletion (Max-Age overrides Expires).
COOKIE_OPTIONS includes a positive maxAge. In @fastify/cookie, clearCookie defaults maxAge to 0; overriding it with a positive value results in a Set-Cookie with Max-Age > 0, which takes precedence over Expires=1970 and can fail to clear the cookie. Fix by forcing maxAge: 0 when clearing.
Apply this diff:
- reply.clearCookie(cookieName, COOKIE_OPTIONS);
+ reply.clearCookie(cookieName, { ...COOKIE_OPTIONS, maxAge: 0 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // clear the cookie if it's invalid | |
| if (err instanceof InvalidSessionError && reply) { | |
| reply.clearCookie(cookieName); | |
| reply.clearCookie(cookieName, COOKIE_OPTIONS); | |
| } | |
| throw err; | |
| // clear the cookie if it's invalid | |
| if (err instanceof InvalidSessionError && reply) { | |
| reply.clearCookie(cookieName, { ...COOKIE_OPTIONS, maxAge: 0 }); | |
| } | |
| throw err; |
🤖 Prompt for AI Agents
In
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts
around lines 239 to 243, the call to reply.clearCookie passes COOKIE_OPTIONS
which contains a positive maxAge and thus prevents the cookie from being
deleted; change the clearCookie invocation to explicitly force deletion by
overriding COOKIE_OPTIONS with maxAge: 0 (and optionally expires: new Date(0))
so the Set-Cookie header will clear the cookie as intended.
Summary by CodeRabbit
Bug Fixes
Chores