diff --git a/actions/setup/js/runtime_import.cjs b/actions/setup/js/runtime_import.cjs index 17e5c37a18..ec31047b8c 100644 --- a/actions/setup/js/runtime_import.cjs +++ b/actions/setup/js/runtime_import.cjs @@ -116,18 +116,52 @@ const ALLOWED_EXPRESSIONS = [ function isSafeExpression(expr) { const trimmed = expr.trim(); + // Block dangerous JavaScript built-in property names + const DANGEROUS_PROPS = [ + "constructor", + "__proto__", + "prototype", + "__defineGetter__", + "__defineSetter__", + "__lookupGetter__", + "__lookupSetter__", + "hasOwnProperty", + "isPrototypeOf", + "propertyIsEnumerable", + "toString", + "valueOf", + "toLocaleString", + ]; + + // Split expression into parts and check each for dangerous properties + // Handle both dot notation (e.g., "github.event.issue") and bracket notation (e.g., "release.assets[0].id") + const parts = trimmed.split(/[.\[\]]+/).filter(p => p && !/^\d+$/.test(p)); + + for (const part of parts) { + if (DANGEROUS_PROPS.includes(part)) { + return false; // Block dangerous property + } + } + // Check exact match in allowed list if (ALLOWED_EXPRESSIONS.includes(trimmed)) { return true; } // Check if it matches dynamic patterns: - // - needs.* and steps.* (job dependencies and step outputs) + // - needs.* and steps.* (job dependencies and step outputs) - max depth 5 levels // - github.event.inputs.* (workflow_dispatch inputs) // - github.aw.inputs.* (shared workflow inputs) // - inputs.* (workflow_call inputs) // - env.* (environment variables) - const dynamicPatterns = [/^(needs|steps)\.[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)*$/, /^github\.event\.inputs\.[a-zA-Z0-9_-]+$/, /^github\.aw\.inputs\.[a-zA-Z0-9_-]+$/, /^inputs\.[a-zA-Z0-9_-]+$/, /^env\.[a-zA-Z0-9_-]+$/]; + // Limit nesting depth to max 5 levels to prevent deep traversal attacks + const dynamicPatterns = [ + /^(needs|steps)\.[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+){0,2}$/, // Max depth: needs.job.outputs.foo.bar (5 levels) + /^github\.event\.inputs\.[a-zA-Z0-9_-]+$/, + /^github\.aw\.inputs\.[a-zA-Z0-9_-]+$/, + /^inputs\.[a-zA-Z0-9_-]+$/, + /^env\.[a-zA-Z0-9_-]+$/, + ]; for (const pattern of dynamicPatterns) { if (pattern.test(trimmed)) { @@ -245,8 +279,13 @@ function evaluateExpression(expr) { inputs: context.payload?.inputs || {}, }; + // Freeze the evaluation context to prevent modification + Object.freeze(evalContext); + Object.freeze(evalContext.github); + // Parse property access (e.g., "github.actor" -> ["github", "actor"]) const parts = trimmed.split("."); + /** @type {any} */ let value = evalContext; for (const part of parts) { @@ -255,9 +294,27 @@ function evaluateExpression(expr) { if (arrayMatch) { const key = arrayMatch[1]; const index = parseInt(arrayMatch[2], 10); - value = value?.[key]?.[index]; + // Use Object.prototype.hasOwnProperty.call() to prevent prototype chain access + if (value && typeof value === "object" && Object.prototype.hasOwnProperty.call(value, key)) { + const arrayValue = value[key]; + if (Array.isArray(arrayValue) && index >= 0 && index < arrayValue.length) { + value = arrayValue[index]; + } else { + value = undefined; + break; + } + } else { + value = undefined; + break; + } } else { - value = value?.[part]; + // Use Object.prototype.hasOwnProperty.call() to prevent prototype chain access + if (value && typeof value === "object" && Object.prototype.hasOwnProperty.call(value, part)) { + value = value[part]; + } else { + value = undefined; + break; + } } if (value === undefined || value === null) { diff --git a/actions/setup/js/runtime_import.test.cjs b/actions/setup/js/runtime_import.test.cjs index 2556b98936..fbc91e1af2 100644 --- a/actions/setup/js/runtime_import.test.cjs +++ b/actions/setup/js/runtime_import.test.cjs @@ -127,6 +127,86 @@ describe("runtime_import", () => { expect(isSafeExpression("inputs.value || secrets.TOKEN")).toBe(!1); expect(isSafeExpression("github.actor || vars.NAME")).toBe(!1); }); + it("should block dangerous property names - constructor", () => { + expect(isSafeExpression("github.constructor")).toBe(!1); + expect(isSafeExpression("inputs.constructor")).toBe(!1); + expect(isSafeExpression("github.event.constructor")).toBe(!1); + expect(isSafeExpression("needs.job.constructor")).toBe(!1); + }); + it("should block dangerous property names - __proto__", () => { + expect(isSafeExpression("github.__proto__")).toBe(!1); + expect(isSafeExpression("inputs.__proto__")).toBe(!1); + expect(isSafeExpression("github.event.__proto__")).toBe(!1); + }); + it("should block dangerous property names - prototype", () => { + expect(isSafeExpression("github.prototype")).toBe(!1); + expect(isSafeExpression("inputs.prototype")).toBe(!1); + expect(isSafeExpression("github.event.prototype")).toBe(!1); + }); + it("should block dangerous property names - __defineGetter__", () => { + expect(isSafeExpression("github.__defineGetter__")).toBe(!1); + expect(isSafeExpression("inputs.__defineGetter__")).toBe(!1); + }); + it("should block dangerous property names - __defineSetter__", () => { + expect(isSafeExpression("github.__defineSetter__")).toBe(!1); + expect(isSafeExpression("inputs.__defineSetter__")).toBe(!1); + }); + it("should block dangerous property names - __lookupGetter__", () => { + expect(isSafeExpression("github.__lookupGetter__")).toBe(!1); + expect(isSafeExpression("inputs.__lookupGetter__")).toBe(!1); + }); + it("should block dangerous property names - __lookupSetter__", () => { + expect(isSafeExpression("github.__lookupSetter__")).toBe(!1); + expect(isSafeExpression("inputs.__lookupSetter__")).toBe(!1); + }); + it("should block dangerous property names - hasOwnProperty", () => { + expect(isSafeExpression("github.hasOwnProperty")).toBe(!1); + expect(isSafeExpression("inputs.hasOwnProperty")).toBe(!1); + }); + it("should block dangerous property names - isPrototypeOf", () => { + expect(isSafeExpression("github.isPrototypeOf")).toBe(!1); + expect(isSafeExpression("inputs.isPrototypeOf")).toBe(!1); + }); + it("should block dangerous property names - propertyIsEnumerable", () => { + expect(isSafeExpression("github.propertyIsEnumerable")).toBe(!1); + expect(isSafeExpression("inputs.propertyIsEnumerable")).toBe(!1); + }); + it("should block dangerous property names - toString", () => { + expect(isSafeExpression("github.toString")).toBe(!1); + expect(isSafeExpression("inputs.toString")).toBe(!1); + }); + it("should block dangerous property names - valueOf", () => { + expect(isSafeExpression("github.valueOf")).toBe(!1); + expect(isSafeExpression("inputs.valueOf")).toBe(!1); + }); + it("should block dangerous property names - toLocaleString", () => { + expect(isSafeExpression("github.toLocaleString")).toBe(!1); + expect(isSafeExpression("inputs.toLocaleString")).toBe(!1); + }); + it("should block dangerous properties in array access", () => { + expect(isSafeExpression("github.event.release.assets[0].constructor")).toBe(!1); + expect(isSafeExpression("github.event.release.assets[0].__proto__")).toBe(!1); + }); + it("should reject excessive nesting depth (more than 5 levels)", () => { + // Valid: max 5 levels (needs.job.outputs.foo.bar) + expect(isSafeExpression("needs.job.outputs.foo.bar")).toBe(!0); + expect(isSafeExpression("steps.step.outputs.foo.bar")).toBe(!0); + // Invalid: 6 levels + expect(isSafeExpression("needs.job.outputs.foo.bar.baz")).toBe(!1); + expect(isSafeExpression("steps.step.outputs.foo.bar.baz")).toBe(!1); + // Invalid: 7+ levels + expect(isSafeExpression("needs.job.outputs.foo.bar.baz.qux")).toBe(!1); + }); + it("should allow valid nested expressions within depth limit", () => { + // 2 levels + expect(isSafeExpression("needs.job.outputs")).toBe(!0); + // 3 levels + expect(isSafeExpression("needs.job.outputs.result")).toBe(!0); + // 4 levels + expect(isSafeExpression("needs.job.outputs.foo.value")).toBe(!0); + // 5 levels (max) + expect(isSafeExpression("needs.job.outputs.foo.bar")).toBe(!0); + }); }), describe("evaluateExpression", () => { beforeEach(() => { @@ -172,6 +252,43 @@ describe("runtime_import", () => { expect(evaluateExpression("inputs.missing")).toContain("${{"); expect(evaluateExpression("inputs.missing")).toContain("inputs.missing"); }); + it("should not access prototype chain properties", () => { + // These should return undefined (wrapped expression) instead of accessing prototype + const result = evaluateExpression("github.constructor"); + expect(result).toContain("${{"); + expect(result).toContain("github.constructor"); + }); + it("should not access __proto__ property", () => { + const result = evaluateExpression("github.__proto__"); + expect(result).toContain("${{"); + expect(result).toContain("github.__proto__"); + }); + it("should safely handle missing properties without prototype pollution", () => { + // These properties don't exist in the context object and should be undefined + expect(evaluateExpression("github.nonexistent")).toContain("${{"); + expect(evaluateExpression("inputs.toString")).toContain("${{"); + }); + it("should handle array access safely with bounds checking", () => { + // Test with actual array in context + global.context = { + actor: "testuser", + job: "test-job", + repo: { owner: "testorg", repo: "testrepo" }, + runId: 12345, + runNumber: 67, + workflow: "test-workflow", + payload: { + inputs: { repository: "testorg/testrepo", name: "test-name" }, + release: { assets: [{ id: 123 }, { id: 456 }] }, + }, + }; + // Valid array access + expect(evaluateExpression("github.event.release.assets[0].id")).toBe("123"); + expect(evaluateExpression("github.event.release.assets[1].id")).toBe("456"); + // Out of bounds - should return undefined + const outOfBounds = evaluateExpression("github.event.release.assets[999].id"); + expect(outOfBounds).toContain("${{"); + }); }), describe("processRuntimeImport", () => { (it("should read and return file content", async () => {