Skip to content

Commit a28f49f

Browse files
fix(csp,setup): permit the socket origin the client actually uses; encode DSN passwords
Review round on #5964: - The socket reconnect was a CSP bug, not a URL bug. getSocketUrl() already falls back to localhost:3002 for a localhost page, but generateRuntimeCSP gated that same fallback on isDev — and compose runs NODE_ENV=production, so connect-src omitted ws://localhost:3002 and the browser blocked the handshake. Key the fallback on the app URL being localhost instead, mirroring getSocketUrl. Revert the compose NEXT_PUBLIC_SOCKET_URL default: an explicit value suppresses the page-origin fallback that reverse-proxied self-hosts depend on, and ':-' treats empty as unset so the documented escape hatch could not work either. LOCALHOST_HOSTNAMES is duplicated locally because csp.ts is loaded by next.config.ts before @/ aliases resolve. - Percent-encode the password when building the Postgres DSN. A user-supplied password containing @ : / # does not merely re-parse to the wrong host — it fails to parse as a URL at all, so a correct password surfaced as a connection failure. - Tell 'Postgres rejected this password' apart from 'Postgres never started'. On the keep-the-volume path a wrong password left a healthy server and the old generic 'container did not become healthy' error, which is the confusion this change set exists to remove. Adds a CSP regression test for the unset-socket-URL production case; verified it fails against the previous condition.
1 parent 54aadbf commit a28f49f

5 files changed

Lines changed: 120 additions & 16 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* The bundled docker-compose stack runs NODE_ENV=production, serves the app from
5+
* localhost, and leaves NEXT_PUBLIC_SOCKET_URL unset. getSocketUrl() falls back
6+
* to localhost:3002 for a localhost page regardless of NODE_ENV, so the CSP has
7+
* to permit that origin or the browser blocks the handshake and Socket.IO
8+
* retries forever. Its own file because vi.mock is hoisted per-module and the
9+
* sibling suite needs NEXT_PUBLIC_SOCKET_URL set.
10+
*/
11+
import { createEnvMock } from '@sim/testing'
12+
import { describe, expect, it, vi } from 'vitest'
13+
14+
vi.mock('@/lib/core/config/env', () =>
15+
createEnvMock({
16+
NEXT_PUBLIC_APP_URL: 'http://localhost:3000',
17+
NEXT_PUBLIC_SOCKET_URL: undefined,
18+
})
19+
)
20+
21+
vi.mock('@/lib/core/config/env-flags', () => ({
22+
isDev: false,
23+
isHosted: false,
24+
isReactGrabEnabled: false,
25+
}))
26+
27+
import { generateRuntimeCSP } from './csp'
28+
29+
describe('generateRuntimeCSP — socket fallback on a localhost origin', () => {
30+
it('permits the default socket origin when NEXT_PUBLIC_SOCKET_URL is unset', () => {
31+
const csp = generateRuntimeCSP()
32+
33+
expect(csp).toContain('http://localhost:3002')
34+
expect(csp).toContain('ws://localhost:3002')
35+
})
36+
37+
it('keeps the socket sources inside connect-src', () => {
38+
const connectSrc = generateRuntimeCSP()
39+
.split(';')
40+
.map((directive) => directive.trim())
41+
.find((directive) => directive.startsWith('connect-src'))
42+
43+
expect(connectSrc).toBeDefined()
44+
expect(connectSrc).toContain('ws://localhost:3002')
45+
})
46+
})

apps/sim/lib/core/security/csp.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,23 @@ function toWebSocketUrl(httpUrl: string): string {
1616
return httpUrl.replace('http://', 'ws://').replace('https://', 'wss://')
1717
}
1818

19+
/**
20+
* Kept in sync with LOCALHOST_HOSTNAMES in ../utils/urls by hand: this module is
21+
* loaded by next.config.ts before `@/` aliases resolve, so it cannot import from
22+
* there (see the note above).
23+
*/
24+
const LOCALHOST_HOSTNAMES = new Set(['localhost', '127.0.0.1', '[::1]', '::1'])
25+
26+
/** Mirrors getSocketUrl's localhost check — those origins fall back to DEFAULT_SOCKET_URL. */
27+
function isLocalhostUrl(url: string): boolean {
28+
if (!url) return false
29+
try {
30+
return LOCALHOST_HOSTNAMES.has(new URL(url).hostname)
31+
} catch {
32+
return false
33+
}
34+
}
35+
1936
function getHostnameFromUrl(url: string | undefined): string[] {
2037
if (!url) return []
2138
try {
@@ -208,7 +225,15 @@ export function buildCSPString(directives: CSPDirectives): string {
208225
export function generateRuntimeCSP(): string {
209226
const appUrl = getEnv('NEXT_PUBLIC_APP_URL') || ''
210227

211-
const socketUrl = getEnv('NEXT_PUBLIC_SOCKET_URL') || (isDev ? DEFAULT_SOCKET_URL : '')
228+
// Must permit whatever getSocketUrl() actually connects to, or the browser
229+
// blocks the handshake and Socket.IO retries forever. That helper falls back
230+
// to DEFAULT_SOCKET_URL whenever the page is served from localhost — which
231+
// includes a production build (docker compose sets NODE_ENV=production), so
232+
// keying this on isDev alone left the bundled stack with a CSP that forbade
233+
// its own realtime port. A non-localhost origin still resolves to the page
234+
// origin, which appUrl already covers, so nothing is loosened there.
235+
const socketUrl =
236+
getEnv('NEXT_PUBLIC_SOCKET_URL') || (isDev || isLocalhostUrl(appUrl) ? DEFAULT_SOCKET_URL : '')
212237
const socketWsUrl = socketUrl ? toWebSocketUrl(socketUrl) : ''
213238
const ollamaUrl = getEnv('OLLAMA_URL') || (isDev ? DEFAULT_OLLAMA_URL : '')
214239

docker-compose.local.yml

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,7 @@ services:
2626
- SIM_AGENT_API_URL=${SIM_AGENT_API_URL:-}
2727
- OLLAMA_URL=${OLLAMA_URL:-http://localhost:11434}
2828
- SOCKET_SERVER_URL=${SOCKET_SERVER_URL:-http://realtime:3002}
29-
# Published on 3002 with no proxy in front, so the browser must target it
30-
# directly; empty would fall back to the page origin, where /socket.io
31-
# answers 308 and the socket reconnects forever.
32-
- NEXT_PUBLIC_SOCKET_URL=${NEXT_PUBLIC_SOCKET_URL:-http://localhost:3002}
29+
- NEXT_PUBLIC_SOCKET_URL=${NEXT_PUBLIC_SOCKET_URL:-}
3330
depends_on:
3431
db:
3532
condition: service_healthy

docker-compose.prod.yml

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,12 @@ services:
3535
- SIM_AGENT_API_URL=${SIM_AGENT_API_URL:-}
3636
- OLLAMA_URL=${OLLAMA_URL:-http://localhost:11434}
3737
- SOCKET_SERVER_URL=${SOCKET_SERVER_URL:-http://realtime:3002}
38-
# NEXT_PUBLIC_SOCKET_URL is read by the browser. This stack publishes the
39-
# app on 3000 and realtime on 3002 with no reverse proxy between them, so
40-
# it must point at 3002 — left empty the client falls back to the page
41-
# origin, where /socket.io answers 308 instead of a handshake and the
42-
# socket reconnects forever. Override when a proxy fronts both on one
43-
# origin (set it to that origin, or empty to use the page origin), or when
44-
# realtime is elsewhere (e.g. wss://socket.example.com).
45-
- NEXT_PUBLIC_SOCKET_URL=${NEXT_PUBLIC_SOCKET_URL:-http://localhost:3002}
38+
# NEXT_PUBLIC_SOCKET_URL is read by the browser. Leave it unset for this
39+
# stack: the client already falls back to localhost:3002 for a localhost
40+
# page, and a proxied deployment needs the page-origin fallback that an
41+
# explicit value would suppress. Set it only when realtime is on a
42+
# different host:port (e.g. wss://socket.example.com).
43+
- NEXT_PUBLIC_SOCKET_URL=${NEXT_PUBLIC_SOCKET_URL:-}
4644
- ADMISSION_GATE_MAX_INFLIGHT=${ADMISSION_GATE_MAX_INFLIGHT:-500}
4745
depends_on:
4846
db:

scripts/setup/db.ts

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,18 @@ import { glyph, theme } from './theme.ts'
99

1010
const DEFAULT_DSN = 'postgresql://postgres:postgres@localhost:5432/simstudio'
1111

12+
/** Postgres' wire message when the password is wrong — a live server, not a dead one. */
13+
const AUTH_FAILURE = /password authentication failed/i
14+
15+
/**
16+
* Percent-encodes the password so characters that are structural in a URL
17+
* (`@`, `:`, `/`, `#`, `?`) can't re-parse the DSN into a different host — which
18+
* would fail a password that is actually correct.
19+
*/
20+
function buildDsn(password: string, hostPort: string | number): string {
21+
return `postgresql://postgres:${encodeURIComponent(password)}@localhost:${hostPort}/simstudio`
22+
}
23+
1224
export function docker(args: string[]): void {
1325
const result = spawnSync('docker', args, { encoding: 'utf8' })
1426
if (result.status !== 0) {
@@ -63,7 +75,9 @@ function inspectManagedContainer(): ManagedContainer | null {
6375

6476
return {
6577
running: running === 'true',
66-
dsn: `postgresql://postgres:${password}@localhost:${hostPort}/simstudio`,
78+
// Read back from the container env verbatim, so it may be a password the
79+
// user supplied for an existing volume — encode it like any other.
80+
dsn: buildDsn(password, hostPort),
6781
}
6882
}
6983

@@ -235,7 +249,9 @@ async function startManagedContainer(detection: Detection): Promise<string> {
235249
const password = volumeInitialized()
236250
? await resolveExistingVolume()
237251
: generateSecret().slice(0, 24)
238-
const dsn = `postgresql://postgres:${password}@localhost:${hostPort}/simstudio`
252+
// A user-supplied password can contain @ : / # — raw interpolation would
253+
// re-parse the DSN into a different host and fail a password that is correct.
254+
const dsn = buildDsn(password, hostPort)
239255
docker([
240256
'run',
241257
'-d',
@@ -255,9 +271,31 @@ async function startManagedContainer(detection: Detection): Promise<string> {
255271
])
256272
const spin = p.spinner()
257273
spin.start(`Starting ${DB_CONTAINER} container on :${hostPort}…`)
258-
const healthy = await waitFor(async () => (await pgProbe(dsn)).ok, 45_000, 1500)
274+
let lastError = ''
275+
const healthy = await waitFor(
276+
async () => {
277+
const probe = await pgProbe(dsn)
278+
if (!probe.ok) lastError = probe.error ?? ''
279+
return probe.ok
280+
},
281+
45_000,
282+
1500
283+
)
259284
if (!healthy) {
260285
spin.stop(`${glyph.fail} container did not become healthy`)
286+
// Postgres running and refusing the password is a different failure from
287+
// Postgres never starting, and it is the likely one on the keep-the-volume
288+
// path. Reporting it as "did not become healthy" is the exact confusion
289+
// this whole change set exists to remove.
290+
if (AUTH_FAILURE.test(lastError)) {
291+
throw new SetupError(
292+
`Postgres started, but rejected that password for the existing ${DB_VOLUME} volume.`,
293+
[
294+
're-run and enter the password the volume was created with',
295+
`or discard the old data: ${theme.command(`docker rm -f ${DB_CONTAINER} && docker volume rm ${DB_VOLUME}`)}`,
296+
]
297+
)
298+
}
261299
const logs = spawnSync('docker', ['logs', '--tail', '20', DB_CONTAINER], { encoding: 'utf8' })
262300
throw new SetupError(
263301
`the Postgres container failed to start. Last logs:\n${logs.stdout}${logs.stderr}`,

0 commit comments

Comments
 (0)