Address copilot review feedback

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)', () => {

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.

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.