diff --git a/packages/cache/__tests__/pathValidation.test.ts b/packages/cache/__tests__/pathValidation.test.ts index 860c7b27..f8b57cad 100644 --- a/packages/cache/__tests__/pathValidation.test.ts +++ b/packages/cache/__tests__/pathValidation.test.ts @@ -74,6 +74,22 @@ describe('deriveAllowedRoots', () => { const negated = IS_WINDOWS ? '!C:\\foo\\secret' : '!/foo/secret' expect(deriveAllowedRoots([allowed, negated], cwd)).toEqual([allowed]) }) + + test('non-leading ! is treated as a literal path character, not a glob', () => { + // Regression: an earlier implementation included `!` in the glob + // metachar set, so a declared path like `cache/my!dir/data` would + // truncate to `cache/`, silently broadening the allowed root. + // `@actions/glob`/minimatch treats `!` literally except as a leading + // negation (handled by the case above), so non-leading `!` must be + // preserved verbatim in the derived root. + const input = IS_WINDOWS + ? 'C:\\cache\\my!dir\\data' + : '/cache/my!dir/data' + const expected = IS_WINDOWS + ? 'C:\\cache\\my!dir\\data' + : '/cache/my!dir/data' + expect(deriveAllowedRoots([input], cwd)).toEqual([expected]) + }) }) describe('path expansion', () => { @@ -588,7 +604,7 @@ describe('validateEntry', () => { }) describe('symlink/hardlink attacks (must reject)', () => { - test('symlink with absolute target', () => { + test('symlink with POSIX absolute target', () => { const r = validateEntry( 'node_modules/link', '/etc/passwd', @@ -597,7 +613,72 @@ describe('validateEntry', () => { cwd ) expect(r.ok).toBe(false) - if (!r.ok) expect(r.code).toBe('LINK_OUTSIDE_ROOTS') + // Syntactic rejection fires before the containment check. + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('symlink with Windows absolute target', () => { + const r = validateEntry( + 'node_modules/link', + 'C:\\Windows\\System32\\drivers\\etc\\hosts', + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('symlink with Windows drive-relative target', () => { + // `C:foo` resolves against the per-drive working directory on Windows, + // not the symlink's containing directory — so even if it happens to + // land under an allowed root after `path.resolve`, its extract-time + // semantics are different. Reject the syntactic form outright. + const r = validateEntry( + 'node_modules/link', + 'C:foo', + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('symlink with UNC target', () => { + const r = validateEntry( + 'node_modules/link', + '\\\\attacker\\share\\payload', + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('UNC_PATH') + }) + + test('symlink with UNC long-path prefix target', () => { + const r = validateEntry( + 'node_modules/link', + '\\\\?\\C:\\foo', + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('UNC_PATH') + }) + + test('symlink with forward-slash UNC target', () => { + const r = validateEntry( + 'node_modules/link', + '//attacker/share/payload', + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('UNC_PATH') }) test('symlink with .. traversal', () => { @@ -612,7 +693,7 @@ describe('validateEntry', () => { if (!r.ok) expect(r.code).toBe('LINK_OUTSIDE_ROOTS') }) - test('hardlink with absolute target', () => { + test('hardlink with POSIX absolute target', () => { const r = validateEntry( 'node_modules/dup', '/etc/passwd', @@ -621,7 +702,31 @@ describe('validateEntry', () => { cwd ) expect(r.ok).toBe(false) - if (!r.ok) expect(r.code).toBe('LINK_OUTSIDE_ROOTS') + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('hardlink with Windows absolute target', () => { + const r = validateEntry( + 'node_modules/dup', + 'C:\\Windows\\System32\\drivers\\etc\\hosts', + 'Link', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('hardlink with UNC target', () => { + const r = validateEntry( + 'node_modules/dup', + '\\\\attacker\\share\\payload', + 'Link', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('UNC_PATH') }) test('hardlink with .. traversal', () => { @@ -641,9 +746,11 @@ describe('validateEntry', () => { // Entry 1: node_modules/x → /etc (a symlink target outside allowed roots) // Entry 2: node_modules/x/payload (which extracts THROUGH entry 1's link // and lands at /etc/payload) - // We must reject Entry 1 because its target is outside the allowed roots. - // Entry 2's nominal path looks safe, so we cannot rely on per-entry path - // validation alone — we must catch the symlink target. + // We must reject Entry 1. The absolute syntactic form `/etc` is now + // rejected up-front (ABSOLUTE_PATH) before the containment check, which + // is strictly stronger than the previous LINK_OUTSIDE_ROOTS reject + // (the entry never gets the chance to be "absolute but happens to + // resolve under a root"). const r = validateEntry( 'node_modules/x', '/etc', @@ -652,7 +759,28 @@ describe('validateEntry', () => { cwd ) expect(r.ok).toBe(false) - if (!r.ok) expect(r.code).toBe('LINK_OUTSIDE_ROOTS') + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') + }) + + test('symlink with absolute target that nominally lands under an allowed root is still rejected', () => { + // Before the syntactic fix, a symlink whose absolute target happened + // to resolve under an allowed root (e.g. POSIX `/workspace/.cache/x` + // when /workspace/.cache is allowed) would pass validation. With the + // syntactic reject, the absolute form alone is enough to fail — we + // don't trust that extract-time resolution will match what + // `path.resolve` says at validation time. + const absoluteUnderRoot = IS_WINDOWS + ? 'C:\\workspace\\.cache\\x' + : '/workspace/.cache/x' + const r = validateEntry( + 'node_modules/link', + absoluteUnderRoot, + 'SymbolicLink', + allowedRoots, + cwd + ) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('ABSOLUTE_PATH') }) test('symlink with empty target is allowed (filtered as undefined linkpath)', () => { diff --git a/packages/cache/docs/path-validation-test-plan.md b/packages/cache/docs/path-validation-test-plan.md index 619e8fcd..601d9c54 100644 --- a/packages/cache/docs/path-validation-test-plan.md +++ b/packages/cache/docs/path-validation-test-plan.md @@ -37,7 +37,7 @@ integrity failures from ordinary cache-miss/network errors. | [`src/options.ts`](../src/options.ts) (new `pathValidation` field) | [`__tests__/options.test.ts`](../__tests__/options.test.ts) | | [`src/cache.ts`](../src/cache.ts) (forwarding + error re-throw) | [`__tests__/restoreCache.test.ts`](../__tests__/restoreCache.test.ts), [`__tests__/restoreCacheV2.test.ts`](../__tests__/restoreCacheV2.test.ts) | -## Unit tests — pure logic (`pathValidation.test.ts`, 86 cases) +## Unit tests — pure logic (`pathValidation.test.ts`) These exercise the platform-agnostic validation logic with no filesystem or network. The suite is split into two groups. @@ -84,12 +84,22 @@ and platform-specific behavior: UNC long-path prefix `\\?\C:\...` - **NUL byte attacks**: NUL in path, NUL in symlink target - **Symlink attacks**: - - Absolute symlink target - - Symlink target with `..` traversal + - **Syntactic link-target rejects** (these fire before the containment check, + on the same allow-list as entry-path syntax): POSIX absolute (`/etc/passwd`), + Windows absolute (`C:\Windows\…`), Windows drive-relative (`C:foo` — has + per-drive-CWD semantics on Windows), backslash UNC (`\\srv\share\x`), + forward-slash UNC (`//srv/share/x`), UNC long-path prefix (`\\?\C:\…`) + - **Absolute target that nominally lands under an allowed root is still + rejected** — defense-in-depth regression: `path.resolve` agreeing with + the allowed root at validation time isn't trusted, because Windows + extract-time resolution semantics can differ + - Symlink target with `..` traversal (rejected as `LINK_OUTSIDE_ROOTS`) - **Symlink-then-write-through-link** (the critical TOCTOU-style attack: archive declares `cache/link → /tmp/evil` followed by `cache/link/file`) - Self-referential symlink to `.` -- **Hardlink attacks**: absolute target, `..` traversal +- **Hardlink attacks**: + - Syntactic link-target rejects: POSIX absolute, Windows absolute, UNC + - `..` traversal (rejected as `LINK_OUTSIDE_ROOTS`) - **Unsupported entry types**: `CharacterDevice`, `BlockDevice`, `FIFO` - **Header-only types accepted unconditionally** (these carry no path content): `GlobalExtendedHeader`, `ExtendedHeader`, `NextFileHasLongPath`, `NextFileHasLongLinkpath` @@ -100,7 +110,7 @@ and platform-specific behavior: - Shows up to N items verbatim - Truncates the tail with a `(... and N more)` summary line -## Integration tests — real archives (`listAndValidate.test.ts`, 14 cases) +## Integration tests — real archives (`listAndValidate.test.ts`) These build small tar archives in memory using `tar.Header`, write them to disk, and run them through the production parser. They cover the gzip and zstd @@ -126,7 +136,7 @@ Skipped on hosts without `zstd` installed. - Traversal in zstd archive → 1 violation - `ZstdWithoutLong` compression method also works -## Integration tests — mocked downstream (`tarPathValidation.test.ts`, 15 cases) +## Integration tests — mocked downstream (`tarPathValidation.test.ts`) These mock `listAndValidate` so the test can deterministically inject "violation lists" and observe `extractTar`'s reaction. They mock `@actions/exec`, @@ -194,6 +204,3 @@ npx jest --testTimeout 70000 \ packages/cache/__tests__/listAndValidate.test.ts \ packages/cache/__tests__/tarPathValidation.test.ts ``` - -Total path-validation test cases: **115** (86 unit + 14 real-archive + 15 mocked). -Combined with the regression updates, the cache package runs **237 tests** total. diff --git a/packages/cache/src/internal/pathValidation.ts b/packages/cache/src/internal/pathValidation.ts index 3cfc8ef1..3caa6707 100644 --- a/packages/cache/src/internal/pathValidation.ts +++ b/packages/cache/src/internal/pathValidation.ts @@ -105,8 +105,16 @@ const HEADER_ONLY_ENTRY_TYPES = new Set([ * Characters that signal a glob portion of a declared cache path. Anything to * the right of the first segment containing one of these is stripped when * deriving the longest non-glob prefix. + * + * `!` is intentionally NOT included here: + * - As a leading character it indicates pattern negation, which is handled + * separately by `deriveAllowedRoots` (whole pattern is dropped). + * - In any other position it has no special meaning to `@actions/glob` + * (minimatch is invoked with extglobs disabled), so treating it as a + * metachar would truncate prefixes for legitimate paths containing `!` + * (e.g. `cache/my!dir/data`), silently broadening the allowed root. */ -const GLOB_CHAR_REGEX = /[*?[\]{}!]/ +const GLOB_CHAR_REGEX = /[*?[\]{}]/ /** * Returns the working directory used for cache extraction. Mirrors the value @@ -199,9 +207,10 @@ function deriveRoot(declaredPath: string, extractCwd: string): string { } /** - * True if `seg` contains a glob metacharacter that isn't part of an - * env-var reference. Strips `${VAR}`, `$VAR`, and `%VAR%` first so the - * curly braces in `${VAR}` aren't misread as a brace-glob. + * True if `seg` contains a glob metacharacter (per {@link GLOB_CHAR_REGEX}) + * that isn't part of an env-var reference. Strips `${VAR}`, `$VAR`, and + * `%VAR%` first so the curly braces in `${VAR}` aren't misread as a + * brace-glob. * * The patterns `\$\{[^}]+\}` and `%[^%]+%` are O(n²) on pathological * input (e.g. a long run of `${` with no closing `}`). That is not a @@ -310,50 +319,15 @@ export function validateEntry( } } - // Reject NUL bytes anywhere in the path. - if (entryPath.includes('\0')) { + // Apply syntactic rejections (NUL byte, UNC, Windows absolute / + // drive-relative, POSIX absolute) to the entry path itself. + const entrySyntaxError = checkPathSyntax(entryPath, 'entry path') + if (entrySyntaxError) { return { ok: false, - code: 'NUL_BYTE', + code: entrySyntaxError.code, resolved: resolvedEntry, - reason: 'NUL byte in entry path' - } - } - - // Reject UNC paths. Check the original string before separator normalization - // because UNC is identified by leading `\\` or `//`. - if ( - entryPath.startsWith('\\\\') || - entryPath.startsWith('//') || - /^[\\/]{2}\?[\\/]/.test(entryPath) - ) { - return { - ok: false, - code: 'UNC_PATH', - resolved: resolvedEntry, - reason: `UNC path not allowed: ${entryPath}` - } - } - - // Reject Windows-style absolute paths and drive-relative paths (`C:foo`). - // These can be present in archives created on Windows even when extracted - // on POSIX, so we reject them on every platform. - if (/^[a-zA-Z]:/.test(entryPath)) { - return { - ok: false, - code: 'ABSOLUTE_PATH', - resolved: resolvedEntry, - reason: `absolute or drive-relative path not allowed: ${entryPath}` - } - } - - // Reject POSIX absolute paths. - if (entryPath.startsWith('/') || entryPath.startsWith('\\')) { - return { - ok: false, - code: 'ABSOLUTE_PATH', - resolved: resolvedEntry, - reason: `absolute path not allowed: ${entryPath}` + reason: entrySyntaxError.reason } } @@ -382,12 +356,21 @@ export function validateEntry( // Hardlink targets are resolved relative to the extraction CWD. resolvedLink = path.resolve(extractCwd, linkPath) } - if (linkPath.includes('\0')) { + // Apply the same syntactic rejections to the link target. Even if a + // special-form link target happens to land under an allowed root after + // `path.resolve`, its semantics at actual extraction time can differ + // — most notably on Windows, where `C:foo` resolves against the + // per-drive working directory rather than the entry's directory, and + // UNC targets bypass the extraction root entirely. Rejecting these + // syntactic forms outright is defense-in-depth on top of the + // containment check below. + const linkSyntaxError = checkPathSyntax(linkPath, 'link target') + if (linkSyntaxError) { return { ok: false, - code: 'NUL_BYTE', + code: linkSyntaxError.code, resolved: resolvedLink, - reason: 'NUL byte in link target' + reason: linkSyntaxError.reason } } if (!isUnderAnyPreparedRoot(resolvedLink, prepared)) { @@ -403,6 +386,69 @@ export function validateEntry( return {ok: true} } +/** + * Categorization of a syntactically-unsafe path string. Returned by + * {@link checkPathSyntax} when a path/link target fails one of the + * pre-resolution syntactic checks. + */ +interface PathSyntaxError { + code: 'NUL_BYTE' | 'UNC_PATH' | 'ABSOLUTE_PATH' + reason: string +} + +/** + * Reject paths whose syntactic form is unsafe regardless of where they + * nominally resolve: NUL bytes, UNC paths (`\\server\share`, `//srv/x`, + * `\\?\…`), Windows absolute / drive-relative paths (`C:\…`, `C:foo`), + * and POSIX absolute paths (`/foo`, `\foo`). + * + * Applied to BOTH archive entry paths and link targets. A link target + * with a special-form path can mean something completely different at + * extract time than its `path.resolve(...)` output suggests at + * validation time — for example, on Windows `C:foo` resolves against + * the per-drive working directory rather than the link's containing + * directory — so we reject these forms outright even if they would + * happen to land under an allowed root. + * + * `kind` is woven into the reason string so the violation message tells + * the user whether the rejected path was an entry path or a link target. + */ +function checkPathSyntax( + p: string, + kind: 'entry path' | 'link target' +): PathSyntaxError | undefined { + // Reject NUL bytes anywhere in the path. + if (p.includes('\0')) { + return {code: 'NUL_BYTE', reason: `NUL byte in ${kind}`} + } + // Reject UNC paths. Check the original string before any separator + // normalization because UNC is identified by leading `\\` or `//`. + if ( + p.startsWith('\\\\') || + p.startsWith('//') || + /^[\\/]{2}\?[\\/]/.test(p) + ) { + return {code: 'UNC_PATH', reason: `UNC ${kind} not allowed: ${p}`} + } + // Reject Windows-style absolute paths and drive-relative paths (`C:foo`). + // These can be present in archives created on Windows even when extracted + // on POSIX, so we reject them on every platform. + if (/^[a-zA-Z]:/.test(p)) { + return { + code: 'ABSOLUTE_PATH', + reason: `absolute or drive-relative ${kind} not allowed: ${p}` + } + } + // Reject POSIX absolute paths. + if (p.startsWith('/') || p.startsWith('\\')) { + return { + code: 'ABSOLUTE_PATH', + reason: `absolute ${kind} not allowed: ${p}` + } + } + return undefined +} + function resolveEntry(entryPath: string, extractCwd: string): string { // Tar paths are POSIX-style. Convert to native separators so path.resolve // produces the right thing on Windows.