Conversation
**Alert Numbers**: #476, #475 **Severity**: Medium **Rule**: G304 - Potential file inclusion via variable ## Vulnerability Description Two instances of potential path traversal vulnerabilities were identified in `pkg/cli/logs_metrics.go` at lines 250 and 373. The `os.ReadFile()` function was called with a variable path (`resolvedAgentOutputFile`) that could potentially be manipulated to read files outside the intended directory, without proper path sanitization. ## Fix Applied Added path sanitization using `filepath.Clean()` before reading files in both the `extractMissingToolsFromRun()` and `extractNoopsFromRun()` functions: 1. Line 250 (Alert #476): Added `cleanPath := filepath.Clean(resolvedAgentOutputFile)` before the `os.ReadFile()` call in `extractMissingToolsFromRun()` 2. Line 373 (Alert #475): Added `cleanPath := filepath.Clean(resolvedAgentOutputFile)` before the `os.ReadFile()` call in `extractNoopsFromRun()` The sanitized path is now used consistently in all subsequent operations including file reads and error messages. ## Security Best Practices - **Path Normalization**: `filepath.Clean()` normalizes the path by removing redundant separators, resolving `.` and `..` elements, preventing path traversal attacks - **Defense in Depth**: While the paths are constructed from trusted sources, this adds an additional layer of security - **Consistent Usage**: Updated all references to use the sanitized path variable ## Testing Considerations - Verify that log metrics extraction continues to work correctly for workflow runs - Test with various directory structures and path formats - Ensure error messages display the correct sanitized paths
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.
Security Fix: Path Traversal Vulnerabilities
Alert Numbers: #476, #475
Severity: Medium
Rule: G304 - Potential file inclusion via variable
File:
pkg/cli/logs_metrics.goLines: 250, 373
Vulnerability Description
Two instances of potential path traversal vulnerabilities were identified in the log metrics extraction code. The
os.ReadFile()function was called with a variable path (resolvedAgentOutputFile) without proper sanitization, which could potentially allow reading files outside the intended directory.Affected Functions:
extractMissingToolsFromRun()- line 250 (Alert [Custom Engine Test] Test Issue Created by Custom Engine #476)extractNoopsFromRun()- line 373 (Alert custom engine support in includes #475)Both functions construct file paths from various sources including filesystem searches and user-controlled paths, then use these paths directly in file read operations.
Fix Applied
Added path sanitization using
filepath.Clean()before reading files:Changes:
filepath.Clean()sanitization inextractMissingToolsFromRun()before line 250filepath.Clean()sanitization inextractNoopsFromRun()before line 373cleanPathvariableSecurity Best Practices
filepath.Clean()normalizes paths by:.and..elementsTesting Considerations
Impact
This is a defensive security fix with no breaking changes. The fix prevents potential path traversal attacks while maintaining all existing functionality. Both alerts #476 and #475 are resolved with this single change.