Skip to content

Fix error on login page on prod. domain. #208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vasilyyaremchuk wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix error on login page on prod. domain. #208

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c211bc
Fix error on login page on prod. domain.
Aug 5, 2025
96aa47d
Fix linter errors.
Aug 5, 2025
074329a
Tests to cover changes in app.js
Aug 5, 2025
f7be780
Revert "Tests to cover changes in app.js"
Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 70 additions & 16 deletions website/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,21 @@ require('dotenv').config({ path: '../.env' });
const { getEnv } = require('./utils/env');

function createAposConfig() {
const isProduction = process.env.NODE_ENV === 'production';
const baseUrl = getEnv('BASE_URL') || (isProduction ? 'https://speedandfunction.com' : 'http://localhost:3000');
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 5, 2025

Choose a reason for hiding this comment

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

💡 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.js

Length 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.


return {
shortName: 'apostrophe-site',
baseUrl: getEnv('BASE_URL'),

baseUrl: baseUrl,
// Session configuration
modules: {
// Core modules configuration
'@apostrophecms/express': {
options: {
// Trust proxy for Railway deployment
trustProxy: true,

session: {
// If using Redis (recommended for production)
secret: getEnv('SESSION_SECRET'),
Expand All @@ -21,20 +27,70 @@ function createAposConfig() {
url: getEnv('REDIS_URI'),
},
},
cookie: {
// Set domain for production to work with custom domain
domain: isProduction ? '.speedandfunction.com' : undefined,
secure: isProduction,
sameSite: 'lax',
httpOnly: true,
maxAge: 24 * 60 * 60 * 1000, // 24 hours
},
},

csrf: {
cookie: {
key: '_csrf',
path: '/',
httpOnly: true,
secure: process.env.NODE_ENV === 'production',
secure: isProduction,
sameSite: 'lax',
maxAge: 3600,
// CRITICAL: Set domain for CSRF cookie to work with custom domain
domain: isProduction ? '.speedandfunction.com' : undefined,
},
// Additional CSRF options for better security
ignoreMethods: ['GET', 'HEAD', 'OPTIONS'],
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'];
}
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 5, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

},

// Add middleware to handle domain-specific headers
middleware: [
{
before: '@apostrophecms/csrf',
middleware: (req, res, next) => {
// Ensure proper headers for custom domain
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'
];
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 5, 2025

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.


const origin = req.headers.origin;
if (allowedOrigins.includes(origin)) {
res.setHeader('Access-Control-Allow-Origin', origin);
res.setHeader('Access-Control-Allow-Credentials', 'true');
res.setHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS');
res.setHeader('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept, Authorization, X-CSRF-Token, X-XSRF-TOKEN');
}

next();
}
}
]
},
},

// Make getEnv function available to templates
'@apostrophecms/template': {
options: {
Expand All @@ -43,13 +99,13 @@ function createAposConfig() {
},
},
},

// Add global data module
'global-data': {},

// Shared constants module
'@apostrophecms/shared-constants': {},

// Configure page types
'@apostrophecms/rich-text-widget': {},
'@apostrophecms/image-widget': {
Expand All @@ -63,7 +119,7 @@ function createAposConfig() {
className: 'bp-video-widget',
},
},

// Custom Widgets
'home-hero-widget': {},
'default-hero-widget': {},
Expand All @@ -79,11 +135,7 @@ function createAposConfig() {
'contact-widget': {},
'page-intro-widget': {},
'whitespace-widget': {},
/*
* 'links-buttons-widget': {},
* 'team-carousel-widget': {},
*/


// The main form module
'@apostrophecms/form': {},
// The form widget module, allowing editors to add forms to content areas
Expand All @@ -92,13 +144,14 @@ function createAposConfig() {
'@apostrophecms/form-text-field-widget': {},
'@apostrophecms/form-textarea-field-widget': {},
'@apostrophecms/form-checkboxes-field-widget': {},

// Custom Pieces
'team-members': {},
'testimonials': {},

// `asset` supports the project"s webpack build for client-side assets.
'asset': {},

// The project"s first custom page type.
'default-page': {},
'@apostrophecms/import-export': {},
Expand All @@ -121,4 +174,5 @@ function createAposConfig() {
if (require.main === module) {
apostrophe(createAposConfig());
}
module.exports = { createAposConfig };

module.exports = { createAposConfig };
Loading