fix: anchor clearPrimitives boundary to working-directory, not GITHUB_WORKSPACE#8
Conversation
…_WORKSPACE The clearPrimitives security guard rejected directories outside GITHUB_WORKSPACE, blocking legitimate use cases where the action runs in an isolated temp directory (e.g., gh-aw activation jobs using /tmp/gh-aw/apm-workspace). Two layered fixes: 1. Early return when no .github/ directory exists — an empty directory already satisfies isolated mode's clean-slate contract. Zero risk, immediate unblock. 2. Anchor the path-traversal guard to the working directory itself instead of GITHUB_WORKSPACE. Each computed sub-path is validated to stay within the resolved working directory. This is actually stricter than the old check (which allowed any path under GITHUB_WORKSPACE). Adds tests covering: early return, primitive removal, non-primitive preservation, cross-workspace operation, and empty .github/ dirs.
There was a problem hiding this comment.
Pull request overview
This PR fixes isolated-mode cleanup so clearPrimitives works correctly when the action’s working-directory is outside GITHUB_WORKSPACE (e.g., a temp directory), and adds tests to cover the intended behavior.
Changes:
- Anchor
clearPrimitivessafety boundary checks to the resolved working directory instead ofGITHUB_WORKSPACE. - Add an early return when
.github/doesn’t exist (nothing to clear). - Add Jest coverage for isolated cleanup scenarios, including cross-workspace operation.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/runner.ts | Updates clearPrimitives boundary logic and adds early-return behavior for missing .github/. |
| src/tests/runner.test.ts | Adds unit tests covering cleanup behavior and cross-workspace execution. |
| dist/runner.d.ts | Updates type declarations to include clearPrimitives export/docs. |
| dist/index.js | Updates bundled output to reflect source changes (but currently contains a critical path construction issue). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ghDir = path.join(tmpDir, '.github'); | ||
| fs.mkdirSync(path.join(ghDir, 'agents'), { recursive: true }); | ||
| fs.writeFileSync(path.join(ghDir, 'agents', 'test.md'), '# agent'); | ||
|
|
||
| // Should NOT throw — the old code threw here | ||
| clearPrimitives(tmpDir); | ||
|
|
||
| expect(fs.existsSync(path.join(ghDir, 'agents'))).toBe(false); | ||
| expect(mockInfo).toHaveBeenCalledWith('Cleared .github/agents/'); | ||
|
|
||
| // Restore | ||
| if (prevWorkspace === undefined) { | ||
| delete process.env.GITHUB_WORKSPACE; | ||
| } else { | ||
| process.env.GITHUB_WORKSPACE = prevWorkspace; |
There was a problem hiding this comment.
This test mutates process.env.GITHUB_WORKSPACE and restores it at the end, but if clearPrimitives unexpectedly throws (or an assertion fails before the restore block), the env var will remain mutated and could affect later tests. Wrap the mutation/restoration in a try/finally so restoration is guaranteed.
| const ghDir = path.join(tmpDir, '.github'); | |
| fs.mkdirSync(path.join(ghDir, 'agents'), { recursive: true }); | |
| fs.writeFileSync(path.join(ghDir, 'agents', 'test.md'), '# agent'); | |
| // Should NOT throw — the old code threw here | |
| clearPrimitives(tmpDir); | |
| expect(fs.existsSync(path.join(ghDir, 'agents'))).toBe(false); | |
| expect(mockInfo).toHaveBeenCalledWith('Cleared .github/agents/'); | |
| // Restore | |
| if (prevWorkspace === undefined) { | |
| delete process.env.GITHUB_WORKSPACE; | |
| } else { | |
| process.env.GITHUB_WORKSPACE = prevWorkspace; | |
| try { | |
| const ghDir = path.join(tmpDir, '.github'); | |
| fs.mkdirSync(path.join(ghDir, 'agents'), { recursive: true }); | |
| fs.writeFileSync(path.join(ghDir, 'agents', 'test.md'), '# agent'); | |
| // Should NOT throw — the old code threw here | |
| clearPrimitives(tmpDir); | |
| expect(fs.existsSync(path.join(ghDir, 'agents'))).toBe(false); | |
| expect(mockInfo).toHaveBeenCalledWith('Cleared .github/agents/'); | |
| } finally { | |
| // Restore | |
| if (prevWorkspace === undefined) { | |
| delete process.env.GITHUB_WORKSPACE; | |
| } else { | |
| process.env.GITHUB_WORKSPACE = prevWorkspace; | |
| } |
ncc's bundler incorrectly rewrites path.resolve() calls with string literal arguments as __nccwpck_require__.ab concatenations, producing garbage paths. Switch subPath computation to path.join (which ncc handles correctly) and resolve separately for the traversal guard.
Problem
clearPrimitivesrejects anyworking-directoryoutsideGITHUB_WORKSPACE:This blocks gh-aw's activation job, which legitimately runs APM in an isolated temp directory
(
/tmp/gh-aw/apm-workspace) — not under the runner workspace.Triggered by: gh-aw run #22918207799
Root Cause
The security guard anchored its boundary check to
GITHUB_WORKSPACE, a runner-level concept.But the action's security scope is its
working-directory— these are different things.Fix (two layered changes)
1. Early return when no
.github/existsAn empty directory already satisfies isolated mode's "clean slate" contract.
No
.github/→ nothing to delete → skip entirely. Zero risk.2. Anchor path-traversal guard to working directory
Each computed sub-path (
.github/instructions,.github/skills, etc.) is validated to staywithin the resolved working directory — regardless of where that directory lives on the filesystem.
This is actually stricter than the old check:
GITHUB_WORKSPACE(e.g.,../../some-other-paththat resolved under it)Tests Added
5 test cases covering:
.github/exists.github/workflows/)/tmp/whileGITHUB_WORKSPACEis elsewhere).github/with no primitive subdirs