From 25cf71ac6fcb57083634bff579b162c7fc06d80a Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 03:06:36 -0400 Subject: [PATCH 1/2] fix(update): support safe symlink extraction during updates Allow extracting symlinks from TarGz and Zip update archives to support Unix npm shim symlinks. Prevent path traversal vulnerabilities by rejecting absolute link targets and verifying that relative link targets do not escape the destination directory. --- internal/update/extract.go | 48 +++++++++ internal/update/extract_test.go | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+) diff --git a/internal/update/extract.go b/internal/update/extract.go index ce0a5acbf..4198e22d3 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -53,6 +53,23 @@ func extractTarGz(archivePath string, destDir string) error { if err := os.MkdirAll(target, 0o755); err != nil { return err } + case tar.TypeSymlink: + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + return err + } + if filepath.IsAbs(header.Linkname) { + return fmt.Errorf("absolute symlink targets are not supported: %s -> %s", header.Name, header.Linkname) + } + // Verify that the symlink target, when resolved, does not escape destDir. + resolvedTarget := filepath.Join(filepath.Dir(target), header.Linkname) + destDirClean := filepath.Clean(destDir) + if !strings.HasPrefix(resolvedTarget, destDirClean+string(os.PathSeparator)) && resolvedTarget != destDirClean { + return fmt.Errorf("archive symlink target escapes destination: %s -> %s", header.Name, header.Linkname) + } + _ = os.Remove(target) + if err := os.Symlink(header.Linkname, target); err != nil { + return err + } case tar.TypeReg: if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return err @@ -87,6 +104,37 @@ func extractZip(archivePath string, destDir string) error { } continue } + if entry.Mode()&fs.ModeSymlink != 0 { + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + return err + } + linknameBytes, err := func() ([]byte, error) { + entryReader, err := entry.Open() + if err != nil { + return nil, err + } + defer entryReader.Close() + return io.ReadAll(entryReader) + }() + if err != nil { + return err + } + linkname := string(linknameBytes) + if filepath.IsAbs(linkname) { + return fmt.Errorf("absolute symlink targets are not supported: %s -> %s", entry.Name, linkname) + } + // Verify that the symlink target, when resolved, does not escape destDir. + resolvedTarget := filepath.Join(filepath.Dir(target), linkname) + destDirClean := filepath.Clean(destDir) + if !strings.HasPrefix(resolvedTarget, destDirClean+string(os.PathSeparator)) && resolvedTarget != destDirClean { + return fmt.Errorf("archive symlink target escapes destination: %s -> %s", entry.Name, linkname) + } + _ = os.Remove(target) + if err := os.Symlink(linkname, target); err != nil { + return err + } + continue + } // Release archives only ever contain regular files and directories; // reject anything else (symlinks, devices) rather than silently write // the link-target string (or other special content) out as an diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index f8aaa8239..b468b4a86 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -215,3 +215,177 @@ func TestFindByBasenameSearchesRecursively(t *testing.T) { t.Fatalf("findByBasename = %q, want empty", notFound) } } + +func symlinksSupported(t *testing.T) bool { + dir := t.TempDir() + err := os.Symlink("target", filepath.Join(dir, "link")) + return err == nil +} + +func TestExtractTarGzAllowsSafeSymlink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.tar.gz") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create: %v", err) + } + gw := gzip.NewWriter(file) + tw := tar.NewWriter(gw) + + h1 := &tar.Header{ + Name: "link.txt", + Typeflag: tar.TypeSymlink, + Linkname: "target.txt", + } + if err := tw.WriteHeader(h1); err != nil { + t.Fatalf("WriteHeader link: %v", err) + } + h2 := &tar.Header{ + Name: "target.txt", + Typeflag: tar.TypeReg, + Mode: 0o644, + Size: 12, + } + if err := tw.WriteHeader(h2); err != nil { + t.Fatalf("WriteHeader target: %v", err) + } + if _, err := tw.Write([]byte("hello symbol")); err != nil { + t.Fatalf("Write target: %v", err) + } + + tw.Close() + gw.Close() + file.Close() + + destDir := filepath.Join(dir, "extracted") + if err := extractArchive(archivePath, destDir); err != nil { + t.Fatalf("extractArchive: %v", err) + } + + targetPath := filepath.Join(destDir, "link.txt") + gotLink, err := os.Readlink(targetPath) + if err != nil { + t.Fatalf("Readlink: %v", err) + } + if gotLink != "target.txt" { + t.Fatalf("link target = %q, want %q", gotLink, "target.txt") + } +} + +func TestExtractTarGzRejectsEscapingSymlink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.tar.gz") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create: %v", err) + } + gw := gzip.NewWriter(file) + tw := tar.NewWriter(gw) + + h1 := &tar.Header{ + Name: "link.txt", + Typeflag: tar.TypeSymlink, + Linkname: "../../../outside.txt", + } + if err := tw.WriteHeader(h1); err != nil { + t.Fatalf("WriteHeader: %v", err) + } + + tw.Close() + gw.Close() + file.Close() + + destDir := filepath.Join(dir, "extracted") + if err := extractArchive(archivePath, destDir); err == nil { + t.Fatal("expected error extracting escaping symlink") + } +} + +func TestExtractZipAllowsSafeSymlink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.zip") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create: %v", err) + } + zw := zip.NewWriter(file) + + header := &zip.FileHeader{Name: "link.txt"} + header.SetMode(os.ModeSymlink | 0o777) + w, err := zw.CreateHeader(header) + if err != nil { + t.Fatalf("CreateHeader: %v", err) + } + if _, err := w.Write([]byte("target.txt")); err != nil { + t.Fatalf("Write target name: %v", err) + } + + w2, err := zw.Create("target.txt") + if err != nil { + t.Fatalf("Create target: %v", err) + } + if _, err := w2.Write([]byte("hello zip symbol")); err != nil { + t.Fatalf("Write target data: %v", err) + } + + zw.Close() + file.Close() + + destDir := filepath.Join(dir, "extracted") + if err := extractArchive(archivePath, destDir); err != nil { + t.Fatalf("extractArchive: %v", err) + } + + targetPath := filepath.Join(destDir, "link.txt") + gotLink, err := os.Readlink(targetPath) + if err != nil { + t.Fatalf("Readlink: %v", err) + } + if gotLink != "target.txt" { + t.Fatalf("link target = %q, want %q", gotLink, "target.txt") + } +} + +func TestExtractZipRejectsEscapingSymlink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.zip") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create: %v", err) + } + zw := zip.NewWriter(file) + + header := &zip.FileHeader{Name: "link.txt"} + header.SetMode(os.ModeSymlink | 0o777) + w, err := zw.CreateHeader(header) + if err != nil { + t.Fatalf("CreateHeader: %v", err) + } + if _, err := w.Write([]byte("../../../outside.txt")); err != nil { + t.Fatalf("Write: %v", err) + } + + zw.Close() + file.Close() + + destDir := filepath.Join(dir, "extracted") + if err := extractArchive(archivePath, destDir); err == nil { + t.Fatal("expected error extracting escaping symlink") + } +} From c709ddb69c061624b4df1c21fa46f5a33c187b8c Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:55:29 -0400 Subject: [PATCH 2/2] revert(update): keep zip symlink entries rejected Addresses jatmn's review: the stated bug (standalone update extraction broken on Unix/macOS) is about .tar.gz archives; the zip path handles Windows release archives. Adding zip symlink support alongside the tar.gz fix reversed the existing zip symlink-rejection hardening (TestExtractZipRejectsSymlinkEntry) and failed Windows CI, since filepath.IsAbs does not reliably reject a slash-rooted target like "/some/other/path" on Windows (which needs a drive letter or UNC prefix to be absolute). Reverted extractZip's symlink handling back to unconditional rejection and removed the two zip-symlink tests that covered it. The tar.gz symlink support and its tests (the actual fix for the reported bug) are unchanged. --- internal/update/extract.go | 40 +++------------- internal/update/extract_test.go | 81 --------------------------------- 2 files changed, 7 insertions(+), 114 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index 4198e22d3..84b86ecef 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -104,42 +104,16 @@ func extractZip(archivePath string, destDir string) error { } continue } - if entry.Mode()&fs.ModeSymlink != 0 { - if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return err - } - linknameBytes, err := func() ([]byte, error) { - entryReader, err := entry.Open() - if err != nil { - return nil, err - } - defer entryReader.Close() - return io.ReadAll(entryReader) - }() - if err != nil { - return err - } - linkname := string(linknameBytes) - if filepath.IsAbs(linkname) { - return fmt.Errorf("absolute symlink targets are not supported: %s -> %s", entry.Name, linkname) - } - // Verify that the symlink target, when resolved, does not escape destDir. - resolvedTarget := filepath.Join(filepath.Dir(target), linkname) - destDirClean := filepath.Clean(destDir) - if !strings.HasPrefix(resolvedTarget, destDirClean+string(os.PathSeparator)) && resolvedTarget != destDirClean { - return fmt.Errorf("archive symlink target escapes destination: %s -> %s", entry.Name, linkname) - } - _ = os.Remove(target) - if err := os.Symlink(linkname, target); err != nil { - return err - } - continue - } // Release archives only ever contain regular files and directories; // reject anything else (symlinks, devices) rather than silently write // the link-target string (or other special content) out as an - // ordinary file — mirrors extractTarGz's rejection of non-regular tar - // entries. + // ordinary file. Unlike extractTarGz, zip symlinks stay rejected: the + // .zip path is the Windows release archive format, where + // filepath.IsAbs does not reliably reject a slash-rooted target like + // "/some/other/path" (Windows absolute paths need a drive letter or + // UNC prefix), and there is no current use case for symlinks in a + // Windows archive. The npm shim symlinks this feature exists for are + // packaged in the Unix/macOS .tar.gz archives, handled above. if !entry.Mode().IsRegular() { return fmt.Errorf("unsupported archive entry type for %s", entry.Name) } diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index b468b4a86..1742866ea 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -308,84 +308,3 @@ func TestExtractTarGzRejectsEscapingSymlink(t *testing.T) { t.Fatal("expected error extracting escaping symlink") } } - -func TestExtractZipAllowsSafeSymlink(t *testing.T) { - if !symlinksSupported(t) { - t.Skip("symlinks not supported") - } - dir := t.TempDir() - archivePath := filepath.Join(dir, "archive.zip") - - file, err := os.Create(archivePath) - if err != nil { - t.Fatalf("Create: %v", err) - } - zw := zip.NewWriter(file) - - header := &zip.FileHeader{Name: "link.txt"} - header.SetMode(os.ModeSymlink | 0o777) - w, err := zw.CreateHeader(header) - if err != nil { - t.Fatalf("CreateHeader: %v", err) - } - if _, err := w.Write([]byte("target.txt")); err != nil { - t.Fatalf("Write target name: %v", err) - } - - w2, err := zw.Create("target.txt") - if err != nil { - t.Fatalf("Create target: %v", err) - } - if _, err := w2.Write([]byte("hello zip symbol")); err != nil { - t.Fatalf("Write target data: %v", err) - } - - zw.Close() - file.Close() - - destDir := filepath.Join(dir, "extracted") - if err := extractArchive(archivePath, destDir); err != nil { - t.Fatalf("extractArchive: %v", err) - } - - targetPath := filepath.Join(destDir, "link.txt") - gotLink, err := os.Readlink(targetPath) - if err != nil { - t.Fatalf("Readlink: %v", err) - } - if gotLink != "target.txt" { - t.Fatalf("link target = %q, want %q", gotLink, "target.txt") - } -} - -func TestExtractZipRejectsEscapingSymlink(t *testing.T) { - if !symlinksSupported(t) { - t.Skip("symlinks not supported") - } - dir := t.TempDir() - archivePath := filepath.Join(dir, "archive.zip") - - file, err := os.Create(archivePath) - if err != nil { - t.Fatalf("Create: %v", err) - } - zw := zip.NewWriter(file) - - header := &zip.FileHeader{Name: "link.txt"} - header.SetMode(os.ModeSymlink | 0o777) - w, err := zw.CreateHeader(header) - if err != nil { - t.Fatalf("CreateHeader: %v", err) - } - if _, err := w.Write([]byte("../../../outside.txt")); err != nil { - t.Fatalf("Write: %v", err) - } - - zw.Close() - file.Close() - - destDir := filepath.Join(dir, "extracted") - if err := extractArchive(archivePath, destDir); err == nil { - t.Fatal("expected error extracting escaping symlink") - } -}