Add GitHub Actions artifact upload command (gha) #137
Conversation
This commit introduces a new `gha` subcommand for seamless integration with GitHub Actions workflows, enabling automated upload of workflow artifacts to Vulnetix for vulnerability analysis. New Features: - `vulnetix gha upload`: Upload all workflow artifacts with metadata - `vulnetix gha status`: Check upload status by txnid or artifact UUID - Automatic GitHub Actions environment variable collection - Transaction-based artifact upload with individual tracking - JSON output support for CI/CD integration Implementation Details: - New package `internal/github` for artifact handling - GitHub Actions artifact collector with download/extraction - API client for transaction and artifact upload endpoints - Comprehensive metadata collection from GHA environment - Status checking for transactions and individual artifacts API Integration: - POST /api/github/artifact-upload - Initiate transaction - POST /api/github/artifact-upload/:txnid - Upload artifacts - GET /api/github/artifact-upload/:txnid/status - Transaction status - GET /api/github/artifact/:uuid/status - Artifact status Documentation: - Comprehensive command documentation in docs/GHA_COMMAND.md - Usage examples for GitHub Actions workflows - API endpoint specifications - Troubleshooting guide Testing: - Unit tests for artifact collector - Unit tests for uploader - All tests passing for new functionality
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a new gha subcommand to enable GitHub Actions workflows to automatically upload artifacts to Vulnetix for vulnerability analysis. The implementation includes artifact collection from GitHub's API, metadata extraction from the workflow environment, and transaction-based upload to the Vulnetix platform.
Key Changes:
- New GitHub Actions integration with artifact collection and upload capabilities
- Transaction-based API client for managing multi-artifact uploads
- Comprehensive CLI commands for upload and status checking with JSON output support
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/github/artifact.go | Implements GitHub artifact collection, download, and zip extraction functionality |
| internal/github/uploader.go | Provides API client for transaction creation, artifact upload, and status checking |
| internal/github/artifact_test.go | Tests for metadata collection and artifact collector initialization |
| internal/github/uploader_test.go | Tests for uploader initialization and struct construction |
| cmd/gha.go | CLI command implementation for upload and status operations with workflow integration |
| docs/GHA_COMMAND.md | Comprehensive documentation including usage examples, API specs, and troubleshooting |
| README.md | Updated documentation links to include new GHA command guide |
Critical Issues Found:
- Security vulnerability: Zip extraction lacks path traversal validation (Zip Slip vulnerability)
- Missing authentication: No authentication headers in Vulnetix API requests
- Resource management: Deferred cleanup in loop doesn't execute until function exit
- Test coverage: Core functionality lacks unit tests with mocked dependencies
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| os.Setenv(key, value) | ||
| defer os.Unsetenv(key) | ||
| } | ||
|
|
||
| artifactNames := []string{"artifact1.zip", "artifact2.zip"} | ||
| metadata := CollectMetadata(artifactNames) |
There was a problem hiding this comment.
The defer statements for os.Unsetenv inside a loop will all execute after the test function completes, not after each iteration. This means all environment variables will remain set during the entire test execution. While this doesn't break the test, it could potentially affect other tests if they run concurrently. Consider using t.Cleanup() for each variable or collecting all defers properly.
| os.Setenv(key, value) | |
| defer os.Unsetenv(key) | |
| } | |
| artifactNames := []string{"artifact1.zip", "artifact2.zip"} | |
| metadata := CollectMetadata(artifactNames) | |
| t.Setenv(key, value) | |
| } | |
| artifactNames := []string{"artifact1.zip", "artifact2.zip"} | |
| metadata := CollectMetadata(artifactNames) | |
| metadata := CollectMetadata(artifactNames) |
| } | ||
|
|
||
| // Create temporary directory for extraction | ||
| tmpDir, err := os.MkdirTemp("", fmt.Sprintf("artifact-%s-*", artifact.Name)) |
There was a problem hiding this comment.
The artifact.Name is used directly in the temporary directory pattern without sanitization. If an artifact name contains special characters or path separators, it could cause unexpected behavior or security issues. Consider sanitizing the artifact name to only allow safe characters before using it in the directory pattern.
|
|
||
| // List all artifacts | ||
| fmt.Println("📦 Fetching workflow artifacts...") | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Using context.Background() instead of the context from cmd (cmd.Context()) means the operation won't be cancelled if the CLI command is interrupted (e.g., with Ctrl+C). Consider using cmd.Context() to ensure proper cancellation propagation and graceful shutdown.
| ctx := context.Background() | |
| ctx := cmd.Context() |
| client: &http.Client{ | ||
| Timeout: 60 * time.Second, |
There was a problem hiding this comment.
The HTTP client timeout of 60 seconds may be insufficient for downloading large artifacts. If an artifact is several hundred MB or larger, or if network conditions are poor, the download could time out. Consider using a longer timeout or making it configurable, especially since artifact sizes are known upfront via SizeInBytes.
| for _, file := range reader.File { | ||
| path := filepath.Join(destDir, file.Name) |
There was a problem hiding this comment.
The zip extraction is vulnerable to path traversal attacks (Zip Slip vulnerability). A malicious zip file could contain entries with paths like "../../../etc/passwd" that would write files outside the intended destination directory. Before constructing the path, validate that file.Name does not contain ".." and that the resulting path is within destDir using filepath.Clean and checking if it has destDir as a prefix.
| package github | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestNewArtifactUploader(t *testing.T) { | ||
| baseURL := "https://api.vulnetix.com" | ||
| orgID := "123e4567-e89b-12d3-a456-426614174000" | ||
|
|
||
| uploader := NewArtifactUploader(baseURL, orgID) | ||
|
|
||
| if uploader.baseURL != baseURL { | ||
| t.Errorf("Expected baseURL '%s', got '%s'", baseURL, uploader.baseURL) | ||
| } | ||
|
|
||
| if uploader.orgID != orgID { | ||
| t.Errorf("Expected orgID '%s', got '%s'", orgID, uploader.orgID) | ||
| } | ||
|
|
||
| if uploader.client == nil { | ||
| t.Error("Expected client to be initialized") | ||
| } | ||
|
|
||
| if uploader.client.Timeout == 0 { | ||
| t.Error("Expected client timeout to be set") | ||
| } | ||
| } | ||
|
|
||
| func TestTransactionRequest(t *testing.T) { | ||
| metadata := &ArtifactMetadata{ | ||
| Repository: "test/repo", | ||
| RepositoryOwner: "test", | ||
| RunID: "123456", | ||
| Artifacts: []string{"artifact1", "artifact2"}, | ||
| } | ||
|
|
||
| artifactNames := []string{"artifact1", "artifact2"} | ||
|
|
||
| req := TransactionRequest{ | ||
| Meta: metadata, | ||
| Artifacts: artifactNames, | ||
| } | ||
|
|
||
| if req.Meta.Repository != "test/repo" { | ||
| t.Errorf("Expected repository 'test/repo', got '%s'", req.Meta.Repository) | ||
| } | ||
|
|
||
| if len(req.Artifacts) != 2 { | ||
| t.Errorf("Expected 2 artifacts, got %d", len(req.Artifacts)) | ||
| } | ||
| } | ||
|
|
||
| func TestArtifactStatusDetail(t *testing.T) { | ||
| status := ArtifactStatusDetail{ | ||
| UUID: "test-uuid", | ||
| Name: "test-artifact", | ||
| Status: "completed", | ||
| QueuePath: "/queue/path", | ||
| } | ||
|
|
||
| if status.UUID != "test-uuid" { | ||
| t.Errorf("Expected UUID 'test-uuid', got '%s'", status.UUID) | ||
| } | ||
|
|
||
| if status.Name != "test-artifact" { | ||
| t.Errorf("Expected name 'test-artifact', got '%s'", status.Name) | ||
| } | ||
|
|
||
| if status.Status != "completed" { | ||
| t.Errorf("Expected status 'completed', got '%s'", status.Status) | ||
| } | ||
|
|
||
| if status.QueuePath != "/queue/path" { | ||
| t.Errorf("Expected queue path '/queue/path', got '%s'", status.QueuePath) | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests only cover struct initialization and field assignment, but none of the actual upload functionality (InitiateTransaction, UploadArtifact, GetTransactionStatus, GetArtifactStatus) is tested. These are the core features of the uploader and should have unit tests with mocked HTTP responses to verify correct behavior, error handling, and API interaction.
| path := filepath.Join(destDir, file.Name) | ||
|
|
||
| if file.FileInfo().IsDir() { | ||
| os.MkdirAll(path, file.Mode()) |
There was a problem hiding this comment.
Error from os.MkdirAll when creating directory is not checked. If directory creation fails, the code will proceed to try to create files, which will fail with potentially confusing error messages. Check and return the error from os.MkdirAll.
| os.MkdirAll(path, file.Mode()) | |
| if err := os.MkdirAll(path, file.Mode()); err != nil { | |
| return err | |
| } |
| return err | ||
| } | ||
|
|
||
| destFile, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) |
There was a problem hiding this comment.
The file mode from the zip entry is used directly when creating files, which could preserve executable permissions or setuid/setgid bits from potentially malicious zip files. Consider using a fixed safe permission mode like 0644 for regular files and 0755 for directories instead of file.Mode().
| destFile.Close() | ||
| return err | ||
| } | ||
|
|
||
| _, err = io.Copy(destFile, fileReader) | ||
| destFile.Close() | ||
| fileReader.Close() | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
| destFile.Close() | |
| return err | |
| } | |
| _, err = io.Copy(destFile, fileReader) | |
| destFile.Close() | |
| fileReader.Close() | |
| if err != nil { | |
| return err | |
| } | |
| if closeErr := destFile.Close(); closeErr != nil { | |
| return closeErr | |
| } | |
| return err | |
| } | |
| n, copyErr := io.Copy(destFile, fileReader) | |
| _ = n | |
| closeErr := destFile.Close() | |
| fileReader.Close() | |
| if copyErr != nil && closeErr != nil { | |
| return fmt.Errorf("copy error: %v; close error: %v", copyErr, closeErr) | |
| } | |
| if closeErr != nil { | |
| return closeErr | |
| } | |
| if copyErr != nil { | |
| return copyErr | |
| } |
| destFile.Close() | ||
| fileReader.Close() | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
| destFile.Close() | |
| fileReader.Close() | |
| if err != nil { | |
| return err | |
| } | |
| destCloseErr := destFile.Close() | |
| readerCloseErr := fileReader.Close() | |
| if err != nil { | |
| return err | |
| } | |
| if destCloseErr != nil { | |
| return destCloseErr | |
| } | |
| if readerCloseErr != nil { | |
| return readerCloseErr | |
| } |
This commit introduces a new
ghasubcommand for seamless integrationwith GitHub Actions workflows, enabling automated upload of workflow
artifacts to Vulnetix for vulnerability analysis.
New Features:
vulnetix gha upload: Upload all workflow artifacts with metadatavulnetix gha status: Check upload status by txnid or artifact UUIDImplementation Details:
internal/githubfor artifact handlingAPI Integration:
Documentation:
Testing: