diff --git a/.changeset/cli-secret-hash-roundtrip.md b/.changeset/cli-secret-hash-roundtrip.md new file mode 100644 index 000000000..b9edeafdf --- /dev/null +++ b/.changeset/cli-secret-hash-roundtrip.md @@ -0,0 +1,5 @@ +--- +"nostream": patch +--- + +fix(cli): reject a SECRET that cannot be saved to .env and read back unchanged diff --git a/src/cli/commands/config.ts b/src/cli/commands/config.ts index c851ff7ce..f3cad5048 100644 --- a/src/cli/commands/config.ts +++ b/src/cli/commands/config.ts @@ -18,6 +18,7 @@ import { maskSecretValue, readEnvValues, upsertEnvValue, + getUnsafeSecretReason, validateEnvPair, validateEnvValues, } from '../utils/env-config' @@ -219,7 +220,8 @@ export const runConfigEnvSet = async (key: string, value: string): Promise { - const trimmed = value.trim() - if ((trimmed.startsWith('"') && trimmed.endsWith('"')) || (trimmed.startsWith("'") && trimmed.endsWith("'"))) { - return trimmed.slice(1, -1).trim() - } - return trimmed -} - const parseEnvValue = (rawValue: string): string => { const trimmed = rawValue.trim() if (!trimmed) { @@ -91,6 +84,15 @@ const needsSecretReplacement = (secret: string | undefined): boolean => { const resolveSecret = async (assumeYes: boolean): Promise => { const inheritedSecret = process.env.SECRET?.trim() if (inheritedSecret && !needsSecretReplacement(inheritedSecret)) { + // It will be written to .env and read back by a loader that would not return it unchanged, which would give the + // relay a different secret, and so different derived keys, from the one this run started with. + const unsafeReason = getUnsafeSecretReason(inheritedSecret) + if (unsafeReason) { + throw new Error( + `${unsafeReason}. The SECRET in the environment cannot be saved to .env and read back unchanged: unset it, or use one without those characters.`, + ) + } + return stripQuotes(inheritedSecret) } @@ -106,13 +108,7 @@ const resolveSecret = async (assumeYes: boolean): Promise => { if (needsSecretReplacement(trimmed)) { return 'SECRET cannot be the default placeholder' } - if (input.includes('#')) { - return 'SECRET cannot contain "#"' - } - if (/[\r\n]/.test(input)) { - return 'SECRET cannot contain newlines' - } - return undefined + return getUnsafeSecretReason(input) }, }) @@ -121,9 +117,10 @@ const resolveSecret = async (assumeYes: boolean): Promise => { throw new SetupCancelledError() } - const trimmed = value.trim() - if (!needsSecretReplacement(trimmed)) { - return trimmed + // Without the quotes, as for an inherited secret: they are not part of the secret, and .env is read without them. + const unquoted = stripQuotes(value) + if (!needsSecretReplacement(unquoted)) { + return unquoted } } @@ -131,6 +128,11 @@ const resolveSecret = async (assumeYes: boolean): Promise => { } const upsertSecret = (content: string, secret: string): string => { + const unsafeReason = getUnsafeSecretReason(secret) + if (unsafeReason) { + throw new Error(unsafeReason) + } + const normalized = content.length > 0 ? content : '' const lines = normalized.split(/\r?\n/) let replaced = false diff --git a/src/cli/utils/env-config.ts b/src/cli/utils/env-config.ts index bc9817e1f..d9372aed1 100644 --- a/src/cli/utils/env-config.ts +++ b/src/cli/utils/env-config.ts @@ -135,6 +135,41 @@ const validateBoolean = (key: string, value: string): string | undefined => { return `${key} must be true or false` } +export const stripQuotes = (value: string): string => { + const trimmed = value.trim() + if ((trimmed.startsWith('"') && trimmed.endsWith('"')) || (trimmed.startsWith("'") && trimmed.endsWith("'"))) { + return trimmed.slice(1, -1).trim() + } + return trimmed +} + +/** + * Why a secret cannot be written to `.env` and read back unchanged, or undefined when it can. An unquoted `#` starts a + * comment for the loader, a newline ends the line, a `$` is expanded by Docker Compose, and a quote the loader cannot + * match changes what it reads. + */ +export const getUnsafeSecretReason = (secret: string): string | undefined => { + if (secret.includes('#')) { + return 'SECRET cannot contain "#"' + } + if (/[\r\n]/.test(secret)) { + return 'SECRET cannot contain newlines' + } + if (secret.includes('$')) { + // Docker Compose reads the same .env and expands `$NAME`, `${NAME}` and `$$`, which Node's loader leaves alone. + return 'SECRET cannot contain "$"' + } + if (/^["'`]/.test(stripQuotes(secret))) { + return 'SECRET cannot start with an unmatched quote' + } + const trimmed = secret.trim() + if (stripQuotes(trimmed) !== trimmed && stripQuotes(trimmed).includes(trimmed[0])) { + // The loader ends a quoted value at the first closing quote, so a quote inside would cut the secret short. + return 'SECRET cannot contain the quote it is wrapped in' + } + return undefined +} + export const validateEnvPair = (key: string, value: string): string | undefined => { if (!isSupportedEnvKey(key)) { return `${key} is not a supported environment setting` diff --git a/test/unit/cli/commands.spec.ts b/test/unit/cli/commands.spec.ts index adc4dfaf7..7e3b79031 100644 --- a/test/unit/cli/commands.spec.ts +++ b/test/unit/cli/commands.spec.ts @@ -1,6 +1,13 @@ import { expect } from 'chai' +import fs from 'fs' +import os from 'os' +import path from 'path' +import sinon from 'sinon' +import { parseEnv } from 'util' -import { isSupportedEnvKey, validateEnvPair } from '../../../src/cli/utils/env-config' +import { runConfigEnvSet } from '../../../src/cli/commands/config' +import { isSupportedEnvKey, validateEnvPair, validateEnvValues } from '../../../src/cli/utils/env-config' +import * as output from '../../../src/cli/utils/output' describe('cli env config helpers', () => { it('accepts supported env keys', () => { @@ -15,4 +22,56 @@ describe('cli env config helpers', () => { expect(validateEnvPair('READ_REPLICA_ENABLED', 'true')).to.equal(undefined) expect(validateEnvPair('READ_REPLICA_ENABLED', 'yes')).to.include('must be true or false') }) + + it('leaves a saved SECRET with a trailing comment valid, as .env.example has', () => { + expect(validateEnvPair('SECRET', 'change_me # Generate: openssl rand -hex 128')).to.equal(undefined) + expect(validateEnvValues({ SECRET: 'change_me # Generate: openssl rand -hex 128' })).to.deep.equal([]) + }) + + describe('runConfigEnvSet for SECRET', () => { + const originalCwd = process.cwd() + let tempDir: string + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'nostream-env-set-')) + process.chdir(tempDir) + }) + + afterEach(() => { + sinon.restore() + process.chdir(originalCwd) + fs.rmSync(tempDir, { recursive: true, force: true }) + }) + + const readSaved = () => parseEnv(fs.readFileSync(path.join(tempDir, '.env'), 'utf-8')).SECRET + + for (const [value, reason] of [ + ['pre$TOKEN', '"$"'], + ['${TOKEN}', '"$"'], + ['abc#def', '"#"'], + ['abc\ndef', 'newlines'], + ['"abc', 'unmatched quote'], + ['"abc"tail"', 'the quote it is wrapped in'], + ["'it's'", 'the quote it is wrapped in'], + ]) { + it(`refuses ${JSON.stringify(value)} and writes nothing`, async () => { + const error = sinon.stub(output, 'logError') + + expect(await runConfigEnvSet('SECRET', value)).to.equal(1) + + expect(error.firstCall.args[0]).to.include(reason) + expect(fs.existsSync(path.join(tempDir, '.env'))).to.equal(false) + }) + } + + for (const value of ['a=b', "it's-fine", 'with spaces', '"quoted value"', "'quoted'", 'ends-with-==']) { + it(`saves ${JSON.stringify(value)} so that Node reads it back as given`, async () => { + sinon.stub(output, 'logInfo') + + expect(await runConfigEnvSet('SECRET', value)).to.equal(0) + + expect(readSaved()).to.equal(value.replace(/^(["'])(.*)\1$/, '$2')) + }) + } + }) }) diff --git a/test/unit/cli/setup.spec.ts b/test/unit/cli/setup.spec.ts index 6e160c5c9..7d0b4014b 100644 --- a/test/unit/cli/setup.spec.ts +++ b/test/unit/cli/setup.spec.ts @@ -3,8 +3,9 @@ import fs from 'fs' import os from 'os' import path from 'path' import sinon from 'sinon' +import { parseEnv } from 'util' -const setupCommand: typeof import('../../../dist/src/cli/commands/setup.js') = require('../../../dist/src/cli/commands/setup.js') +const setupCommand: typeof import('../../../src/cli/commands/setup') = require('../../../src/cli/commands/setup') describe('runSetup', () => { const originalCwd = process.cwd() @@ -158,6 +159,135 @@ describe('runSetup', () => { expect(process.env.SECRET).to.equal('my-strong-interactive-secret') }) + describe('a secret that cannot be saved to .env and read back unchanged', () => { + const placeholderEnv = 'SECRET=change_me_to_something_long_and_random\nFOO=bar\n' + + beforeEach(() => { + fs.writeFileSync(path.join(tempDir, '.env'), placeholderEnv, 'utf-8') + }) + + const unsafe: Array<[string, string, string]> = [ + ['contains "#"', 'my-secret#suffix', 'SECRET cannot contain "#"'], + ['contains a newline', 'first\nsecond', 'SECRET cannot contain newlines'], + ['contains a variable reference that Docker Compose would expand', 'pre$TOKEN', 'SECRET cannot contain "$"'], + ['contains a braced variable reference', 'pre${TOKEN}post', 'SECRET cannot contain "$"'], + ['contains "$$", which Docker Compose turns into one "$"', 'pa$$word', 'SECRET cannot contain "$"'], + ['contains a lone "$"', 'cost$', 'SECRET cannot contain "$"'], + ['starts with an unmatched double quote', '"unterminated', 'SECRET cannot start with an unmatched quote'], + ['starts with an unmatched single quote', "'unterminated", 'SECRET cannot start with an unmatched quote'], + ] + + for (const [label, secret, reason] of unsafe) { + it(`rejects an inherited SECRET that ${label}, and leaves .env alone`, async () => { + process.env.SECRET = secret + + let error: Error | undefined + try { + await setupCommand.runSetup({ yes: true }) + } catch (caught) { + error = caught as Error + } + + expect(error?.message).to.contain(reason) + expect(error?.message).to.contain('unset it') + expect(readEnv()).to.equal(placeholderEnv) + }) + } + + it('rejects a secret the prompt hands back with a "#", even though the prompt validator should have refused it', async () => { + process.stdin.isTTY = true + sinon.stub(setupCommand.setupPrompts, 'text').resolves('typed#secret') + sinon.stub(setupCommand.setupPrompts, 'confirm').resolves(false as any) + + let error: Error | undefined + try { + await setupCommand.runSetup({ yes: false }) + } catch (caught) { + error = caught as Error + } + + expect(error?.message).to.equal('SECRET cannot contain "#"') + expect(readEnv()).to.equal(placeholderEnv) + }) + + it('saves a quoted secret typed at the prompt without its quotes, so every start has the same value', async () => { + process.stdin.isTTY = true + sinon.stub(setupCommand.setupPrompts, 'text').resolves('"quoted-secret"') + sinon.stub(setupCommand.setupPrompts, 'confirm').resolves(false as any) + + const code = await setupCommand.runSetup({ yes: false }) + + expect(code).to.equal(0) + expect(process.env.SECRET).to.equal('quoted-secret') + expect(readEnv()).to.include('SECRET=quoted-secret\n') + expect(parseEnv(readEnv()).SECRET).to.equal('quoted-secret') + + delete process.env.SECRET + expect(await setupCommand.runSetup({ yes: true })).to.equal(0) + expect(process.env.SECRET).to.equal('quoted-secret') + }) + + it('saves a single-quoted secret typed at the prompt without its quotes too', async () => { + process.stdin.isTTY = true + sinon.stub(setupCommand.setupPrompts, 'text').resolves("'quoted-secret'") + sinon.stub(setupCommand.setupPrompts, 'confirm').resolves(false as any) + + await setupCommand.runSetup({ yes: false }) + + expect(process.env.SECRET).to.equal('quoted-secret') + expect(parseEnv(readEnv()).SECRET).to.equal('quoted-secret') + }) + + it('refuses the same values in the interactive prompt', async () => { + process.stdin.isTTY = true + let validateFn: ((input: string) => string | undefined) | undefined + sinon.stub(setupCommand.setupPrompts, 'text').callsFake((opts: any) => { + validateFn = opts.validate + return Promise.resolve('my-strong-interactive-secret') + }) + sinon.stub(setupCommand.setupPrompts, 'confirm').resolves(false as any) + + await setupCommand.runSetup({ yes: false }) + + expect(validateFn!('"unterminated')).to.equal('SECRET cannot start with an unmatched quote') + expect(validateFn!("'unterminated")).to.equal('SECRET cannot start with an unmatched quote') + expect(validateFn!('"quoted-secret"')).to.equal(undefined) + expect(validateFn!('$TOKEN')).to.equal('SECRET cannot contain "$"') + expect(validateFn!("'$TOKEN'")).to.equal('SECRET cannot contain "$"') + }) + }) + + describe('a secret that is accepted', () => { + const safe = [ + 'plain-hex-0123456789abcdef', + 'a=b', + "it's-fine", + 'with space inside', + 'trailing-equals==', + '"quoted-secret"', + ] + + for (const secret of safe) { + it(`reads back from .env as the same value: ${secret}`, async () => { + fs.writeFileSync(path.join(tempDir, '.env'), 'SECRET=change_me_to_something_long_and_random\n', 'utf-8') + process.env.SECRET = secret + + const code = await setupCommand.runSetup({ yes: true }) + + // what the CLI hands the relay, and what a later start reads from .env with Node's own loader + const expected = secret.replace(/^"(.*)"$/, '$1') + expect(code).to.equal(0) + expect(process.env.SECRET).to.equal(expected) + expect(parseEnv(readEnv()).SECRET).to.equal(expected) + + // and what a later run of setup reads back through its own parser, with nothing inherited + delete process.env.SECRET + expect(await setupCommand.runSetup({ yes: true })).to.equal(0) + expect(process.env.SECRET).to.equal(expected) + }) + } + }) + it('replaces placeholder with generated secret if interactive input somehow resolves to placeholder', async () => { process.stdin.isTTY = true fs.writeFileSync(path.join(tempDir, '.env.example'), 'SECRET=change_me_to_something_long_and_random\n', 'utf-8')