fix: path traversal sanitization for scriptFilename in safe_output_handler_manager

.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",

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,8 +168,23 @@ 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 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") {

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 = [

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)
 	}
 

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.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)
+	}
+}