Skip to content

Commit f823e35

Browse files
author
Gyan Ranjan A
committed
fix: address PR review — merge order, regex, prototype pollution
- Fix component input merge order: remove duplicate cliGlobalInputs spread that caused global CLI to override component-specific CLI inputs on key conflicts (P1) - Broaden component name regex to allow '/' for paths like templates/deploy (P2) - Add prototype pollution guard for __proto__, constructor, prototype keys in --input parsing (P1) - Add 7 new tests covering edge cases - Fix existing test that relied on buggy merge order
1 parent 5e3c321 commit f823e35

9 files changed

Lines changed: 138 additions & 5 deletions

File tree

src/argv.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,13 +227,18 @@ export class Argv {
227227
const val = this.map.get("input");
228228
const inputs: {[key: string]: any} = {};
229229
const pairs = typeof val == "string" ? val.split(" ") : val;
230+
const dangerousKeys = new Set(["__proto__", "constructor", "prototype"]);
230231
(pairs ?? []).forEach((inputPair: string) => {
231232
// Support component-specific syntax: component:key=value or key=value
232-
const exec = /(?:(?<component>[\w-]+):)?(?<key>[\w-]+)(=)(?<value>(.|\n|\r)*)/.exec(inputPair);
233+
// Component names may contain word chars, hyphens, and slashes (e.g. templates/deploy)
234+
const exec = /(?:(?<component>[\w\-/]+):)?(?<key>[\w-]+)(=)(?<value>(.|\n|\r)*)/.exec(inputPair);
233235
if (exec?.groups?.key) {
234236
const value = exec?.groups?.value;
235237
const key = exec.groups.key;
236238
const component = exec.groups.component;
239+
240+
// Guard against prototype pollution
241+
if (dangerousKeys.has(key) || dangerousKeys.has(component ?? "")) return;
237242

238243
// Try to parse as JSON for arrays/objects/booleans/numbers
239244
let parsedValue;

src/parser-includes.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ export class ParserIncludes {
174174
const componentName = component.name.replace(/^templates\//, "");
175175
const fileComponentInputs = isStructured ? (fileInputs[componentName] ?? {}) : {};
176176
const cliComponentSpecificInputs = cliComponentInputs[componentName] ?? {};
177-
const mergedInputs = {...(value.inputs ?? {}), ...globalInputs, ...fileComponentInputs, ...cliComponentSpecificInputs, ...cliGlobalInputs};
177+
const mergedInputs = {...(value.inputs ?? {}), ...globalInputs, ...fileComponentInputs, ...cliComponentSpecificInputs};
178178
const fileDoc = await Parser.loadYaml(file, {inputs: mergedInputs}, expandVariables, writeStreams);
179179
// Expand local includes inside to a "project"-like include
180180
fileDoc["include"] = this.expandInnerLocalIncludes(fileDoc["include"], component.projectPath, component.ref, opts);

tests/argv-input.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import "../src/global.js";
2+
import {Argv} from "../src/argv.js";
3+
import {WriteStreamsMock} from "../src/write-streams.js";
4+
5+
let writeStreams: WriteStreamsMock;
6+
7+
beforeEach(() => {
8+
writeStreams = new WriteStreamsMock();
9+
});
10+
11+
test("input parses component names with slashes", async () => {
12+
const argv = await Argv.build({input: ["templates/deploy:replicas=5"]}, writeStreams);
13+
const result = argv.input;
14+
expect(result["templates/deploy"]).toEqual({replicas: 5});
15+
});
16+
17+
test("input keeps global and component keys separate", async () => {
18+
const argv = await Argv.build({input: ["deploy:replicas=5", "environment=prod"]}, writeStreams);
19+
const result = argv.input;
20+
expect(result["deploy"]).toEqual({replicas: 5});
21+
expect(result["environment"]).toEqual("prod");
22+
});
23+
24+
test("input ignores __proto__ component to prevent prototype pollution", async () => {
25+
const argv = await Argv.build({input: ["__proto__:polluted=true"]}, writeStreams);
26+
const result = argv.input;
27+
expect(Object.hasOwn(result, "__proto__")).toBe(false);
28+
expect(({} as any).polluted).toBeUndefined();
29+
});
30+
31+
test("input ignores __proto__ key to prevent prototype pollution", async () => {
32+
const argv = await Argv.build({input: ["__proto__=true"]}, writeStreams);
33+
const result = argv.input;
34+
expect(Object.keys(result)).not.toContain("__proto__");
35+
});
36+
37+
test("input ignores constructor key to prevent prototype pollution", async () => {
38+
const argv = await Argv.build({input: ["constructor:toString=bad"]}, writeStreams);
39+
const result = argv.input;
40+
expect(Object.hasOwn(result, "constructor")).toBe(false);
41+
});

tests/test-cases/.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ artifacts/log.txt
66
!gitignore/.gitlab-ci-local
77
!component-inputs-cli/.gitlab-ci-local-inputs.yml
88
!component-inputs-multiple/.gitlab-ci-local-inputs.yml
9+
!component-inputs-edge-cases/.gitlab-ci-local-inputs.yml
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
_global:
3+
environment: production
4+
5+
deploy:
6+
replicas: 3
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
include:
3+
- component: $CI_SERVER_HOST/$CI_PROJECT_PATH/deploy@$CI_COMMIT_SHA
4+
5+
stages:
6+
- deploy
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import {WriteStreamsMock} from "../../../src/write-streams.js";
2+
import {handler} from "../../../src/handler.js";
3+
import {initSpawnSpy} from "../../mocks/utils.mock.js";
4+
import {WhenStatics} from "../../mocks/when-statics.js";
5+
6+
beforeAll(() => {
7+
initSpawnSpy(WhenStatics.all);
8+
});
9+
10+
// Issue: component-specific CLI input must override global CLI input on key conflict
11+
test("component-specific CLI wins over global CLI on same key", async () => {
12+
const writeStreams = new WriteStreamsMock();
13+
await handler({
14+
cwd: "tests/test-cases/component-inputs-edge-cases",
15+
input: ["replicas=1", "deploy:replicas=10"],
16+
preview: true,
17+
}, writeStreams);
18+
19+
const expected = `---
20+
stages:
21+
- .pre
22+
- deploy
23+
- .post
24+
deploy-production:
25+
stage: deploy
26+
script:
27+
- echo "Deploying to production"
28+
- echo "Replicas 10"`;
29+
30+
expect(writeStreams.stdoutLines[0]).toEqual(expected);
31+
});
32+
33+
// Global CLI applies but file component-specific takes precedence
34+
test("file component-specific overrides global CLI", async () => {
35+
const writeStreams = new WriteStreamsMock();
36+
await handler({
37+
cwd: "tests/test-cases/component-inputs-edge-cases",
38+
input: ["replicas=7"],
39+
preview: true,
40+
}, writeStreams);
41+
42+
// File has deploy.replicas=3 (component-specific), which overrides global CLI replicas=7
43+
const expected = `---
44+
stages:
45+
- .pre
46+
- deploy
47+
- .post
48+
deploy-production:
49+
stage: deploy
50+
script:
51+
- echo "Deploying to production"
52+
- echo "Replicas 3"`;
53+
54+
expect(writeStreams.stdoutLines[0]).toEqual(expected);
55+
});
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
spec:
3+
inputs:
4+
environment:
5+
type: string
6+
default: dev
7+
replicas:
8+
type: number
9+
default: 1
10+
---
11+
deploy-$[[ inputs.environment ]]:
12+
stage: deploy
13+
script:
14+
- echo "Deploying to $[[ inputs.environment ]]"
15+
- echo "Replicas $[[ inputs.replicas ]]"

tests/test-cases/component-inputs-multiple/integration.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,18 @@ build-job:
3333
expect(writeStreams.stdoutLines[0]).toEqual(expected);
3434
});
3535

36-
test("component-inputs-multiple CLI overrides component-specific", async () => {
36+
test("component-inputs-multiple global CLI does not override file component-specific", async () => {
3737
const writeStreams = new WriteStreamsMock();
3838
await handler({
3939
cwd: "tests/test-cases/component-inputs-multiple",
4040
input: ["replicas=10", "go_version=\"1.22\""],
4141
preview: true,
4242
}, writeStreams);
4343

44+
// Global CLI inputs do NOT override file component-specific values.
45+
// File has deploy.replicas=5 and build.go_version="1.21" (component-specific),
46+
// so those take precedence over global CLI replicas=10 and go_version="1.22".
47+
// Use component-specific CLI syntax (deploy:replicas=10) to override.
4448
const expected = `---
4549
stages:
4650
- .pre
@@ -50,11 +54,11 @@ stages:
5054
deploy-job:
5155
stage: deploy
5256
script:
53-
- echo "Deploy to production with 10 replicas"
57+
- echo "Deploy to production with 5 replicas"
5458
build-job:
5559
stage: build
5660
script:
57-
- echo "Build with Go 1.22"
61+
- echo "Build with Go 1.21"
5862
- echo "Cache true"`;
5963

6064
expect(writeStreams.stdoutLines[0]).toEqual(expected);

0 commit comments

Comments
 (0)