diff --git a/src/__tests__/env-commands.test.ts b/src/__tests__/env-commands.test.ts index 3d04b98de..8aaff3f77 100644 --- a/src/__tests__/env-commands.test.ts +++ b/src/__tests__/env-commands.test.ts @@ -191,6 +191,20 @@ scope: 'user', // ─── envAdd ────────────────────────────────────────────── describe('envAdd', () => { + it('refuses a key that would not survive the round trip into env.sh', async () => { + // `generateEnvFile` drops any key that is not a shell identifier, so + // accepting one here would write a variable that never reaches the + // member's shell — and `FOO;cmd` would run `cmd` there if it did. Better + // to reject it at the point the user can still see the mistake. + await envAdd('bad key', 'v', {}); + + expect(log.error).toHaveBeenCalledWith(expect.stringContaining('bad key')); + // Nothing written, and no env.yaml is created just to hold nothing. + const envYamlPath = path.join(repoPath, 'env', 'env.yaml'); + expect(await fse.pathExists(envYamlPath)).toBe(false); + expect(log.success).not.toHaveBeenCalled(); + }); + it('should add a new variable locally and show push hint', async () => { await envAdd('NEW_VAR', 'new_value', {}); diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index be1676e8a..afdd96c75 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -396,6 +396,35 @@ scope: 'user', const content = handler.generateEnvFile([]); expect(content).toBe('\n'); }); + + it('should drop keys that are not valid shell identifiers', () => { + // A key is interpolated raw into `export =...`, so anything that is + // not an identifier either breaks the line or runs as shell code. The + // whole variable is dropped, not the line rewritten: `parseEnvFile` skips + // such a line anyway, so emitting it would put a variable in env.sh that + // the CLI can never read back. + const content = handler.generateEnvFile([ + { key: 'GOOD_KEY', value: 'ok' }, + { key: 'bad key', value: 'oops' }, + { key: 'FOO;touch /tmp/pwned', value: 'y' }, + { key: '$(whoami)', value: 'w' }, + { key: 'A=B', value: 'z' }, + { key: '9LEADING', value: 'n' }, + ]); + + expect(content).toBe("export GOOD_KEY='ok'\n"); + }); + + it('should keep keys that are valid shell identifiers', () => { + // The guard must not narrow what a legitimate team repo can express: + // digits and underscores after the first character are all valid. + const content = handler.generateEnvFile([ + { key: '_PRIVATE', value: 'a' }, + { key: 'A1_b2', value: 'b' }, + ]); + + expect(content).toBe("export _PRIVATE='a'\nexport A1_b2='b'\n"); + }); }); // ─── pullItem ──────────────────────────────────────────── diff --git a/src/env-commands.ts b/src/env-commands.ts index e65c8e15a..611732af5 100644 --- a/src/env-commands.ts +++ b/src/env-commands.ts @@ -4,7 +4,7 @@ import { requireInit, detectProjectConfig } from './config.js'; import { pullRepo } from './utils/git.js'; import { ensureDir, readFileSafe, writeFile, pathExists } from './utils/fs.js'; import { log, spinner } from './utils/logger.js'; -import { EnvHandler, maskEnvValue } from './resources/env.js'; +import { EnvHandler, maskEnvValue, ENV_KEY_RE } from './resources/env.js'; import type { GlobalOptions } from './types.js'; import { isSelfMode } from './types.js'; @@ -59,6 +59,18 @@ export async function envAdd( value: string, options: GlobalOptions & { description?: string }, ): Promise { + // env.sh is generated as `export =...` and sourced by every member, so a + // key that is not a shell identifier either breaks that line or runs as code. + // `generateEnvFile` drops such keys, which would make this command report + // success for a variable that never reaches anyone's shell — reject it here, + // where the user still sees what they typed. + if (!ENV_KEY_RE.test(key)) { + log.error( + `Invalid env variable name "${key}": use letters, digits and underscores, starting with a letter or underscore.`, + ); + return; + } + const projectConfig = await detectProjectConfig(); const localConfig = projectConfig ?? (await requireInit()).localConfig; const repoPath = localConfig.repo.localPath; diff --git a/src/resources/env.ts b/src/resources/env.ts index 42b0f5537..2ba91e16b 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -107,6 +107,15 @@ export function maskEnvValue(value: string): string { return `${value.slice(0, 2)}****`; } +/** + * A key this module will write into env.sh, and the only shape it reads back. + * + * Shared by `parseEnvFile` and `generateEnvFile` on purpose: the write side has + * to reject exactly what the read side skips, or a variable can exist in env.sh + * that the CLI can never see again. + */ +export const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; + /** * Read back the assignments `generateEnvFile` writes, as key → value. * @@ -118,14 +127,13 @@ export function maskEnvValue(value: string): string { */ export function parseEnvFile(content: string): Map { const PREFIX = 'export '; - const KEY = /^[A-Za-z_][A-Za-z0-9_]*$/; const assignments = new Map(); let i = 0; while (i < content.length) { const eq = content.startsWith(PREFIX, i) ? content.indexOf('=', i + PREFIX.length) : -1; const key = eq === -1 ? '' : content.slice(i + PREFIX.length, eq); - if (eq === -1 || !KEY.test(key) || content[eq + 1] !== "'") { + if (eq === -1 || !ENV_KEY_RE.test(key) || content[eq + 1] !== "'") { const nl = content.indexOf('\n', i); if (nl === -1) break; i = nl + 1; @@ -437,9 +445,19 @@ export class EnvHandler extends ResourceHandler { * the sourced script. An embedded single quote is encoded with the standard * `'\''` sequence. env.sh is sourced from every team member's shell profile, * so values (which originate from the team repo's env/env.yaml) must be safe. + * + * Keys are interpolated raw into the export statement, so they are held to + * the same identifier rule `parseEnvFile` applies when reading env.sh back. A + * key that fails it is dropped rather than emitted: `export bad key='x'` is + * not valid shell, and `export FOO;cmd='x'` would run `cmd` in every member's + * shell. Dropping keeps the write and read sides in agreement — a line + * `parseEnvFile` must skip anyway is better left unwritten. One member's bad + * key must not take the whole file down with it, so the rest still ship. */ generateEnvFile(variables: EnvVariable[]): string { - const lines = variables.map(v => `export ${v.key}=${shellQuoteValue(v.value)}`); + const lines = variables + .filter(v => ENV_KEY_RE.test(v.key)) + .map(v => `export ${v.key}=${shellQuoteValue(v.value)}`); return lines.join('\n') + '\n'; }