fix: add CLIENT_URL to CORS allowed origins for self-hosted deployments#859
fix: add CLIENT_URL to CORS allowed origins for self-hosted deployments#859ramadanomar wants to merge 1 commit intouseautumn:devfrom
Conversation
|
@ramadanomar is attempting to deploy a commit to the Autumn Team on Vercel. A member of the Team first needs to authorize it. |
cbcc3c5 to
6ba9b35
Compare
| afterEach(() => { | ||
| process.env.NODE_ENV = originalNodeEnv; | ||
| process.env.CLIENT_URL = originalClientUrl; | ||
| process.env.CHECKOUT_BASE_URL = originalCheckoutBaseUrl; | ||
| }); |
There was a problem hiding this comment.
afterEach sets env vars to the string "undefined" instead of unsetting them
In Node.js/Bun, assigning undefined to a process.env key converts it to the string "undefined", rather than deleting the key. If CLIENT_URL or CHECKOUT_BASE_URL were unset in the environment before the test suite ran (the common case in CI), originalClientUrl and originalCheckoutBaseUrl will both be undefined. After each self-hosted test, afterEach will leave process.env.CLIENT_URL === "undefined" (a truthy string), causing any subsequent test that doesn't explicitly delete the key to see a non-empty CLIENT_URL.
The existing test ordering means this doesn't currently break anything, but it is incorrect cleanup and will silently corrupt state if tests are reordered or new tests are added. The cleanup should use delete when the original value was absent:
| afterEach(() => { | |
| process.env.NODE_ENV = originalNodeEnv; | |
| process.env.CLIENT_URL = originalClientUrl; | |
| process.env.CHECKOUT_BASE_URL = originalCheckoutBaseUrl; | |
| }); | |
| afterEach(() => { | |
| process.env.NODE_ENV = originalNodeEnv; | |
| if (originalClientUrl === undefined) { | |
| delete process.env.CLIENT_URL; | |
| } else { | |
| process.env.CLIENT_URL = originalClientUrl; | |
| } | |
| if (originalCheckoutBaseUrl === undefined) { | |
| delete process.env.CHECKOUT_BASE_URL; | |
| } else { | |
| process.env.CHECKOUT_BASE_URL = originalCheckoutBaseUrl; | |
| } | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/tests/unit/corsOrigins.test.ts
Line: 9-13
Comment:
**`afterEach` sets env vars to the string `"undefined"` instead of unsetting them**
In Node.js/Bun, assigning `undefined` to a `process.env` key converts it to the **string** `"undefined"`, rather than deleting the key. If `CLIENT_URL` or `CHECKOUT_BASE_URL` were unset in the environment before the test suite ran (the common case in CI), `originalClientUrl` and `originalCheckoutBaseUrl` will both be `undefined`. After each self-hosted test, `afterEach` will leave `process.env.CLIENT_URL === "undefined"` (a truthy string), causing any subsequent test that doesn't explicitly `delete` the key to see a non-empty `CLIENT_URL`.
The existing test ordering means this doesn't currently break anything, but it is incorrect cleanup and will silently corrupt state if tests are reordered or new tests are added. The cleanup should use `delete` when the original value was absent:
```suggestion
afterEach(() => {
process.env.NODE_ENV = originalNodeEnv;
if (originalClientUrl === undefined) {
delete process.env.CLIENT_URL;
} else {
process.env.CLIENT_URL = originalClientUrl;
}
if (originalCheckoutBaseUrl === undefined) {
delete process.env.CHECKOUT_BASE_URL;
} else {
process.env.CHECKOUT_BASE_URL = originalCheckoutBaseUrl;
}
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pre-existing pattern — the original test already does process.env.NODE_ENV = originalNodeEnv the same way. Happy to fix all three in this PR if maintainers prefer, but wanted to keep the diff focused.
server/src/utils/corsOrigins.ts
Outdated
| // Allow CLIENT_URL and CHECKOUT_BASE_URL for self-hosted deployments | ||
| if (process.env.CLIENT_URL && origin === process.env.CLIENT_URL) { | ||
| return origin; | ||
| } | ||
| if ( | ||
| process.env.CHECKOUT_BASE_URL && | ||
| origin === process.env.CHECKOUT_BASE_URL | ||
| ) { | ||
| return origin; | ||
| } |
There was a problem hiding this comment.
Consider normalizing env var URLs to their origin before comparing
Browsers always send scheme://host[:port] — no path — in the Origin header. If CLIENT_URL or CHECKOUT_BASE_URL is configured with a path component (e.g. https://checkout.example.com/app), the exact string comparison will never match the browser's origin (https://checkout.example.com) and CORS will silently fail for that deployment.
Extracting just the origin makes the check robust against trailing slashes and paths:
// Allow CLIENT_URL and CHECKOUT_BASE_URL for self-hosted deployments
const clientOrigin = process.env.CLIENT_URL
? new URL(process.env.CLIENT_URL).origin
: null;
const checkoutOrigin = process.env.CHECKOUT_BASE_URL
? new URL(process.env.CHECKOUT_BASE_URL).origin
: null;
if (clientOrigin && origin === clientOrigin) return origin;
if (checkoutOrigin && origin === checkoutOrigin) return origin;The same normalisation should be applied in auth.ts before origins.push(...).
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/utils/corsOrigins.ts
Line: 23-32
Comment:
**Consider normalizing env var URLs to their origin before comparing**
Browsers always send `scheme://host[:port]` — no path — in the `Origin` header. If `CLIENT_URL` or `CHECKOUT_BASE_URL` is configured with a path component (e.g. `https://checkout.example.com/app`), the exact string comparison will never match the browser's origin (`https://checkout.example.com`) and CORS will silently fail for that deployment.
Extracting just the origin makes the check robust against trailing slashes and paths:
```ts
// Allow CLIENT_URL and CHECKOUT_BASE_URL for self-hosted deployments
const clientOrigin = process.env.CLIENT_URL
? new URL(process.env.CLIENT_URL).origin
: null;
const checkoutOrigin = process.env.CHECKOUT_BASE_URL
? new URL(process.env.CHECKOUT_BASE_URL).origin
: null;
if (clientOrigin && origin === clientOrigin) return origin;
if (checkoutOrigin && origin === checkoutOrigin) return origin;
```
The same normalisation should be applied in `auth.ts` before `origins.push(...)`.
How can I resolve this? If you propose a fix, please make it concise.Normalize CLIENT_URL and CHECKOUT_BASE_URL to strict HTTP(S) origins so browser Origin headers match even when env values include paths or trailing slashes. Reuse shared dashboard-origin checks in API auth and add focused unit coverage for malformed URL and env-restore edge cases.
8f6e89c to
e4748ea
Compare
Fixes #857
CLIENT_URL(andCHECKOUT_BASE_URL) were not included in the CORS allowlist or Better Auth'strustedOriginsin production, blocking self-hosted deployments on custom domains (e.g. Railway).Changes:
corsOrigins.ts— allowCLIENT_URLandCHECKOUT_BASE_URLas valid origins unconditionallyauth.ts— moveCLIENT_URLout of the dev-only guard sotrustedOriginsincludes it in production; addCHECKOUT_BASE_URLcorsOrigins.test.ts— add tests for self-hosted origin scenariosSummary by cubic
Always allow CLIENT_URL and CHECKOUT_BASE_URL for self-hosted deployments by adding them to CORS and Better Auth trusted origins. Normalize env URLs to strict HTTP(S) origins so paths/trailing slashes match browser Origin headers.
Written for commit e4748ea. Summary will update on new commits.
Greptile Summary
This PR fixes CORS and Better Auth
trustedOriginsmisconfigurations that blocked self-hosted deployments (e.g. on Railway) from communicating with the server when using custom domains set viaCLIENT_URLandCHECKOUT_BASE_URL.Key Changes:
corsOrigins.ts:CLIENT_URLandCHECKOUT_BASE_URLare now checked unconditionally inisAllowedOrigin, allowing custom-domain self-hosted deployments through CORS in production.auth.ts:CLIENT_URLis moved outside theNODE_ENV !== "production"guard so Better Auth'strustedOriginsincludes it in production;CHECKOUT_BASE_URLis also added.corsOrigins.test.ts: Newself-hosted env URLstest suite covers production allowance, simultaneous use of both env vars, rejection of unrelated origins, and rejection when env vars are unset. TheafterEachcleanup has a subtle issue where assigningundefinedto aprocess.envkey sets it to the string"undefined"rather than deleting it (see inline comment).Confidence Score: 4/5
corsOrigins.tsandauth.tsare straightforward and well-scoped: they add exact-match allowances for two operator-configured env vars. The only notable issues are (1) a testafterEachcleanup bug that doesn't break current tests but is semantically incorrect, and (2) a robustness gap if env vars include a path component. Neither blocks the fix from working in the typical deployment scenario.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming request with Origin header] --> B{isAllowedOrigin} B --> C{In ALLOWED_ORIGINS hardcoded list?} C -- Yes --> ALLOW[Return origin ✅] C -- No --> D{CLIENT_URL set AND origin matches?} D -- Yes --> ALLOW D -- No --> E{CHECKOUT_BASE_URL set AND origin matches?} E -- Yes --> ALLOW E -- No --> F{NODE_ENV !== production AND localhost:port pattern?} F -- Yes --> ALLOW F -- No --> REJECT[Return undefined ❌] G[Better Auth trustedOrigins IIFE] --> H[Hardcoded: localhost:3000, app/staging/*.useautumn.com] H --> I{CLIENT_URL set?} I -- Yes --> J[Push CLIENT_URL] I -- No --> K{CHECKOUT_BASE_URL set?} J --> K K -- Yes --> L[Push CHECKOUT_BASE_URL] K -- No --> M{NODE_ENV !== production?} L --> M M -- Yes --> N[Push localhost:3000–3010] M -- No --> O[Return origins array] N --> OLast reviewed commit: 6ba9b35