feat: add download rulesets handler #49
Conversation
|
Warning Rate limit exceeded@matiasdaloia has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a ruleset download feature: new gRPC/REST DownloadRuleset endpoint and handler, a use-case to resolve versions/read metadata/verify checksums/read tarballs, server wiring and config for ruleset storage, telemetry metrics and helpers, tests, and test-support fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server as CryptographyServer
participant Handler as RulesetDownloadHandler
participant UseCase as RulesetDownloadUseCase
participant FS as FileSystem
Client->>Server: DownloadRuleset(name, version)
Server->>Handler: delegate request
Handler->>Handler: validateRequest()
alt invalid request
Handler-->>Client: InvalidArgument (gRPC/HTTP)
else valid
Handler->>UseCase: DownloadRuleset(ctx, name, version)
UseCase->>FS: Resolve version path (handle "latest" symlink/file)
alt not found
UseCase-->>Handler: NotFound error
Handler-->>Client: NotFound (gRPC/HTTP)
else found
UseCase->>FS: Read manifest.json -> Metadata
UseCase->>FS: Read tarball `<name>-<version>.tar.gz` -> bytes
UseCase->>UseCase: verify SHA-256 checksum
alt checksum mismatch
UseCase-->>Handler: checksum error
Handler-->>Client: Internal (gRPC/HTTP)
else ok
UseCase-->>Handler: RulesetDownloadOutput
Handler->>Client: HttpBody (tarball) + metadata headers/trailers
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
[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
matiasdaloia
force-pushed
the
feature/mdaloia/SP-3798-SCANOSS-Crypto-Framework-Download-rules-API
branch
from
f704a36 to
63d08d0
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.