Fix security vulnerabilities from Armis scan #28
Merged
Conversation
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
Test Coverage Reporttotal: (statements) 76.8% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive security hardening to address vulnerabilities identified by an Armis security scan. The changes focus on preventing path traversal attacks (CWE-22), goroutine leaks, credential exposure, and improving resource management through defense-in-depth mitigations across input validation, path handling, progress tracking, and output formatting.
Changes:
- Implemented path traversal prevention with new
SafeJoinPathutility and enhancedSanitizePathvalidation, applied across file operations and install scripts - Added goroutine leak prevention through context-aware spinners with timeout safety nets and idempotent stop operations
- Introduced secret masking functionality to redact sensitive values in code snippets when secrets are detected
- Enhanced input validation for severity levels and exit codes, with normalized POSIX-compliant exit code ranges
- Improved resource management with proper HTTP response body cleanup and file close error propagation
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Added version and install directory validation to prevent command injection and path traversal |
| scripts/install.ps1 | Added path traversal validation for install directory |
| internal/util/path.go | New SafeJoinPath function and enhanced SanitizePath for path traversal prevention |
| internal/util/path_test.go | Comprehensive test coverage for path validation (176+ tests) |
| internal/util/mask.go | New secret masking utility with regex patterns for common credential formats |
| internal/util/mask_test.go | Test coverage for secret masking functionality (152+ tests) |
| internal/progress/progress.go | Context-aware spinners with timeout, idempotent Stop(), and goroutine leak prevention |
| internal/progress/progress_test.go | Extensive spinner behavior tests including leak detection (261+ tests) |
| internal/scan/repo/repo.go | Applied path validation and context-aware spinners with proper defer cleanup |
| internal/scan/image/image.go | Applied secret masking and context-aware spinners |
| internal/output/human.go | Applied SafeJoinPath for file snippet loading and git blame operations |
| internal/output/output.go | Exit code normalization to POSIX range (0-255) |
| internal/output/json.go | Debug mode support with format options metadata |
| internal/httpclient/client.go | HTTP response body cleanup on error to prevent resource leaks |
| internal/api/client.go | Filename sanitization using filepath.Base for multipart uploads, non-HTTPS warning |
| internal/cmd/root.go | Severity level validation with validateFailOn() |
| internal/cmd/root_test.go | Test coverage for severity validation (78+ tests) |
| internal/cmd/scan_repo.go | Integrated severity validation with error handling |
| internal/cmd/scan_image.go | Integrated severity validation with error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Improve path sanitization in util/path.go to detect traversal patterns before filepath.Clean() normalizes them - Fix HTTP response body leak in httpclient/client.go when requests fail - Sanitize filename in multipart upload using filepath.Base() - Add HTTPS warning for non-localhost URLs in API client - Validate InstallDir in PowerShell install script - Add secret masking utility to redact sensitive values in code snippets when HasSecret=true - Fix JWT regex to correctly detect tokens (header starts with eyJ, but payload does not necessarily) Co-Authored-By: Claude <noreply@anthropic.com>
- Reject absolute paths without repository context in loadSnippetFromFile - Add immutability comments for thread-safe Spinner fields - Reduce false positives in credential masking with word boundaries and 8-char min - Remove overly strict consecutive slash check from install script Co-Authored-By: Claude <noreply@anthropic.com>
- Reorder secret patterns from most specific to least specific to prevent early matches (e.g., AWS credentials before generic "secret" pattern) - Use conventional hyphen placement [-_] in regex character classes - Fix SanitizePath to check ".." as path component instead of substring, allowing valid filenames like "my..file.txt" - Recreate spinner channels on Start() for robustness against future reuse - Simplify PowerShell path traversal check for clarity Co-Authored-By: Claude <noreply@anthropic.com>
- Add documentation explaining SafeJoinPath early check is optimization, final filepath.Rel verification is authoritative (path.go) - Use path segment check instead of substring match in install.sh to allow valid paths like /opt/app..v1/bin while rejecting traversal - Reset stopOnce along with channels in spinner Start() for consistency - Increase minimum lengths in mask patterns to reduce false positives: private keys 10→16, API keys 8→10, passwords 4→8, generic creds 8→10 - Wrap both errors in defer if scanner and close errors both occur (human.go) Co-Authored-By: Claude <noreply@anthropic.com>
yiftach-armis
force-pushed
the
fix/cli-findings
branch
from
eca4c89 to
93794af
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add commonLiterals filter to mask.go to skip masking common programming literals (true, false, null, nil, undefined, none, empty) - Add version format validation to install.ps1 matching the bash script Co-Authored-By: Claude <noreply@anthropic.com>
- Add ANSI clear-to-end-of-line escape sequence (\033[K) on every spinner frame update to prevent character leaking when output length changes - Add explicit spinner.Stop() calls before fmt.Printf statements to prevent race condition between spinner goroutine and main output - Remove leading newline from Printf and move spacing to end to prevent visual flicker during spinner transitions Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot code review comments: - Capture startTime and message in local variables while holding mutex before using them in CI path and goroutine - Set s.cancel under mutex protection after context creation - Read s.cancel under mutex in Stop() to avoid race with Start() - Protect GetElapsed() read of startTime with mutex All tests pass with -race flag enabled. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Description
This PR addresses critical security vulnerabilities identified during an Armis scan, including path traversal attacks, goroutine leaks, credential exposure, and improper resource management. The changes implement defense-in-depth mitigations across input validation, path handling, progress tracking, and output formatting.
Type of Change
Changes Summary
Path Traversal Prevention
SafeJoinPathutility that validates path joining stays within base directorySanitizePathwith early detection of ".." traversal patternsfilepath.BaseGoroutine Leak Prevention
NewSpinnerWithContextStop()usingsync.Once- safe to call multiple timesSecret Masking
util/mask.gowith regex patterns for detecting common secrets (API keys, passwords, AWS credentials, JWT tokens)HasSecretis detectedInput Validation & Exit Codes
validateFailOn()for severity level validation (INFO, LOW, MEDIUM, HIGH, CRITICAL)Resource Management Improvements
Testing
SanitizePath,SafeJoinPath)Checklist
Security Improvements