Skip to content

Commit e2b256a

Browse files
brian-smith-tcrilclaude
brian-smith-tcril
and
claude
committed
fix: use execFileSync to prevent shell injection in translations:pull
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 3cadb1d commit e2b256a

3 files changed

Lines changed: 57 additions & 55 deletions

File tree

‎tools/cli/commands/translations.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export function runPrepare(): void {
88
export function runPull(): void {
99
pull({
1010
siteRoot: process.cwd(),
11-
execSync: (cmd) => child_process.execSync(cmd, { stdio: 'inherit' }),
11+
execFileSync: (file, args) => child_process.execFileSync(file, args, { stdio: 'inherit' }),
1212
shouldPrepare: !process.argv.includes('--no-prepare'),
1313
atlasOptions: process.argv.find(a => a.startsWith('--atlas-options='))?.slice('--atlas-options='.length),
1414
});

‎tools/cli/utils/translations/pull.test.ts‎

Lines changed: 51 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,12 @@ function createSite(baseDir: string, atlasTranslations?: AtlasTranslations): voi
3232
const tmpPrefix = path.join(os.tmpdir(), 'translations-test-');
3333

3434
describe('pull', () => {
35-
let mockExecSync: jest.Mock;
35+
let mockExecFileSync: jest.Mock;
3636
let warnSpy: jest.SpyInstance;
3737

3838
beforeEach(() => {
3939
jest.clearAllMocks();
40-
mockExecSync = jest.fn();
40+
mockExecFileSync = jest.fn();
4141
warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
4242
});
4343

@@ -52,28 +52,28 @@ describe('pull', () => {
5252
path: 'translations/frontend-app-authn/src/i18n',
5353
});
5454

55-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
55+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
5656

57-
expect(mockExecSync).toHaveBeenCalledTimes(1);
58-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('atlas pull'));
57+
expect(mockExecFileSync).toHaveBeenCalledTimes(1);
58+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining(['pull']));
5959
});
6060

6161
it('does not call atlas pull when dependencies list is empty', () => {
6262
using tmp = fs.mkdtempDisposableSync(tmpPrefix);
6363
createSite(tmp.path, { dependencies: [] });
6464

65-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
65+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
6666

67-
expect(mockExecSync).not.toHaveBeenCalled();
67+
expect(mockExecFileSync).not.toHaveBeenCalled();
6868
});
6969

7070
it('does not call atlas pull when all dependencies fail to resolve', () => {
7171
using tmp = fs.mkdtempDisposableSync(tmpPrefix);
7272
createSite(tmp.path, { dependencies: ['@openedx/missing'] });
7373

74-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
74+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
7575

76-
expect(mockExecSync).not.toHaveBeenCalled();
76+
expect(mockExecFileSync).not.toHaveBeenCalled();
7777
});
7878

7979
it('calls atlas pull with one FROM:TO mapping', () => {
@@ -83,10 +83,10 @@ describe('pull', () => {
8383
path: 'translations/frontend-app-authn/src/i18n',
8484
});
8585

86-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
86+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
8787

88-
expect(mockExecSync).toHaveBeenCalledWith(
89-
expect.stringContaining('translations/frontend-app-authn/src/i18n:src/i18n/messages/@openedx/frontend-app-authn'),
88+
expect(mockExecFileSync).toHaveBeenCalledWith(
89+
'atlas', expect.arrayContaining(['translations/frontend-app-authn/src/i18n:src/i18n/messages/@openedx/frontend-app-authn']),
9090
);
9191
});
9292

@@ -100,12 +100,14 @@ describe('pull', () => {
100100
const atlasOptions = '--repository=https://github.com/example/translations --revision=main';
101101
pull({
102102
siteRoot: tmp.path,
103-
execSync: mockExecSync,
103+
execFileSync: mockExecFileSync,
104104
shouldPrepare: false,
105105
atlasOptions,
106106
});
107107

108-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining(atlasOptions));
108+
expect(mockExecFileSync).toHaveBeenCalledWith(
109+
'atlas', expect.arrayContaining(['--repository=https://github.com/example/translations', '--revision=main']),
110+
);
109111
});
110112

111113
it('collects transitive dependency paths', () => {
@@ -121,11 +123,11 @@ describe('pull', () => {
121123
});
122124
createPackage(tmp.path, '@openedx/paragon', { path: 'translations/paragon/src/i18n' });
123125

124-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
126+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
125127

126-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('translations/authn/src/i18n:src/i18n/messages/@openedx/authn'));
127-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('translations/frontend-base/src/i18n:src/i18n/messages/@openedx/frontend-base'));
128-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon'));
128+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining(['translations/authn/src/i18n:src/i18n/messages/@openedx/authn']));
129+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining(['translations/frontend-base/src/i18n:src/i18n/messages/@openedx/frontend-base']));
130+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining(['translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon']));
129131
});
130132

131133
it('deduplicates shared dependencies so each appears only once', () => {
@@ -141,11 +143,10 @@ describe('pull', () => {
141143
});
142144
createPackage(tmp.path, '@openedx/paragon', { path: 'translations/paragon/src/i18n' });
143145

144-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
146+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
145147

146-
const atlasArgs = mockExecSync.mock.calls[0][0] as string;
147-
// split yields N+1 parts when the target appears N times
148-
const occurrences = atlasArgs.split('translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon').length - 1;
148+
const atlasArgs = mockExecFileSync.mock.calls[0][1] as string[];
149+
const occurrences = atlasArgs.filter(a => a === 'translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon').length;
149150
expect(occurrences).toBe(1);
150151
});
151152

@@ -161,11 +162,11 @@ describe('pull', () => {
161162
dependencies: ['@openedx/authn'], // circular
162163
});
163164

164-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
165+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
165166

166167
expect(warnSpy).toHaveBeenCalledWith('translations:pull: Circular dependency detected: test-site → @openedx/authn → @openedx/paragon → @openedx/authn, skipping.');
167-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('@openedx/authn'));
168-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('@openedx/paragon'));
168+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/authn')]));
169+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/paragon')]));
169170
});
170171

171172
it('uses the full scoped package name as the TO alias', () => {
@@ -175,10 +176,10 @@ describe('pull', () => {
175176
path: 'translations/authn/src/i18n',
176177
});
177178

178-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
179+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
179180

180-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining(':src/i18n/messages/@openedx/frontend-app-authn'));
181-
expect(mockExecSync).not.toHaveBeenCalledWith(expect.stringContaining(':src/i18n/messages/frontend-app-authn'));
181+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining(':src/i18n/messages/@openedx/frontend-app-authn')]));
182+
expect(mockExecFileSync).not.toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining(':src/i18n/messages/frontend-app-authn')]));
182183
});
183184

184185
it('does not touch the site-messages directory', () => {
@@ -191,7 +192,7 @@ describe('pull', () => {
191192
const arJson = path.join(siteMessagesDir, 'ar.json');
192193
fs.writeFileSync(arJson, '{"key":"value"}', { encoding: 'utf8' });
193194

194-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
195+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
195196

196197
expect(fs.readdirSync(siteMessagesDir)).toEqual(['ar.json']);
197198
expect(fs.readFileSync(arJson, { encoding: 'utf8' })).toBe('{"key":"value"}');
@@ -206,7 +207,7 @@ describe('pull', () => {
206207
fs.mkdirSync(path.dirname(staleFile), { recursive: true });
207208
fs.writeFileSync(staleFile, '{"key":"stale"}', { encoding: 'utf8' });
208209

209-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
210+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
210211

211212
expect(fs.existsSync(staleFile)).toBe(false);
212213
});
@@ -220,13 +221,13 @@ describe('pull', () => {
220221
fs.mkdirSync(path.dirname(translationFile), { recursive: true });
221222
fs.writeFileSync(translationFile, '{"key":"stale"}', { encoding: 'utf8' });
222223

223-
mockExecSync.mockImplementation(() => {
224+
mockExecFileSync.mockImplementation(() => {
224225
// wx flag fails if the file already exists, so this throws unless clearing happened first
225226
fs.mkdirSync(path.dirname(translationFile), { recursive: true });
226227
fs.writeFileSync(translationFile, '{"key":"new"}', { encoding: 'utf8', flag: 'wx' });
227228
});
228229

229-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
230+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
230231

231232
expect(fs.readFileSync(translationFile, { encoding: 'utf8' })).toBe('{"key":"new"}');
232233
});
@@ -236,7 +237,7 @@ describe('pull', () => {
236237
createSite(tmp.path, { dependencies: ['@openedx/authn'] });
237238
createPackage(tmp.path, '@openedx/authn', { path: 'translations/authn/src/i18n' });
238239

239-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
240+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
240241

241242
expect(jest.mocked(prepare)).toHaveBeenCalledWith({ siteRoot: tmp.path });
242243
});
@@ -246,7 +247,7 @@ describe('pull', () => {
246247
createSite(tmp.path, { dependencies: ['@openedx/authn'] });
247248
createPackage(tmp.path, '@openedx/authn', { path: 'translations/authn/src/i18n' });
248249

249-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
250+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
250251

251252
expect(jest.mocked(prepare)).not.toHaveBeenCalled();
252253
});
@@ -256,11 +257,11 @@ describe('pull', () => {
256257
createSite(tmp.path, { dependencies: ['@openedx/authn', '@openedx/missing'] });
257258
createPackage(tmp.path, '@openedx/authn', { path: 'translations/authn/src/i18n' });
258259

259-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
260+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
260261

261262
expect(warnSpy).toHaveBeenCalledWith('translations:pull: Package @openedx/missing not found in node_modules, skipping.');
262-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('@openedx/authn'));
263-
expect(mockExecSync).not.toHaveBeenCalledWith(expect.stringContaining('@openedx/missing'));
263+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/authn')]));
264+
expect(mockExecFileSync).not.toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/missing')]));
264265
});
265266

266267
it('warns and continues when a dependency has no atlasTranslations config', () => {
@@ -269,21 +270,21 @@ describe('pull', () => {
269270
createPackage(tmp.path, '@openedx/authn', { path: 'translations/authn/src/i18n' });
270271
createPackage(tmp.path, '@openedx/no-translations');
271272

272-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
273+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
273274

274275
expect(warnSpy).toHaveBeenCalledWith('translations:pull: No atlasTranslations config in @openedx/no-translations, skipping.');
275-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('@openedx/authn'));
276-
expect(mockExecSync).not.toHaveBeenCalledWith(expect.stringContaining('@openedx/no-translations'));
276+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/authn')]));
277+
expect(mockExecFileSync).not.toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/no-translations')]));
277278
});
278279

279280
it('pulls translations for the top-level package when atlasTranslations.path is set', () => {
280281
using tmp = fs.mkdtempDisposableSync(tmpPrefix);
281282
createSite(tmp.path, { path: 'translations/test-site/src/i18n' });
282283

283-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
284+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
284285

285-
expect(mockExecSync).toHaveBeenCalledWith(
286-
expect.stringContaining('translations/test-site/src/i18n:src/i18n/messages/test-site'),
286+
expect(mockExecFileSync).toHaveBeenCalledWith(
287+
'atlas', expect.arrayContaining(['translations/test-site/src/i18n:src/i18n/messages/test-site']),
287288
);
288289
});
289290

@@ -296,7 +297,7 @@ describe('pull', () => {
296297
);
297298

298299
expect(() => {
299-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
300+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
300301
}).toThrow('atlasTranslations.path is set');
301302
});
302303

@@ -305,7 +306,7 @@ describe('pull', () => {
305306
createSite(tmp.path);
306307

307308
expect(() => {
308-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: true });
309+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: true });
309310
}).toThrow('No atlasTranslations field in');
310311
});
311312

@@ -314,12 +315,12 @@ describe('pull', () => {
314315
createSite(tmp.path, { dependencies: ['@openedx/authn'] });
315316
createPackage(tmp.path, '@openedx/authn', { path: 'translations/authn/src/i18n' });
316317

317-
const failingExecSync = () => {
318+
const failingExecFileSync = () => {
318319
throw new Error('atlas exited with code 1');
319320
};
320321

321322
expect(() => {
322-
pull({ siteRoot: tmp.path, execSync: failingExecSync, shouldPrepare: false });
323+
pull({ siteRoot: tmp.path, execFileSync: failingExecFileSync, shouldPrepare: false });
323324
}).toThrow('atlas exited with code 1');
324325
});
325326

@@ -331,10 +332,10 @@ describe('pull', () => {
331332
});
332333
createPackage(tmp.path, '@openedx/paragon', { path: 'translations/paragon/src/i18n' });
333334

334-
pull({ siteRoot: tmp.path, execSync: mockExecSync, shouldPrepare: false });
335+
pull({ siteRoot: tmp.path, execFileSync: mockExecFileSync, shouldPrepare: false });
335336

336337
expect(warnSpy).not.toHaveBeenCalled();
337-
expect(mockExecSync).toHaveBeenCalledWith(expect.stringContaining('translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon'));
338-
expect(mockExecSync).not.toHaveBeenCalledWith(expect.stringContaining('@openedx/meta-package'));
338+
expect(mockExecFileSync).toHaveBeenCalledWith('atlas', expect.arrayContaining(['translations/paragon/src/i18n:src/i18n/messages/@openedx/paragon']));
339+
expect(mockExecFileSync).not.toHaveBeenCalledWith('atlas', expect.arrayContaining([expect.stringContaining('@openedx/meta-package')]));
339340
});
340341
});

‎tools/cli/utils/translations/pull.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,12 @@ function clearMessages(messagesDir: string): void {
117117

118118
export function pull({
119119
siteRoot,
120-
execSync,
120+
execFileSync,
121121
shouldPrepare,
122122
atlasOptions = '',
123123
}: {
124124
siteRoot: string,
125-
execSync: (command: string) => void,
125+
execFileSync: (file: string, args: string[]) => void,
126126
shouldPrepare: boolean,
127127
atlasOptions?: string,
128128
}): void {
@@ -144,7 +144,8 @@ export function pull({
144144
return;
145145
}
146146

147-
const atlasMappings = mappings.map(m => `${m.from}:src/i18n/messages/${m.to}`).join(' ');
148-
execSync(`atlas pull ${atlasOptions} ${atlasMappings}`);
147+
const atlasOptionsArgs = atlasOptions.trim().split(/\s+/).filter(Boolean);
148+
const atlasMappingArgs = mappings.map(m => `${m.from}:src/i18n/messages/${m.to}`);
149+
execFileSync('atlas', ['pull', ...atlasOptionsArgs, ...atlasMappingArgs]);
149150
if (shouldPrepare) prepare({ siteRoot });
150151
}

0 commit comments

Comments
 (0)