Skip to content

Commit f6f4d72

Browse files
committed
fix: parse dotenv files without shell execution
1 parent 88e9e8c commit f6f4d72

6 files changed

Lines changed: 90 additions & 49 deletions

File tree

src/domain/deploy/backend-strategy.ts

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { getInstallCommand, getRunCommand, detectPkgManager } from '../framework
88
import { RSYNC_DEFAULT_EXCLUDES } from '../../shared/constants.js';
99
import { DeployError } from '../../shared/errors.js';
1010
import type { DeploymentStrategy, StrategyContext } from './strategy.js';
11+
import { runWithDotenv } from './dotenv.js';
1112

1213
function escapeSingleQuotes(s: string): string {
1314
return s.replace(/'/g, "\\'");
@@ -25,20 +26,6 @@ function parseCommand(command: string | undefined, pkgManager: string): { script
2526
return { script: parts[0], args: parts.slice(1).join(' ') };
2627
}
2728

28-
/**
29-
* Shell snippet that sources the workDir-local `.env` into the current shell
30-
* so subsequent && commands inherit the variables. Returns empty string when
31-
* the project has no envFile so callers can interpolate it unconditionally.
32-
*
33-
* Used during install, build, and post-symlink relink. The PM2 wrapper takes
34-
* the same approach (see generateAppBlock) but sources `<shared>/<envFile>`
35-
* directly — it doesn't depend on the workDir symlink.
36-
*/
37-
function sourceEnvCommand(envFile: string | undefined): string {
38-
if (!envFile) return '';
39-
return `set -a && . ./.env && set +a`;
40-
}
41-
4229
export class BackendStrategy implements DeploymentStrategy {
4330
readonly name = 'backend';
4431

@@ -98,11 +85,6 @@ export class BackendStrategy implements DeploymentStrategy {
9885
`{ echo ${shellSingleQuote(missingEnvMessage)} >&2; exit 1; }`,
9986
);
10087
commands.push(`ln -sf "${this.appPath}/shared/${this.app.envFile}" .env`);
101-
// Source it so install/build see env vars (private-registry tokens in
102-
// `.npmrc` via `${TOKEN}`, build-time secrets, etc.). Affects this shell
103-
// chain only; the PM2 wrapper sources independently at process start.
104-
const sourceCmd = sourceEnvCommand(this.app.envFile);
105-
if (sourceCmd) commands.push(sourceCmd);
10688
}
10789

10890
// Ensure third-party package managers are available on the remote
@@ -114,10 +96,10 @@ export class BackendStrategy implements DeploymentStrategy {
11496
commands.push(`command -v bun &>/dev/null || npm install -g bun`);
11597
}
11698

117-
commands.push(installCmd);
99+
const envDependentCommands = [installCmd];
118100

119101
if (!ctx.skipBuild) {
120-
commands.push(`if [ -f package.json ] && jq -e '.scripts.build' package.json >/dev/null 2>&1; then ${runCmd} build; fi`);
102+
envDependentCommands.push(`if [ -f package.json ] && jq -e '.scripts.build' package.json >/dev/null 2>&1; then ${runCmd} build; fi`);
121103
}
122104

123105
// Symlink `.env` into compiled-output directories so frameworks whose env
@@ -128,7 +110,9 @@ export class BackendStrategy implements DeploymentStrategy {
128110
// 3. obvious monorepo layouts: `apps/*/build`, `packages/*/build`, dist twins
129111
// We never traverse into node_modules and never overwrite an existing
130112
// `.env` file.
131-
commands.push(this.getEnvSymlinkCommand(ctx.workDir));
113+
envDependentCommands.push(this.getEnvSymlinkCommand(ctx.workDir));
114+
115+
commands.push(runWithDotenv(this.app.envFile ? '.env' : undefined, envDependentCommands.join(' && ')));
132116

133117
const installResult = await ctx.executor.exec(commands.join(' && '));
134118
this.assertNoBuildScriptsIgnored(pkgManager, installResult);
@@ -311,12 +295,8 @@ export class BackendStrategy implements DeploymentStrategy {
311295
): Promise<void> {
312296
const baseInstall = this.workspace.installCommand ?? getInstallCommand(pkgManager);
313297
const relinkInstall = this.workspace.installCommand ? baseInstall : `${baseInstall} --prefer-offline`;
314-
// Source env before relinking — private-registry tokens in `.npmrc` use
315-
// env-var interpolation.
316-
const sourceCmd = sourceEnvCommand(this.app.envFile);
317-
const sourceStep = sourceCmd ? `${sourceCmd} && ` : '';
318298
const installResult = await ctx.executor.exec(
319-
`cd "${cdPath}" && ${mise} && ${sourceStep}${relinkInstall}`,
299+
`cd "${cdPath}" && ${mise} && ${runWithDotenv(this.app.envFile ? '.env' : undefined, relinkInstall)}`,
320300
);
321301
this.assertNoBuildScriptsIgnored(pkgManager, installResult);
322302
if (installResult.exitCode !== 0) {
@@ -393,16 +373,9 @@ ${appBlocks.join(',\n')}
393373

394374
if (useWrapper) {
395375
const tail = origArgs ? `${origScript} ${origArgs}` : origScript;
396-
// Sourcing the dotenv file happens inside the PM2-launched shell, so it
397-
// can overwrite inline PM2 values such as the blue-green port. Re-apply
398-
// the ecosystem's explicit values afterwards to preserve their normal
399-
// precedence over the shared environment.
400-
const explicitEnv = Object.entries(env)
401-
.map(([key, value]) => `${key}=${shellSingleQuote(String(value))}`)
402-
.join(' ');
403-
const inner = `set -a && . '${escapeSingleQuotes(envFilePath)}' && set +a && export ${explicitEnv} && exec ${tail}`;
376+
const runner = runWithDotenv(envFilePath, `exec ${tail}`, env);
404377
scriptLine = `script: 'bash',`;
405-
argsLine = `\n args: ['-c', '${escapeSingleQuotes(inner)}'],`;
378+
argsLine = `\n args: ['-c', '${escapeSingleQuotes(runner)}'],`;
406379
} else {
407380
scriptLine = `script: '${escapeSingleQuotes(origScript)}',`;
408381
argsLine = origArgs ? `\n args: '${escapeSingleQuotes(origArgs)}',` : '';

src/domain/deploy/dotenv.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
function shellSingleQuote(value: string): string {
2+
return `'${value.replace(/'/g, "'\"'\"'")}'`;
3+
}
4+
5+
// This program deliberately parses dotenv as data. It is passed to the remote
6+
// Node runtime with `-e`, so values from the dotenv file never become shell
7+
// syntax. It supports the dotenv forms Node applications commonly use:
8+
// comments, `export KEY=...`, quoted values and quoted multiline values.
9+
const dotenvRunner = String.raw`const fs=require('node:fs');const{spawn}=require('node:child_process');const[file,encodedEnv,command,...args]=process.argv.slice(1);const text=fs.readFileSync(file,'utf8').replace(/^\uFEFF/,'');const parsed={};const line=/(?:^|\n)\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=\s*(\s*'(?:\\'|[^'])*'|\s*"(?:\\"|[^"])*"|\s*\x60(?:\\\x60|[^\x60])*\x60|[^\n]*)/g;let match;while((match=line.exec(text))!==null){let value=match[2].trim();const quote=value[0];if((quote==='"'||quote==="'"||quote==='\x60')&&value.at(-1)===quote){value=value.slice(1,-1);if(quote==='"')value=value.replace(/\\n/g,'\n').replace(/\\r/g,'\r');}else{value=value.replace(/\s+#.*$/,'').trim();}parsed[match[1]]=value;}const explicit=JSON.parse(Buffer.from(encodedEnv,'base64url').toString('utf8'));Object.assign(process.env,parsed,explicit);const child=spawn(command,args,{env:process.env,stdio:'inherit'});child.on('error',error=>{console.error(error.message);process.exit(1);});child.on('exit',(code,signal)=>process.exit(code??(signal?1:0)));`;
10+
11+
/**
12+
* Runs a shell command with values parsed from a dotenv file without sourcing
13+
* that file. Explicit process values win over the dotenv file.
14+
*/
15+
export function runWithDotenv(
16+
envFile: string | undefined,
17+
command: string,
18+
explicitEnvironment: Record<string, string | number> = {},
19+
): string {
20+
if (!envFile) return command;
21+
22+
const encodedEnvironment = Buffer.from(JSON.stringify(explicitEnvironment)).toString('base64url');
23+
return `node -e ${shellSingleQuote(dotenvRunner)} -- ` +
24+
`${shellSingleQuote(envFile)} ${shellSingleQuote(encodedEnvironment)} bash -c ${shellSingleQuote(command)}`;
25+
}

src/domain/deploy/orchestrator.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
import type { DeploymentStrategy, StrategyContext } from './strategy.js';
1919
import { BackendStrategy } from './backend-strategy.js';
2020
import { FrontendStrategy } from './frontend-strategy.js';
21+
import { runWithDotenv } from './dotenv.js';
2122

2223
export class DeployOrchestrator {
2324
constructor(
@@ -241,9 +242,6 @@ export class DeployOrchestrator {
241242

242243
const mise = `export PATH="$HOME/.local/bin:$HOME/.local/share/mise/shims:$PATH"`;
243244
const hookCwd = app.appRoot ? `${workDir}/${app.appRoot}` : workDir;
244-
const envSource = app.envFile
245-
? `set -a && . "${workDir}/.env" && set +a && `
246-
: '';
247245
const prefix = chalk.dim(' │ ');
248246

249247
const onData = (chunk: string): void => {
@@ -257,7 +255,7 @@ export class DeployOrchestrator {
257255
env: 'production',
258256
exec: async (cmd: string): Promise<ExecResult> => {
259257
const result = await this.executor.exec(
260-
`cd "${hookCwd}" && ${mise} && ${envSource}${cmd}`,
258+
`cd "${hookCwd}" && ${mise} && ${runWithDotenv(app.envFile ? `${workDir}/.env` : undefined, cmd)}`,
261259
{ onData },
262260
);
263261
if (result.exitCode !== 0) {

tests/unit/backend-strategy.test.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -233,16 +233,17 @@ describe('BackendStrategy.setupEnvironment', () => {
233233
expect(cmd).toContain('"dist"');
234234
});
235235

236-
it('sources .env before install so private-registry tokens (npmrc ${VAR}) resolve', async () => {
236+
it('loads .env with Node before install so private-registry tokens (npmrc ${VAR}) resolve', async () => {
237237
const strategy = makeStrategy(makeConfig({ pkgManager: 'pnpm' }), '/local/project');
238238
const executor = new FakeRemoteExecutor();
239239
await strategy.setupEnvironment!(makeCtx(executor));
240240

241241
const cmd = executor.getLastCommand()!.command;
242-
const sourceIdx = cmd.indexOf('set -a && . ./.env && set +a');
242+
const sourceIdx = cmd.indexOf('node -e');
243243
const installIdx = cmd.indexOf('pnpm install');
244244
expect(sourceIdx).toBeGreaterThan(-1);
245245
expect(installIdx).toBeGreaterThan(sourceIdx);
246+
expect(cmd).not.toContain('set -a');
246247
});
247248

248249
it('links the shared env using the configured envFile name, not a hardcoded .env', async () => {
@@ -426,7 +427,7 @@ describe('BackendStrategy.startApp', () => {
426427
await expect(strategy.startApp!(makeCtx(executor))).rejects.toThrow('pnpm approve-builds');
427428
});
428429

429-
it('drops the env_file ecosystem entry in favor of a bash -c env wrapper (ADR-0003)', async () => {
430+
it('drops the env_file ecosystem entry in favor of a Node dotenv wrapper (ADR-0003)', async () => {
430431
const strategy = makeStrategy(makeConfig({ envFile: '.env.production' }), '/local/project');
431432
const executor = new FakeRemoteExecutor();
432433
await strategy.startApp!(makeCtx(executor));
@@ -435,11 +436,11 @@ describe('BackendStrategy.startApp', () => {
435436
expect(writeCmd).not.toContain('env_file:');
436437
expect(writeCmd).toContain(`script: '"'"'bash'"'"',`);
437438
expect(writeCmd).toContain('shared/.env.production');
438-
expect(writeCmd).toContain('set -a');
439-
expect(writeCmd).toContain('export NODE_ENV=');
440-
expect(writeCmd).toContain('PORT=');
439+
expect(writeCmd).toContain('node -e');
440+
expect(writeCmd).not.toContain('set -a');
441+
expect(writeCmd).toContain('base64url');
442+
expect(writeCmd).toContain('NODE_ENV');
441443
expect(writeCmd).toContain('3000');
442-
expect(writeCmd.indexOf('export NODE_ENV=')).toBeGreaterThan(writeCmd.indexOf('set +a'));
443444
expect(writeCmd).toContain('exec');
444445
});
445446

tests/unit/dotenv.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { afterEach, describe, expect, it } from 'vitest';
2+
import { execa } from 'execa';
3+
import { mkdtemp, rm, writeFile } from 'node:fs/promises';
4+
import { tmpdir } from 'node:os';
5+
import { join } from 'node:path';
6+
import { runWithDotenv } from '../../src/domain/deploy/dotenv.js';
7+
8+
const temporaryDirectories: string[] = [];
9+
10+
afterEach(async () => {
11+
await Promise.all(temporaryDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true })));
12+
});
13+
14+
describe('runWithDotenv', () => {
15+
it('passes shell-sensitive and multiline values as data without executing them', async () => {
16+
const directory = await mkdtemp(join(tmpdir(), 'shipnode-dotenv-'));
17+
temporaryDirectories.push(directory);
18+
const envFile = join(directory, '.env');
19+
const marker = join(directory, 'must-not-exist');
20+
await writeFile(envFile, `TOKEN="safe; touch ${marker}"\nMULTILINE="first\\nsecond"\n`);
21+
22+
const command = runWithDotenv(
23+
envFile,
24+
"node -e 'process.stdout.write(JSON.stringify({ token: process.env.TOKEN, multiline: process.env.MULTILINE }))'",
25+
);
26+
const result = await execa('bash', ['-c', command]);
27+
28+
expect(result.stdout).toBe(JSON.stringify({ token: `safe; touch ${marker}`, multiline: 'first\nsecond' }));
29+
await expect(execa('test', ['-e', marker])).rejects.toMatchObject({ exitCode: 1 });
30+
});
31+
32+
it('lets explicit process values override dotenv values', async () => {
33+
const directory = await mkdtemp(join(tmpdir(), 'shipnode-dotenv-'));
34+
temporaryDirectories.push(directory);
35+
const envFile = join(directory, '.env');
36+
await writeFile(envFile, 'PORT=3000\n');
37+
38+
const command = runWithDotenv(envFile, "node -e 'process.stdout.write(process.env.PORT)'", { PORT: 13000 });
39+
const result = await execa('bash', ['-c', command]);
40+
41+
expect(result.stdout).toBe('13000');
42+
});
43+
});

tests/unit/orchestrator.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ describe('DeployOrchestrator', () => {
6565
expect(lockRelease.length).toBeGreaterThan(0);
6666
});
6767

68-
it('sources .env before user preDeploy commands when envFile is configured', async () => {
68+
it('loads .env with Node before user preDeploy commands when envFile is configured', async () => {
6969
const executor = new FakeRemoteExecutor();
7070
let captured: string | undefined;
7171
const config = makeConfig({
@@ -106,10 +106,11 @@ describe('DeployOrchestrator', () => {
106106
await orchestrator.deploy({ cwd: '/test', skipBuild: false });
107107

108108
expect(captured).toBeDefined();
109-
const sourceIdx = captured!.search(/set -a && \. "[^"]*\/\.env" && set \+a/);
109+
const sourceIdx = captured!.indexOf('node -e');
110110
const userIdx = captured!.indexOf('node ace.js migration:run');
111111
expect(sourceIdx).toBeGreaterThan(-1);
112112
expect(userIdx).toBeGreaterThan(sourceIdx);
113+
expect(captured).not.toContain('set -a');
113114
});
114115

115116
it('runs hooks from <workDir>/<appRoot> when appRoot is configured', async () => {

0 commit comments

Comments
 (0)