Skip to content

Commit a17b8a8

Browse files
authored
fix(sim-setup): stop doctor --fix from turning ${VAR} references into literals (#9868)
* fix(sim-setup): mirror env values as written so ${VAR} references survive doctor --fix * fix(sim-setup): leave $ references for a hand copy instead of mirroring them, drop the single-quote rewrite * fix(sim-setup): never generate a shared secret doctor can't copy, and report unwritable values instead of aborting * fix(sim-setup): give a placeholder shared secret the same fix as a missing one
1 parent 1a605bc commit a17b8a8

4 files changed

Lines changed: 199 additions & 31 deletions

File tree

‎packages/sim-setup/src/checks.test.ts‎

Lines changed: 99 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,21 @@
1-
import { describe, expect, it } from 'vitest'
2-
import { type CheckContext, runChecks } from './checks'
3-
import type { EnvFile, EnvTarget } from './env-files'
1+
import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
2+
import path from 'node:path'
3+
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
4+
5+
const { fakeRoot } = vi.hoisted(() => {
6+
const { mkdtempSync } = require('node:fs') as typeof import('node:fs')
7+
const { tmpdir } = require('node:os') as typeof import('node:os')
8+
const { join } = require('node:path') as typeof import('node:path')
9+
return { fakeRoot: mkdtempSync(join(tmpdir(), 'sim-setup-checks-')) }
10+
})
11+
12+
vi.mock('./context', async (importOriginal) => {
13+
const actual = await importOriginal<typeof import('./context')>()
14+
return { ...actual, SETUP_CONTEXT: { kind: 'source', root: fakeRoot } }
15+
})
16+
17+
import { type CheckContext, loadCheckContext, runChecks } from './checks'
18+
import { type EnvFile, type EnvTarget, parseEnv } from './env-files'
419

520
function envFile(target: EnvTarget, values: Record<string, string> = {}): EnvFile {
621
return {
@@ -9,6 +24,7 @@ function envFile(target: EnvTarget, values: Record<string, string> = {}): EnvFil
924
exists: target === 'root',
1025
content: '',
1126
vars: new Map(Object.entries(values)),
27+
verbatim: new Map(Object.entries(values)),
1228
}
1329
}
1430

@@ -65,3 +81,83 @@ describe('setup coherence checks', () => {
6581
})
6682
})
6783
})
84+
85+
describe('doctor --fix on env files', () => {
86+
const files = {
87+
sim: path.join(fakeRoot, 'apps/sim/.env'),
88+
realtime: path.join(fakeRoot, 'apps/realtime/.env'),
89+
db: path.join(fakeRoot, 'packages/db/.env'),
90+
}
91+
const referencingDsn = `postgresql://\${DB_USER}@localhost/sim`
92+
93+
function writeFiles(contents: Record<keyof typeof files, string>) {
94+
for (const [target, content] of Object.entries(contents)) {
95+
const file = files[target as keyof typeof files]
96+
mkdirSync(path.dirname(file), { recursive: true })
97+
writeFileSync(file, content)
98+
}
99+
}
100+
101+
async function fixConsistency() {
102+
const findings = await runChecks(loadCheckContext(false), ['consistency'])
103+
for (const finding of findings) finding.autofix?.()
104+
return findings.find((finding) => finding.message.startsWith('DATABASE_URL differs'))
105+
}
106+
107+
beforeEach(() => rmSync(fakeRoot, { recursive: true, force: true }))
108+
afterAll(() => rmSync(fakeRoot, { recursive: true, force: true }))
109+
110+
it.each([
111+
['a different URL', 'DATABASE_URL=postgresql://other@localhost/sim\n'],
112+
[
113+
'the reference escaped into a literal',
114+
`DATABASE_URL="postgresql://\\\${DB_USER}@localhost/sim"\n`,
115+
],
116+
])('reports a $VAR DATABASE_URL against %s and leaves it for a hand copy', async (_, dbFile) => {
117+
writeFiles({
118+
sim: `DB_USER=alice\nDATABASE_URL=${referencingDsn}\n`,
119+
realtime: '',
120+
db: dbFile,
121+
})
122+
123+
const finding = await fixConsistency()
124+
125+
expect(finding).toMatchObject({ status: 'fail', autofix: undefined })
126+
expect(readFileSync(files.db, 'utf8')).toBe(dbFile)
127+
})
128+
129+
it.each([
130+
['missing', 'DATABASE_URL=postgresql://localhost/sim\n'],
131+
['a placeholder', 'BETTER_AUTH_SECRET=your_auth_secret_here\n'],
132+
])(
133+
'never generates a shared secret it cannot copy when realtime has it %s',
134+
async (_, realtime) => {
135+
const secret = `BETTER_AUTH_SECRET=\${AUTH_SECRET_BASE}0123456789abcdef0123456789abcdef`
136+
writeFiles({ sim: `${secret}\n`, realtime, db: '' })
137+
138+
for (const finding of await runChecks(loadCheckContext(false), ['schema']))
139+
finding.autofix?.()
140+
141+
const after = parseEnv(readFileSync(files.realtime, 'utf8')).get('BETTER_AUTH_SECRET')
142+
expect(after).toBe(parseEnv(realtime).get('BETTER_AUTH_SECRET'))
143+
}
144+
)
145+
146+
it('reports a value it cannot write instead of aborting the other fixes', async () => {
147+
const unwritable = 'DATABASE_URL="postgresql://u:p\\$x\\y@localhost/sim"\n'
148+
const dbFile = 'DATABASE_URL=postgresql://other@localhost/sim\n'
149+
writeFiles({ sim: unwritable, realtime: '', db: dbFile })
150+
151+
const finding = await fixConsistency()
152+
153+
expect(finding).toMatchObject({ status: 'fail', autofix: undefined })
154+
expect(readFileSync(files.db, 'utf8')).toBe(dbFile)
155+
})
156+
157+
it('accepts a $VAR DATABASE_URL written the same way in both files', async () => {
158+
const line = `DATABASE_URL=${referencingDsn}\n`
159+
writeFiles({ sim: `DB_USER=alice\n${line}`, realtime: '', db: line })
160+
161+
expect(await fixConsistency()).toBeUndefined()
162+
})
163+
})

‎packages/sim-setup/src/checks.ts‎

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { portOpen } from './detect'
1717
import {
1818
type EnvFile,
1919
type EnvTarget,
20+
envValueRefusal,
2021
generateSecret,
2122
isPlaceholder,
2223
isTruthy,
@@ -30,6 +31,35 @@ import {
3031
import { httpHealth, pgProbe, redisPing } from './probes'
3132
import { FLAG_TWINS, LOGIN_PROVIDERS } from './twins'
3233

34+
/**
35+
* Whether `key` is written with an unescaped `$`, a reference Next, Bun, and
36+
* Compose expand against the file it sits in. Copied to another file it can
37+
* resolve differently, and copied as a literal it stops expanding, so doctor
38+
* leaves such values for the user to mirror by hand.
39+
*/
40+
function holdsReference(file: EnvFile, key: string): boolean {
41+
return /(^|[^\\])\$/.test(file.verbatim.get(key) ?? '')
42+
}
43+
44+
/**
45+
* Whether `key` holds the same thing in both files. Parsed values hide the one
46+
* difference that matters for `$`: `${VAR}` expands while `"\${VAR}"` does not.
47+
*/
48+
function sameEnvValue(a: EnvFile, b: EnvFile, key: string): boolean {
49+
if (a.vars.get(key) !== b.vars.get(key)) return false
50+
if (!holdsReference(a, key) && !holdsReference(b, key)) return true
51+
return a.verbatim.get(key) === b.verbatim.get(key)
52+
}
53+
54+
/** Why doctor cannot copy `key` out of `file` for the user, or `undefined` when it can. */
55+
function mirrorBlocker(file: EnvFile, key: string, value: string): string | undefined {
56+
if (holdsReference(file, key)) {
57+
return 'it contains a $ reference, so copy it and the variables it uses by hand'
58+
}
59+
const refusal = envValueRefusal(value)
60+
return refusal && `${refusal}, so copy it by hand`
61+
}
62+
3363
export type CheckGroup = 'files' | 'schema' | 'consistency' | 'coherence' | 'live'
3464
export type CheckStatus = 'pass' | 'warn' | 'fail' | 'skip'
3565

@@ -144,7 +174,9 @@ function checkFiles(ctx: CheckContext): Finding[] {
144174
const value = ctx.env.sim.vars.get(key)
145175
// Skip placeholders so seeding never copies an .env.example stub
146176
// into the new file (matches autofixForMissing).
147-
if (value && !isPlaceholder(value)) values[key] = value
177+
if (value && !isPlaceholder(value) && !mirrorBlocker(ctx.env.sim, key, value)) {
178+
values[key] = value
179+
}
148180
}
149181
writeEnvValues(target, values)
150182
}
@@ -166,6 +198,8 @@ function autofixForMissing(
166198
simValue &&
167199
!isPlaceholder(simValue)
168200
) {
201+
// A generated secret would differ from the app's, so a value doctor can't copy gets no fix.
202+
if (mirrorBlocker(ctx.env.sim, key, simValue)) return undefined
169203
return () => writeEnvValues(target, { [key]: simValue })
170204
}
171205
if (MIN_32_KEYS.has(key)) {
@@ -195,16 +229,16 @@ function checkSchema(ctx: CheckContext): Finding[] {
195229
continue
196230
}
197231
if (isPlaceholder(value)) {
232+
// Same fix as a missing key: a shared secret is mirrored from the app, never regenerated.
233+
const autofix = MIN_32_KEYS.has(key) ? autofixForMissing(ctx, target, key) : undefined
198234
findings.push({
199235
group: 'schema',
200236
status: production ? 'fail' : 'warn',
201237
message: `${rel(file)}: ${key} still has the .env.example placeholder`,
202-
fix: MIN_32_KEYS.has(key)
203-
? 'doctor --fix generates a real value'
238+
fix: autofix
239+
? 'doctor --fix sets a real value'
204240
: `replace the placeholder in ${rel(file)}`,
205-
autofix: MIN_32_KEYS.has(key)
206-
? () => writeEnvValues(target, { [key]: generateSecret() })
207-
: undefined,
241+
autofix,
208242
})
209243
continue
210244
}
@@ -262,28 +296,34 @@ function checkConsistency(ctx: CheckContext): Finding[] {
262296
const simValue = sim.vars.get(key)
263297
const realtimeValue = realtime.vars.get(key)
264298
if (!simValue || !realtimeValue) continue
265-
if (simValue !== realtimeValue) {
299+
if (!sameEnvValue(sim, realtime, key)) {
300+
const blocker = mirrorBlocker(sim, key, simValue)
266301
findings.push({
267302
group: 'consistency',
268303
status: 'fail',
269304
message: `${key} differs between apps/sim/.env and apps/realtime/.env`,
270-
fix: 'doctor --fix mirrors the apps/sim/.env value',
271-
autofix: () => writeEnvValues('realtime', { [key]: simValue }),
305+
fix: blocker
306+
? `mirror the apps/sim/.env value: ${blocker}`
307+
: 'doctor --fix mirrors the apps/sim/.env value',
308+
autofix: blocker ? undefined : () => writeEnvValues('realtime', { [key]: simValue }),
272309
})
273310
}
274311
}
275312
}
276313
if (sim.exists && db.exists) {
277314
const simDsn = sim.vars.get('DATABASE_URL')
278315
const dbDsn = db.vars.get('DATABASE_URL')
279-
if (simDsn && dbDsn && simDsn !== dbDsn) {
316+
if (simDsn && dbDsn && !sameEnvValue(sim, db, 'DATABASE_URL')) {
317+
const blocker = mirrorBlocker(sim, 'DATABASE_URL', simDsn)
280318
findings.push({
281319
group: 'consistency',
282320
status: 'fail',
283321
message:
284322
'DATABASE_URL differs between apps/sim/.env and packages/db/.env — migrations would hit a different database',
285-
fix: 'doctor --fix mirrors the apps/sim/.env value',
286-
autofix: () => writeEnvValues('db', { DATABASE_URL: simDsn }),
323+
fix: blocker
324+
? `mirror the apps/sim/.env value: ${blocker}`
325+
: 'doctor --fix mirrors the apps/sim/.env value',
326+
autofix: blocker ? undefined : () => writeEnvValues('db', { DATABASE_URL: simDsn }),
287327
})
288328
}
289329
}

‎packages/sim-setup/src/env-files.ts‎

Lines changed: 42 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,22 @@ export interface EnvFile {
5858
exists: boolean
5959
content: string
6060
vars: Map<string, string>
61+
/** Each value as written in the file, quotes and escapes included. */
62+
verbatim: Map<string, string>
6163
}
6264

6365
const LINE_RE = /^\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=\s*(.*)$/
6466

67+
/** The value as written: a quoted value up to its closing quote, else up to an inline comment. */
68+
function verbatimValue(raw: string): string {
69+
const trimmed = raw.trim()
70+
if (trimmed.startsWith('"') || trimmed.startsWith("'")) {
71+
const end = trimmed.indexOf(trimmed[0], 1)
72+
return end === -1 ? trimmed : trimmed.slice(0, end + 1)
73+
}
74+
return trimmed.replace(/\s+#.*$/, '').trim()
75+
}
76+
6577
function parseValue(raw: string): string {
6678
const trimmed = raw.trim()
6779
if (trimmed.startsWith('"') || trimmed.startsWith("'")) {
@@ -86,7 +98,12 @@ export function readEnvFile(target: EnvTarget): EnvFile {
8698
const filePath = ENV_PATHS[target]
8799
const exists = existsSync(filePath)
88100
const content = exists ? readFileSync(filePath, 'utf8') : ''
89-
return { target, path: filePath, exists, content, vars: parseEnv(content) }
101+
const verbatim = new Map<string, string>()
102+
for (const line of content.split('\n')) {
103+
const match = LINE_RE.exec(line)
104+
if (match) verbatim.set(match[1], verbatimValue(match[2]))
105+
}
106+
return { target, path: filePath, exists, content, vars: parseEnv(content), verbatim }
90107
}
91108

92109
/**
@@ -96,24 +113,34 @@ export function readEnvFile(target: EnvTarget): EnvFile {
96113
* expand `$NAME` even inside single quotes, and the one spelling all four read as a
97114
* literal `$` is `\$` inside double quotes.
98115
*/
99-
function formatEnvValue(key: string, value: string): string {
100-
if (/[\r\n]/.test(value)) {
101-
throw new Error(`${key} cannot be written to an environment file: it contains a line break`)
102-
}
116+
function spellEnvValue(value: string): { text: string } | { refusal: string } {
117+
if (/[\r\n]/.test(value)) return { refusal: 'it contains a line break' }
103118
if (value.includes('$')) {
104119
if (/["\\`]/.test(value)) {
105-
throw new Error(
106-
`${key} cannot be written to an environment file: it mixes $ with a double quote, backslash, or backtick`
107-
)
120+
return { refusal: 'it mixes $ with a double quote, backslash, or backtick' }
108121
}
109-
return `"${value.replaceAll('$', '\\$')}"`
122+
return { text: `"${value.replaceAll('$', '\\$')}"` }
123+
}
124+
if (value === value.trim() && !value.includes('#') && !/^["'`]/.test(value)) {
125+
return { text: value }
126+
}
127+
if (!value.includes("'")) return { text: `'${value}'` }
128+
if (!/["\\`]/.test(value)) return { text: `"${value}"` }
129+
return { refusal: 'it mixes quotes with a # or edge whitespace' }
130+
}
131+
132+
/** Why `upsertEnv` cannot write `value` so every loader reads it back unchanged, if it cannot. */
133+
export function envValueRefusal(value: string): string | undefined {
134+
const spelling = spellEnvValue(value)
135+
return 'refusal' in spelling ? spelling.refusal : undefined
136+
}
137+
138+
function formatEnvValue(key: string, value: string): string {
139+
const spelling = spellEnvValue(value)
140+
if ('refusal' in spelling) {
141+
throw new Error(`${key} cannot be written to an environment file: ${spelling.refusal}`)
110142
}
111-
if (value === value.trim() && !value.includes('#') && !/^["'`]/.test(value)) return value
112-
if (!value.includes("'")) return `'${value}'`
113-
if (!/["\\`]/.test(value)) return `"${value}"`
114-
throw new Error(
115-
`${key} cannot be written to an environment file: it mixes quotes with a # or edge whitespace`
116-
)
143+
return spelling.text
117144
}
118145

119146
/**

‎packages/sim-setup/src/lifecycle.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,9 +595,14 @@ async function reset(install: Install | null): Promise<void> {
595595
// With the daemon down nothing managed is visible, so a reset would archive the
596596
// env files while the volumes they unlock survive.
597597
if (install?.kind !== 'k8s' && executableExists('docker') && !dockerReachable()) {
598+
// With no install detected, a Kubernetes install whose context isn't current looks the same.
599+
const hints = [`start Docker, then re-run ${theme.command('npx sim-setup reset')}`]
600+
if (!install) {
601+
hints.push(`if Sim runs on Kubernetes, switch kubectl to its cluster's context and re-run`)
602+
}
598603
throw new SetupError(
599604
"Docker isn't reachable, so the managed containers and volumes can't be removed. Your env files were left in place.",
600-
[`start Docker, then re-run ${theme.command('npx sim-setup reset')}`]
605+
hints
601606
)
602607
}
603608
const ok = await p.confirm({

0 commit comments

Comments
 (0)