fix: Only check for origin header if user session cookie is present #640
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 6bdd325 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
WalkthroughShort-circuits session handling to first check for a session cookie and return early if absent, avoiding Origin/CSRF checks without a session. Removes duplicate cookie retrieval. Adds a patch changeset for @baseplate-dev/plugin-auth documenting the behavior change. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as UserSessionService
participant K as Cookie/Signer
participant DB as SessionStore
C->>S: getSessionInfoFromRequest(req)
alt No session cookie present
S-->>C: return undefined (skip Origin/CSRF)
else Session cookie present
S->>K: Unsign/parse cookie
alt Invalid/unsigned
S-->>C: return undefined
else Valid cookie
S->>DB: Fetch session by ID
alt Not found/expired
S-->>C: return undefined
else Found
S->>S: Optional renewal logic
S-->>C: return session info
end
end
end
note over S: Origin/CSRF checks are only relevant when a session cookie exists
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/tasty-years-tease.md (1)
5-5: Polish the summary phrasing and capitalization.Capitalize “Origin” (proper header name) and add a period for consistency with other entries.
-Only check for origin header if user session cookie is present +Only check the Origin header if a user session cookie is present.plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts (2)
175-176: Guard against missingreq.cookies(defensive).In practice Fastify’s cookie plugin ensures
req.cookiesexists, but using optional chaining makes this more robust if the plugin is not registered on a route.- const sessionCookieValue = req.cookies[cookieName]; + const sessionCookieValue = req.cookies?.[cookieName];
160-165: JSDoc does not match runtime behavior (undefined vs null, missing thrown error).The method returns
undefined(notnull) when no cookie is present and can also throwForbiddenErroron invalid Origin.- * @returns A promise that resolves to the authenticated user session information or null if the session is invalid. - * @throws {InvalidSessionError} If the session is invalid or expired. + * @returns A promise that resolves to the authenticated user session information or undefined if no session cookie is present. + * @throws {InvalidSessionError} If the session is invalid or expired. + * @throws {ForbiddenError} If the Origin header is invalid for state-changing requests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/tasty-years-tease.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)
.changeset/*.md
📄 CodeRabbit Inference Engine (CLAUDE.md)
When adding a new feature or changing an existing feature, add a new Changeset in the
.changeset/directory in the specified markdown format.
Files:
.changeset/tasty-years-tease.md
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Import UI components from the@baseplate-dev/ui-componentspackage as shown in the provided examples.
Use both standalone and React Hook Form controller variants for form components from@baseplate-dev/ui-componentsas appropriate.
If a particular interface or type is not exported, change the file so it is exported.
**/*.{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 exported
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
🪛 LanguageTool
.changeset/tasty-years-tease.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...dev/plugin-auth': patch --- Only check for origin header if user session cookie is...
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...patch --- Only check for origin header if user session cookie is present
(QB_NEW_EN)
🔇 Additional comments (3)
.changeset/tasty-years-tease.md (1)
1-3: Changeset metadata looks correct for a patch bump.Package scope and bump type are appropriate and aligned with the implementation.
plugins/plugin-auth/src/local-auth/core/generators/auth-module/templates/module/services/user-session.service.ts (2)
172-177: Early return when cookie is absent is the right call and matches the PR objective.This avoids unnecessary Origin/CSRF checks for unauthenticated requests while preserving checks when a session cookie is present. Nicely removes duplicated cookie lookups later in the method.
178-186: Confirm Fastifyreq.hostavailabilityI searched the repo and found only this single use of
req.host. There are no occurrences ofreq.hostnameorreq.headers.host, which suggests we haven’t standardized host extraction. Fastify’s Request API doesn’t guarantee ahostproperty across all versions, so this CSRF check may be unreliable in some setups.Please verify that your Fastify version populates
req.host. If not, consider normalizing the host value before the check:• Extract a reliable host string:
const host = req.host ?? req.hostname ?? req.headers.host; if ( (req.method !== 'GET' || req.headers.upgrade?.toLowerCase() === 'websocket') && req.method !== 'HEAD' && !verifyRequestOrigin(req, [host]) ) { throw new ForbiddenError('Invalid Origin header'); }This ensures the CSRF protection uses a valid host regardless of Fastify version.
kingston
deleted the
kingston/eng-854-dont-check-origin-header-if-no-auth-cookies-attached
branch
kingston
restored the
kingston/eng-854-dont-check-origin-header-if-no-auth-cookies-attached
branch
Summary by CodeRabbit