Skip to content

Mariano/portal login 2#2275

Merged
Marfuen merged 3 commits intomainfrom
mariano/portal-login-2
Mar 10, 2026
Merged

Mariano/portal login 2#2275
Marfuen merged 3 commits intomainfrom
mariano/portal-login-2

Conversation

@Marfuen
Copy link
Contributor

@Marfuen Marfuen commented Mar 10, 2026

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes COMP-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

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):

  • Show screen recordings of the issue or feature.
  • Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

  • Add side-by-side screenshots of the original and updated change.
  • Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

Marfuen added 3 commits March 10, 2026 18:23
…dation

- Implemented in-memory rate limiting for authentication requests to mitigate brute force attacks, with stricter limits for sensitive endpoints.
- Added redirect URL validation to prevent open redirects, allowing only specified hosts.
- Improved logging for rate limit exceedances during development.
- Cleaned up old rate limit entries periodically to optimize memory usage.
- Updated the OTP form to remove unnecessary API_URL declaration.
- Changed references from AUTH_BASE_URL and BETTER_AUTH_URL to BASE_URL for consistency in cookie domain handling.
- Updated comments to clarify the purpose of BASE_URL in relation to OAuth callbacks and cookie domains.
@vercel
Copy link

vercel bot commented Mar 10, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
app Ready Ready Preview, Comment Mar 10, 2026 10:50pm
portal Ready Ready Preview, Comment Mar 10, 2026 10:50pm

Request Review

@Marfuen Marfuen merged commit f216975 into main Mar 10, 2026
7 of 9 checks passed
@Marfuen Marfuen deleted the mariano/portal-login-2 branch March 10, 2026 22:48
Comment on lines +139 to 145
// BASE_URL must contain the production domain (e.g., api.trycomp.ai)
// so getCookieDomain() can derive the cross-subdomain cookie domain.
baseURL:
process.env.BASE_URL ||
process.env.AUTH_BASE_URL ||
process.env.BETTER_AUTH_URL ||
'http://localhost:3000',
Copy link

Choose a reason for hiding this comment

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

🔴 Bug: BASE_URL semantic conflict breaks auth configuration. Throughout the codebase, BASE_URL refers to the API server URL (defaulting to http://localhost:3333 in .env.example and 5+ other locations), but betterAuth.baseURL historically expects the app URL (port 3000) for OAuth callback routing. Adding BASE_URL as highest priority silently changes baseURL from the app URL to the API URL, which can break OAuth sign-in flows. Additionally, the JSDoc at lines 128-134 still says to use the app URL (e.g., https://app.trycomp.ai), directly contradicting the new inline comment at line 139 that says api.trycomp.ai. Finally, the NestJS startup validator in better-auth.config.ts was not updated to check BASE_URL, so a deployment using only BASE_URL will crash at startup.

Extended reasoning...

What the bug is

This PR adds process.env.BASE_URL as the highest-priority value for better-auth's baseURL configuration (line 142), getCookieDomain() (line 22), and validateSecurityConfig() (line 112). However, BASE_URL has an established semantic meaning throughout the codebase as the API server URL, not the app URL.

Evidence of the semantic conflict

Multiple files in the codebase consistently use BASE_URL to mean the API URL at port 3333:

  • apps/api/.env.example line 1: BASE_URL="http://localhost:3333"
  • apps/api/src/trigger/integration-platform/run-connection-checks.ts:89: const apiUrl = process.env.BASE_URL || 'http://localhost:3333'
  • apps/api/src/trigger/integration-platform/sync-employees-schedule.ts:5: const API_BASE_URL = process.env.BASE_URL || 'http://localhost:3333'
  • apps/api/src/integration-platform/controllers/admin-integrations.controller.ts:124: uses BASE_URL for API callbacks at port 3333

Meanwhile, the default fallback in auth.server.ts line 145 is still http://localhost:3000 (the app port), and the JSDoc at lines 128-134 (not updated by this PR) still says: "Set AUTH_BASE_URL to the app's URL (e.g., http://localhost:3000 in dev)" and "In production, use the app's public URL (e.g., https://app.trycomp.ai)".

Step-by-step proof of the issue

  1. A developer clones the repo and uses apps/api/.env.example, which sets BASE_URL="http://localhost:3333".
  2. Before this PR, baseURL would resolve via AUTH_BASE_URL || BETTER_AUTH_URL || 'http://localhost:3000' — pointing to the app.
  3. After this PR, baseURL resolves via BASE_URL || AUTH_BASE_URL || BETTER_AUTH_URL || 'http://localhost:3000' — now http://localhost:3333 (the API) takes priority.
  4. better-auth uses baseURL to generate OAuth callback URLs. These now point to port 3333 (API) instead of port 3000 (app), bypassing the app's auth proxy.
  5. In production, if BASE_URL=https://api.trycomp.ai, OAuth callbacks would point to api.trycomp.ai instead of app.trycomp.ai, potentially breaking OAuth sign-in.

Note: The better-auth.config.ts NestJS validator (line 13 comment) does say "AUTH_BASE_URL should point to the API itself", suggesting an architectural shift is underway. However, this PR doesn't complete that migration — the JSDoc is stale, and the intent is ambiguous.

Documentation contradiction

The JSDoc at lines 128-134 says:

Set AUTH_BASE_URL to the app's URL (e.g., http://localhost:3000 in dev)
In production, use the app's public URL (e.g., https://app.trycomp.ai)

The new inline comment at lines 139-140 says:

BASE_URL must contain the production domain (e.g., api.trycomp.ai)

These directly contradict each other. The PR should update the JSDoc to reflect the new architecture.

NestJS validator inconsistency

The NestJS config validator in apps/api/src/config/better-auth.config.ts (line 24) checks process.env.AUTH_BASE_URL || process.env.BETTER_AUTH_URL but does NOT check process.env.BASE_URL. If a deployment sets only BASE_URL (following this PR's new guidance), the API will crash at startup with: "AUTH_BASE_URL or BETTER_AUTH_URL environment variable is required" — even though auth.server.ts would happily use BASE_URL.

How to fix

  1. Update the JSDoc at lines 128-134 to match the new inline comment, clarifying that baseURL should now point to the API.
  2. Update better-auth.config.ts line 24 to also check process.env.BASE_URL: const url = process.env.BASE_URL || process.env.AUTH_BASE_URL || process.env.BETTER_AUTH_URL.
  3. Verify that OAuth callback URLs work correctly when baseURL points to the API rather than the app, or document the architectural change.

Comment on lines +112 to 117
process.env.BASE_URL || process.env.AUTH_BASE_URL || process.env.BETTER_AUTH_URL || '';
if (baseUrl.includes('localhost')) {
console.warn(
'SECURITY WARNING: AUTH_BASE_URL contains "localhost" in production. ' +
'SECURITY WARNING: BASE_URL contains "localhost" in production. ' +
'This may cause issues with OAuth callbacks and cookies.',
);
Copy link

Choose a reason for hiding this comment

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

🟡 Nit: The security warning on line 116 always says BASE_URL contains "localhost" but the resolved value may actually come from AUTH_BASE_URL or BETTER_AUTH_URL via the fallback chain. A developer seeing this warning in production could check BASE_URL, find it unset, and be confused about the source of the issue. Consider dynamically identifying which env var matched.

Extended reasoning...

What the bug is

In validateSecurityConfig() (lines 112-117), the code resolves baseUrl using a fallback chain:

const baseUrl = process.env.BASE_URL || process.env.AUTH_BASE_URL || process.env.BETTER_AUTH_URL || '';

However, the warning message on line 116 is hardcoded to always reference BASE_URL:

SECURITY WARNING: BASE_URL contains "localhost" in production.

Step-by-step proof

  1. Suppose BASE_URL is not set in the environment.
  2. Suppose AUTH_BASE_URL is set to http://localhost:3000.
  3. The || chain resolves baseUrl to "http://localhost:3000" (from AUTH_BASE_URL).
  4. baseUrl.includes('localhost') evaluates to true.
  5. The warning fires: SECURITY WARNING: BASE_URL contains "localhost" in production.
  6. A developer reads this, checks BASE_URL, finds it is not set, and is confused — the actual culprit is AUTH_BASE_URL.

Why existing code doesn't prevent it

The warning message is a simple hardcoded string. There is no logic to determine which environment variable in the fallback chain actually provided the value that matched "localhost". Notably, the old code had a similar issue (it hardcoded AUTH_BASE_URL even when BETTER_AUTH_URL was the source), and this PR changed the label to BASE_URL without addressing the underlying pattern.

Impact

This is low-impact since the warning is a console.warn only visible to operators in a production misconfiguration scenario. The functional behavior (detecting localhost in production) is correct — only the env var name in the message is potentially misleading. However, the whole purpose of a security warning is to help operators quickly diagnose and fix configuration issues, and a misleading message works against that goal.

How to fix

Dynamically identify which env var matched, for example:

const envVarName = process.env.BASE_URL ? 'BASE_URL'
  : process.env.AUTH_BASE_URL ? 'AUTH_BASE_URL'
  : 'BETTER_AUTH_URL';
console.warn(`SECURITY WARNING: ${envVarName} contains "localhost" in production. ...`);

Alternatively, the message could mention all three env vars generically (e.g., "The resolved base URL contains localhost").

@claudfuen
Copy link
Contributor

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants