stop using deprecated mholt/archiver #5951
Conversation
|
Warning Rate limit exceeded@AdallomRoy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 42 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThe pull request introduces updates to the project's Go version and dependencies, focusing on file and archive processing. The primary changes involve upgrading the Go version to 1.22.2, replacing the archiver library with a new implementation, and updating various dependency versions. The modifications enhance file handling capabilities, particularly for compressed archives like ZIP and GZIP, with improved error management and more explicit file processing logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FileProcessor
participant ArchiveHandler
participant FileSystem
Client->>FileProcessor: Execute file request
FileProcessor->>FileSystem: Open file
FileSystem-->>FileProcessor: File stream
FileProcessor->>ArchiveHandler: Detect archive type
ArchiveHandler-->>FileProcessor: Archive format
FileProcessor->>ArchiveHandler: Extract files
ArchiveHandler-->>FileProcessor: Extracted content
FileProcessor-->>Client: Processing results
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/protocols/file/request.go (2)
62-67: Consider more descriptive error logging.While this error handling is functionally correct, consider appending file context or a clearer message to help with diagnostics (e.g.,
gologger.Error().Msgf("failed to open file %s: %v", filePath, err)).
118-118: Avoid discardingfi.Stat()error.Currently,
fi.Stat()is called with_, _ := fi.Stat(). If it fails, the subsequent logic could consume invalid or partial data. Consider capturing and handling the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(10 hunks)pkg/protocols/file/request.go(4 hunks)pkg/protocols/file/request_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
pkg/protocols/file/request_test.go
[failure] 30-30:
Error return value of w1.Write is not checked (errcheck)
🪛 GitHub Actions: 🔨 Tests
pkg/protocols/file/request_test.go
[error] 30-30: Error return value of w1.Write is not checked (errcheck)
🔇 Additional comments (16)
pkg/protocols/file/request.go (9)
5-5: No concerns with thecontextimport.
13-13: Migration togithub.com/mholt/archiveslooks correct.
68-68: Revisit ignored error fromarchives.Identify.The return signature includes an error, but it's being discarded as
_. IfIdentifyfails, it might be helpful to check or log it to catch issues with malformed archives or unexpected file types.
70-73: Clean archive handling logic.
80-85: Archived file extraction is well-handled.
86-86: Processing reader for archived file.
108-108: Single-file decompression approach is straightforward.
111-111: Good error check onOpenReader(stream).
128-129: Data copy error handling is correct.pkg/protocols/file/request_test.go (4)
4-5: New imports for ZIP and bytes are appropriate.
36-44: GZIP creation logic looks correct with error checks.
46-67: Test harness setup is methodical.
68-130: Comprehensive test coverage for multiple file types.The loop structure tests uncompressed, GZIP, and ZIP successfully. Logging and result validations are thorough.
go.mod (3)
3-3: Go version upgrade to 1.22.2.This upgrade provides performance enhancements and security fixes. Ensure that build environments and CI pipelines support Go 1.22.2 to avoid compatibility issues.
80-80: Dependency switch fromarchivertoarchives.This aligns directly with the PR objective of discontinuing deprecated and vulnerable libraries.
Line range hint
124-279: Multiple indirect dependency additions and updates.No issues flagged. For completeness, consider scanning these updated libraries for known vulnerabilities before release.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/protocols/file/request_test.go (1)
25-34:⚠️ Potential issueCheck the write error in
zipFile.The error from
w1.Write(data)is not checked, which could lead to silent failures.Apply this diff to fix the error handling:
-w1.Write(data) +_, err = w1.Write(data) +require.NoError(t, err)🧰 Tools
🪛 GitHub Check: Lint
[failure] 30-30:
Error return value ofw1.Writeis not checked (errcheck)🪛 golangci-lint (1.62.2)
30-30: Error return value of
w1.Writeis not checked(errcheck)
🧹 Nitpick comments (1)
pkg/protocols/file/request.go (1)
Line range hint
62-108: Good implementation of the new archives library!The migration to
mholt/archivesis well-implemented with proper error handling and logging. The code effectively handles both archive extraction and decompression.Consider adding debug logging before archive extraction to help with troubleshooting:
case format != nil: + gologger.Debug().Msgf("Identified archive format: %s for file: %s", format.Name(), filePath) switch archiveInstance := format.(type) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(10 hunks)pkg/protocols/file/request.go(4 hunks)pkg/protocols/file/request_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
pkg/protocols/file/request_test.go
[failure] 30-30:
Error return value of w1.Write is not checked (errcheck)
🪛 golangci-lint (1.62.2)
pkg/protocols/file/request_test.go
30-30: Error return value of w1.Write is not checked
(errcheck)
🔇 Additional comments (4)
pkg/protocols/file/request_test.go (2)
36-44: LGTM!The function correctly implements GZIP compression with proper error handling.
46-130: Well-structured test cases!The test implementation effectively validates both GZIP and ZIP formats using table-driven tests, ensuring compatibility with the new archives library.
pkg/protocols/file/request.go (1)
Line range hint
108-151: Solid implementation of decompression handling!The code effectively manages single-file compressed archives with proper temporary file handling and cleanup. The error handling is comprehensive, ensuring reliable decompression.
go.mod (1)
80-80: Verify the latest stable versions of compression-related dependencies.The migration to
mholt/archiveslooks good. Let's verify the versions of related dependencies for potential updates or security fixes.Run the following script to check for latest versions and any security advisories:
Also applies to: 124-124, 137-139, 180-181, 194-194, 210-210, 216-216, 230-231
✅ Verification successful
Let me try a different approach to verify the dependency versions.
Let me try one more verification approach using GitHub API to check the latest releases.
All compression-related dependencies are at their latest stable versions
Based on the verification results:
mholt/archivesis using the latest commit (fc8400ac35 from Dec 26, 2024)klauspost/compressis at v1.17.11 (latest)bodgit/sevenzipis at v1.6.0 (latest)bodgit/plumbingis at v1.3.0 (latest)bodgit/windowsis at v1.0.1 (latest)sorairolake/lzip-gois at v0.3.5 (latest)- Other dependencies are at their latest stable versions with no newer releases available
All compression-related dependencies are properly versioned and up-to-date. No security advisories or critical updates are pending.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest versions and security advisories for compression-related dependencies # Check latest versions on pkg.go.dev echo "Checking latest versions..." for pkg in "github.com/mholt/archives" "github.com/STARRY-S/zip" "github.com/bodgit/plumbing" "github.com/bodgit/sevenzip" "github.com/bodgit/windows" "github.com/hashicorp/errwrap" "github.com/hashicorp/go-multierror" "github.com/klauspost/compress" "github.com/nwaples/rardecode/v2" "github.com/pierrec/lz4/v4" "github.com/sorairolake/lzip-go" "github.com/therootcompany/xz" do curl -s "https://pkg.go.dev/$pkg" | grep -A 1 "Latest version" done # Check for security advisories for pkg in ${pkgs[@]}; do gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "'$pkg'") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' doneLength of output: 5650
Script:
#!/bin/bash # Check go.mod for current versions echo "Current versions in go.mod:" cat go.mod | grep -A 1 -E "github.com/(mholt/archives|STARRY-S/zip|bodgit/plumbing|bodgit/sevenzip|bodgit/windows|hashicorp/errwrap|hashicorp/go-multierror|klauspost/compress|nwaples/rardecode|pierrec/lz4|sorairolake/lzip-go|therootcompany/xz)" # Use go list to check latest versions echo -e "\nLatest versions from go list:" for pkg in "github.com/mholt/archives" "github.com/STARRY-S/zip" "github.com/bodgit/plumbing" "github.com/bodgit/sevenzip" "github.com/bodgit/windows" "github.com/hashicorp/errwrap" "github.com/hashicorp/go-multierror" "github.com/klauspost/compress" "github.com/nwaples/rardecode/v2" "github.com/pierrec/lz4/v4" "github.com/sorairolake/lzip-go" "github.com/therootcompany/xz" do go list -m -versions $pkg 2>/dev/null || true doneLength of output: 6732
Script:
#!/bin/bash # Check latest releases on GitHub for repo in "mholt/archives" "STARRY-S/zip" "bodgit/plumbing" "bodgit/sevenzip" "bodgit/windows" "hashicorp/errwrap" "hashicorp/go-multierror" "klauspost/compress" "nwaples/rardecode" "pierrec/lz4" "sorairolake/lzip-go" "therootcompany/xz"; do echo "Latest release for $repo:" gh api repos/$repo/releases/latest --jq '.tag_name, .published_at' 2>/dev/null || echo "No releases found" echo "---" done # Also check the commit history for mholt/archives since it's using a commit hash echo "Latest commits for mholt/archives:" gh api repos/mholt/archives/commits --jq '.[0:3] | .[] | {sha: .sha[0:10], date: .commit.author.date, message: .commit.message}' 2>/dev/nullLength of output: 6939
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
pkg/protocols/file/request.go (2)
Line range hint
108-134: Add memory limits for decompression.The decompression logic should include memory limits to prevent zip bomb attacks.
case archives.Decompressor: request.options.Progress.AddToTotal(1) + const maxDecompressedSize = 1 << 30 // 1GB limit + var totalBytesRead int64 reader, err := archiveInstance.OpenReader(stream) if err != nil { gologger.Error().Msgf("%s\n", err) request.options.Progress.IncrementFailedRequestsBy(1) return } fileStat, _ := fi.Stat() tmpFileOut, err := os.CreateTemp("", "") if err != nil { gologger.Error().Msgf("%s\n", err) request.options.Progress.IncrementFailedRequestsBy(1) return } defer tmpFileOut.Close() defer os.RemoveAll(tmpFileOut.Name()) - _, err = io.Copy(tmpFileOut, reader) + _, err = io.Copy(tmpFileOut, io.LimitReader(reader, maxDecompressedSize)) if err != nil { gologger.Error().Msgf("%s\n", err) request.options.Progress.IncrementFailedRequestsBy(1) return }
Line range hint
70-107: Add validation for archive paths.The archive extraction should validate paths to prevent directory traversal attacks.
case format != nil: switch archiveInstance := format.(type) { case archives.Extractor: err := archiveInstance.Extract(input.Context(), stream, func(ctx context.Context, file archives.FileInfo) error { + // Prevent directory traversal + if strings.Contains(file.Name(), "..") { + return fmt.Errorf("invalid path: %s", file.Name()) + } if !request.validatePath("/", file.Name(), true) { return nil }
🧹 Nitpick comments (5)
pkg/protocols/file/request_test.go (4)
25-35: Consider usingdeferfor cleanup inzipFile.The implementation looks good with proper error handling. However, consider using
defer w.Close()right after creating the writer to ensure cleanup in case of panics.func zipFile(t *testing.T, fileName string, data []byte) []byte { var b bytes.Buffer w := zip.NewWriter(&b) + defer w.Close() w1, err := w.Create(fileName) require.NoError(t, err) _, err = w1.Write(data) require.NoError(t, err) - err = w.Close() - require.NoError(t, err) return b.Bytes() }
37-45: Consider usingdeferfor cleanup ingzipFile.Similar to the
zipFilefunction, consider usingdefer w.Close()right after creating the writer.func gzipFile(t *testing.T, data []byte) []byte { var b bytes.Buffer w := gzip.NewWriter(&b) + defer w.Close() _, err := w.Write(data) require.NoError(t, err) - err = w.Close() - require.NoError(t, err) return b.Bytes() }
50-67: Add test case descriptions and edge cases.Consider adding descriptions for each test case and including edge cases:
- Add comments describing the purpose of each test case
- Consider adding edge cases like:
- Empty files
- Large files
- Files with special characters in names
- Nested archives (zip containing gzip)
var testCases = []struct { fileName string data []byte + description string // Add description field }{ { fileName: testCaseBaseFilename, data: testCaseBase, + description: "Plain text file", }, { fileName: testCaseBaseFilename + ".gz", data: gzipFile(t, testCaseBase), + description: "GZIP compressed file", }, { fileName: "config.yaml.zip", data: zipFile(t, testCaseBaseFilename, testCaseBase), + description: "ZIP archive with single file", }, + { + fileName: "empty.yaml", + data: []byte{}, + description: "Empty file", + }, }
103-113: Ensure proper cleanup of temporary files.While
defer os.RemoveAll(tempDir)is used, consider adding error handling for file operations and using a cleanup function to ensure all resources are properly released.+ cleanup := func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Errorf("Failed to cleanup temporary directory: %v", err) + } + } tempDir, err := os.MkdirTemp("", "test-*") require.Nil(t, err, "could not create temporary directory") - defer os.RemoveAll(tempDir) + defer cleanup()pkg/protocols/file/request.go (1)
62-84: Consider adding context timeout for archive operations.The archive identification and processing could benefit from a timeout context to prevent hanging on malicious or corrupted archives.
+ ctx, cancel := context.WithTimeout(input.Context(), 30*time.Second) + defer cancel() fi, err := os.Open(filePath) if err != nil { gologger.Error().Msgf("%s\n", err) return } defer fi.Close() - format, stream, _ := archives.Identify(input.Context(), filePath, fi) + format, stream, err := archives.Identify(ctx, filePath, fi) + if err != nil { + gologger.Error().Msgf("Failed to identify archive format: %s\n", err) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(10 hunks)pkg/protocols/file/request.go(4 hunks)pkg/protocols/file/request_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
|
@dogancanbakir the linting errors here are related to the go 1.22 upgrade that is required because archives is a go 1.22 lib - it's all ast.Package usage that was deprecated in 1.22 and it used in a tool and not actually in nuclei. |
|
What CVE? |
|
@AdallomRoy I don't see a reason why we shouldn't upgrade to a newer version. Could you make the necessary changes? toda! |
AdallomRoy
force-pushed
the
dev
branch
2 times, most recently
from
82b281c to
4409a20
Compare
|
After looking into it, it seems like we're not directly affected by GO-2024-2698. From what I can see in the PR mholt/archiver#396, the issue specifically affects the Based on this observation, I would say the risk level here is quite tolerable. The potentially vulnerable code doesn't seem to be actively used in our context. Of course, this assessment could change if someone provides a reproducible PoC that demonstrates the vulnerability in our specific implementation (in the file-protocol-based template). Until then, it doesn't look like we're significantly at risk. |
|
I agree, I wasn't trying to imply that you are currently vulnerable. but:
Hope this makes sense. |
|
@AdallomRoy understood. I came across a forked repo that appears to have implemented a patch for the issue - mholt/archiver#396 (comment), and wondering whether the patch would be fully compatible with our setup (w/o need to bump current Go version)? This could be worth exploring to ensure it integrates smoothly without introducing additional dependencies or compatibility issues. |
|
I think anyone who's importing your library would have to replace it as well. |
|
+1 for not using unmaintained library. |
Proposed changes
Stop using deprecated (and CVE-ful mholt/archiver) and migrate to the new mholt/archives
I added tests (that were missing) to validate the decompression part
Checklist
Summary by CodeRabbit
Dependency Updates
Library Changes
github.com/mholt/archiverwithgithub.com/mholt/archivesTesting Improvements