Skip to content

fix: path traversal sanitization for scriptFilename in safe_output_handler_manager#22280

Merged
pelikhan merged 5 commits intomainfrom
copilot/fix-unsanitized-path-in-require
Mar 22, 2026
Merged

fix: path traversal sanitization for scriptFilename in safe_output_handler_manager#22280
pelikhan merged 5 commits intomainfrom
copilot/fix-unsanitized-path-in-require

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 22, 2026

scriptFilename values from GH_AW_SAFE_OUTPUT_SCRIPTS were passed unsanitized into path.join() before being loaded via require(), allowing path traversal (e.g. ../evil.cjs) to load arbitrary files at runtime.

Changes

actions/setup/js/safe_output_handler_manager.cjs

  • Move require("path") to module top-level as nodePath
  • Strip directory components via path.basename() and reject any filename where basename !== original (contains /, \, or ..)
  • Defense-in-depth: use path.relative() to assert the resolved path stays within scriptBaseDir — portable across platforms unlike a startsWith(dir + sep) check
const safeFilename = nodePath.basename(scriptFilename);
if (safeFilename !== scriptFilename) {
  core.error(`Invalid script filename for ${scriptType}: path traversal detected in "${scriptFilename}" — skipping`);
  continue;
}
const relativeToBase = nodePath.relative(scriptBaseDir, scriptPath);
if (relativeToBase.startsWith("..") || nodePath.isAbsolute(relativeToBase)) {
  core.error(`Script path outside expected directory for ${scriptType}: "${scriptPath}" — skipping`);
  continue;
}

pkg/workflow/safe_scripts.go

  • Adds isSafeScriptName() to reject names containing /, \, or ..
  • Validates at compile time in buildCustomSafeOutputScriptsJSON() before any script name is written into GH_AW_SAFE_OUTPUT_SCRIPTS, blocking traversal sequences at the source rather than relying solely on the runtime JS check

Tests

actions/setup/js/safe_output_handler_manager.test.cjs

Added 7 adversarial tests in a new loadHandlers - path traversal sanitization suite covering:

  • Leading ../ in filename
  • Embedded / directory separator
  • Absolute Unix path (e.g. /etc/passwd)
  • Multi-level traversal (e.g. ../../etc/shadow)
  • Bare .. filename
  • Mixed map with one evil and one safe entry — evil rejected, safe attempted normally
  • Valid plain filename — no traversal error emitted

pkg/workflow/safe_scripts_test.go

  • TestIsSafeScriptName: 11-case table-driven test covering valid names and all traversal patterns
  • TestBuildCustomSafeOutputScriptsJSONPathTraversal: 5 adversarial keys verified absent from output JSON
  • TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe: safe names preserved when mixed with traversal names

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

Copilot AI and others added 2 commits March 22, 2026 14:57
…t_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
Copilot AI changed the title [WIP] Fix unsanitized path in dynamic require() for security fix: path traversal sanitization for scriptFilename in safe_output_handler_manager Mar 22, 2026
Copilot AI requested a review from pelikhan March 22, 2026 15:07
Copy link
Copy Markdown
Collaborator

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot add adverserial tests

const customScriptHandlers = loadCustomSafeOutputScriptHandlers();
if (customScriptHandlers.size > 0) {
core.info(`Loading ${customScriptHandlers.size} custom script handler(s): ${[...customScriptHandlers.keys()].join(", ")}`);
const nodePath = require("path");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot require top of file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved require("path") to a module-level constant at the top of the file (commit 870ee71). The inline const nodePath = require("path") inside loadHandlers is gone — the function now uses the module-level nodePath.

…l 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
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 22, 2026

@copilot add adverserial tests

Added adversarial tests in commit 870ee71:

JavaScript (safe_output_handler_manager.test.cjs) — 7 new tests in loadHandlers - path traversal sanitization:

  • Leading ../core.error called, handler skipped
  • Embedded / separator → rejected
  • Absolute Unix path (/etc/passwd) → rejected
  • Multi-level traversal (../../etc/shadow) → rejected
  • Just .. → rejected
  • Mixed map (evil + safe entry) → evil rejected, safe attempted normally
  • Plain valid filename → no traversal error emitted

Go (safe_scripts_test.go) — 3 new test functions:

  • TestIsSafeScriptName: 11-case table covering valid names and all traversal patterns
  • TestBuildCustomSafeOutputScriptsJSONPathTraversal: 5 adversarial keys verified absent from output JSON
  • TestBuildCustomSafeOutputScriptsJSONPathTraversalMixedWithSafe: verifies safe names survive alongside filtered traversal names

Copilot AI requested a review from pelikhan March 22, 2026 15:21
@pelikhan pelikhan marked this pull request as ready for review March 22, 2026 15:27
Copilot AI review requested due to automatic review settings March 22, 2026 15:27
@pelikhan
Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens custom safe-output script loading to prevent path traversal via GH_AW_SAFE_OUTPUT_SCRIPTS, adding both runtime JS checks (before require()) and compile-time Go validation plus new adversarial tests.

Changes:

  • Add path traversal sanitization when resolving/require()-ing custom script handlers in safe_output_handler_manager.cjs.
  • Add isSafeScriptName() validation in the Go workflow compiler to filter unsafe script names before emitting GH_AW_SAFE_OUTPUT_SCRIPTS.
  • Add adversarial test suites for both the JS loader and Go compiler validation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/workflow/safe_scripts.go Adds isSafeScriptName() and filters unsafe script names when building GH_AW_SAFE_OUTPUT_SCRIPTS JSON.
pkg/workflow/safe_scripts_test.go Adds table-driven tests for isSafeScriptName() and JSON emission behavior under adversarial keys.
actions/setup/js/safe_output_handler_manager.cjs Sanitizes custom script filenames before require() and asserts resolved paths remain under the expected base dir.
actions/setup/js/safe_output_handler_manager.test.cjs Adds a dedicated path traversal sanitization test suite for loadHandlers().
.github/aw/actions-lock.json Adds a pinned lock entry for github/gh-aw-actions/setup@v0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (safeFilename !== scriptFilename) {
core.error(`Invalid script filename for ${scriptType}: path traversal detected in "${scriptFilename}" — skipping`);
continue;
}
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sanitization still allows scriptFilename values like "" or "." which make nodePath.join(scriptBaseDir, safeFilename) resolve to scriptBaseDir and then require(scriptBaseDir) will load a directory (package.json/main or index.js). To fully constrain what can be required, explicitly reject empty/"." (and consider enforcing the expected .cjs extension/prefix for custom scripts) before building scriptPath.

Suggested change
}
}
// Reject empty or "." basenames, which would resolve to the base directory itself.
if (!safeFilename || safeFilename === ".") {
core.error(`Invalid script filename for ${scriptType}: "${scriptFilename}" is not a valid script file name — skipping`);
continue;
}
// Enforce the expected .cjs extension for custom script handlers.
if (nodePath.extname(safeFilename) !== ".cjs") {
core.error(`Invalid script filename for ${scriptType}: "${scriptFilename}" must have a .cjs extension — skipping`);
continue;
}

Copilot uses AI. Check for mistakes.
Comment on lines 110 to 118
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)
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSafeScriptName is only applied when building GH_AW_SAFE_OUTPUT_SCRIPTS, but normalizedName is also used to generate on-disk filenames for these scripts (see pkg/workflow/compiler_safe_outputs_job.go where safeOutputScriptFilename(normalizedName) is written via shell redirection). If an unsafe script name contains path separators or traversal patterns, the workflow could still attempt to write outside the intended directory even though the env-var mapping is skipped. Consider enforcing the same validation at the point scripts are compiled/emitted (or failing compilation) so unsafe names can’t affect any file paths.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 22, 2026

@copilot lint go https://github.com/github/gh-aw/actions/runs/23406251174/job/68085572313

Fixed in 6384cb3 — replaced the two assert.False(t, strings.Contains(...)) calls with assert.NotContains as required by testifylint.

@pelikhan pelikhan merged commit 794e266 into main Mar 22, 2026
53 checks passed
@pelikhan pelikhan deleted the copilot/fix-unsanitized-path-in-require branch March 22, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🚨 [SECURITY] Security Red Team Findings - 2026-03-22 (Weekly Full Scan)

3 participants