Conversation
…dation - Updated ChecksController and ConnectionsController to include organization ID in relevant endpoints, ensuring that connections are validated against the correct organization. - Introduced getConnectionForOrg method in ConnectionService to streamline organization-specific connection retrieval. - Enhanced VariablesController and QuestionnaireController to incorporate organization ID handling for connection variables and questionnaire responses. - Updated tests to cover new organization validation logic across affected controllers and services.
… proxy routes - Updated auth server configuration to consistently use BASE_URL for OAuth callbacks and cookie domain handling. - Removed references to AUTH_BASE_URL and BETTER_AUTH_URL, simplifying environment variable usage. - Deleted obsolete auth proxy routes from the app and portal, streamlining the authentication flow. - Enhanced comments and documentation to clarify the purpose of BASE_URL in relation to security and session management.
- Implemented custom middleware in main.ts to skip body parsing for /api/auth routes, ensuring better-auth can read the raw request stream for OAuth flows. - Updated auth.module.ts to disable the module's body parser to prevent conflicts with the custom middleware. - Configured auth.server.ts to skip the state cookie CSRF check for OAuth flows, improving compatibility with cross-origin setups.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR disables the OAuth state cookie CSRF check (skipStateCookieCheck: true) and changes body parsing/CORS configuration for auth routes — these are security-sensitive changes that warrant a human security review even though the rationale in the comments is reasonable.
Extended reasoning...
Overview
This PR modifies three files in the API authentication layer: auth.module.ts (adds disableTrustedOriginsCors and disableBodyParser flags to BetterAuthModule), auth.server.ts (adds skipStateCookieCheck: true to the account config), and main.ts (refactors body parsing to skip /api/auth routes so better-auth can read the raw request stream).
Security risks
The most security-sensitive change is skipStateCookieCheck: true in auth.server.ts. This disables the state cookie CSRF validation for OAuth flows. The comment explains the justification — in a cross-origin setup, the state cookie may not survive the OAuth redirect, and the DB-stored OAuth state parameter already provides CSRF protection. While this reasoning appears sound and mirrors better-auth's own oAuthProxy plugin approach, disabling a CSRF defense layer is inherently risky and should be verified by someone with deep knowledge of the OAuth flow and deployment architecture.
Additionally, disableTrustedOriginsCors: true delegates CORS entirely to the main.ts configuration (origin: true which allows all origins). A human should verify this is the intended CORS policy.
Level of scrutiny
This PR touches authentication and security middleware configuration — production-critical code paths that handle OAuth, CSRF protection, CORS, and request body parsing for the auth system. The changes are small in line count but high in security impact. This warrants careful human review, particularly from someone familiar with the OAuth flow and cross-origin cookie behavior in the deployment environment.
Other factors
The PR template is not filled out (no issue linked, all mandatory checkboxes unchecked, negative checklist items). There are no tests added. The body parsing refactor in main.ts is well-commented and logically sound, but the interaction between Express-level middleware and NestJS module middleware for auth routes is subtle enough to benefit from manual testing verification.
| // own SkipBodyParsingMiddleware to avoid conflicts. | ||
| disableBodyParser: true, | ||
| }), | ||
| ], | ||
| controllers: [AuthController], |
There was a problem hiding this comment.
🟡 Stale comment at main.ts:17: "The library will re-add body parsers after handling auth routes" is now incorrect. This PR adds disableBodyParser: true in auth.module.ts, which explicitly prevents the library from re-adding body parsers. Body parsing for non-auth routes is instead handled by the custom middleware in STEP 3. The comment should be updated to reflect the new approach.
Extended reasoning...
What the bug is
The comment at main.ts lines 16-17 reads:
// Disable body parser - required for better-auth NestJS integration
// The library will re-add body parsers after handling auth routes
This second line is now factually incorrect given the changes in this PR.
How this PR changes the behavior
This PR adds disableBodyParser: true to the BetterAuthModule.forRoot() configuration in auth.module.ts (line 25). The accompanying comment in that file correctly explains the intent: "Body parsing for non-auth routes is handled in main.ts with a custom middleware that skips /api/auth paths. Disable the module's own SkipBodyParsingMiddleware to avoid conflicts."
The disableBodyParser: true option explicitly tells the @thallesp/nestjs-better-auth module not to re-add body parsers. This directly contradicts the comment at main.ts:17 which claims "The library will re-add body parsers after handling auth routes."
Step-by-step proof
main.ts:19creates the NestJS app withbodyParser: false, disabling Express's default body parsing.main.ts:17claims the library (BetterAuthModule) will re-add body parsers after auth routes.- However,
auth.module.ts:25setsdisableBodyParser: truein the BetterAuthModule config, which prevents the module from adding any body parsing middleware. - Instead,
main.tslines 53-63 (STEP 3) add a custom Express middleware that appliesexpress.json()andexpress.urlencoded()to all routes except those starting with/api/auth. - So the actual flow is: body parsing is disabled globally, the library does NOT re-add it (contradicting the comment), and a custom middleware selectively applies body parsing to non-auth routes.
Impact
This is a documentation/comment issue only — no functional impact. The code itself works correctly. However, a developer reading the comment at main.ts:17 would get a false understanding of how body parsing works in the application, potentially leading to confusion when debugging body-parsing issues or making future changes.
How to fix
Update the comment at main.ts:16-17 to accurately describe the current approach, e.g.:
// Disable body parser - required for better-auth NestJS integration
// Custom middleware in STEP 3 applies body parsing selectively, skipping /api/auth routes
|
🎉 This PR is included in version 3.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist