diff --git a/actions/setup/js/runtime_import.cjs b/actions/setup/js/runtime_import.cjs index bd62c7d093..583935de97 100644 --- a/actions/setup/js/runtime_import.cjs +++ b/actions/setup/js/runtime_import.cjs @@ -1043,6 +1043,78 @@ async function processRuntimeImports(content, workspaceDir, importedFiles = new return processedContent; } +/** + * Checks if a file path is allowed for @filepath expansion. + * Allowed paths must be absolute, clean (no .. or . traversal components in the raw + * path string — paths are refused rather than silently normalized), and within the + * workspace directory or /tmp/gh-aw. + * @param {string} filePath - The absolute file path to validate + * @param {string} workspaceDir - The GITHUB_WORKSPACE directory + * @returns {boolean} - True if the path is allowed + */ +function isAllowedFileReference(filePath, workspaceDir) { + // Must be absolute path + if (!path.isAbsolute(filePath)) return false; + + // Must be a clean path: refuse paths with .. or . traversal components rather than + // silently normalizing them. This enforces the requirement that the caller provides + // the canonical path (e.g. @/tmp/gh-aw/../etc/passwd is refused outright). + const parts = filePath.split("/"); + for (const part of parts) { + if (part === ".." || part === ".") return false; + } + + const normalizedPath = path.normalize(filePath); + + // Check if within workspace directory + const normalizedWorkspace = path.normalize(workspaceDir); + const rel = path.relative(normalizedWorkspace, normalizedPath); + if (!rel.startsWith("..") && !path.isAbsolute(rel)) return true; + + // Check if within /tmp/gh-aw + if (normalizedPath === "/tmp/gh-aw" || normalizedPath.startsWith("/tmp/gh-aw/")) return true; + + return false; +} + +/** + * Expands @/absolute/path file references in content by replacing them with file contents. + * Paths must be absolute, clean (no .. or . traversal), and within the workspace directory + * or /tmp/gh-aw. Invalid paths, paths outside allowed directories, and missing files are + * silently ignored (the reference is left as-is with a warning). + * @param {string} content - The content with potential @/path references + * @param {string} workspaceDir - The GITHUB_WORKSPACE directory path + * @returns {string} - Content with valid file references expanded + */ +function expandFileReferences(content, workspaceDir) { + // Pattern: @ followed by an absolute path (starts with /) + // Restricts to standard filesystem characters to avoid matching shell-special sequences. + // Distinguishes @/path from @username mentions (which don't start with /). + const pattern = /@(\/[a-zA-Z0-9_./-]+)/g; + + return content.replace(pattern, (match, filePath) => { + if (!isAllowedFileReference(filePath, workspaceDir)) { + core.info(`[expandFileReferences] Ignoring disallowed path reference: ${filePath}`); + return match; + } + + if (!fs.existsSync(filePath)) { + core.warning(`[expandFileReferences] File not found: ${filePath}`); + return match; + } + + try { + const fileContent = fs.readFileSync(filePath, "utf8"); + core.info(`[expandFileReferences] Expanded file reference: ${filePath} (${fileContent.length} chars)`); + return fileContent; + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + core.warning(`[expandFileReferences] Failed to read file ${filePath}: ${errorMessage}`); + return match; + } + }); +} + module.exports = { processRuntimeImports, processRuntimeImport, @@ -1055,4 +1127,6 @@ module.exports = { wrapExpressionsInTemplateConditionals, extractAndReplacePlaceholders, generatePlaceholderName, + isAllowedFileReference, + expandFileReferences, }; diff --git a/actions/setup/js/runtime_import.test.cjs b/actions/setup/js/runtime_import.test.cjs index 1a9c78d733..75d6a79f6d 100644 --- a/actions/setup/js/runtime_import.test.cjs +++ b/actions/setup/js/runtime_import.test.cjs @@ -15,6 +15,8 @@ const { wrapExpressionsInTemplateConditionals, extractAndReplacePlaceholders, generatePlaceholderName, + isAllowedFileReference, + expandFileReferences, } = require("./runtime_import.cjs"); describe("runtime_import", () => { let tempDir; @@ -1698,4 +1700,114 @@ describe("runtime_import", () => { expect(result).not.toContain("__GH_AW_"); }); }); + + describe("isAllowedFileReference", () => { + let wsDir; + beforeEach(() => { + wsDir = fs.mkdtempSync(path.join(os.tmpdir(), "isallowed-test-")); + }); + afterEach(() => { + fs.rmSync(wsDir, { recursive: true, force: true }); + }); + + it("should allow absolute path within workspace", () => { + expect(isAllowedFileReference(path.join(wsDir, "file.txt"), wsDir)).toBe(true); + }); + it("should allow absolute path in subdirectory of workspace", () => { + expect(isAllowedFileReference(path.join(wsDir, "sub", "file.txt"), wsDir)).toBe(true); + }); + it("should allow /tmp/gh-aw/ paths", () => { + expect(isAllowedFileReference("/tmp/gh-aw/agent/comment-body.md", wsDir)).toBe(true); + }); + it("should allow /tmp/gh-aw root path", () => { + expect(isAllowedFileReference("/tmp/gh-aw", wsDir)).toBe(true); + }); + it("should reject relative paths", () => { + expect(isAllowedFileReference("relative/path.txt", wsDir)).toBe(false); + }); + it("should reject paths with .. traversal", () => { + expect(isAllowedFileReference("/tmp/gh-aw/../etc/passwd", wsDir)).toBe(false); + }); + it("should reject paths with . component", () => { + expect(isAllowedFileReference("/tmp/gh-aw/./file.txt", wsDir)).toBe(false); + }); + it("should reject /etc/passwd", () => { + expect(isAllowedFileReference("/etc/passwd", wsDir)).toBe(false); + }); + it("should reject /tmp/ paths that are not /tmp/gh-aw", () => { + expect(isAllowedFileReference("/tmp/other/file.txt", wsDir)).toBe(false); + }); + it("should reject /tmp/gh-aw-evil path (not a prefix match confusion)", () => { + expect(isAllowedFileReference("/tmp/gh-aw-evil/file.txt", wsDir)).toBe(false); + }); + }); + + describe("expandFileReferences", () => { + let wsDir; + let ghAwDir; + beforeEach(() => { + wsDir = fs.mkdtempSync(path.join(os.tmpdir(), "expand-fileref-test-")); + ghAwDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-test-")); + vi.clearAllMocks(); + }); + afterEach(() => { + fs.rmSync(wsDir, { recursive: true, force: true }); + fs.rmSync(ghAwDir, { recursive: true, force: true }); + }); + + it("should expand a valid @/path reference within workspace", () => { + const filePath = path.join(wsDir, "data.txt"); + fs.writeFileSync(filePath, "file contents here"); + const result = expandFileReferences(`prefix @${filePath} suffix`, wsDir); + expect(result).toBe("prefix file contents here suffix"); + }); + it("should expand @/path at start of line (standalone reference)", () => { + const filePath = path.join(wsDir, "body.md"); + fs.writeFileSync(filePath, "# Body\n\nContent."); + const result = expandFileReferences(`@${filePath}`, wsDir); + expect(result).toBe("# Body\n\nContent."); + }); + it("should leave @username mentions unchanged", () => { + const result = expandFileReferences("@copilot please review", wsDir); + expect(result).toBe("@copilot please review"); + }); + it("should leave disallowed paths unchanged and log info", () => { + const result = expandFileReferences("See @/etc/passwd for details", wsDir); + expect(result).toBe("See @/etc/passwd for details"); + expect(core.info).toHaveBeenCalledWith(expect.stringContaining("Ignoring disallowed path reference: /etc/passwd")); + }); + it("should leave non-existent allowed paths unchanged and warn", () => { + const filePath = path.join(wsDir, "missing.txt"); + const result = expandFileReferences(`@${filePath}`, wsDir); + expect(result).toBe(`@${filePath}`); + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining(`File not found: ${filePath}`)); + }); + it("should expand multiple @/path references in the same content", () => { + const file1 = path.join(wsDir, "a.txt"); + const file2 = path.join(wsDir, "b.txt"); + fs.writeFileSync(file1, "AAA"); + fs.writeFileSync(file2, "BBB"); + const result = expandFileReferences(`@${file1} and @${file2}`, wsDir); + expect(result).toBe("AAA and BBB"); + }); + it("should reject paths with .. traversal even when within allowed prefix", () => { + const result = expandFileReferences(`@${wsDir}/../etc/passwd`, wsDir); + expect(result).toBe(`@${wsDir}/../etc/passwd`); + }); + it("should reject URL-encoded traversal paths", () => { + // URL-encoded .. (%2e%2e) should not be expanded since the regex only + // matches [a-zA-Z0-9_./-] — % is excluded so the reference won't even match + const result = expandFileReferences("@/tmp/gh-aw/%2e%2e/etc/passwd", wsDir); + expect(result).toBe("@/tmp/gh-aw/%2e%2e/etc/passwd"); + }); + it("should reject mixed-separator traversal paths", () => { + const result = expandFileReferences(`@${wsDir}/subdir/../../etc/passwd`, wsDir); + expect(result).toBe(`@${wsDir}/subdir/../../etc/passwd`); + }); + it("should leave content without @/path unchanged", () => { + const input = "No file references here. ${{ github.actor }}"; + const result = expandFileReferences(input, wsDir); + expect(result).toBe(input); + }); + }); }); diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 7887e59095..e5cc04d9e4 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -24,6 +24,7 @@ const { createManifestLogger, ensureManifestExists, extractCreatedItemFromResult const { loadCustomSafeOutputJobTypes, loadCustomSafeOutputScriptHandlers, loadCustomSafeOutputActionHandlers, isStagedMode } = require("./safe_output_helpers.cjs"); const { emitSafeOutputActionOutputs } = require("./safe_outputs_action_outputs.cjs"); const { listCommentMemoryFiles, COMMENT_MEMORY_DIR } = require("./comment_memory_helpers.cjs"); +const { expandFileReferences } = require("./runtime_import.cjs"); const nodePath = require("path"); const fs = require("fs"); @@ -564,6 +565,19 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } } + // Pre-process: expand @/absolute/path file references in the message body. + // Agents may write a path like @/tmp/gh-aw/agent/comment-body.md as the body + // value; this resolves the file and inlines its contents before the handler runs. + // Only paths within GITHUB_WORKSPACE or /tmp/gh-aw are expanded; all others are ignored. + if (effectiveMessage.body && typeof effectiveMessage.body === "string") { + const workspaceDir = process.env.GITHUB_WORKSPACE || ""; + const expandedBody = expandFileReferences(effectiveMessage.body, workspaceDir); + if (expandedBody !== effectiveMessage.body) { + effectiveMessage = { ...effectiveMessage, body: expandedBody }; + core.info(`Expanded @filepath reference(s) in ${messageType} body`); + } + } + // Call the message handler with the individual message and resolved temp IDs const result = await messageHandler(effectiveMessage, resolvedTemporaryIds, temporaryIdMap); diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index dc80c292da..0e748e6089 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -2,6 +2,8 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import fs from "fs"; +import os from "os"; +import path from "path"; import { loadConfig, loadHandlers, processMessages, buildCommentMemoryMessagesFromFiles } from "./safe_output_handler_manager.cjs"; describe("Safe Output Handler Manager", () => { @@ -706,6 +708,92 @@ describe("Safe Output Handler Manager", () => { expect(result.outputsWithUnresolvedIds.length).toBe(0); }); + describe("@filepath expansion in message body", () => { + let tempWorkspace; + + beforeEach(() => { + tempWorkspace = fs.mkdtempSync(path.join(os.tmpdir(), "handler-manager-ws-")); + process.env.GITHUB_WORKSPACE = tempWorkspace; + }); + + afterEach(() => { + delete process.env.GITHUB_WORKSPACE; + fs.rmSync(tempWorkspace, { recursive: true, force: true }); + }); + + it("should expand @filepath reference in add_comment body to file contents", async () => { + const bodyFile = path.join(tempWorkspace, "comment-body.md"); + fs.writeFileSync(bodyFile, "# Hello\n\nThis is the comment body."); + + const messages = [{ type: "add_comment", body: `@${bodyFile}` }]; + + const mockHandler = vi.fn().mockResolvedValue({ success: true }); + const handlers = new Map([["add_comment", mockHandler]]); + + await processMessages(handlers, messages); + + const calledMessage = mockHandler.mock.calls[0][0]; + expect(calledMessage.body).toBe("# Hello\n\nThis is the comment body."); + expect(calledMessage.body).not.toContain("@/"); + }); + + it("should expand @filepath reference in create_issue body to file contents", async () => { + const bodyFile = path.join(tempWorkspace, "issue-body.md"); + fs.writeFileSync(bodyFile, "Issue body content."); + + const messages = [{ type: "create_issue", title: "My Issue", body: `@${bodyFile}` }]; + + const mockHandler = vi.fn().mockResolvedValue({ repo: "owner/repo", number: 42 }); + const handlers = new Map([["create_issue", mockHandler]]); + + await processMessages(handlers, messages); + + const calledMessage = mockHandler.mock.calls[0][0]; + expect(calledMessage.body).toBe("Issue body content."); + }); + + it("should allow @filepath reference from /tmp/gh-aw when GITHUB_WORKSPACE is set", async () => { + // Create a temp file simulating /tmp/gh-aw by writing to tempWorkspace/subdir instead + // (actual /tmp/gh-aw may not be writable in all CI environments; we test via workspace) + const bodyFile = path.join(tempWorkspace, "agent-body.md"); + fs.writeFileSync(bodyFile, "Agent comment body."); + + const messages = [{ type: "add_comment", body: `@${bodyFile}` }]; + + const mockHandler = vi.fn().mockResolvedValue({ success: true }); + const handlers = new Map([["add_comment", mockHandler]]); + + await processMessages(handlers, messages); + + const calledMessage = mockHandler.mock.calls[0][0]; + expect(calledMessage.body).toBe("Agent comment body."); + }); + + it("should leave body unchanged when no @filepath reference is present", async () => { + const messages = [{ type: "add_comment", body: "Plain comment body without file refs." }]; + + const mockHandler = vi.fn().mockResolvedValue({ success: true }); + const handlers = new Map([["add_comment", mockHandler]]); + + await processMessages(handlers, messages); + + const calledMessage = mockHandler.mock.calls[0][0]; + expect(calledMessage.body).toBe("Plain comment body without file refs."); + }); + + it("should leave disallowed @filepath references unchanged", async () => { + const messages = [{ type: "add_comment", body: "See @/etc/passwd for details." }]; + + const mockHandler = vi.fn().mockResolvedValue({ success: true }); + const handlers = new Map([["add_comment", mockHandler]]); + + await processMessages(handlers, messages); + + const calledMessage = mockHandler.mock.calls[0][0]; + expect(calledMessage.body).toBe("See @/etc/passwd for details."); + }); + }); + it("should silently skip message types handled by standalone steps", async () => { const messages = [ { type: "create_issue", title: "Issue" },