fix(cli): reject a SECRET that cannot be saved to .env and read back unchanged

.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

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<numbe
     return 1
   }
 
-  const issue = validateEnvPair(normalizedKey, value)
+  const issue =
+    validateEnvPair(normalizedKey, value) ?? (normalizedKey === 'SECRET' ? getUnsafeSecretReason(value) : undefined)
   if (issue) {
     logError(issue)
     return 1

src/cli/commands/setup.ts

@@ -3,6 +3,7 @@ import { randomBytes } from 'crypto'
 import { intro, outro, confirm, text, isCancel, cancel } from '@clack/prompts'
 
 import { ensureConfigBootstrap } from '../utils/bootstrap'
+import { getUnsafeSecretReason, stripQuotes } from '../utils/env-config'
 import { getProjectPath } from '../utils/paths'
 import { runStart } from './start'
 
@@ -29,14 +30,6 @@ class SetupCancelledError extends Error {
   }
 }
 
-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
-}
-
 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<string> => {
   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<string> => {
         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,16 +117,22 @@ const resolveSecret = async (assumeYes: boolean): Promise<string> => {
       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
     }
   }
 
   return randomBytes(64).toString('hex')
 }
 
 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

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`

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'))
+      })
+    }
+  })
 })

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')