[PPSC-602] fix(security): armisignore size limit and go-git CVE remediation#136
Merged
yiftach-armis merged 4 commits intomainfrom Mar 31, 2026
Merged
Conversation
…70, CVE-2026-34165) - Add 1MB size check before reading .armisignore files to prevent memory exhaustion from maliciously large ignore files - Upgrade go-git/go-git/v5 from v5.17.0 to v5.17.2 to address CVE-2026-34165 - Add test for oversized .armisignore rejection
There was a problem hiding this comment.
Pull request overview
This PR addresses two security findings in the CLI repo scanner: it introduces a maximum size limit for .armisignore files to prevent unbounded memory reads, and updates go-git to remediate a reported CVE.
Changes:
- Enforce a 1 MiB maximum size for
.armisignorefiles during ignore pattern loading. - Add unit coverage for oversized
.armisignorehandling. - Bump
github.com/go-git/go-git/v5fromv5.17.0tov5.17.2(and updatego.sum).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/scan/repo/ignore.go |
Adds a .armisignore size limit check before reading ignore files. |
internal/scan/repo/ignore_test.go |
Adds a test ensuring oversized .armisignore files produce an error. |
go.mod |
Updates go-git dependency version for CVE remediation. |
go.sum |
Updates checksums for the new go-git version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
34
to
38
| if !info.IsDir() && info.Name() == ".armisignore" { | ||
| if info.Size() > maxIgnoreFileSize { | ||
| return fmt.Errorf(".armisignore file too large (%d bytes, max %d): %s", info.Size(), maxIgnoreFileSize, path) | ||
| } | ||
| patterns, err := loadIgnoreFile(path, repoRoot) |
There was a problem hiding this comment.
The size guard is based on the os.FileInfo returned by filepath.Walk (which uses Lstat) and is checked before os.ReadFile. This can be bypassed by a symlink named .armisignore pointing to a large file (symlink size is small but ReadFile follows it), and it’s also a TOCTOU window if the file grows after the check. Consider enforcing the limit in loadIgnoreFile by opening the file and reading via a bounded reader (e.g., io.LimitReader) and/or rejecting symlinks explicitly, so the limit applies to the actual bytes read.
|
Test Coverage Reporttotal: (statements) 81.1% Coverage by function |
Address Copilot review: the Walk-based info.Size() check was bypassable via symlinks (Lstat returns symlink size, not target) and had a TOCTOU window. Replace with: - Reject symlinked .armisignore files in Walk callback - Use io.LimitReader in loadIgnoreFile to enforce the 1MB cap on actual bytes read, eliminating both the symlink and TOCTOU bypass vectors
The Coverage Comment job failed with a transient GitHub API 503. The github-script action defaults to 0 retries, so any transient server error causes an immediate failure. Adding retries: 3 makes the step resilient to temporary GitHub API outages.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
35
to
+38
| if !info.IsDir() && info.Name() == ".armisignore" { | ||
| if info.Mode()&os.ModeSymlink != 0 { | ||
| return fmt.Errorf(".armisignore is a symlink (rejected): %s", path) | ||
| } |
There was a problem hiding this comment.
The newly added behavior that rejects symlinked .armisignore files isn’t covered by a unit test. Since this is security-related behavior, add a test that creates a symlink .armisignore and asserts LoadIgnorePatterns returns an error (skip the test on platforms/environments where symlink creation isn’t permitted).
Cover the symlink rejection path in LoadIgnorePatterns with a test that creates a symlinked .armisignore and asserts the expected error. Skips gracefully on platforms where symlinks aren't supported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
After merging PR #135 (security code scanning remediations), GitHub Code Scanning reported 21 new/pre-existing open alerts. Triage revealed:
Solution
Alert triage (19 of 21 dismissed via GitHub API):
Code fixes for 2 true positives:
internal/scan/repo/ignore.go): Added 1MB size check beforeos.ReadFileon.armisignorefiles to prevent memory exhaustion from maliciously large ignore filesgo.mod): Upgradedgo-git/go-git/v5from v5.17.0 to v5.17.2Testing
Automated Tests
Manual Testing
go test ./internal/scan/repo/... -run Ignore— all 4 tests pass including new oversized file testChecklist