feat: add download rulesets handler #49
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete ruleset download feature with configuration management, gRPC/HTTP endpoint, business logic, telemetry integration, and comprehensive testing. It includes a new handler, use-case layer with path validation and version resolution, test infrastructure updates, and test data fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as gRPC Client
participant Handler as RulesetDownloadHandler
participant UseCase as RulesetDownloadUseCase
participant FS as File System
participant Config as ServerConfig
Client->>Handler: DownloadRuleset(ctx, name, version)
activate Handler
Handler->>Handler: validateRequest()
alt Invalid Request
Handler->>Client: Error (name/version empty or invalid semver)
else Valid Request
Handler->>UseCase: DownloadRuleset(ctx, name, version)
activate UseCase
UseCase->>Config: Get StoragePath
UseCase->>FS: Check base directory exists
UseCase->>UseCase: resolveVersion(name, version)
alt "latest" version
UseCase->>FS: Read latest symlink
else Specific version
UseCase->>FS: Verify version path exists
end
UseCase->>UseCase: verifyPathContainment()
alt Path Traversal Detected
UseCase->>Handler: Error (path outside base)
else Path Valid
UseCase->>FS: Read manifest.json
UseCase->>UseCase: readMetadata()
UseCase->>FS: Read tarball (name-version.tar.gz)
UseCase->>UseCase: readTarball()
UseCase->>UseCase: verifyTarballChecksum()
alt Checksum Mismatch
UseCase->>Handler: Error (checksum failed)
else Checksum Valid
UseCase->>Handler: RulesetDownloadOutput (metadata + tarball data)
end
end
deactivate UseCase
alt Use Case Error
Handler->>Handler: handleUseCaseError()
Handler->>Handler: Map to gRPC status (NotFound, Internal, DataLoss)
Handler->>Client: Error response
else Success
Handler->>Handler: Build response headers (SCANOSS-Ruleset-*, X-Checksum-SHA256)
Handler->>Handler: telemetryDownloadRulesetRequestTime()
Handler->>Handler: telemetryAddRulesetDownloaded()
Handler->>Client: HttpBody (tarball data + headers)
end
end
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/handlers/ruleset_download_handler.go (1)
101-108: Use resolved version from metadata for filename consistency.Line 101 constructs the filename using
request.Version, but when a client requests "latest", this creates a filename likeruleset-latest.tar.gz. The actual resolved version is available inruleset.Metadata.Versionand would create more meaningful filenames likeruleset-v1.2.3.tar.gz.🔎 Apply this diff to use the resolved version:
- filename := fmt.Sprintf("%s-%s.tar.gz", request.RulesetName, request.Version) + filename := fmt.Sprintf("%s-%s.tar.gz", ruleset.Metadata.Name, ruleset.Metadata.Version)pkg/usecase/ruleset_download.go (1)
119-142: Consider streaming large tarballs instead of loading into memory.Line 133 loads the entire tarball into memory with
os.ReadFile. For large rulesets, this could consume significant memory, especially under concurrent requests. While this may be acceptable for typical ruleset sizes, consider whether streaming the file directly to the response would be more scalable.This would require changes to the handler layer to stream the response, so it may be deferred based on actual ruleset sizes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
CHANGELOG.md(2 hunks)config/app-config-dev.json(1 hunks)go.mod(2 hunks)pkg/config/server_config.go(2 hunks)pkg/handlers/ruleset_download_handler.go(1 hunks)pkg/handlers/ruleset_download_handler_test.go(1 hunks)pkg/service/cryptography_service.go(3 hunks)pkg/usecase/ruleset_download.go(1 hunks)pkg/usecase/ruleset_download_test.go(1 hunks)test-support/rulesets/dca/latest(1 hunks)test-support/rulesets/dca/v1.0.0/manifest.json(1 hunks)test-support/rulesets/dca/v1.0.1/manifest.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/usecase/ruleset_download_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(30-74)pkg/usecase/ruleset_download.go (3)
NewRulesetDownload(51-55)RulesetDownloadOutput(40-43)RulesetMetadata(31-37)
pkg/service/cryptography_service.go (1)
pkg/handlers/ruleset_download_handler.go (2)
RulesetDownloadHandler(42-45)NewRulesetDownloadHandler(57-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
go.mod (1)
40-40: The// indirectcomment removal is justified.The RulesetDownload handler imports
google.golang.org/genproto/googleapis/api/httpbody, confirming this should be a direct dependency rather than indirect.config/app-config-dev.json (1)
12-14: LGTM! Configuration is appropriate for dev environment.The new Rulesets configuration with the test-support path is well-suited for development and testing.
test-support/rulesets/dca/v1.0.0/manifest.json (1)
1-7: LGTM! Test data is well-structured.The manifest structure includes all necessary fields for testing the ruleset download functionality.
test-support/rulesets/dca/latest (1)
1-1: LGTM! Version indicator is correctly configured.This file correctly points to v1.0.1 as the latest version for testing.
pkg/config/server_config.go (2)
71-73: LGTM! Configuration structure follows established patterns.The new Rulesets configuration section is well-integrated with the existing ServerConfig structure and follows the same conventions as other configuration blocks.
111-111: LGTM! Default path follows Linux filesystem conventions.The default storage path under
/var/libis appropriate for application data in production environments.test-support/rulesets/dca/v1.0.1/manifest.json (1)
1-7: LGTM! Test data is consistent with v1.0.0 manifest.The v1.0.1 manifest follows the same structure as v1.0.0, providing appropriate test coverage for multiple versions.
pkg/handlers/ruleset_download_handler_test.go (4)
34-54: LGTM! Initialization test is thorough.The test correctly verifies that the handler, config, and use case are properly initialized.
56-170: LGTM! Validation test coverage is comprehensive.The table-driven tests effectively cover edge cases including nil requests, empty/whitespace fields, and semver validation.
172-283: LGTM! Download flow tests provide good coverage.The tests effectively validate error handling for invalid requests and not-found scenarios using real test data.
285-372: LGTM! Error handling test is thorough.The test effectively validates error mapping to appropriate gRPC status codes for various failure scenarios.
pkg/usecase/ruleset_download_test.go (6)
31-45: LGTM! Initialization test is appropriate.The test correctly verifies that the use case is properly initialized with the expected configuration.
47-183: LGTM! Download test provides comprehensive coverage.The test effectively validates the complete download flow including version resolution, metadata parsing, and tarball retrieval with proper error handling.
185-271: LGTM! Version resolution test is thorough.The test effectively validates version resolution including the "latest" symlink mechanism and error cases.
273-351: LGTM! Metadata reading test is well-structured.The test properly validates metadata parsing and field extraction with appropriate error handling.
353-425: LGTM! Tarball reading test is appropriate.The test validates tarball retrieval and ensures data is properly loaded from the filesystem.
427-466: LGTM! JSON serialization test is thorough.The test effectively validates that RulesetMetadata correctly marshals and unmarshals JSON, ensuring all fields round-trip properly.
pkg/service/cryptography_service.go (1)
27-27: LGTM! Clean integration of the new download handler.The wiring follows the established patterns in this service:
- Import added for the HTTP body response type
- Handler field added alongside existing handlers
- Constructor initialization matches other handlers
- Delegation method follows the same simple pattern
Also applies to: 44-44, 51-59, 154-157
pkg/handlers/ruleset_download_handler.go (1)
141-145: The validation is correct—the Masterminds/semver/v3 library handles "v" prefix parsing.The
NewVersionfunction attempts to coerce a version into a semantic version, including handling a leading "v" prefix. The error message format suggesting 'v1.2.3' is appropriate and aligns with the library's documented behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/usecase/ruleset_download.go (1)
33-40: Minor: Comment formatting.The comment on line 33 should end with a period per Go style guidelines.
🔎 Apply this diff:
-// RulesetMetadata represents the manifest.json structure +// RulesetMetadata represents the manifest.json structure. type RulesetMetadata struct {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(3 hunks)pkg/usecase/ruleset_download.go(1 hunks)test-support/rulesets/dca/v1.0.0/manifest.json(1 hunks)test-support/rulesets/dca/v1.0.1/manifest.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test-support/rulesets/dca/v1.0.0/manifest.json
- test-support/rulesets/dca/v1.0.1/manifest.json
🧰 Additional context used
🪛 GitHub Actions: Go Unit Test
pkg/usecase/ruleset_download.go
[error] 159-159: Tarball not found while reading tarball for ruleset 'dca' v1.0.0. tarball path: /home/runner/work/cryptography/cryptography/test-support/rulesets/dca/v1.0.0/dca-v1.0.0.tar.gz
[error] 163-163: Tarball not found while reading tarball for ruleset 'dca' v1.0.1. tarball path: /home/runner/work/cryptography/cryptography/test-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gz
[error] 159-159: Tarball not found while reading tarball for ruleset 'dca' latest. tarball path: /home/runner/work/cryptography/cryptography/test-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gz
🪛 GitHub Check: build
pkg/usecase/ruleset_download.go
[failure] 33-33:
Comment should end in a period (godot)
🔇 Additional comments (10)
go.mod (3)
73-73: ✅ Past issue resolved: replace directive is now commented.The active replace directive that was flagged as breaking CI/CD builds has been properly commented out. This ensures that Go module resolution uses the canonical remote module path in all environments (local, CI/CD, containers, deployments).
40-40: Dependency change verified — moving to direct dependency is justified.The
google.golang.org/genproto/googleapis/apidependency change from indirect to direct is appropriate. The httpbody package is used for streaming API methods in both request and response, and the codebase explicitly imports and uses it in theRulesetDownloadHandlerfor binary tarball responses.
13-13: Verify papi v0.27.0 release availability.The module
github.com/scanoss/papi v0.27.0could not be verified in public Go registries. Confirm this is a valid, published release or check if the module path is correct.pkg/usecase/ruleset_download.go (7)
42-58: LGTM!The output structure, use-case struct, and constructor are well-designed and straightforward.
60-80: Excellent security implementation!The path containment verification correctly prevents path traversal attacks. The logic properly handles:
- Converting paths to absolute form to resolve any relative segments
- Checking for prefix match with separator to prevent "/base/dir" matching "/base/dir-malicious"
- Handling the edge case where the resolved path equals the base exactly
82-128: Path traversal vulnerability has been addressed!The symlink resolution now correctly validates that the resolved target remains within the storage directory (lines 115-119). This addresses the critical security concern raised in previous reviews where a malicious "latest" symlink could point to sensitive system files.
The implementation properly:
- Handles both relative and absolute symlink targets
- Validates containment after resolution
- Logs security violations appropriately
130-152: LGTM!The metadata reading and parsing is well-implemented with appropriate error handling and logging.
180-202: Checksum validation properly implemented!This method successfully addresses the security concern raised in previous reviews about missing checksum validation. The implementation correctly:
- Validates that the expected checksum is not empty
- Computes SHA256 hash of the tarball data
- Compares actual vs expected checksums
- Returns descriptive errors on mismatch
This ensures that corrupted or tampered tarballs are rejected before being served to clients.
204-233: Excellent orchestration with proper security checks!The download workflow correctly sequences all operations and includes the checksum validation (line 225) that was requested in previous reviews. The tarball integrity is now verified before being returned to the caller, ensuring that only valid, untampered files are served.
The context cancellation check at the beginning is also a good practice for long-running operations.
154-178: Test fixture files are missing—code is correct.The
readTarballfunction constructs filenames correctly usingfmt.Sprintf("%s-%s.tar.gz", rulesetName, rulesetVersion). The test case expects it to readdca-v1.0.1.tar.gz, which correctly uses the version string from manifest.json ("v1.0.1") without modification.The actual issue is that the test-support directory is missing the tarball files themselves. Only manifest.json files exist; the corresponding
.tar.gzfiles that the test expects to find are not present in the repository. This is a test fixture issue, not a code defect.The version string handling is consistent and correct—no prefix stripping is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/handlers/ruleset_download_handler.go (2)
127-148: Add input validation for rulesetName to prevent path traversal and header injection.The current validation checks for empty values and version format but doesn't sanitize
rulesetNamefor dangerous characters. While the use case provides path validation as a backstop, the handler should validate input as a defense-in-depth measure. Additionally,rulesetNameis used directly in the filename construction at line 101, which could allow manipulation of theContent-Dispositionheader.🔎 Apply this diff to add rulesetName validation:
func (r *RulesetDownloadHandler) validateRequest(request *pb.RulesetDownloadRequest) error { if request == nil { return fmt.Errorf("request cannot be empty") } if strings.TrimSpace(request.RulesetName) == "" { return fmt.Errorf("ruleset_name cannot be empty") } + + // Validate rulesetName doesn't contain path traversal or unsafe characters + rulesetName := strings.TrimSpace(request.RulesetName) + if strings.Contains(rulesetName, "..") || + strings.Contains(rulesetName, "/") || + strings.Contains(rulesetName, "\\") || + strings.ContainsAny(rulesetName, "\x00\r\n") { + return fmt.Errorf("ruleset_name contains invalid characters") + } if strings.TrimSpace(request.Version) == "" { return fmt.Errorf("version cannot be empty, you must provide a specific version or 'latest'") }Note: This addresses a similar concern raised in previous review comments.
150-166: LGTM! Pragmatic error mapping implementation.The error handling correctly:
- Maps use case errors to appropriate gRPC and HTTP status codes
- Covers the main error categories (not found, internal errors)
- Sets HTTP codes via trailer for REST gateway compatibility
- Provides clear error messages to clients
The string-based error detection is pragmatic for the current implementation. For future enhancement, consider using typed errors for more robust error classification, but the current approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/handlers/ruleset_download_handler.go(1 hunks)pkg/usecase/ruleset_download.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/handlers/ruleset_download_handler.go (3)
pkg/usecase/ruleset_download.go (2)
RulesetDownloadUseCase(49-51)NewRulesetDownload(54-58)pkg/config/server_config.go (1)
ServerConfig(30-74)pkg/httphelper/http_helper.go (1)
SetHTTPCodeOnTrailer(33-38)
pkg/usecase/ruleset_download.go (1)
pkg/config/server_config.go (1)
ServerConfig(30-74)
🪛 GitHub Actions: Go Unit Test
pkg/usecase/ruleset_download.go
[error] 163-163: tarball file not found: /home/runner/work/cryptography/cryptography/test-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gz
🪛 GitHub Actions: Golang CI Lint
pkg/usecase/ruleset_download.go
[error] 42-42: golangci-lint: Comment should end in a period (godot)
🪛 GitHub Check: build
pkg/usecase/ruleset_download.go
[failure] 53-53:
Comment should end in a period (godot)
[failure] 48-48:
Comment should end in a period (godot)
[failure] 42-42:
Comment should end in a period (godot)
🔇 Additional comments (8)
pkg/usecase/ruleset_download.go (6)
60-80: LGTM! Robust path containment validation.The implementation correctly prevents path traversal by:
- Canonicalizing both paths with
filepath.Abs- Requiring the resolved path to start with the base directory followed by a separator (preventing partial matches like "/base/dir" vs "/base/dir-malicious")
- Allowing exact match as a valid case
This addresses the path traversal concerns raised in previous reviews.
82-128: LGTM! Secure symlink resolution with proper path validation.The implementation correctly handles the "latest" symlink resolution with comprehensive security checks:
- Validates basePath containment before processing
- Resolves both relative and absolute symlink targets
- Validates the final resolved path stays within storage directory (line 116)
- Provides clear error messages for each failure case
This properly addresses the critical path traversal vulnerability flagged in previous reviews.
130-152: LGTM! Clean metadata parsing implementation.The metadata reading logic is straightforward and secure:
- Works with pre-validated paths from
resolveVersion- Includes appropriate error handling and logging
- Returns structured metadata for downstream use
154-178: LGTM! Tarball reading with appropriate validation.The implementation correctly:
- Constructs the expected tarball filename from metadata
- Validates file existence with
os.Stat- Reads and returns the tarball data
- Includes comprehensive logging
The use of metadata fields (name, version) from the manifest is safe since the manifest itself must reside in a pre-validated storage directory, limiting exposure to malicious manifest content.
180-202: LGTM! Comprehensive checksum validation.The implementation properly validates tarball integrity:
- Rejects empty checksums (preventing bypass)
- Computes SHA256 hash of the tarball data
- Compares against expected checksum from metadata
- Provides clear error messages and debug logging
This addresses the major security concern raised in previous reviews about missing checksum validation.
204-233: LGTM! Well-orchestrated download flow.The main orchestration method correctly:
- Checks for context cancellation early
- Executes steps in the right order: resolve → read metadata → read tarball → verify checksum
- Propagates errors appropriately at each stage
- Returns a clean, structured output combining metadata and tarball data
The integration of checksum verification ensures end-to-end integrity validation.
pkg/handlers/ruleset_download_handler.go (2)
41-62: LGTM! Clean handler initialization.The handler structure and constructor are well-designed:
- Properly initializes the use case with server configuration
- Maintains clean dependencies
- Includes comprehensive documentation
64-125: LGTM! Well-structured request handler with good observability.The download handler implementation is solid:
- Proper request validation with clear error codes
- Delegates business logic to the use case appropriately
- Includes comprehensive metadata headers (content-type, disposition, checksums)
- Excellent observability with logging and telemetry tracking
- Appropriate error mapping through
handleUseCaseErrorNote: The filename construction at line 101 relies on validated input, which should be enhanced per the comment on the
validateRequestmethod.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/usecase/ruleset_download.go (1)
130-152: Consider validating critical metadata fields.The metadata parsing is correct, but you could optionally add validation to ensure
name,version, andchecksum_sha256fields are non-empty after unmarshaling. This would provide clearer error messages if the manifest.json is malformed.🔎 Optional validation to add after line 146:
var metadata RulesetMetadata if err := json.Unmarshal(data, &metadata); err != nil { s.Warnf("Failed to parse metadata JSON from %s: %v", metadataPath, err) return RulesetMetadata{}, fmt.Errorf("failed to parse metadata: %w", err) } + + // Validate required fields + if strings.TrimSpace(metadata.Name) == "" || strings.TrimSpace(metadata.Version) == "" { + return RulesetMetadata{}, fmt.Errorf("manifest.json missing required name or version field") + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test-support/rulesets/dca/v1.0.0/dca-v1.0.0.tar.gzis excluded by!**/*.gztest-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
.gitignore(1 hunks)pkg/usecase/ruleset_download.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/usecase/ruleset_download.go (1)
pkg/config/server_config.go (1)
ServerConfig(30-74)
🔇 Additional comments (7)
.gitignore (1)
47-47: Looks good.The negated ignore rule correctly unignores
.tar.gztarballs under rulesets directories while preserving the blanket ignore of other.gz,.tar, and.tgzfiles globally. Placement after the ignore rules it negates is correct.pkg/usecase/ruleset_download.go (6)
1-31: LGTM! Clean imports and proper licensing.The license header and import structure are well-organized. All dependencies are appropriate for the use case.
33-58: LGTM! Well-structured types and constructor.The type definitions are clean with appropriate JSON tags, and the constructor follows Go conventions.
60-80: Excellent path traversal protection!The containment verification properly addresses the previous critical security vulnerability. The logic correctly handles edge cases including the separator check to prevent prefix confusion attacks.
82-128: LGTM! Secure version resolution with proper symlink handling.The method correctly resolves versions including "latest" symlinks, with appropriate containment validation at both the ruleset directory level (line 88) and symlink target level (line 116). This fully addresses the previous path traversal concerns.
154-178: LGTM! Clean tarball reading with appropriate error handling.The implementation correctly constructs the tarball filename and reads the file with good logging and error handling.
204-233: LGTM! Well-orchestrated download flow with proper security checks.The method correctly orchestrates all steps: version resolution, metadata parsing, tarball reading, and checksum verification. Context cancellation is properly checked, and the checksum validation addresses the previous major security concern.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/handlers/ruleset_download_handler.go (1)
137-157: Path traversal validation for rulesetName is still missing (security critical).As noted in the previous review,
rulesetNamelacks validation to prevent path traversal attacks. An attacker could potentially access files outside the intended storage directory by using inputs like../../../etc/passwdor similar patterns.Although downstream filesystem operations may fail, explicit validation here provides defense-in-depth and clearer error messages.
🔎 Recommended validation (from previous review)
func (r *RulesetDownloadHandler) validateRequest(request *pb.RulesetDownloadRequest) error { if request == nil { return fmt.Errorf("request cannot be empty") } if strings.TrimSpace(request.RulesetName) == "" { return fmt.Errorf("ruleset_name cannot be empty") } + + // Prevent path traversal attacks + rulesetName := strings.TrimSpace(request.RulesetName) + if strings.Contains(rulesetName, "..") || strings.Contains(rulesetName, "/") || strings.Contains(rulesetName, "\\") { + return fmt.Errorf("ruleset_name contains invalid characters") + } if strings.TrimSpace(request.Version) == "" { return fmt.Errorf("version cannot be empty, you must provide a specific version or 'latest'") }Based on learnings, address past review comments to ensure security vulnerabilities are not carried forward.
🧹 Nitpick comments (2)
pkg/handlers/ruleset_download_handler.go (2)
98-101: Consider validating tarball data is non-empty.Although the use case likely handles this, adding a defensive check here would catch edge cases where an empty tarball is returned (e.g., corrupted file, incomplete read).
🔎 Suggested validation
ruleset, err := r.rulesetDownloadUseCase.DownloadRuleset(ctx, s, request.RulesetName, request.Version) if err != nil { return r.handleUseCaseError(ctx, s, err) } + + if len(ruleset.TarballData) == 0 { + s.Errorf("Empty tarball data for ruleset %s/%s", request.RulesetName, request.Version) + httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusInternalServerError) + return nil, status.Errorf(codes.Internal, "empty tarball data") + }
159-175: Consider using typed errors for more robust error handling.The current string-matching approach works but is fragile if error messages change. Using typed errors (via
errors.Isor custom error types) would make this more maintainable.💡 Example approach with typed errors
In the use case layer, define error types:
var ( ErrRulesetNotFound = errors.New("ruleset not found") ErrInternal = errors.New("internal error") )Then wrap errors accordingly in the use case:
return fmt.Errorf("%w: version does not exist", ErrRulesetNotFound)And check them here:
func (r *RulesetDownloadHandler) handleUseCaseError(ctx context.Context, s *zap.SugaredLogger, err error) (*httpbody.HttpBody, error) { switch { case errors.Is(err, ErrRulesetNotFound): httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusNotFound) return nil, status.Errorf(codes.NotFound, "ruleset not found: %v", err) default: httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusInternalServerError) return nil, status.Errorf(codes.Internal, "internal error: %v", err) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/handlers/ruleset_download_handler.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/handlers/ruleset_download_handler.go (3)
pkg/usecase/ruleset_download.go (2)
RulesetDownloadUseCase(49-51)NewRulesetDownload(54-58)pkg/config/server_config.go (1)
ServerConfig(30-74)pkg/httphelper/http_helper.go (1)
SetHTTPCodeOnTrailer(33-38)
🔇 Additional comments (1)
pkg/handlers/ruleset_download_handler.go (1)
41-62: LGTM! Clean design with good documentation.The handler structure and constructor follow best practices with clear dependency injection and comprehensive documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/handlers/ruleset_download_handler.go (3)
103-103: Use resolved version in filename instead of request version.When
request.Versionis "latest", the filename becomesruleset-latest.tar.gzinstead of the actual resolved version. This issue was previously flagged and remains unresolved.🔎 Proposed fix
- filename := fmt.Sprintf("%s-%s.tar.gz", request.RulesetName, request.Version) + filename := fmt.Sprintf("%s-%s.tar.gz", request.RulesetName, ruleset.Metadata.Version)
107-110: Sanitize metadata fields to prevent header injection attacks.Metadata fields from the filesystem are directly inserted into HTTP headers without sanitization. If metadata contains newlines, carriage returns, or control characters, it could enable header injection attacks. This critical security issue was previously flagged and remains unresolved.
🔎 Recommended sanitization approach
Add a helper function to sanitize header values:
// sanitizeHeaderValue removes control characters that could enable header injection func sanitizeHeaderValue(value string) string { return strings.Map(func(r rune) rune { if r == '\n' || r == '\r' || r < 32 || r == 127 { return -1 // remove control characters } return r }, value) }Then apply it to all metadata fields:
headers := []string{ "content-type", "application/gzip", "content-disposition", fmt.Sprintf("attachment; filename=\"%s\"", filename), - "scanoss-ruleset-name", ruleset.Metadata.Name, - "scanoss-ruleset-version", ruleset.Metadata.Version, - "scanoss-ruleset-created-at", ruleset.Metadata.CreatedAt, + "scanoss-ruleset-name", sanitizeHeaderValue(ruleset.Metadata.Name), + "scanoss-ruleset-version", sanitizeHeaderValue(ruleset.Metadata.Version), + "scanoss-ruleset-created-at", sanitizeHeaderValue(ruleset.Metadata.CreatedAt), - "x-checksum-sha256", ruleset.Metadata.ChecksumSHA256, + "x-checksum-sha256", sanitizeHeaderValue(ruleset.Metadata.ChecksumSHA256), }And also sanitize the Description field on line 114:
if strings.TrimSpace(ruleset.Metadata.Description) != "" { - headers = append(headers, "scanoss-ruleset-description", ruleset.Metadata.Description) + headers = append(headers, "scanoss-ruleset-description", sanitizeHeaderValue(ruleset.Metadata.Description)) }
137-158: Add path traversal validation for rulesetName.The validation checks the version format but doesn't validate
rulesetNamefor dangerous characters. Although filesystem operations in the use case may fail for invalid paths, explicit validation here would provide better security and clearer error messages. This security issue was previously flagged and remains unresolved.🔎 Proposed validation enhancement
func (r *RulesetDownloadHandler) validateRequest(request *pb.RulesetDownloadRequest) error { if request == nil { return fmt.Errorf("request cannot be empty") } if strings.TrimSpace(request.RulesetName) == "" { return fmt.Errorf("ruleset_name cannot be empty") } + + // Prevent path traversal attacks + rulesetName := strings.TrimSpace(request.RulesetName) + if strings.Contains(rulesetName, "..") || strings.Contains(rulesetName, "/") || strings.Contains(rulesetName, "\\") { + return fmt.Errorf("ruleset_name contains invalid characters") + } if strings.TrimSpace(request.Version) == "" { return fmt.Errorf("version cannot be empty, you must provide a specific version or 'latest'") } version := strings.TrimSpace(request.Version) _, err := semver.NewVersion(version) if err != nil && version != "latest" { return fmt.Errorf("version must be 'latest' or a valid semver string (e.g., 'v1.2.3')") } return nil }
🧹 Nitpick comments (1)
pkg/handlers/ruleset_download_handler.go (1)
160-176: Consider using typed errors instead of string matching.The current error handling relies on string matching (
strings.Contains) to determine HTTP status codes, which is fragile and can break if error messages change in the use case layer.Consider defining typed errors in the use case package:
// In pkg/usecase/errors.go var ( ErrRulesetNotFound = errors.New("ruleset not found") ErrVersionNotFound = errors.New("version not found") ErrInternalError = errors.New("internal error") )Then use
errors.Is()for checking:func (r *RulesetDownloadHandler) handleUseCaseError(ctx context.Context, s *zap.SugaredLogger, err error) (*httpbody.HttpBody, error) { switch { case errors.Is(err, usecase.ErrRulesetNotFound), errors.Is(err, usecase.ErrVersionNotFound): httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusNotFound) return nil, status.Errorf(codes.NotFound, "ruleset not found: %v", err) case errors.Is(err, usecase.ErrInternalError): httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusInternalServerError) return nil, status.Errorf(codes.Internal, "internal error: %v", err) default: httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusInternalServerError) return nil, status.Errorf(codes.Internal, "unexpected error: %v", err) } }This approach is more maintainable and less prone to breaking when error messages are updated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/handlers/cryptography_support.gopkg/handlers/ruleset_download_handler.gopkg/handlers/utils_handler.go
🔇 Additional comments (4)
pkg/handlers/cryptography_support.go (2)
38-39: LGTM! Metrics additions follow existing patterns.The new histogram and counter metrics for ruleset downloads are well-structured and consistent with the existing
cryptoAlgorithmsHistogrammetric.
44-52: LGTM! Metric initialization follows established patterns.The metric initialization is consistent with the existing code style. The metric names (
crypto.rulesets.download_time,crypto.rulesets.downloaded) follow the established naming convention, and the descriptions are clear.pkg/handlers/utils_handler.go (1)
60-72: LGTM! Counter increment follows telemetry best practices.The function correctly increments the download counter with proper telemetry enablement checks.
pkg/handlers/ruleset_download_handler.go (1)
47-62: LGTM! Constructor follows clean initialization pattern.The handler initialization is straightforward and properly encapsulates the use case dependency.
matiasdaloia
force-pushed
the
feature/mdaloia/SP-3798-SCANOSS-Crypto-Framework-Download-rules-API
branch
from
f704a36 to
63d08d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/handlers/ruleset_download_handler.go (2)
103-115: Address past security review: sanitize metadata and use resolved version.Two concerns from past reviews remain:
- Line 103: Uses
ruleset.Metadata.Version(resolved) in filename - this is correct and addresses the past comment ✓- Lines 107-110: Metadata fields are inserted into headers without sanitization, enabling header injection if they contain newlines or control characters
🔎 Add sanitization helper
+// sanitizeHeaderValue removes control characters that could enable header injection. +func sanitizeHeaderValue(value string) string { + return strings.Map(func(r rune) rune { + if r == '\n' || r == '\r' || r < 32 || r == 127 { + return -1 + } + return r + }, value) +} + func (r *RulesetDownloadHandler) DownloadRuleset(ctx context.Context, request *pb.RulesetDownloadRequest) (*httpbody.HttpBody, error) { // ... existing code ... filename := fmt.Sprintf("%s-%s.tar.gz", ruleset.Metadata.Name, ruleset.Metadata.Version) headers := []string{ "content-type", "application/gzip", "content-disposition", fmt.Sprintf("attachment; filename=\"%s\"", filename), - "scanoss-ruleset-name", ruleset.Metadata.Name, - "scanoss-ruleset-version", ruleset.Metadata.Version, - "scanoss-ruleset-created-at", ruleset.Metadata.CreatedAt, - "x-checksum-sha256", ruleset.Metadata.ChecksumSHA256, + "scanoss-ruleset-name", sanitizeHeaderValue(ruleset.Metadata.Name), + "scanoss-ruleset-version", sanitizeHeaderValue(ruleset.Metadata.Version), + "scanoss-ruleset-created-at", sanitizeHeaderValue(ruleset.Metadata.CreatedAt), + "x-checksum-sha256", sanitizeHeaderValue(ruleset.Metadata.ChecksumSHA256), }
137-158: Address past security review: add path traversal validation for rulesetName.The validation checks version format but doesn't sanitize
rulesetNamefor path traversal sequences. While the use-case hasverifyPathContainment, explicit validation here provides defense in depth and clearer error messages.🔎 Add rulesetName validation
func (r *RulesetDownloadHandler) validateRequest(request *pb.RulesetDownloadRequest) error { if request == nil { return fmt.Errorf("request cannot be empty") } if strings.TrimSpace(request.RulesetName) == "" { return fmt.Errorf("ruleset_name cannot be empty") } + + // Prevent path traversal attacks + rulesetName := strings.TrimSpace(request.RulesetName) + if strings.Contains(rulesetName, "..") || strings.Contains(rulesetName, "/") || strings.Contains(rulesetName, "\\") { + return fmt.Errorf("ruleset_name contains invalid characters") + } if strings.TrimSpace(request.Version) == "" { return fmt.Errorf("version cannot be empty, you must provide a specific version or 'latest'") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumtest-support/rulesets/dca/v1.0.0/dca-v1.0.0.tar.gzis excluded by!**/*.gztest-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (15)
.gitignoreCHANGELOG.mdconfig/app-config-dev.jsongo.modpkg/config/server_config.gopkg/handlers/cryptography_support.gopkg/handlers/ruleset_download_handler.gopkg/handlers/ruleset_download_handler_test.gopkg/handlers/utils_handler.gopkg/service/cryptography_service.gopkg/usecase/ruleset_download.gopkg/usecase/ruleset_download_test.gotest-support/rulesets/dca/latesttest-support/rulesets/dca/v1.0.0/manifest.jsontest-support/rulesets/dca/v1.0.1/manifest.json
🚧 Files skipped from review as they are similar to previous changes (7)
- test-support/rulesets/dca/latest
- pkg/handlers/utils_handler.go
- go.mod
- pkg/usecase/ruleset_download_test.go
- pkg/handlers/cryptography_support.go
- config/app-config-dev.json
- test-support/rulesets/dca/v1.0.1/manifest.json
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/cryptography_service.go (1)
pkg/handlers/ruleset_download_handler.go (2)
RulesetDownloadHandler(42-45)NewRulesetDownloadHandler(57-62)
pkg/handlers/ruleset_download_handler_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(33-77)pkg/handlers/ruleset_download_handler.go (1)
NewRulesetDownloadHandler(57-62)
pkg/usecase/ruleset_download.go (1)
pkg/config/server_config.go (1)
ServerConfig(33-77)
🪛 GitHub Actions: Go Unit Test
pkg/handlers/ruleset_download_handler.go
[error] 164-164: Ruleset download test: handleUseCaseError path produced 'internal server error' but test expected message containing 'internal error'.
🔇 Additional comments (21)
.gitignore (1)
45-47: LGTM! Exception pattern is appropriate.The negation rule correctly allows versioned ruleset tarballs to be committed while maintaining the general ignore for compressed files. The pattern is specific and aligns with the test fixture structure.
CHANGELOG.md (1)
7-16: Release documentation looks comprehensive.The changelog entry clearly documents the new DownloadRuleset feature with appropriate detail on endpoints, headers, and metadata handling. The format follows existing conventions.
pkg/handlers/ruleset_download_handler_test.go (3)
34-54: LGTM! Initialization test is thorough.The test properly validates that all handler fields are initialized correctly.
56-170: LGTM! Validation test coverage is comprehensive.The test cases appropriately cover nil requests, empty fields, whitespace-only values, invalid semver, and valid inputs including the "latest" keyword.
172-283: LGTM! Download flow tests cover key scenarios.The tests appropriately validate invalid requests, not-found cases, and the general error handling flow with correct status code expectations.
test-support/rulesets/dca/v1.0.0/manifest.json (1)
1-7: LGTM! Test manifest is well-formed.The JSON structure is valid and includes all required fields with appropriate test data.
pkg/config/server_config.go (3)
74-76: LGTM! Configuration structure is clean.The new Rulesets section follows existing patterns and is properly tagged for environment variable support.
93-95: LGTM! Validation enforced at initialization.Calling ValidateRulesetsFolder during config loading provides fail-fast behavior, preventing the server from starting with invalid configuration.
120-139: LGTM! Path validation is thorough.The validation correctly checks for empty path, existence, and directory type with clear error messages.
pkg/service/cryptography_service.go (3)
27-27: LGTM! Appropriate import for binary responses.The httpbody import enables returning tarball data in gRPC/REST responses.
44-44: LGTM! Handler integration follows existing patterns.The new handler is initialized and wired consistently with other handlers in the service.
Also applies to: 58-58
154-157: LGTM! Clean delegation to handler.The method appropriately delegates to the handler without adding unnecessary logic at the service layer.
pkg/handlers/ruleset_download_handler.go (2)
41-62: LGTM! Handler structure and initialization are clean.The constructor properly initializes both the config and use-case dependencies.
87-101: LGTM! Request validation and delegation are appropriate.The handler validates input, delegates to the use-case, and handles errors with proper status codes and logging.
pkg/usecase/ruleset_download.go (7)
33-58: LGTM! Type definitions and constructor are well-structured.The metadata structure aligns with the JSON format, and the constructor properly initializes the use-case with config.
60-80: LGTM! Path containment check prevents traversal attacks.The verification uses absolute paths and correctly checks for the separator suffix to prevent matching similar prefixes like
/base/dirvs/base/dir-malicious.
82-128: LGTM! Symlink resolution is secure.The method properly:
- Verifies the ruleset base path is contained
- Handles both relative and absolute symlink targets
- Validates the resolved symlink target stays within storage
- Returns clear errors for invalid scenarios
This addresses the critical path traversal concern from past reviews.
130-152: LGTM! Metadata reading with proper error handling.The method reads and parses the manifest.json with appropriate logging and error wrapping.
154-178: LGTM! Tarball reading with size logging.The method properly stats the file before reading and logs the size for observability.
180-202: LGTM! Checksum validation with case-insensitive comparison.The validation:
- Checks for empty checksum
- Computes SHA-256 hash
- Uses case-insensitive comparison with
strings.ToLower- Provides clear error messages
This addresses the major security concern from past reviews.
204-233: LGTM! Orchestration method properly sequences operations.The method correctly:
- Checks for context cancellation
- Resolves version (including "latest")
- Reads metadata
- Reads tarball
- Validates checksum
- Returns combined output
All steps have appropriate error handling and the checksum validation prevents serving corrupted/tampered files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/handlers/ruleset_download_handler_test.go (1)
285-384: LGTM! The error message patterns now match the handler implementation.The test expectations have been corrected and now properly expect "internal server error" instead of "internal error", resolving the previous pipeline failure.
Note: This addresses the past review comment about error message mismatches.
🧹 Nitpick comments (2)
pkg/config/server_config_test.go (1)
126-191: Consider simplifying the error validation logic for clarity.The test coverage is comprehensive, but line 139 has confusing validation logic that could be clearer.
🔎 Optional refactor for improved clarity
Current logic at line 139:
if !os.IsNotExist(err) && err.Error() != "rulesets storage path does not exist: /path/that/does/not/exist" {This is logically correct but could be more readable. Consider:
- if !os.IsNotExist(err) && err.Error() != "rulesets storage path does not exist: /path/that/does/not/exist" { - t.Errorf("unexpected error message: %v", err) - } + expectedMsg := "rulesets storage path does not exist: /path/that/does/not/exist" + if err.Error() != expectedMsg { + t.Errorf("expected error message '%s', got '%s'", expectedMsg, err.Error()) + }This focuses on the specific error message validation, similar to the pattern used in the "FileInsteadOfDirectory" subtest at lines 186-189.
pkg/handlers/ruleset_download_handler_test.go (1)
172-283: Consider adding a success case test.The test suite comprehensively covers error scenarios but lacks a test case for successful download. The
validateResultfunction parameter (line 199) is defined but never used.💡 Suggested success test case
Add a test case that validates successful download:
{ name: "successful download with specific version", request: &pb.RulesetDownloadRequest{ RulesetName: "dca", Version: "v1.0.0", }, expectError: false, validateResult: func(t *testing.T, contentType string, dataLen int) { if contentType != "application/gzip" { t.Errorf("Expected content type 'application/gzip', got '%s'", contentType) } if dataLen == 0 { t.Error("Expected non-zero data length") } }, }, { name: "successful download with latest version", request: &pb.RulesetDownloadRequest{ RulesetName: "dca", Version: "latest", }, expectError: false, validateResult: func(t *testing.T, contentType string, dataLen int) { if contentType != "application/gzip" { t.Errorf("Expected content type 'application/gzip', got '%s'", contentType) } if dataLen == 0 { t.Error("Expected non-zero data length") } }, },This would exercise the complete download flow and validate the response structure.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/config/server_config_test.gopkg/handlers/algorithm_handler_test.gopkg/handlers/algorithm_in_range_handler_test.gopkg/handlers/encryption_hints_handler_test.gopkg/handlers/hints_in_range_handler_test.gopkg/handlers/ruleset_download_handler_test.gopkg/handlers/utils_handler_test.gopkg/handlers/versions_in_range_handler_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/config/server_config_test.go (1)
pkg/config/server_config.go (1)
NewServerConfig(80-97)
pkg/handlers/ruleset_download_handler_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(33-77)pkg/handlers/ruleset_download_handler.go (1)
NewRulesetDownloadHandler(57-62)
🔇 Additional comments (12)
pkg/handlers/hints_in_range_handler_test.go (1)
33-33: LGTM! Consistent test environment setup.The addition of
defer setupTestRulesetsDir(t)()to all four test functions ensures proper test isolation and aligns with the test setup pattern used across the handler test suite. The defer placement is correct—cleanup will execute after other deferred operations, ensuring the rulesets directory and environment variable are cleaned up last.Also applies to: 62-62, 183-183, 333-333
pkg/handlers/algorithm_in_range_handler_test.go (1)
33-33: LGTM! Consistent test setup across all test functions.The addition of
defer setupTestRulesetsDir(t)()ensures proper test environment isolation by creating a temporary rulesets directory and cleaning it up after each test.Also applies to: 62-62, 183-183, 322-322
pkg/handlers/utils_handler_test.go (1)
27-43: LGTM! Well-designed test helper.The
setupTestRulesetsDirhelper provides clean test environment isolation with proper cleanup. The error handling ensures that temporary directories are cleaned up even if environment variable setup fails.pkg/handlers/versions_in_range_handler_test.go (1)
33-33: LGTM! Consistent test environment setup.The deferred
setupTestRulesetsDir(t)()calls properly initialize and clean up the test environment for each test function.Also applies to: 62-62, 183-183, 322-322
pkg/handlers/encryption_hints_handler_test.go (1)
33-33: LGTM! Test environment properly configured.The consistent use of
setupTestRulesetsDir(t)()across all test functions ensures proper environment isolation.Also applies to: 62-62, 183-183, 333-333
pkg/handlers/algorithm_handler_test.go (1)
33-33: LGTM! Test setup properly implemented.The deferred
setupTestRulesetsDir(t)()calls ensure consistent test environment across all test functions.Also applies to: 62-62, 183-183, 322-322
pkg/config/server_config_test.go (3)
29-56: LGTM! Proper test setup for rulesets storage path.The test correctly creates a temporary directory and validates that the
RULESETS_STORAGE_PATHenvironment variable is properly loaded into the configuration.
64-92: LGTM! DotEnv configuration test properly validates rulesets path.The test ensures that the rulesets storage path is correctly configured when using DotEnv feeder.
95-124: LGTM! JSON configuration test properly validates rulesets path.The test ensures that the rulesets storage path is correctly configured when using JSON feeder.
pkg/handlers/ruleset_download_handler_test.go (3)
34-54: LGTM! Proper handler initialization test.The test validates that the handler and its dependencies are correctly initialized.
56-170: LGTM! Comprehensive request validation coverage.The test covers all validation scenarios including nil requests, empty/whitespace fields, invalid semver, and valid formats with and without "v" prefix.
386-393: LGTM! Appropriate test helper.The
customErrortype is a simple and effective helper for testing error handling scenarios.
[SP-3798] feat: add unit tests, update changelog [SP-3798] feat: update papi version, add path validation, add checksum verification [SP-3798] fix: lint errors [SP-3798] fix: lint errors, remove tar.gz rules form .gitignore [SP-3798] chore: add ruleset description and created-at into response headers [SP-3798] chore: add metrics [SP-3798] chore: update changelog [SP-3798] chore: final adjustments [SP-3798] chore: return less verbose errors, validate rulesets directory exists on startup [SP-3798] fix: unit tests [SP-3798] fix: unit tests
matiasdaloia
force-pushed
the
feature/mdaloia/SP-3798-SCANOSS-Crypto-Framework-Download-rules-API
branch
from
dd254c9 to
8349344
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/handlers/ruleset_download_handler.go (1)
104-115: Validate metadata fields to prevent header injection.Metadata fields from the manifest.json are inserted directly into HTTP headers without sanitization. If the manifest contains newlines, carriage returns, or control characters, it could enable header injection attacks. While the manifest is typically under operator control, sanitizing header values is a defensive best practice.
🔎 Suggested sanitization helper
// sanitizeHeaderValue removes control characters that could enable header injection func sanitizeHeaderValue(value string) string { return strings.Map(func(r rune) rune { if r == '\n' || r == '\r' || r < 32 || r == 127 { return -1 // remove control characters } return r }, value) }Apply it to metadata fields:
headers := []string{ "content-type", "application/gzip", "content-disposition", fmt.Sprintf("attachment; filename=\"%s\"", filename), - "scanoss-ruleset-name", ruleset.Metadata.Name, - "scanoss-ruleset-version", ruleset.Metadata.Version, - "scanoss-ruleset-created-at", ruleset.Metadata.CreatedAt, - "x-checksum-sha256", ruleset.Metadata.ChecksumSHA256, + "scanoss-ruleset-name", sanitizeHeaderValue(ruleset.Metadata.Name), + "scanoss-ruleset-version", sanitizeHeaderValue(ruleset.Metadata.Version), + "scanoss-ruleset-created-at", sanitizeHeaderValue(ruleset.Metadata.CreatedAt), + "x-checksum-sha256", sanitizeHeaderValue(ruleset.Metadata.ChecksumSHA256), }
🧹 Nitpick comments (2)
pkg/handlers/ruleset_download_handler_test.go (2)
172-283: Consider adding a positive test case for successful download.The
TestRulesetDownloadHandler_DownloadRulesettests cover error scenarios (invalid request, not found) but lacks a positive test case that validates a successful download flow. ThevalidateResultcallback is defined but never used since there's no success case.🔎 Suggested addition for a success test case
{ name: "version not found", request: &pb.RulesetDownloadRequest{ RulesetName: "dca", Version: "v99.99.99", }, expectError: true, expectedCode: codes.NotFound, errorContains: "not found", }, + { + name: "successful download with specific version", + request: &pb.RulesetDownloadRequest{ + RulesetName: "dca", + Version: "v1.0.0", + }, + expectError: false, + validateResult: func(t *testing.T, contentType string, dataLen int) { + if contentType != "application/gzip" { + t.Errorf("Expected content-type 'application/gzip', got '%s'", contentType) + } + if dataLen == 0 { + t.Error("Expected non-empty data") + } + }, + }, }
179-184: Path resolution relies on relative directory structure.Using
filepath.Abs("../../")to find the project root is fragile and will break if the test file is moved to a different directory depth. Consider using a more robust approach.🔎 Alternative approach using environment variable or runtime detection
// Option 1: Use an environment variable testStoragePath := os.Getenv("TEST_RULESETS_PATH") if testStoragePath == "" { // Fallback to relative path projectRoot, err := filepath.Abs("../../") if err != nil { t.Fatalf("failed to get project root: %v", err) } testStoragePath = filepath.Join(projectRoot, "test-support", "rulesets") } // Option 2: Use runtime.Caller to get the actual file location _, filename, _, ok := runtime.Caller(0) if !ok { t.Fatal("failed to get current file path") } projectRoot := filepath.Dir(filepath.Dir(filepath.Dir(filename))) testStoragePath := filepath.Join(projectRoot, "test-support", "rulesets")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumtest-support/rulesets/dca/v1.0.0/dca-v1.0.0.tar.gzis excluded by!**/*.gztest-support/rulesets/dca/v1.0.1/dca-v1.0.1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (31)
.gitignoreCHANGELOG.mdconfig/app-config-dev.jsongo.modpkg/config/server_config.gopkg/config/server_config_test.gopkg/handlers/algorithm_handler_test.gopkg/handlers/algorithm_in_range_handler_test.gopkg/handlers/cryptography_support.gopkg/handlers/encryption_hints_handler_test.gopkg/handlers/hints_in_range_handler_test.gopkg/handlers/ruleset_download_handler.gopkg/handlers/ruleset_download_handler_test.gopkg/handlers/utils_handler.gopkg/handlers/versions_in_range_handler_test.gopkg/models/all_urls_test.gopkg/models/common_test.gopkg/models/crypto_usage_test.gopkg/models/library_usage_test.gopkg/service/cryptography_service.gopkg/service/cryptography_service_test.gopkg/testutils/test_helpers.gopkg/usecase/cryptography_major_test.gopkg/usecase/cryptography_search_test.gopkg/usecase/cryptography_versions_using_test.gopkg/usecase/library_detections_test.gopkg/usecase/ruleset_download.gopkg/usecase/ruleset_download_test.gotest-support/rulesets/dca/latesttest-support/rulesets/dca/v1.0.0/manifest.jsontest-support/rulesets/dca/v1.0.1/manifest.json
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/handlers/algorithm_handler_test.go
- test-support/rulesets/dca/latest
- pkg/handlers/hints_in_range_handler_test.go
- test-support/rulesets/dca/v1.0.0/manifest.json
- pkg/handlers/versions_in_range_handler_test.go
- .gitignore
- pkg/usecase/ruleset_download_test.go
- pkg/config/server_config_test.go
- pkg/handlers/cryptography_support.go
- config/app-config-dev.json
🧰 Additional context used
🧬 Code graph analysis (12)
pkg/usecase/library_detections_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/models/all_urls_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/service/cryptography_service_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/usecase/cryptography_search_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/usecase/cryptography_major_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/models/library_usage_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/usecase/cryptography_versions_using_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/handlers/ruleset_download_handler_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(33-77)pkg/handlers/ruleset_download_handler.go (1)
NewRulesetDownloadHandler(57-62)
pkg/service/cryptography_service.go (2)
pkg/handlers/ruleset_download_handler.go (2)
RulesetDownloadHandler(42-45)NewRulesetDownloadHandler(57-62)pkg/handlers/cryptography_support.go (1)
SetupMetrics(48-53)
pkg/handlers/encryption_hints_handler_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
pkg/handlers/ruleset_download_handler.go (3)
pkg/usecase/ruleset_download.go (2)
RulesetDownloadUseCase(49-51)NewRulesetDownload(54-58)pkg/config/server_config.go (1)
ServerConfig(33-77)pkg/httphelper/http_helper.go (1)
SetHTTPCodeOnTrailer(33-38)
pkg/models/crypto_usage_test.go (1)
pkg/testutils/test_helpers.go (1)
SetupTestRulesetsDir(33-49)
🔇 Additional comments (24)
CHANGELOG.md (1)
7-17: LGTM!The new version entry is well-formatted and accurately documents the DownloadRuleset feature additions.
pkg/usecase/cryptography_search_test.go (1)
31-31: LGTM!The test setup pattern correctly initializes the rulesets directory before config validation and ensures proper cleanup after test completion.
Also applies to: 35-36
pkg/usecase/cryptography_versions_using_test.go (1)
32-32: LGTM!The test setup pattern is consistently applied to both test functions, ensuring proper rulesets directory initialization and cleanup.
Also applies to: 36-37, 167-168
pkg/usecase/cryptography_major_test.go (1)
31-31: LGTM!The test setup correctly prepares the rulesets directory environment for the test execution.
Also applies to: 35-36
pkg/models/crypto_usage_test.go (1)
27-27: LGTM!The test setup pattern is correctly implemented, ensuring the rulesets directory is available during test execution.
Also applies to: 31-32
pkg/models/library_usage_test.go (1)
26-26: LGTM!The test setup pattern is correctly applied, ensuring proper initialization and cleanup of the test rulesets directory.
Also applies to: 30-31
pkg/usecase/library_detections_test.go (1)
31-31: Consistent test setup for rulesets directory.The addition of
defer testutils.SetupTestRulesetsDir(t)()across all test functions ensures proper test isolation by providing each test with a clean temporary rulesets directory. The pattern is applied consistently and the setup is correctly placed before other test initialization steps.Also applies to: 35-35, 109-109, 192-192
pkg/models/all_urls_test.go (1)
28-28: Test infrastructure setup applied consistently.The test setup pattern is correctly applied across all test functions in this file, matching the approach used throughout the codebase. This ensures consistent test environments and proper cleanup.
Also applies to: 32-32, 96-96, 136-136, 183-183, 215-215, 246-246, 278-278
pkg/testutils/test_helpers.go (1)
24-49: Well-designed test helper following Go best practices.The
SetupTestRulesetsDirfunction provides clean test isolation with proper error handling and cleanup. The pattern of returning a cleanup function for use withdeferis idiomatic Go, and the implementation correctly handles both setup operations (create temp dir, set env var) and their corresponding cleanup steps.pkg/service/cryptography_service_test.go (1)
36-36: Comprehensive test isolation applied throughout.The test setup infrastructure has been consistently applied across all 16 test functions in this service layer, ensuring proper test isolation and environment consistency.
Also applies to: 40-40, 101-101, 212-212, 268-268, 323-323, 379-379, 430-430, 547-547, 640-640, 757-757, 848-848, 924-924, 1017-1017, 1130-1130, 1252-1252, 1365-1365
pkg/handlers/encryption_hints_handler_test.go (1)
30-30: Handler tests properly configured with test infrastructure.Test setup is correctly applied across all handler test functions, maintaining consistency with the broader test infrastructure pattern.
Also applies to: 34-34, 63-63, 184-184, 334-334
pkg/handlers/algorithm_in_range_handler_test.go (1)
30-30: Test setup consistently applied.The test infrastructure setup follows the same pattern established across all other test files, ensuring uniform test environments.
Also applies to: 34-34, 63-63, 184-184, 323-323
test-support/rulesets/dca/v1.0.1/manifest.json (1)
1-7: Valid test manifest for ruleset v1.0.1.The manifest structure is well-formed and includes appropriate metadata fields (name, version, description, timestamp, checksum) for testing ruleset download functionality.
pkg/config/server_config.go (2)
74-76: Rulesets storage configuration added.The addition of
Rulesets.StoragePathwith environment variable binding provides flexible configuration for ruleset storage. The default path follows standard Linux filesystem conventions.Also applies to: 117-117
93-95: Fail-fast validation ensures correct configuration at startup.The
ValidateRulesetsFoldermethod enforces that the rulesets directory exists and is properly configured before the server starts. This fail-fast approach prevents runtime failures and provides clear error messages for operators. The validation checks are comprehensive (non-empty, exists, is directory) and the error messages are actionable.Also applies to: 120-139
pkg/service/cryptography_service.go (2)
44-59: LGTM!The integration follows the established handler delegation pattern used by other handlers in this service. The
rulesetDownloadHandleris correctly initialized with only the config (no database dependency) since rulesets are served from the filesystem.
153-157: LGTM!The
DownloadRulesetmethod correctly delegates to the handler and follows the same pattern as other service methods.pkg/handlers/utils_handler.go (1)
43-72: LGTM!The telemetry helper functions follow the established pattern and correctly gate on
config.Telemetry.Enabled. The comment issue from the previous review has been addressed.pkg/handlers/ruleset_download_handler.go (2)
138-158: Path traversal protection is handled at the use-case layer.The previous review suggested adding path traversal validation here. While not implemented in the handler, I see that
verifyPathContainmentin the use-case layer (lines 62-80 ofruleset_download.go) now provides this protection. This is an acceptable approach, though adding basic validation here would provide defense-in-depth.
87-135: LGTM!The
DownloadRulesetmethod is well-structured with proper validation, error handling, telemetry integration, and response building. The use ofruleset.Metadata.Versionin the filename (line 103) correctly addresses the previous review comment about using the resolved version.pkg/usecase/ruleset_download.go (4)
60-80: LGTM!The
verifyPathContainmentfunction properly prevents path traversal attacks by:
- Resolving both paths to absolute paths
- Checking that the resolved path starts with the base directory followed by a separator (preventing
/base/dirfrom matching/base/dir-malicious)This addresses the previous security concern about symlink resolution.
82-128: LGTM!The
resolveVersionfunction properly handles both specific versions and the "latest" symlink case. The containment verification (lines 88-90 and 116-119) ensures symlinks cannot escape the storage directory.
180-202: LGTM!The checksum validation:
- Rejects empty checksums (line 184-186)
- Computes SHA256 and compares case-insensitively (line 195)
- Provides clear error messages
This addresses the previous concerns about checksum validation and case sensitivity.
204-233: LGTM!The
DownloadRulesetorchestrator function properly:
- Checks for context cancellation early
- Resolves version with path safety
- Reads and validates metadata
- Reads and validates tarball integrity via checksum
Clean implementation with good separation of concerns.
| github.com/scanoss/go-grpc-helper v0.9.0 | ||
| github.com/scanoss/go-purl-helper v0.2.1 | ||
| github.com/scanoss/papi v0.25.1 | ||
| github.com/scanoss/papi v0.27.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n go.mod | head -50Repository: scanoss/cryptography
Length of output: 2465
🌐 Web query:
github.com/scanoss/papi v0.27.0 security vulnerabilities
💡 Result:
I can look that up — do you mean the GitHub repo github.com/scanoss/papi (the SCANOSS project) at tag/version v0.27.0, or a different package named “papi” (there are many projects named papi)? Please confirm which exact repository/package and I'll search for known security vulnerabilities for that version.
🌐 Web query:
google.golang.org/genproto security vulnerabilities CVE
💡 Result:
I found no CVE entries that name google.golang.org/genproto itself. However there are public CVEs in closely-related Go protobuf packages that commonly appear in genproto dependency graphs:
- CVE-2023-24535 / GO-2023-1631 — parsing invalid text-format protobuf messages can cause a panic (DoS). Affects google.golang.org/protobuf (fixed in v1.29.1). [1][2]
- CVE-2024-24786 — infinite loop in protojson.Unmarshal for certain invalid JSON, affecting google.golang.org/protobuf < 1.33.0 (fix in v1.33.0). [3][4]
If you use google.golang.org/genproto, check your dependency tree for google.golang.org/protobuf (and google.golang.org/grpc) versions and upgrade those transitive deps to the fixed releases. For details and patches see the cited advisories.
Sources: [1] go.dev vuln GO-2023-1631 (CVE-2023-24535). [2] Snyk / advisories on CVE-2023-24535. [3] Snyk/Seal/Resolved listings for CVE-2024-24786. [4] additional vulnerability pages summarizing CVE-2024-24786.
🏁 Script executed:
# Check if these packages exist in Go registry by examining their module info
# We can use a simple approach to verify package existence
echo "=== Checking papi v0.27.0 availability ==="
curl -s -I "https://pkg.go.dev/github.com/scanoss/papi@v0.27.0" | head -5
echo -e "\n=== Checking genproto version ==="
curl -s -I "https://pkg.go.dev/google.golang.org/genproto@v0.0.0-20250707201910-8d1bb00bc6a7" | head -5Repository: scanoss/cryptography
Length of output: 1676
🏁 Script executed:
cat -n go.mod | tail -50Repository: scanoss/cryptography
Length of output: 2854
🏁 Script executed:
# Check for protobuf and grpc versions in go.mod
rg "google.golang.org/(protobuf|grpc)" go.modRepository: scanoss/cryptography
Length of output: 240
🌐 Web query:
SCANOSS papi github releases v0.27.0
💡 Result:
I couldn't find a GitHub release or tag named "v0.27.0" for scanoss/papi. The scanoss GitHub organization and the papi repository pages show the project but no v0.27.0 release; libraries.io lists papi at around v0.17.0. [1][2]
Sources:
- SCANOSS GitHub (organization / repos). [1]
- Libraries.io page for github.com/scanoss/papi (shows v0.17.0). [2]
🏁 Script executed:
# Check if we can find more details about the dependencies
# Let's verify the genproto package requirements more closely
echo "=== Genproto version analysis ==="
echo "Pseudo-version: v0.0.0-20250707201910-8d1bb00bc6a7"
echo "This is a development snapshot, not a release version"
echo "Format: v0.0.0-YYYYMMDDHHMMSS-githash"
echo ""
echo "Note: The date 2025-07-07 is in the future (current: Dec 2025)"
echo "This suggests the version is from July 7, 2025"Repository: scanoss/cryptography
Length of output: 351
🏁 Script executed:
# Check GitHub for SCANOSS papi package information
# Search for papi releases and any security information
curl -s "https://api.github.com/repos/scanoss/papi/releases/tags/v0.27.0" | head -20Repository: scanoss/cryptography
Length of output: 1213
The genproto dependency uses a pseudo-version instead of a stable release.
papi v0.27.0 exists and is a valid published release with no known security vulnerabilities. However, google.golang.org/genproto/googleapis/api is pinned to a pseudo-version (v0.0.0-20250707201910-8d1bb00bc6a7) rather than a stable release. While this specific commit snapshot appears to be from July 2025 and the underlying transitive dependencies (protobuf v1.36.6, grpc v1.75.0) include patches for known CVEs, using a pseudo-version for a direct dependency is unusual and should be replaced with a proper release version to improve reproducibility and maintainability.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.