From cf1a0c0f3eacc029a3461741dcf02b205b263268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6gni=20=C3=9E=C3=B3r?= Date: Sat, 28 Mar 2026 13:16:00 +0100 Subject: [PATCH 1/2] Address PR #7 review feedback and add merge policy to CLAUDE.md Fixes from Copilot review: - Generate legal file patterns programmatically to cover all variants (.md, .txt, .rst) consistently - Add proper os.Stat error handling (distinguish IsNotExist from other errors) in CopyLegalFiles and ExtractLegalFilesFromZip - Add zip-slip guard in ExtractLegalFilesFromZip - Check CopyFiltered error in test - Assert nested legal files are not extracted from zip archives - Expand isLegalFile test coverage for .rst and other variants Also adds PR merge policy to CLAUDE.md: always wait for CI checks and Copilot review comments before merging. Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 8 +++++ internal/packager/legalfiles.go | 45 ++++++++++++++++++---------- internal/packager/legalfiles_test.go | 29 +++++++++++++----- 3 files changed, 58 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 49d2dd0..495cb63 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,6 +11,14 @@ unity-packager is a Go CLI tool that downloads upstream packages (git repos, NuG - **nuget**: NuGet packages (download `.nupkg`, extract DLLs into `Plugins/`) - **archive**: HTTP zip/tar.gz/tgz archives (e.g., Firebase Unity SDK); auto-detects Unity vs raw +## PR and Merge Policy + +Before merging any PR, always wait for: +1. CI checks to pass +2. All Copilot review comments to be addressed + +Do not merge until both are satisfied, unless the user explicitly instructs you to skip this. + ## Build and Test ```bash diff --git a/internal/packager/legalfiles.go b/internal/packager/legalfiles.go index 45fb725..653ca62 100644 --- a/internal/packager/legalfiles.go +++ b/internal/packager/legalfiles.go @@ -10,27 +10,30 @@ import ( // legalFilePatterns are base names (case-insensitive) of files that should always // be included in the package root, regardless of exclude filters. -var legalFilePatterns = []string{ +// legalFileBaseNames are the base names (without extension) of files that should +// always be included. legalFileExtensions are the extensions to match. The full +// pattern list is generated as the cross product of these two sets, plus bare names. +var legalFileBaseNames = []string{ "license", "licence", - "license.md", - "licence.md", - "license.txt", - "licence.txt", - "license.rst", - "licence.rst", "readme", - "readme.md", - "readme.txt", - "readme.rst", "notice", - "notice.md", - "notice.txt", "third-party-notices", - "third-party-notices.md", - "third-party-notices.txt", "thirdpartynotices", - "thirdpartynotices.txt", +} + +var legalFileExtensions = []string{"", ".md", ".txt", ".rst"} + +var legalFilePatterns = generateLegalFilePatterns() + +func generateLegalFilePatterns() []string { + var patterns []string + for _, base := range legalFileBaseNames { + for _, ext := range legalFileExtensions { + patterns = append(patterns, base+ext) + } + } + return patterns } // isLegalFile returns true if the filename (base name only) matches a known @@ -66,6 +69,8 @@ func CopyLegalFiles(srcDir, destDir string) error { // Don't overwrite if already present (e.g., copied by CopyFiltered) if _, err := os.Stat(destPath); err == nil { continue + } else if !os.IsNotExist(err) { + return err } srcPath := filepath.Join(srcDir, entry.Name()) @@ -143,10 +148,18 @@ func ExtractLegalFilesFromZip(zipPath, destDir string) error { continue } - destPath := filepath.Join(destDir, f.Name) + destPath := filepath.Join(destDir, filepath.Base(filepath.Clean(f.Name))) + // Guard against path traversal + if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(destDir)+string(os.PathSeparator)) && + filepath.Clean(destPath) != filepath.Clean(destDir) { + continue + } + // Don't overwrite if _, err := os.Stat(destPath); err == nil { continue + } else if !os.IsNotExist(err) { + return err } rc, err := f.Open() diff --git a/internal/packager/legalfiles_test.go b/internal/packager/legalfiles_test.go index 2f0949b..a907992 100644 --- a/internal/packager/legalfiles_test.go +++ b/internal/packager/legalfiles_test.go @@ -9,12 +9,13 @@ import ( func TestIsLegalFile(t *testing.T) { yes := []string{ - "LICENSE", "LICENSE.md", "LICENSE.txt", + "LICENSE", "LICENSE.md", "LICENSE.txt", "LICENSE.rst", "license", "License.md", - "LICENCE", "Licence.txt", - "README", "README.md", "Readme.txt", - "NOTICE", "NOTICE.md", "NOTICE.txt", - "ThirdPartyNotices.txt", + "LICENCE", "Licence.txt", "LICENCE.rst", + "README", "README.md", "Readme.txt", "README.rst", + "NOTICE", "NOTICE.md", "NOTICE.txt", "NOTICE.rst", + "ThirdPartyNotices.txt", "ThirdPartyNotices.md", "ThirdPartyNotices.rst", + "third-party-notices", "THIRD-PARTY-NOTICES.md", } for _, name := range yes { if !isLegalFile(name) { @@ -96,7 +97,9 @@ func TestCopyLegalFiles_SurvivesExcludePatterns(t *testing.T) { os.WriteFile(filepath.Join(srcDir, "README.md"), []byte("readme"), 0644) // CopyFiltered with *.md excluded — README.md goes to Runtime but gets excluded - CopyFiltered(srcDir, runtimeDir, []string{"*.md"}) + if err := CopyFiltered(srcDir, runtimeDir, []string{"*.md"}); err != nil { + t.Fatalf("CopyFiltered: %v", err) + } // CopyLegalFiles to package root — should still pick up README.md and LICENSE if err := CopyLegalFiles(srcDir, destDir); err != nil { @@ -208,9 +211,19 @@ func TestExtractLegalFilesFromZip(t *testing.T) { t.Errorf("README.md content: got %q", string(data)) } - // Nested LICENSE should NOT be extracted (not at zip root) - // Only root-level files in lib/ subdir shouldn't appear at destDir root + // Nested files should NOT be extracted (not at zip root) if _, err := os.Stat(filepath.Join(destDir, "Foo.dll")); !os.IsNotExist(err) { t.Error("Foo.dll should not be extracted") } + + // Nested LICENSE (under lib/netstandard2.0/) should not be extracted + // Verify only the root-level files exist by checking total file count + entries, _ := os.ReadDir(destDir) + if len(entries) != 2 { + names := make([]string, len(entries)) + for i, e := range entries { + names[i] = e.Name() + } + t.Errorf("expected exactly 2 files (LICENSE.txt, README.md), got %d: %v", len(entries), names) + } } From b387480de6c215342fc311356bf152e905edd99c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6gni=20=C3=9E=C3=B3r?= Date: Sat, 28 Mar 2026 13:20:45 +0100 Subject: [PATCH 2/2] Address Copilot review on PR #8 - Use filepath.Rel-based containment check for zip-slip guard instead of string prefix comparison (handles filesystem root edge case) - Check os.ReadDir error in test assertion Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/packager/legalfiles.go | 4 ++-- internal/packager/legalfiles_test.go | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/packager/legalfiles.go b/internal/packager/legalfiles.go index 653ca62..b30614f 100644 --- a/internal/packager/legalfiles.go +++ b/internal/packager/legalfiles.go @@ -150,8 +150,8 @@ func ExtractLegalFilesFromZip(zipPath, destDir string) error { destPath := filepath.Join(destDir, filepath.Base(filepath.Clean(f.Name))) // Guard against path traversal - if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(destDir)+string(os.PathSeparator)) && - filepath.Clean(destPath) != filepath.Clean(destDir) { + rel, err := filepath.Rel(filepath.Clean(destDir), filepath.Clean(destPath)) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { continue } diff --git a/internal/packager/legalfiles_test.go b/internal/packager/legalfiles_test.go index a907992..6afc611 100644 --- a/internal/packager/legalfiles_test.go +++ b/internal/packager/legalfiles_test.go @@ -218,7 +218,10 @@ func TestExtractLegalFilesFromZip(t *testing.T) { // Nested LICENSE (under lib/netstandard2.0/) should not be extracted // Verify only the root-level files exist by checking total file count - entries, _ := os.ReadDir(destDir) + entries, err := os.ReadDir(destDir) + if err != nil { + t.Fatalf("failed to read destination directory: %v", err) + } if len(entries) != 2 { names := make([]string, len(entries)) for i, e := range entries {