Skip to content

Commit 5e8e10b

Browse files
icecrasher321claude
andcommitted
fix(urls): give base URLs the no-trailing-slash form their call sites assume
`getBaseUrl()` returned `NEXT_PUBLIC_APP_URL` as the operator spelled it, while almost every consumer builds `${base}/path`. A base configured with a trailing slash therefore produced a `//path` pathname that matches no route, and broke the `startsWith(`${base}/`)` prefix checks that decide whether a redirect target is our own — the OAuth authorize route rejected its own completion callback and fell back to the workspace page, so the desktop handoff never ran on those deployments. The previous commit fixed one such URL; this fixes the reason it was wrong, for the ~30 concatenation sites that share the assumption. `normalizeBaseUrl` now strips trailing slashes alongside the protocol it already added, which is the invariant SITE_URL has always documented. A path-prefixed base keeps its path. `getInternalApiBaseUrl` gets the same treatment, since its callers concatenate identically. `@sim/testing`'s urls mock is a hand-written mirror of this module, so it moves in step. `internal-api-base-url.test.ts` now unmocks the module it names — otherwise it asserts against that mirror and any drift between the two passes unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d9b50f1 commit 5e8e10b

4 files changed

Lines changed: 82 additions & 11 deletions

File tree

apps/sim/lib/core/utils/internal-api-base-url.test.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,15 @@
1212
* @vitest-environment node
1313
*/
1414
import { resetEnvMock, setEnv } from '@sim/testing'
15-
import { afterEach, describe, expect, it } from 'vitest'
15+
import { afterEach, describe, expect, it, vi } from 'vitest'
16+
17+
/**
18+
* `vitest.setup.ts` mocks this module globally with a hand-written mirror, so
19+
* without this the suite would assert against that mirror rather than the
20+
* function it names — and any drift between the two would pass unnoticed.
21+
*/
22+
vi.unmock('@/lib/core/utils/urls')
23+
1624
import { getInternalApiBaseUrl } from '@/lib/core/utils/urls'
1725

1826
const PUBLIC_URL = 'https://sim.ai'
@@ -33,6 +41,17 @@ describe('getInternalApiBaseUrl', () => {
3341
expect(getInternalApiBaseUrl()).toBe(LOOPBACK)
3442
})
3543

44+
/** Callers concatenate `${base}/api/...`, exactly as they do with getBaseUrl(). */
45+
it('strips a trailing slash from the internal URL', () => {
46+
setEnv({
47+
INTERNAL_API_BASE_URL: `${LOOPBACK}/`,
48+
NEXT_PUBLIC_APP_URL: PUBLIC_URL,
49+
DB_APP_NAME: 'sim',
50+
})
51+
52+
expect(getInternalApiBaseUrl()).toBe(LOOPBACK)
53+
})
54+
3655
it('IGNORES the internal URL on a Trigger.dev worker and falls back to the public URL', () => {
3756
setEnv({
3857
INTERNAL_API_BASE_URL: LOOPBACK,

apps/sim/lib/core/utils/urls.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,38 @@ describe('getBaseUrl', () => {
5656
expect(getBaseUrl()).toBe('https://app.example.com')
5757
})
5858

59+
/**
60+
* Call sites build `${getBaseUrl()}/path`, so a trailing slash would give them
61+
* a `//path` pathname that matches no route — and would break the
62+
* `startsWith(`${base}/`)` prefix checks that decide whether a redirect target
63+
* is our own, silently sending those redirects to their fallback instead.
64+
*/
65+
it('strips trailing slashes so concatenated paths stay single-slashed', () => {
66+
for (const configured of ['https://app.example.com/', 'https://app.example.com///']) {
67+
mockGetEnv.mockImplementation((key) =>
68+
key === 'NEXT_PUBLIC_APP_URL' ? configured : undefined
69+
)
70+
expect(getBaseUrl()).toBe('https://app.example.com')
71+
expect(new URL(`${getBaseUrl()}/desktop/connect/complete`).pathname).toBe(
72+
'/desktop/connect/complete'
73+
)
74+
}
75+
})
76+
77+
it('keeps the path of a path-prefixed base URL', () => {
78+
mockGetEnv.mockImplementation((key) =>
79+
key === 'NEXT_PUBLIC_APP_URL' ? 'https://example.com/sim/' : undefined
80+
)
81+
expect(getBaseUrl()).toBe('https://example.com/sim')
82+
})
83+
84+
it('adds the protocol and strips the trailing slash together', () => {
85+
mockGetEnv.mockImplementation((key) =>
86+
key === 'NEXT_PUBLIC_APP_URL' ? 'app.example.com/' : undefined
87+
)
88+
expect(getBaseUrl()).toBe('http://app.example.com')
89+
})
90+
5991
/**
6092
* Never guesses from `window.location.origin`: an opaque origin (a sandboxed
6193
* iframe) serializes to the truthy string `'null'`, which would silently

apps/sim/lib/core/utils/urls.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,22 @@ function hasHttpProtocol(url: string): boolean {
1212
return /^https?:\/\//i.test(url)
1313
}
1414

15+
/**
16+
* Brings a configured base URL to the no-trailing-slash form {@link SITE_URL}
17+
* documents: adds the protocol when the operator omitted it, then strips
18+
* trailing slashes.
19+
*
20+
* Call sites overwhelmingly build URLs as `${base}/path`, so a base spelled
21+
* `https://host/` gives every one of them a `//path` pathname that matches no
22+
* route, and breaks the `startsWith(`${base}/`)` prefix checks that decide
23+
* whether a redirect target is our own. Normalizing once here is what lets
24+
* those call sites stay simple instead of each defending against the operator's
25+
* spelling. A path-prefixed base (`https://host/sim/`) keeps its path.
26+
*/
1527
function normalizeBaseUrl(url: string): string {
16-
if (hasHttpProtocol(url)) {
17-
return url
18-
}
19-
2028
const protocol = isProd ? 'https://' : 'http://'
21-
return `${protocol}${url}`
29+
const withProtocol = hasHttpProtocol(url) ? url : `${protocol}${url}`
30+
return withProtocol.replace(/\/+$/, '')
2231
}
2332

2433
/**
@@ -89,7 +98,9 @@ export function getInternalApiBaseUrl(): string {
8998
)
9099
}
91100

92-
return internalBaseUrl
101+
// Protocol is proven present above, so this only trims trailing slashes —
102+
// callers concatenate `${base}/api/...` exactly as they do with getBaseUrl().
103+
return normalizeBaseUrl(internalBaseUrl)
93104
}
94105

95106
/**

packages/testing/src/mocks/urls.mock.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,25 @@ function hasHttpProtocol(url: string): boolean {
2424
return /^https?:\/\//i.test(url)
2525
}
2626

27+
/**
28+
* Mirrors the real module's `normalizeBaseUrl`: protocol-less values get
29+
* https:// under isProd, then trailing slashes are stripped so `${base}/path`
30+
* stays single-slashed at every call site.
31+
*/
32+
function normalizeBaseUrl(url: string): string {
33+
const protocol = envFlagsMock.isProd ? 'https://' : 'http://'
34+
const withProtocol = hasHttpProtocol(url) ? url : `${protocol}${url}`
35+
return withProtocol.replace(/\/+$/, '')
36+
}
37+
2738
function getBaseUrlImpl(): string {
2839
const baseUrl = readEnv('NEXT_PUBLIC_APP_URL')?.trim()
2940
if (!baseUrl) {
3041
throw new Error(
3142
'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly'
3243
)
3344
}
34-
// Mirrors the real module: protocol-less values get https:// under isProd.
35-
const protocol = envFlagsMock.isProd ? 'https://' : 'http://'
36-
return hasHttpProtocol(baseUrl) ? baseUrl : `${protocol}${baseUrl}`
45+
return normalizeBaseUrl(baseUrl)
3746
}
3847

3948
function getInternalApiBaseUrlImpl(): string {
@@ -47,7 +56,7 @@ function getInternalApiBaseUrlImpl(): string {
4756
'INTERNAL_API_BASE_URL must include protocol (http:// or https://), e.g. http://sim-app.default.svc.cluster.local:3000'
4857
)
4958
}
50-
return internalBaseUrl
59+
return normalizeBaseUrl(internalBaseUrl)
5160
}
5261

5362
function ensureAbsoluteUrlImpl(pathOrUrl: string): string {

0 commit comments

Comments
 (0)