Repository navigation
fix(safe-outputs): restrict upload_artifact to canonical allowed roots, reject sensitive paths - #50779
Conversation
…ensitive paths - Add validateUploadSourcePath/validateSourcePath helpers that reject paths under /etc, system dirs, .git directory components, and HOME credential locations (.ssh, .aws, .gnupg, .docker, etc.) - Add validateAllowedRoot to restrict absolute paths to staging dir, GITHUB_WORKSPACE, and RUNNER_TEMP only - Canonicalize paths with fs.realpathSync before policy checks to prevent traversal escapes and symlink chains - Apply restrictive permissions (0o600) on all staged files in both safe_outputs_handlers.cjs and upload_artifact.cjs copySingleFileToStaging - Clean up upload and download temp directories in artifact_client.cjs using try/finally blocks in success and failure paths - Add security tests covering /etc rejection, .git component rejection, out-of-bounds root rejection, allowed workspace paths, and 0o600 perms - Update auto-copy tests to set GITHUB_WORKSPACE so they match the new allowed-roots policy Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…alpathSync error - Use canonicalFilePath (not filePath) in the allowed-root error message so the resolved path is always shown, preventing misleading messages on traversals - Replace silent path.resolve fallbacks with explicit errors when fs.realpathSync fails, eliminating any risk of a non-canonical path bypassing validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Thanks for working on artifact upload security! 🔐 This PR is properly scoped around securing artifact uploads to canonical roots, which aligns well with the security focus of #50770. Since this is still in draft with pending implementation tasks, here's what will get this across the finish line:
Looks like you have a solid plan in place. Keep pushing forward! 🚀
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0). |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
🟡 Changes recommended
Sensitive nested paths and runner temporary files remain uploadable, while cleanup and canonical-root handling are incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Restricts artifact uploads to approved roots, blocks sensitive paths, hardens staging permissions, and cleans temporary artifact directories.
Changes:
- Adds canonical path and sensitive-location validation.
- Applies
0600permissions to staged files. - Adds temporary-directory cleanup and security tests.
File summaries
| File | Description |
|---|---|
actions/setup/js/upload_artifact.test.cjs |
Tests path policy and permissions. |
actions/setup/js/upload_artifact.cjs |
Validates and stages upload sources. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests handler-side security checks. |
actions/setup/js/safe_outputs_handlers.cjs |
Enforces source restrictions during staging. |
actions/setup/js/artifact_client.cjs |
Cleans upload/download temporary directories. |
Review details
Suppressed comments (4)
actions/setup/js/safe_outputs_handlers.cjs:2230
- Directory requests validate only the requested directory itself.
copyDirectoryRecursivethen copies nested entries without calling this validator, so uploading the workspace root will stage.git/configeven though a direct request for that file is rejected; nested symlinks are also silently skipped instead of rejecting the request as required. Validate every recursive entry and fail the whole request on a denied path or symlink.
// Reject sensitive paths (system dirs, .git, HOME credentials).
const sensitiveError = validateUploadSourcePath(canonicalFilePath);
actions/setup/js/safe_outputs_handlers.cjs:2246
- Allowing the entire
RUNNER_TEMPtree preserves a sensitive-file exfiltration path: it contains runner command files such asGITHUB_ENV/GITHUB_OUTPUTand may contain action-created credentials. The linked issue limits defaults to the staging directory and configured workspace paths; allow only the staging subdirectory underRUNNER_TEMP, not the whole temp root.
if (process.env.RUNNER_TEMP) {
allowedRoots.push(path.resolve(process.env.RUNNER_TEMP));
}
actions/setup/js/upload_artifact.cjs:176
- Adding all of
RUNNER_TEMPas an allowed root permits uploads of runner command files (GITHUB_ENV,GITHUB_OUTPUT) and action-created temporary credentials, which defeats the explicit-root restriction. Keep the staging directory allowed, but do not authorize unrelated files merely because they share its temp parent.
if (process.env.RUNNER_TEMP) {
allowedRoots.push(path.resolve(process.env.RUNNER_TEMP));
}
actions/setup/js/safe_outputs_handlers.cjs:2245
path.resolveis lexical, whilecanonicalFilePathcomes fromrealpathSync. If an allowed workspace or temp root is itself a symlink, an ordinary file canonicalizes under the target path and is incorrectly rejected because it cannot match the unresolved root. Canonicalize existing allowed roots withrealpathSyncbefore comparing them.
const allowedRoots = [path.resolve(stagingDir)];
if (process.env.GITHUB_WORKSPACE) {
allowedRoots.push(path.resolve(process.env.GITHUB_WORKSPACE));
}
if (process.env.RUNNER_TEMP) {
allowedRoots.push(path.resolve(process.env.RUNNER_TEMP));
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| const sensitiveErr = validateSourcePath(canonical); | ||
| if (sensitiveErr) { | ||
| return { copied: false, relPath: "", error: sensitiveErr }; | ||
| } | ||
| const rootErr = validateAllowedRoot(canonical); |
| // Canonicalize to detect traversal escapes and symlink chains. | ||
| let canonicalFilePath; | ||
| try { | ||
| canonicalFilePath = fs.realpathSync(filePath); | ||
| } catch (err) { |
| function validateAllowedRoot(canonicalPath) { | ||
| const allowedRoots = [path.resolve(STAGING_DIR)]; | ||
| if (process.env.GITHUB_WORKSPACE) { | ||
| allowedRoots.push(path.resolve(process.env.GITHUB_WORKSPACE)); | ||
| } | ||
| if (process.env.RUNNER_TEMP) { | ||
| allowedRoots.push(path.resolve(process.env.RUNNER_TEMP)); |
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-artifact-upload-")); | ||
| uploadPath = path.join(tmpDir, `${artifactName || "artifact"}.zip`); | ||
| createZipFromFiles(files, rootDirectory, uploadPath); |
| try { | ||
| fs.copyFileSync(sourcePath, destPath); | ||
| fs.chmodSync(destPath, 0o600); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: fix(safe-outputs): restrict upload_artifact to canonical allowed roots, reject sensitive paths
This is a solid security improvement — mandatory root enforcement and sensitive-path rejection close real exfiltration vectors. The temp-directory cleanup (try/finally) and 0o600 staging permissions are good hygiene. A few issues worth noting:
Blocking
RUNNER_TEMP as an allowed root undermines the boundary
On GitHub-hosted runners RUNNER_TEMP resolves to /home/runner/work/_temp — but any path an agent writes to $RUNNER_TEMP can now be uploaded without restriction. The sensitive-path check only blocks a known allow-list of names; it won't catch arbitrary agent-created temp files that happen to contain sensitive data. Allowing the entire RUNNER_TEMP tree as a source root largely negates the workspace boundary.
Consider restricting source roots to the explicit staging dir + GITHUB_WORKSPACE only, and requiring that any temp-file upload be explicitly staged first.
validateSourcePath / validateUploadSourcePath are identical and duplicated
The two functions are byte-for-byte the same across safe_outputs_handlers.cjs and upload_artifact.cjs. Additions to one won't automatically apply to the other, creating a maintenance/divergence risk. Extract into safe_output_helpers.cjs (already used for isStagedMode).
Non-blocking
Directory upload only validates the top-level path — when a directory passes root/sensitive-path checks, its individual files are not re-validated. A sensitive file placed inside an allowed-root directory won't be caught. A comment documenting this assumption would help.
/tmp fallback for stagingDir — the process.env.RUNNER_TEMP || "/tmp" fallback on line 239 of safe_outputs_handlers.cjs means the staging dir can fall under /tmp in non-runner environments, overlapping with the system temp tree in an unexpected way.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.4 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 97/100 — Excellent
📊 Metrics (9 tests)
Verdict
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a few correctness and maintainability issues.
📋 Key Themes & Highlights
Key Themes
os.homedir()not symlink-resolved —path.resolve(homeDir)only normalises the string;realpathSyncmust be used to match against the already-resolvedcanonicalPath. Silent bypass on macOS runners.- Duplicated security logic —
validateSourcePath/validateUploadSourcePathare copy-pasted across two files. Future changes to the deny list must be made in two places; divergence is a bypass risk. - No per-file validation inside
copyDirectoryRecursive— top-level directory validation is correct but individual files inside are not re-canonicalized before copying. - Incomplete env-var cleanup in one test —
RUNNER_TEMPis deleted but not restored after the out-of-bounds test, which can cause downstream test pollution.
Positive Highlights
- ✅ Excellent defense-in-depth: allowed-root enforcement and sensitive-path deny list
- ✅
realpathSyncused correctly at the entry point before any check — prevents symlink traversal - ✅
try/finallytemp dir cleanup inartifact_client.cjsis solid - ✅ Comprehensive security test suite covering all major rejection categories
- ✅
0o600staging permissions are a good hygiene addition - ✅ Clear, actionable error messages for each rejection category
| } | ||
| } | ||
|
|
||
| const homeDir = os.homedir(); |
There was a problem hiding this comment.
[/diagnosing-bugs] path.resolve(homeDir) normalises the string but does not follow symlinks — on macOS os.homedir() often returns a symlink path (e.g. /Users/runner→/private/Users/runner), so the .ssh prefix check may never match a realpathSync-resolved canonical path.
💡 Suggested fix
Resolve the home directory with realpathSync once (outside the loop), matching exactly how canonicalPath was produced:
let resolvedHome = homeDir;
try { resolvedHome = fs.realpathSync(homeDir); } catch {}
for (const name of sensitiveNames) {
const sensitive = path.join(resolvedHome, name);
...
}Apply the same fix to the mirror function in safe_outputs_handlers.cjs.
@copilot please address this.
| * Validate that a canonical absolute path does not refer to sensitive system or credential | ||
| * locations. Mirrors the check in safe_outputs_handlers.cjs for defense-in-depth. | ||
| * | ||
| * @param {string} canonicalPath - Resolved absolute path |
There was a problem hiding this comment.
[/diagnosing-bugs] The validateSourcePath function is a near-identical duplicate of validateUploadSourcePath in safe_outputs_handlers.cjs. Divergence between the two copies (e.g. one gets a new sensitive name, the other does not) is a likely source of future bypass.
💡 Suggested fix
Extract the shared validation logic into a common helper module (e.g. path_security_helpers.cjs) and require it from both files. This is a shallow interface / deep implementation — the module can expose:
// path_security_helpers.cjs
module.exports = { validateSensitivePath, validateAllowedRoot };Both callers keep their own error-formatting layer but delegate the security logic to the shared module.
@copilot please address this.
| @@ -241,6 +300,23 @@ function autoCopyToStaging(reqPath) { | |||
| if (stat === null) { | |||
There was a problem hiding this comment.
[/tdd] The autoCopyToStaging path that handles a directory (not a file) is validated for root/sensitive paths, but the copyDirectoryRecursive helper does not re-validate individual files inside the directory. A hardlink or bind-mount inside an otherwise-allowed directory could bypass the per-entry symlink check if the dirent reports isFile() rather than isSymbolicLink().
💡 Suggested test + fix
Add a test that places a file outside allowed roots inside an otherwise-valid directory via a hardlink, and confirm it is rejected. For defense-in-depth, copyDirectoryRecursive should call validateSensitivePath(fs.realpathSync(srcPath)) before each copyFileSync (the canonical path of each entry might escape the parent directory on some overlay filesystems).
@copilot please address this.
| const sensitive = path.join(path.resolve(homeDir), name); | ||
| if (canonicalPath === sensitive || canonicalPath.startsWith(sensitive + path.sep)) { | ||
| return `path refers to a sensitive HOME location: ${canonicalPath}`; | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The system-denied list (/lib, /lib64, /usr/lib, /usr/local/lib) uses string prefix matching after path.resolve, but /usr/local/lib is a strict prefix of /usr/local/lib64 on some Linux distributions. The check canonicalPath.startsWith("/usr/local/lib" + sep) correctly handles this, but the static list approach will silently miss any new system dirs (e.g. /snap, /nix, /opt) that hold sensitive credentials on non-standard runner images.
💡 Suggested improvement
Consider adding /snap, /nix/store, and /opt to the deny list, and add a brief comment explaining that the list is conservative-but-not-exhaustive, with the allowed-root check as the primary defence. The deny list is secondary hardening, not the sole gatekeeper.
@copilot please address this.
|
|
||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("system directory")); | ||
| expect(mockArtifactClient.uploadArtifact).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The test "rejects an absolute path outside allowed roots" unconditionally calls delete process.env.RUNNER_TEMP but does not restore it in finally. If RUNNER_TEMP was set by the test environment, subsequent tests in the same process run without it, which could cause unrelated failures.
💡 Suggested fix
const savedRunnerTemp = process.env.RUNNER_TEMP;
delete process.env.RUNNER_TEMP;
try {
// ... assertions
} finally {
if (savedRunnerTemp !== undefined) process.env.RUNNER_TEMP = savedRunnerTemp;
}The same pattern is correctly applied for GITHUB_WORKSPACE in the outer beforeEach/afterEach — apply it consistently for RUNNER_TEMP too.
@copilot please address this.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please run the pr-finisher skill and address the active review feedback before maintainer review:
Failed check: agent — https://github.com/github/gh-aw/actions/runs/31075406960/job/92532429104
|
… recursive copy Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…puts_handlers Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all feedback in commits
All 293 targeted JS tests pass (58 |
|
🎉 This pull request is included in a new release. Release: |
The
upload_artifacthandler accepted any absolute path, allowing agents to stage and upload arbitrary host files (/etc/passwd,$HOME/.ssh/id_rsa,.git/config, etc.) with no root restriction beyond symlink rejection.Changes
Path validation (
safe_outputs_handlers.cjs,upload_artifact.cjs)GITHUB_WORKSPACE,RUNNER_TEMP, or the staging directory; everything else is rejected with an actionable errorvalidateUploadSourcePath/validateSourcePath:.gitdirectory components/etc,/proc,/sys,/dev,/run,/boot,/lib*,/usr/lib*).ssh,.aws,.gnupg,.docker,.kube,.netrc,.npmrc,.gitconfig, etc.)fs.realpathSyncresolves symlink chains before any check; failure throws rather than falling back topath.resolveRestrictive staging permissions
fs.chmodSync(destPath, 0o600)applied after every file copy incopySingleFileToStagingandcopyDirectoryRecursive.Temp directory cleanup (
artifact_client.cjs)Upload (
gh-aw-artifact-upload-*) and download (gh-aw-artifact-download-*) temp directories are now removed intry/finallyblocks, covering both success and failure paths.Tests
New
path validation (security)describe blocks in both test files cover:/etcrejection,.git-component rejection, out-of-bounds root rejection, allowed-workspace path acceptance, and0o600permission verification. Existing auto-copy tests updated to setGITHUB_WORKSPACEto match the new policy.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.