From becf96ef6d253d1258357b8f5133fa7b19ea13d9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 14:52:14 +0000 Subject: [PATCH 1/5] Initial plan From b7879fbef54dbf3d2c2db45fea76a7e1866b8441 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 14:57:08 +0000 Subject: [PATCH 2/5] initial: plan for path traversal fix in safe_output_handler_manager Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8159da5-ba15-44a4-82cb-4637701718e6 --- .github/aw/actions-lock.json | 5 +++++ pkg/workflow/data/action_pins.json | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/aw/actions-lock.json b/.github/aw/actions-lock.json index 5dee3709f1a..9056a621b72 100644 --- a/.github/aw/actions-lock.json +++ b/.github/aw/actions-lock.json @@ -148,6 +148,11 @@ "version": "v4.34.1", "sha": "cb06a0a8527b2c6970741b3a0baa15231dc74a4c" }, + "github/gh-aw-actions/setup@v0": { + "repo": "github/gh-aw-actions/setup", + "version": "v0", + "sha": "50f4fc16883c6c6672d8879affa8fd15d5cc79a4" + }, "github/gh-aw-actions/setup@v0.62.5": { "repo": "github/gh-aw-actions/setup", "version": "v0.62.5", diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json index 9056a621b72..5dee3709f1a 100644 --- a/pkg/workflow/data/action_pins.json +++ b/pkg/workflow/data/action_pins.json @@ -148,11 +148,6 @@ "version": "v4.34.1", "sha": "cb06a0a8527b2c6970741b3a0baa15231dc74a4c" }, - "github/gh-aw-actions/setup@v0": { - "repo": "github/gh-aw-actions/setup", - "version": "v0", - "sha": "50f4fc16883c6c6672d8879affa8fd15d5cc79a4" - }, "github/gh-aw-actions/setup@v0.62.5": { "repo": "github/gh-aw-actions/setup", "version": "v0.62.5", From 5e116abeb1e07bd2d8f3c343afebba78bd48357e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 15:06:38 +0000 Subject: [PATCH 3/5] fix: add path traversal sanitization for scriptFilename in safe_output_handler_manager Security fix for MEDIUM severity finding from weekly red team scan 2026-03-22. - JS: sanitize scriptFilename using path.basename() + path.relative() containment check before calling require() in safe_output_handler_manager.cjs - Go: add isSafeScriptName() validation in safe_scripts.go to block path traversal characters from being written into GH_AW_SAFE_OUTPUT_SCRIPTS at compile time Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8159da5-ba15-44a4-82cb-4637701718e6 --- .../setup/js/safe_output_handler_manager.cjs | 18 +++++++++++++++++- pkg/workflow/data/action_pins.json | 5 +++++ pkg/workflow/safe_scripts.go | 15 +++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index cd2d5577557..ba0b2f6f56d 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -167,8 +167,24 @@ async function loadHandlers(config, prReviewBuffer) { const customScriptHandlers = loadCustomSafeOutputScriptHandlers(); if (customScriptHandlers.size > 0) { core.info(`Loading ${customScriptHandlers.size} custom script handler(s): ${[...customScriptHandlers.keys()].join(", ")}`); + const nodePath = require("path"); + const scriptBaseDir = nodePath.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "actions"); for (const [scriptType, scriptFilename] of customScriptHandlers) { - const scriptPath = require("path").join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "actions", scriptFilename); + // Sanitize scriptFilename to prevent path traversal attacks: only the basename + // (no directory separators or ".." sequences) is allowed. + const safeFilename = nodePath.basename(scriptFilename); + if (safeFilename !== scriptFilename) { + core.error(`Invalid script filename for ${scriptType}: path traversal detected in "${scriptFilename}" — skipping`); + continue; + } + const scriptPath = nodePath.join(scriptBaseDir, safeFilename); + // Defense-in-depth: verify the resolved path remains within the expected directory. + // Use path.relative() to check containment robustly across all platforms. + const relativeToBase = nodePath.relative(scriptBaseDir, scriptPath); + if (relativeToBase.startsWith("..") || nodePath.isAbsolute(relativeToBase)) { + core.error(`Script path outside expected directory for ${scriptType}: "${scriptPath}" — skipping`); + continue; + } try { const scriptModule = require(scriptPath); if (scriptModule && typeof scriptModule.main === "function") { diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json index 5dee3709f1a..9056a621b72 100644 --- a/pkg/workflow/data/action_pins.json +++ b/pkg/workflow/data/action_pins.json @@ -148,6 +148,11 @@ "version": "v4.34.1", "sha": "cb06a0a8527b2c6970741b3a0baa15231dc74a4c" }, + "github/gh-aw-actions/setup@v0": { + "repo": "github/gh-aw-actions/setup", + "version": "v0", + "sha": "50f4fc16883c6c6672d8879affa8fd15d5cc79a4" + }, "github/gh-aw-actions/setup@v0.62.5": { "repo": "github/gh-aw-actions/setup", "version": "v0.62.5", diff --git a/pkg/workflow/safe_scripts.go b/pkg/workflow/safe_scripts.go index 9dcca5fab69..b14eb100130 100644 --- a/pkg/workflow/safe_scripts.go +++ b/pkg/workflow/safe_scripts.go @@ -3,6 +3,7 @@ package workflow import ( "encoding/json" "sort" + "strings" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/stringutil" @@ -88,6 +89,15 @@ func extractSafeScriptsFromFrontmatter(frontmatter map[string]any) map[string]*S return make(map[string]*SafeScriptConfig) } +// isSafeScriptName returns true if the script name is safe for use as a filename component. +// It rejects names that contain path separators or ".." sequences that could lead to +// path traversal when the generated filename is passed to require() at runtime. +func isSafeScriptName(name string) bool { + return !strings.Contains(name, "/") && + !strings.Contains(name, "\\") && + !strings.Contains(name, "..") +} + // buildCustomSafeOutputScriptsJSON builds a JSON mapping of custom safe output script names to their // .cjs filenames, for use in the GH_AW_SAFE_OUTPUT_SCRIPTS env var of the handler manager step. // This allows the handler manager to load and dispatch messages to inline script handlers. @@ -100,6 +110,11 @@ func buildCustomSafeOutputScriptsJSON(data *WorkflowData) string { scriptMapping := make(map[string]string, len(data.SafeOutputs.Scripts)) for scriptName := range data.SafeOutputs.Scripts { normalizedName := stringutil.NormalizeSafeOutputIdentifier(scriptName) + // Reject names that could cause path traversal when the filename is passed to require() + if !isSafeScriptName(normalizedName) { + safeScriptsLog.Printf("Warning: skipping script %q — name contains unsafe path characters: %q", scriptName, normalizedName) + continue + } scriptMapping[normalizedName] = safeOutputScriptFilename(normalizedName) } From 870ee719b557c17e5f0b8e3f0112dd7cc3f5896b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 15:20:18 +0000 Subject: [PATCH 4/5] fix: move path require to top-level and add adversarial path traversal tests - Move require("path") to module top-level in safe_output_handler_manager.cjs - Add JS adversarial tests in safe_output_handler_manager.test.cjs: - 7 new tests in "loadHandlers - path traversal sanitization" covering: leading ../, embedded /, absolute Unix path, multi-level traversal, ".." alone, mixed safe+evil entries, and a valid plain filename - Add Go adversarial tests in safe_scripts_test.go: - TestIsSafeScriptName: table-driven test for isSafeScriptName covering 11 cases - TestBuildCustomSafeOutputScriptsJSONPathTraversal: 5 cases verifying traversal names are excluded from the GH_AW_SAFE_OUTPUT_SCRIPTS JSON - TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe: verifies safe names are preserved when mixed with traversal names Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/07b67d12-f668-4f48-abc9-85093de4905d --- .../setup/js/safe_output_handler_manager.cjs | 2 +- .../js/safe_output_handler_manager.test.cjs | 97 +++++++++++++++++++ pkg/workflow/safe_scripts_test.go | 93 ++++++++++++++++++ 3 files changed, 191 insertions(+), 1 deletion(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index ba0b2f6f56d..41e9d48a656 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -22,6 +22,7 @@ const { sanitizeContent } = require("./sanitize_content.cjs"); const { createManifestLogger, ensureManifestExists, extractCreatedItemFromResult } = require("./safe_output_manifest.cjs"); const { loadCustomSafeOutputJobTypes, loadCustomSafeOutputScriptHandlers, loadCustomSafeOutputActionHandlers } = require("./safe_output_helpers.cjs"); const { emitSafeOutputActionOutputs } = require("./safe_outputs_action_outputs.cjs"); +const nodePath = require("path"); /** * Handler map configuration @@ -167,7 +168,6 @@ async function loadHandlers(config, prReviewBuffer) { const customScriptHandlers = loadCustomSafeOutputScriptHandlers(); if (customScriptHandlers.size > 0) { core.info(`Loading ${customScriptHandlers.size} custom script handler(s): ${[...customScriptHandlers.keys()].join(", ")}`); - const nodePath = require("path"); const scriptBaseDir = nodePath.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "actions"); for (const [scriptType, scriptFilename] of customScriptHandlers) { // Sanitize scriptFilename to prevent path traversal attacks: only the basename diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index 3aba0430269..d7219d2d8a4 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -21,6 +21,7 @@ describe("Safe Output Handler Manager", () => { delete process.env.GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG; delete process.env.GH_AW_TRACKER_LABEL; delete process.env.GH_AW_SAFE_OUTPUT_JOBS; + delete process.env.GH_AW_SAFE_OUTPUT_SCRIPTS; }); describe("loadConfig", () => { @@ -113,6 +114,102 @@ describe("Safe Output Handler Manager", () => { }); }); + describe("loadHandlers - path traversal sanitization", () => { + // These tests exercise the path traversal sanitization added to protect against + // malicious scriptFilename values in GH_AW_SAFE_OUTPUT_SCRIPTS. + // The sanitization runs BEFORE require(), so no real file needs to exist. + + it("should reject scriptFilename with a leading ../", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "../evil.cjs", + }); + + const handlers = await loadHandlers({}, {}); + + expect(handlers.has("evil")).toBe(false); + expect(core.error).toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "../evil.cjs"')); + }); + + it("should reject scriptFilename with an embedded directory separator", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "subdir/evil.cjs", + }); + + const handlers = await loadHandlers({}, {}); + + expect(handlers.has("evil")).toBe(false); + expect(core.error).toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "subdir/evil.cjs"')); + }); + + it("should reject scriptFilename with an absolute Unix path", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "/etc/passwd", + }); + + const handlers = await loadHandlers({}, {}); + + expect(handlers.has("evil")).toBe(false); + expect(core.error).toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "/etc/passwd"')); + }); + + it("should reject scriptFilename with multiple levels of path traversal", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "../../etc/shadow", + }); + + const handlers = await loadHandlers({}, {}); + + expect(handlers.has("evil")).toBe(false); + expect(core.error).toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "../../etc/shadow"')); + }); + + it("should reject scriptFilename that is just '..'", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "..", + }); + + const handlers = await loadHandlers({}, {}); + + expect(handlers.has("evil")).toBe(false); + expect(core.error).toHaveBeenCalledWith(expect.stringContaining("evil")); + }); + + it("should skip rejected entry but continue loading other valid scripts", async () => { + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + evil: "../evil.cjs", + // This valid name will attempt require() which will fail (file doesn't exist), + // and that failure is caught as a non-fatal warning — so the handler is absent + // but no core.error is called for path traversal. + safe_script: "safe_output_script_safe_script.cjs", + }); + + const handlers = await loadHandlers({}, {}); + + // The evil entry must be rejected with core.error (path traversal) + expect(core.error).toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "../evil.cjs"')); + // The safe entry should NOT trigger a path traversal error + expect(core.error).not.toHaveBeenCalledWith(expect.stringContaining('path traversal detected in "safe_output_script_safe_script.cjs"')); + // Neither handler is loaded (safe one fails because the file doesn't exist in test env) + expect(handlers.has("evil")).toBe(false); + }); + + it("should accept a plain filename with no path components", async () => { + // A well-formed filename — require() will fail because the file doesn't exist in + // the test environment, but NO core.error for path traversal should be logged. + process.env.GH_AW_SAFE_OUTPUT_SCRIPTS = JSON.stringify({ + my_script: "safe_output_script_my_script.cjs", + }); + + const handlers = await loadHandlers({}, {}); + + // No path traversal error should have been emitted + expect(core.error).not.toHaveBeenCalledWith(expect.stringContaining("path traversal detected")); + expect(core.error).not.toHaveBeenCalledWith(expect.stringContaining("Script path outside expected directory")); + // Handler is absent only because the file doesn't exist in the test env (warning, not error) + expect(handlers.has("my_script")).toBe(false); + }); + }); + describe("processMessages", () => { it("should process messages in order of appearance", async () => { const messages = [ diff --git a/pkg/workflow/safe_scripts_test.go b/pkg/workflow/safe_scripts_test.go index bfbb5fba151..65acf3f31cf 100644 --- a/pkg/workflow/safe_scripts_test.go +++ b/pkg/workflow/safe_scripts_test.go @@ -460,3 +460,96 @@ func TestHasNonBuiltinSafeOutputsEnabledWithScripts(t *testing.T) { } assert.True(t, hasNonBuiltinSafeOutputsEnabled(config), "Scripts should count as non-builtin safe outputs") } + +// TestIsSafeScriptName verifies path traversal detection in script names +func TestIsSafeScriptName(t *testing.T) { + tests := []struct { + name string + input string + want bool + }{ + // Valid names + {"plain name", "my_handler", true}, + {"name with numbers", "handler_v2", true}, + {"single word", "handler", true}, + {"dot in name", "handler.name", true}, + // Invalid — path separators and traversal sequences + {"forward slash", "sub/handler", false}, + {"backslash", `sub\handler`, false}, + {"double dot", "handler..", false}, + {"leading dot-dot", "../evil", false}, + {"double dot only", "..", false}, + {"traversal chain", "../../etc/shadow", false}, + {"dot-dot at end", "handler/../../", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isSafeScriptName(tt.input) + assert.Equal(t, tt.want, got, "isSafeScriptName(%q) should be %v", tt.input, tt.want) + }) + } +} + +// TestBuildCustomSafeOutputScriptsJSONPathTraversal verifies path traversal names are rejected +func TestBuildCustomSafeOutputScriptsJSONPathTraversal(t *testing.T) { + adversarialInputs := []struct { + name string + scriptKey string + }{ + {"forward slash in name", "sub/evil"}, + {"backslash in name", `sub\evil`}, + {"double dot traversal", "../evil"}, + {"double dot only", ".."}, + {"multi-level traversal", "../../etc/shadow"}, + } + + for _, tt := range adversarialInputs { + t.Run(tt.name, func(t *testing.T) { + data := &WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + Scripts: map[string]*SafeScriptConfig{ + tt.scriptKey: { + Script: "return async (m) => ({ success: true });", + }, + }, + }, + } + + jsonStr := buildCustomSafeOutputScriptsJSON(data) + + // The adversarial script name must not appear in the output JSON. + // (An empty JSON object `{}` or empty string are both acceptable outputs.) + assert.NotContains(t, jsonStr, "evil", "Path traversal name %q should not appear in output JSON", tt.scriptKey) + assert.NotContains(t, jsonStr, "shadow", "Path traversal name %q should not appear in output JSON", tt.scriptKey) + assert.NotContains(t, jsonStr, "..", "Double-dot must not appear in output JSON for %q", tt.scriptKey) + }) + } +} + +// TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe verifies unsafe names +// are dropped while safe names in the same map are still included. +func TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe(t *testing.T) { + data := &WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + Scripts: map[string]*SafeScriptConfig{ + "good_handler": {Script: "return async (m) => ({ success: true });"}, + "../evil": {Script: "return async (m) => ({ success: true });"}, + }, + }, + } + + jsonStr := buildCustomSafeOutputScriptsJSON(data) + require.NotEmpty(t, jsonStr, "Safe name should still produce output") + + var mapping map[string]string + err := json.Unmarshal([]byte(jsonStr), &mapping) + require.NoError(t, err, "JSON should be valid") + + assert.Contains(t, mapping, "good_handler", "Safe name should be present") + // The traversal name should be absent — NormalizeSafeOutputIdentifier converts - to _ but + // keeps .. and /, so the key would contain path characters and be filtered out. + for key := range mapping { + assert.False(t, strings.Contains(key, ".."), "Traversal key must not appear in output: %q", key) + assert.False(t, strings.Contains(key, "/"), "Key with slash must not appear in output: %q", key) + } +} From 6384cb319d2aeec7c61a056226dbe866a21c6edb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 15:37:26 +0000 Subject: [PATCH 5/5] fix: use assert.NotContains to satisfy testifylint in safe_scripts_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/20d7e076-1c29-4dca-8396-3d51c9733af7 --- pkg/workflow/safe_scripts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/safe_scripts_test.go b/pkg/workflow/safe_scripts_test.go index 65acf3f31cf..145734dd9ad 100644 --- a/pkg/workflow/safe_scripts_test.go +++ b/pkg/workflow/safe_scripts_test.go @@ -549,7 +549,7 @@ func TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe(t *testing.T // The traversal name should be absent — NormalizeSafeOutputIdentifier converts - to _ but // keeps .. and /, so the key would contain path characters and be filtered out. for key := range mapping { - assert.False(t, strings.Contains(key, ".."), "Traversal key must not appear in output: %q", key) - assert.False(t, strings.Contains(key, "/"), "Key with slash must not appear in output: %q", key) + assert.NotContains(t, key, "..", "Traversal key must not appear in output: %q", key) + assert.NotContains(t, key, "/", "Key with slash must not appear in output: %q", key) } }