Fix error on login page on prod. domain. #208
Conversation
📝 WalkthroughWalkthroughThe configuration function for the ApostropheCMS app was updated to introduce environment-aware settings, enhanced session and CSRF cookie security, proxy support, and custom CORS handling. Middleware was added to adjust headers for custom domains and API requests. Minor formatting changes and widget declaration adjustments were also made. Extensive new tests were added to verify environment-dependent configuration, cookie and CSRF settings, token extraction, and middleware behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressServer
participant Middleware
participant ApostropheCMS
Client->>ExpressServer: HTTP Request
ExpressServer->>Middleware: Apply proxy & CORS middleware
Middleware->>ExpressServer: Adjust headers, set CORS if API
ExpressServer->>Middleware: CSRF middleware
Middleware->>ExpressServer: Validate CSRF (with enhanced extraction)
ExpressServer->>ApostropheCMS: Route handling
ApostropheCMS-->>ExpressServer: Response
ExpressServer-->>Client: HTTP Response (with secure cookies)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: applies to **/*.e2e.{js,jsx} : e2e test files: relaxed import, ternary, and sorting rules...Applied to files:
📚 Learning: applies to **/*.test.{js,jsx} : test files have relaxed rules for function length, statements, extra...Applied to files:
📚 Learning: for the speed and function website project, the server is managed through docker containers rather t...Applied to files:
⏰ 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)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:5622f7f61e30c116a38c8637d20ede2a8247222ad697da9e9b8ef36c648070fd |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 291 MB |
| packages | 984 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |     |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/app.js(7 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: IhorMasechko
PR: speedandfunction/website#154
File: website/modules/case-studies-page/services/UrlService.js:102-116
Timestamp: 2025-06-09T16:52:54.510Z
Learning: When reviewing ApostropheCMS code, eslint-disable comments for `no-underscore-dangle` are often necessary and appropriate when accessing framework-provided properties like `_url`, as they conflict with general JavaScript linting rules but follow ApostropheCMS conventions.
📚 Learning: the project intentionally uses the `getenv` function for all environment variable access, including ...
Learnt from: killev
PR: speedandfunction/website#100
File: website/modules/@apostrophecms/form/index.js:227-227
Timestamp: 2025-05-19T20:40:04.914Z
Learning: The project intentionally uses the `getEnv` function for all environment variable access, including in placeholder values. This ensures that all required environment variables are present at application startup (fail-fast approach), even for features that might not be enabled.
Applied to files:
website/app.js
📚 Learning: when reviewing apostrophecms code, eslint-disable comments for `no-underscore-dangle` are often nece...
Learnt from: IhorMasechko
PR: speedandfunction/website#154
File: website/modules/case-studies-page/services/UrlService.js:102-116
Timestamp: 2025-06-09T16:52:54.510Z
Learning: When reviewing ApostropheCMS code, eslint-disable comments for `no-underscore-dangle` are often necessary and appropriate when accessing framework-provided properties like `_url`, as they conflict with general JavaScript linting rules but follow ApostropheCMS conventions.
Applied to files:
website/app.js
📚 Learning: in apostrophecms projects, even when a global page-type module removes the 'orphan' field, individua...
Learnt from: IhorMasechko
PR: speedandfunction/website#143
File: website/modules/default-page/index.js:24-24
Timestamp: 2025-06-02T16:04:13.069Z
Learning: In ApostropheCMS projects, even when a global page-type module removes the 'orphan' field, individual page modules (like default-page, home-page, case-studies-page) may still need explicit `remove: ['orphan']` for the removal to work correctly. The global improvement may not be sufficient in all cases.
Applied to files:
website/app.js
📚 Learning: applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maxi...
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Applied to files:
website/app.js
📚 Learning: applies to modules/asset/ui/src/swipers.js : modules/asset/ui/src/swipers.js: relaxed import rules...
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/swipers.js : modules/asset/ui/src/swipers.js: relaxed import rules
Applied to files:
website/app.js
📚 Learning: applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and...
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and style rules
Applied to files:
website/app.js
📚 Learning: applies to **/*.{js,jsx} : no duplicate object keys...
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : No duplicate object keys
Applied to files:
website/app.js
⏰ 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: e2e-tests
- GitHub Check: security-scan
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (3)
website/app.js (3)
30-37: LGTM! Proper session cookie configuration for productionThe session cookie configuration correctly addresses the CSRF issue by:
- Setting the domain with a leading dot (
.speedandfunction.com) to ensure cookies work across subdomains- Enabling secure flag for HTTPS-only transmission in production
- Using
sameSite: 'lax'which allows the cookie to be sent with top-level navigations (necessary for login)- Setting appropriate security flags (
httpOnly: true)
18-19: LGTM! Essential for proxy deploymentsEnabling
trustProxyis correct and necessary for Railway deployments or any deployment behind a reverse proxy. This ensures Express properly interprets X-Forwarded-* headers for client IP and protocol detection.
48-49: Good documentation of critical fixThe comment clearly identifies this as the critical fix for the CSRF issue with custom domains. This helps future maintainers understand the importance of this configuration.
website/app.js
Outdated
| const isProduction = process.env.NODE_ENV === 'production'; | ||
| const baseUrl = getEnv('BASE_URL') || (isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000'); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider fail-fast approach for BASE_URL environment variable
The current implementation uses getEnv('BASE_URL') || ... which provides a fallback, but this contradicts the project's fail-fast approach for required environment variables (as mentioned in the learnings). If BASE_URL is a required configuration, consider:
- const baseUrl = getEnv('BASE_URL') || (isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000');
+ const baseUrl = getEnv('BASE_URL', isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000');Or if BASE_URL should be optional:
- const baseUrl = getEnv('BASE_URL') || (isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000');
+ const baseUrl = process.env.BASE_URL || (isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000');Let me verify the getEnv function signature to understand if it supports default values:
🏁 Script executed:
#!/bin/bash
# Search for the getEnv function definition
rg -A 10 "function getEnv|const getEnv|exports\.getEnv" website/utils/env.jsLength of output: 295
Fix unreachable fallback due to getEnv signature
The helper getEnv(name) only accepts a single argument and throws if the variable is undefined, so
getEnv('BASE_URL') || …will never fall back. To provide a default you must use process.env for optional variables or split logic:
• If BASE_URL is optional (with a built-in fallback for all environments):
--- a/website/app.js
+++ b/website/app.js
@@ (around line 6)
- const baseUrl = getEnv('BASE_URL') || (isProduction
- ? 'https://speedandfunction.com'
- : 'http://localhost:3000');
+ const baseUrl = process.env.BASE_URL
+ || (isProduction
+ ? 'https://speedandfunction.com'
+ : 'http://localhost:3000');• If you want to fail-fast in production but allow a dev fallback:
--- a/website/app.js
+++ b/website/app.js
@@ (around line 6)
- const baseUrl = getEnv('BASE_URL') || (isProduction
- ? 'https://speedandfunction.com'
- : 'http://localhost:3000');
+ const baseUrl = isProduction
+ ? getEnv('BASE_URL') // will throw if unset
+ : (process.env.BASE_URL || 'http://localhost:3000');Please update lines 6–7 in website/app.js accordingly so fallbacks behave as intended.
🤖 Prompt for AI Agents
In website/app.js around lines 6 to 7, the current use of getEnv('BASE_URL')
prevents fallback because getEnv throws if the variable is undefined. To fix
this, replace getEnv('BASE_URL') with process.env.BASE_URL to allow optional
fallback, or restructure the logic to call getEnv only in production for
fail-fast behavior and use a fallback for development. Adjust the assignment of
baseUrl accordingly to ensure the fallback URL is reachable as intended.
website/app.js
Outdated
| value: (req) => { | ||
| return req.body && req.body._csrf || | ||
| req.query && req.query._csrf || | ||
| req.headers['x-csrf-token'] || | ||
| req.headers['x-xsrf-token'] || | ||
| req.headers['csrf-token']; | ||
| } |
There was a problem hiding this comment.
Fix potential issue in CSRF token extraction logic
The current implementation might return falsy values (empty string, 0) instead of checking all possible locations. Consider using nullish coalescing or explicit checks:
- value: (req) => {
- return req.body && req.body._csrf ||
- req.query && req.query._csrf ||
- req.headers['x-csrf-token'] ||
- req.headers['x-xsrf-token'] ||
- req.headers['csrf-token'];
- }
+ value: (req) => {
+ return req.body?._csrf ||
+ req.query?._csrf ||
+ req.headers['x-csrf-token'] ||
+ req.headers['x-xsrf-token'] ||
+ req.headers['csrf-token'];
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In website/app.js around lines 53 to 59, the CSRF token extraction logic uses ||
which can return falsy values like empty strings or 0 prematurely. Replace the
|| operators with nullish coalescing operators (??) or add explicit checks to
ensure that only null or undefined values cause fallback to the next location,
so all possible token sources are properly checked.
website/app.js
Outdated
| if (req.hostname === 'speedandfunction.com' || req.get('host') === 'speedandfunction.com') { | ||
| req.headers['x-forwarded-host'] = 'speedandfunction.com'; | ||
| req.headers['x-forwarded-proto'] = 'https'; | ||
| } | ||
|
|
||
| // Set CORS headers for API requests | ||
| const allowedOrigins = [ | ||
| 'https://speedandfunction.com', | ||
| 'https://apostrophe-cms-production.up.railway.app' | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making domain and CORS origins configurable
The hardcoded domains and allowed origins reduce maintainability. Consider using environment variables:
- if (req.hostname === 'speedandfunction.com' || req.get('host') === 'speedandfunction.com') {
- req.headers['x-forwarded-host'] = 'speedandfunction.com';
+ const customDomain = process.env.CUSTOM_DOMAIN || 'speedandfunction.com';
+ if (req.hostname === customDomain || req.get('host') === customDomain) {
+ req.headers['x-forwarded-host'] = customDomain;
req.headers['x-forwarded-proto'] = 'https';
}
// Set CORS headers for API requests
- const allowedOrigins = [
- 'https://speedandfunction.com',
- 'https://apostrophe-cms-production.up.railway.app'
- ];
+ const allowedOrigins = (process.env.ALLOWED_ORIGINS || 'https://speedandfunction.com,https://apostrophe-cms-production.up.railway.app').split(',');📝 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.
| if (req.hostname === 'speedandfunction.com' || req.get('host') === 'speedandfunction.com') { | |
| req.headers['x-forwarded-host'] = 'speedandfunction.com'; | |
| req.headers['x-forwarded-proto'] = 'https'; | |
| } | |
| // Set CORS headers for API requests | |
| const allowedOrigins = [ | |
| 'https://speedandfunction.com', | |
| 'https://apostrophe-cms-production.up.railway.app' | |
| ]; | |
| const customDomain = process.env.CUSTOM_DOMAIN || 'speedandfunction.com'; | |
| if (req.hostname === customDomain || req.get('host') === customDomain) { | |
| req.headers['x-forwarded-host'] = customDomain; | |
| req.headers['x-forwarded-proto'] = 'https'; | |
| } | |
| // Set CORS headers for API requests | |
| const allowedOrigins = (process.env.ALLOWED_ORIGINS || 'https://speedandfunction.com,https://apostrophe-cms-production.up.railway.app').split(','); |
🤖 Prompt for AI Agents
In website/app.js around lines 68 to 77, the domain and CORS allowed origins are
hardcoded, which reduces maintainability. Refactor the code to read these values
from environment variables instead of hardcoding them. Update the hostname
checks and the allowedOrigins array to use environment variables, providing
sensible defaults if needed, so the domains can be easily changed without
modifying the source code.
|
And Error on the site message: CSRF exception
apos-module-bundle.js:3 POST https://speedandfunction.com/api/v1/@apostrophecms/login/context?aposLocale=en 403 (Forbidden)