Skip to content

Skip private MCP cache during artifact redaction #65368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 6 commits into from
Oct 3, 2026
Merged

Skip private MCP cache during artifact redaction #65368

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca83c93
Initial plan
Copilot Oct 3, 2026
a318c5e
Plan fix for redaction permission failure
Copilot Oct 3, 2026
2068f0f
Avoid scanning private MCP cache during artifact redaction
Copilot Oct 3, 2026
163822f
Plan MCP log redaction review fixes
Copilot Oct 3, 2026
4c52bd1
Preserve MCP log secret redaction
Copilot Oct 3, 2026
3557234
Restore unrelated workflow lock to base
Copilot Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions actions/setup/js/redact_secrets.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ function findFiles(dir, extensions) {
const entries = fs.readdirSync(dir, { withFileTypes: true });
for (const entry of entries) {
const fullPath = path.join(dir, entry.name);
// MCP tool downloads are a private runtime cache, not an artifact source.
// The sandbox may own this directory with permissions the runner cannot traverse.
if (entry.isDirectory() && fullPath === "/tmp/gh-aw/aw-mcp") continue;

Copilot AI Oct 3, 2026

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.

Updated in 4c52bd1: removed the subtree skip so readable MCP logs are redacted. Added coverage for both direct and broad safe-artifact declarations; they remain included because their files are still scanned.

@github-actions github-actions Bot Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] This fix only skips the hardcoded /tmp/gh-aw/aw-mcp path, but findFiles is also called with ${RUNNER_TEMP}/gh-aw as a root (line 356). If the sandbox mounts an equivalent cache under RUNNER_TEMP, the same EACCES would recur there, unaddressed by this patch.

💡 Suggested fix

Compute both roots explicitly, e.g.:

const SANDBOX_MCP_CACHE_DIRS = new Set([
  "/tmp/gh-aw/aw-mcp",
  path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/aw-mcp"),
]);
...
if (entry.isDirectory() && SANDBOX_MCP_CACHE_DIRS.has(fullPath)) continue;

Also worth considering: rather than a single exact-path check, catching EACCES/EPERM specifically when recursing into subdirectories would guard against any sandbox-owned path (not just this one), which is the actual root cause described in the PR body (a permission error during traversal), not just this one directory.

@copilot please address this.

Copilot AI Oct 3, 2026

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.

Updated in 4c52bd1: EACCES and EPERM from directory reads are handled at any scanned root, including RUNNER_TEMP; readable directories continue to be scanned.

@github-actions github-actions Bot Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/codebase-design] The "/tmp/gh-aw/aw-mcp" path is a magic string duplicated from pkg/constants/constants.go's TmpAwMcpLogsDir (/tmp/gh-aw/aw-mcp/logs) and this file's own TMP_GH_AW_PATH constant in constants.cjs. A future rename of the cache dir only updates one side, silently breaking this skip check again.

💡 Suggested fix

This file already imports from ./error_helpers.cjs and ./error_codes.cjs; consider adding an AW_MCP_CACHE_DIR constant to constants.cjs (alongside TMP_GH_AW_PATH) and importing it here, so the Go and JS sides have a single documented source of truth (per the file header comment: "Where a constant has a counterpart in pkg/constants/constants.go, their values should be kept in sync").

@copilot please address this.

Copilot AI Oct 3, 2026

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.

Updated in 4c52bd1: removed the hard-coded cache path check entirely, so there is no duplicated path constant; readable MCP logs continue through normal redaction.

@github-actions github-actions Bot Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

findFiles is invoked for both /tmp/gh-aw (line 355) and ${RUNNER_TEMP}/gh-aw (line 356), but this skip only matches the literal /tmp/gh-aw/aw-mcp path. The MCP logs guardrail writes to constants.TmpAwMcpLogsDir (/tmp/gh-aw/aw-mcp/logs) unconditionally regardless of topology, so this specific match is safe for that call site — but if any future caller passes a different base dir (e.g. a ${RUNNER_TEMP}/gh-aw equivalent cache), the same EACCES regression would recur unguarded here. Consider matching on path.basename(fullPath) === "aw-mcp" (scoped to direct children of a gh-aw root) instead of a single absolute literal, so the fix generalizes to any base directory findFiles is called with.

@copilot please address this.

Copilot AI Oct 3, 2026

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.

Updated in 4c52bd1: the permission-error handling is root-independent and applies to any inaccessible directory, rather than matching one aw-mcp path.

if (entry.isSymbolicLink()) {
fs.unlinkSync(fullPath);
core.warning(`Removed symbolic link before artifact upload: ${fullPath}`);
Expand Down
22 changes: 22 additions & 0 deletions actions/setup/js/redact_secrets.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,28 @@ describe("redact_secrets.cjs", () => {
for (const key of Object.keys(process.env)) key.startsWith("SECRET_") && delete process.env[key];
}),
describe("main function integration", () => {
it("skips the sandbox-owned MCP cache while redacting artifact files", async () => {
const cache = path.join(tempDir, "aw-mcp");
const artifact = path.join(tempDir, "agent-stdio.log");
fs.mkdirSync(cache);
fs.writeFileSync(artifact, "secret-value");
process.env.GH_AW_SECRET_NAMES = "TEST_SECRET";
process.env.SECRET_TEST_SECRET = "secret-value";
const readDir = fs.readdirSync;
const readDirSpy = vi.spyOn(fs, "readdirSync").mockImplementation((dir, options) => {
if (dir === cache) throw Object.assign(new Error("permission denied"), { code: "EACCES" });
return readDir(dir, options);
});
try {
const modifiedScript = redactScript.replaceAll("/tmp/gh-aw/aw-mcp", cache).replace('findFiles("/tmp/gh-aw", targetExtensions)', `findFiles("${tempDir}", targetExtensions)`);
await eval(`(async () => { ${modifiedScript}; await main(); })()`);
expect(readDirSpy).not.toHaveBeenCalledWith(cache, expect.anything());
expect(fs.readFileSync(artifact, "utf8")).toBe("***REDACTED***");
expect(mockCore.setFailed).not.toHaveBeenCalled();
} finally {
readDirSpy.mockRestore();
}
});
(it("should scan for built-in patterns even when GH_AW_SECRET_NAMES is not set", async () => {
(await eval(`(async () => { ${redactScript}; await main(); })()`),
expect(mockCore.info).toHaveBeenCalledWith(`Starting secret redaction in /tmp/gh-aw and ${process.env.RUNNER_TEMP}/gh-aw directories`),
Expand Down
Loading