fix bug atmos vendor pull URI cannot contain path traversal sequences and git schema #899
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to Atmos' vendoring functionality, focusing on improving URI handling, validation, and path processing across multiple files. The changes primarily address issues with local file paths, Git URLs, and path traversal support, implementing more robust mechanisms for parsing and validating source URIs in vendor configurations. Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
By Sparta's spear, we've fortified the vendoring process! These changes shall make our URI handling as precise as a warrior's strike. Onward! 🏹 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Please add test conditions or at least examples to examples/tests so we verify this works and don't introduce future regressions |
osterman
changed the title
the tests for many vendoring scenarios, including imports, are here https://github.com/cloudposse/atmos/blob/main/examples/tests/vendor.yaml |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/demo-vendoring/vendor.yaml (1)
12-12: Consider standardizing URL format across all componentsWhile the new format with
git::https://is more secure, we should maintain consistency. Theweatherandipinfocomponents still use the old format.Consider updating all components to use the new format:
- component: "weather" - source: "github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}" + source: "git::https://github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}" - component: "ipinfo" - source: "github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}" + source: "git::https://github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}"Also applies to: 23-24, 31-32
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/demo-vendoring/vendor.yaml(1 hunks)
🔇 Additional comments (1)
examples/demo-vendoring/vendor.yaml (1)
12-12: APPROVED: Enhanced security with explicit Git protocol and HTTPS! 🛡️
The addition of git:: prefix and explicit HTTPS protocol improves security and clarity. Let's verify this new format works as expected.
✅ Verification successful
Let me check the validation code to understand how the URI is validated.
Let me check the actual implementation of the vendor utility functions.
Let me check the vendoring documentation for any additional context.
The new Git URL format is valid and secure! 🛡️
Based on the code analysis, the git::https:// format is fully supported by the validation logic in validateURI() function. The function specifically checks for valid schemes including git, and the URL format matches the expected pattern. The change improves security by making the protocol explicit while maintaining compatibility with the vendoring system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new Git URL format in other vendor configurations
# Look for any existing usage of the new format to confirm it's supported
# Search for similar Git URL patterns in vendor configs
echo "Searching for existing Git URL patterns in vendor configs..."
rg -g '*.yaml' -g '*.yml' 'git::https://'
# Look for any validation-related code that might affect this URL format
echo "Checking for URL validation code..."
rg -t go 'validateURI|validateURL|validateGitURL'
Length of output: 581
Script:
#!/bin/bash
# Check the URI validation implementation
echo "Checking URI validation implementation..."
ast-grep --pattern 'func validateURI($_) {
$$$
}'
# Look for any tests related to URI validation
echo "Checking for URI validation tests..."
rg -t go 'TestValidateURI|TestVendorURI'
# Check for any vendor-related test files
echo "Looking for vendor test files..."
fd -e go -e yaml test.*vendor
Length of output: 381
Script:
#!/bin/bash
# Get the content of vendor_utils.go
echo "Checking vendor utils implementation..."
cat internal/exec/vendor_utils.go
# Look for any vendor-related configuration or documentation
echo "Checking for vendor documentation..."
fd -e md -e txt vendor
Length of output: 21139
Script:
#!/bin/bash
# Check vendoring documentation
echo "Checking vendoring documentation..."
cat website/docs/glossary/vendoring.md
# Look for any test files with vendor examples
echo "Looking for vendor test examples..."
fd -e yaml -e yml test.*vendor
Length of output: 454
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/tests/vendor.yaml (1)
Line range hint
16-24: Well-documented source configuration provides clear guidance 📘The comprehensive documentation of supported protocols and URL formats serves as both a test case and implementation guide. This is particularly valuable for validating the URI validation changes.
Consider adding an explicit example of what path traversal sequences are not allowed in the comments to help prevent future security issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/tests/vendor.yaml(1 hunks)
🔇 Additional comments (1)
examples/tests/vendor.yaml (1)
Line range hint 1-7: Excellent test coverage! This file serves multiple test scenarios 🎯
The comment block at the top documents various test commands, making this file an excellent test fixture for the URI changes. It covers:
- Basic vendor pull
- Component-specific pulls
- Tag-based filtering
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
515-523: Enhance error handling for URL parsingThe URL parsing error is silently ignored, which could mask potential issues. Consider handling the error explicitly or logging it for debugging purposes.
- u, err := url.Parse(*uri) - if err == nil && u.Scheme != "" { + if parsedURL, err := url.Parse(*uri); err == nil && parsedURL.Scheme != "" { + if parsedURL.Scheme == "file" { + trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/") + *uri = filepath.Clean(trimmedPath) + useLocalFileSystem = true + } + } else if err != nil { + u.LogTrace(atmosConfig, fmt.Sprintf("URL parsing warning: %v", err)) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/vendor_model.go(2 hunks)internal/exec/vendor_utils.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model.go
🔇 Additional comments (4)
internal/exec/vendor_utils.go (4)
510-515: LGTM! Path handling improvements
The changes to handle absolute paths and join them with the vendor config path look good. This addresses the previous review comments about supporting relative paths.
593-594: LGTM! Consistent path handling
The use of filepath.ToSlash ensures consistent path separators across different platforms.
Line range hint 646-664: Verify security implications of relaxed URI validation
The simplified URI validation removes checks for path traversal sequences (..). While this allows for relative paths as requested, we should ensure that path traversal is properly handled during the actual file operations to prevent unauthorized access to system files.
Let's verify the path handling in the codebase:
✅ Verification successful
Based on the results, I can see extensive use of filepath.Clean, filepath.Abs, and proper path handling throughout the codebase. Let me verify the specific file operations:
Path traversal is properly handled, warrior! The codebase is secure.
The codebase demonstrates robust path handling practices:
- All file paths are cleaned using
filepath.Cleanbefore operations - File names are sanitized using
sanitizeFileNamefunction - Absolute paths are resolved using
filepath.Abs - File operations are performed after proper path validation
The removal of path traversal checks from URI validation is safe because:
- The codebase uses
filepath.Joinfor path construction - File operations are performed after sanitization
- Target paths are properly constructed and validated before operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for path traversal handling in file operations
echo "Checking for path normalization and validation..."
rg -A 3 "filepath\.(Clean|Abs|Join|EvalSymlinks)"
# Check for potential file access outside intended directories
echo "Checking file access patterns..."
ast-grep --pattern 'os.(Open|Create|ReadFile|WriteFile|Remove|RemoveAll)($path)'
Length of output: 35394
Script:
#!/bin/bash
# Check for file operations in vendor_utils.go and related files
echo "Checking file operations in vendor-related code..."
rg -A 3 "os\.(Open|Create|ReadFile|WriteFile|Remove|RemoveAll)" internal/exec/vendor_utils.go internal/exec/vendor_component_utils.go
# Check for path sanitization in vendor-related code
echo "Checking path sanitization..."
rg -A 3 "sanitize(File|Path|Base)" internal/exec/vendor_utils.go internal/exec/vendor_component_utils.go internal/exec/vendor_model.go
Length of output: 2093
510-523: Add test cases for relative path vendoring
Following up on @osterman's previous comments, we should add test cases for relative path vendoring, specifically including the ../../demo-library/weather scenario.
Let's check the existing test coverage:
haitham911
force-pushed
the
issues-888
branch
from
d89c75d to
063f955
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/tests/test-vendor/test-components/README.md (1)
13-13: Enhance output documentation and fix punctuation.Add a period at the end of the line for consistency. Consider adding:
- Example of the JSON structure returned in the metadata
- Description of key fields in the response
-- `metadata`: The data retrieved from IPinfo for the specified IP address, in JSON format. ++ `metadata`: The data retrieved from IPinfo for the specified IP address, in JSON format. Example: ++ ```json ++ { ++ "ip": "8.8.8.8", ++ "hostname": "dns.google", ++ "city": "Mountain View", ++ "region": "California", ++ "country": "US", ++ "loc": "37.4056,-122.0775", ++ "org": "AS15169 Google LLC", ++ "postal": "94043", ++ "timezone": "America/Los_Angeles" ++ } ++ ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ...empty string. ### Outputs -metadata: The data retrieved from IPinfo for the ...(UNLIKELY_OPENING_PUNCTUATION)
examples/tests/test-vendor/atmos.yaml (2)
19-28: Fix trailing spaces in the vendor section.Remove trailing spaces from lines 19, 22, and 25 to maintain consistent formatting.
-vendor: +vendor: # Single file -base_path: "./vendor.yaml" +base_path: "./vendor.yaml" # Directory with multiple files -#base_path: "./vendor" +#base_path: "./vendor"🧰 Tools
🪛 yamllint (1.35.1)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
36-40: Fix YAML indentation.The indentation is inconsistent. The
commandslist items should be properly indented.commands: -- name: "test" - description: "Run all tests" - steps: - - atmos vendor pull --everything + - name: "test" + description: "Run all tests" + steps: + - atmos vendor pull --everything🧰 Tools
🪛 yamllint (1.35.1)
[warning] 37-37: wrong indentation: expected 2 but found 0
(indentation)
[warning] 40-40: wrong indentation: expected 4 but found 2
(indentation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
examples/tests/test-vendor/atmos.yaml(1 hunks)examples/tests/test-vendor/test-components/README.md(1 hunks)examples/tests/test-vendor/test-components/main.tf(1 hunks)examples/tests/test-vendor/test-components/outputs.tf(1 hunks)examples/tests/test-vendor/test-components/providers.tf(1 hunks)examples/tests/test-vendor/test-components/variables.tf(1 hunks)examples/tests/test-vendor/test-components/versions.tf(1 hunks)examples/tests/test-vendor/vendor.yaml(1 hunks)internal/exec/vendor_component_utils.go(2 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_utils.go(5 hunks)tests/cli_test.go(5 hunks)tests/fixtures/scenarios/complete/vendor.yaml(1 hunks)tests/test-cases/demo-stacks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/tests/test-vendor/test-components/variables.tf
- examples/tests/test-vendor/test-components/outputs.tf
- examples/tests/test-vendor/test-components/providers.tf
- examples/tests/test-vendor/test-components/versions.tf
- examples/tests/test-vendor/test-components/main.tf
- internal/exec/vendor_model.go
- tests/cli_test.go
- examples/tests/test-vendor/vendor.yaml
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/vendor_utils.go
[failure] 655-655:
CustomGitHubDetector redeclared in this block
[failure] 660-660:
method CustomGitHubDetector.Detect already declared at internal/exec/go_getter_utils.go:76:32
[failure] 726-726:
RegisterCustomDetectors redeclared in this block
🪛 GitHub Check: Build (windows-latest, windows)
internal/exec/vendor_utils.go
[failure] 655-655:
CustomGitHubDetector redeclared in this block
[failure] 660-660:
method CustomGitHubDetector.Detect already declared at internal\exec\go_getter_utils.go:76:32
[failure] 726-726:
RegisterCustomDetectors redeclared in this block
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/vendor_utils.go
[failure] 655-655:
CustomGitHubDetector redeclared in this block
[failure] 660-660:
method CustomGitHubDetector.Detect already declared at internal/exec/go_getter_utils.go:76:32
[failure] 726-726:
RegisterCustomDetectors redeclared in this block
🪛 GitHub Actions: Tests
internal/exec/vendor_utils.go
[error] 655-655: CustomGitHubDetector redeclared in this block
🪛 yamllint (1.35.1)
examples/tests/test-vendor/atmos.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[warning] 37-37: wrong indentation: expected 2 but found 0
(indentation)
[warning] 40-40: wrong indentation: expected 4 but found 2
(indentation)
🪛 LanguageTool
examples/tests/test-vendor/test-components/README.md
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ...empty string. ### Outputs - metadata: The data retrieved from IPinfo for the ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
examples/tests/test-vendor/atmos.yaml (1)
36-40: Consider using explicit component selection instead of --everything flag.The
--everythingflag in the test command will pull all defined components. Instead of using--everything, consider one of these more explicit approaches:
- Use
--tags demoto pull all demo components- List specific components:
atmos vendor pull -c github/stargazers -c weather- Create a separate test command for each component group you need to test
This provides better control over which components are pulled during testing and makes the dependencies more explicit.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 37-37: wrong indentation: expected 2 but found 0
(indentation)
[warning] 40-40: wrong indentation: expected 4 but found 2
(indentation)
tests/fixtures/scenarios/complete/vendor.yaml (1)
48-48: LGTM! Improved source URL format.The source URL format has been updated to use the explicit
git::httpsprefix, which aligns with the enhanced URI validation and handling.tests/test-cases/demo-stacks.yaml (1)
134-191: LGTM! Comprehensive test coverage added.The new test cases effectively validate:
- Custom command functionality with and without arguments
- Vendor pull command execution and file existence verification
This addresses the previous request for proper testing of the vendor functionality.
internal/exec/vendor_component_utils.go (2)
252-259: LGTM! Enhanced URI handling for file:// schema.The addition of file:// schema support improves the flexibility of the vendoring system while maintaining proper path handling.
Line range hint
571-572: LGTM! Improved cross-platform path handling.The use of
filepath.ToSlashensures consistent path handling across different operating systems.internal/exec/vendor_utils.go (1)
626-640: LGTM! Robust URI validation.The new
validateURIfunction provides comprehensive validation including:
- Empty URI check
- Space character check
- OCI URI format validation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
388-388: Adjust path handling for cross-platform compatibility.Using
filepath.ToSlashon path components beforefilepath.Joinmay cause incorrect path separators on Windows systems. It's advisable to join the paths first and then normalize if necessary.Apply this diff to modify the path handling:
-targetPath := filepath.Join(filepath.ToSlash(vendorConfigFilePath), filepath.ToSlash(target)) +targetPath := filepath.Join(vendorConfigFilePath, target) +targetPath = filepath.ToSlash(targetPath)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/vendor_utils.go
625-625: func validateURI is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
internal/exec/go_getter_utils.go (2)
Line range hint
89-117: Review security implications of debug logging.The debug logging could potentially expose sensitive information in log files. Consider:
- Redacting tokens and credentials from URLs in debug logs
- Limiting the token source information in logs
- u.LogDebug(d.AtmosConfig, fmt.Sprintf("Injecting token from %s for %s\n", tokenSource, src)) + u.LogDebug(d.AtmosConfig, "Injecting authentication token\n")
Consider adding download size limits for enhanced security
The codebase properly implements path cleaning for local filesystem operations, but could benefit from explicit size limits for downloads. While go-getter handles basic security, consider adding:
- Maximum file size configuration in
AtmosConfiguration- Size validation before processing downloaded content
🔗 Analysis chain
Line range hint
156-187: Verify security measures for file downloads.While the scheme support is well documented, please verify:
- File scheme path validation for directory traversal
- Download size limits to prevent resource exhaustion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential directory traversal validation in the codebase rg -l "path\.Clean|filepath\.Clean" . # Look for download size limit implementations rg -l "MaxSize|size.*limit|limit.*size" .Length of output: 251
Script:
#!/bin/bash # Check path cleaning implementation in the relevant files rg -A 5 "filepath\.Clean|path\.Clean" ./internal/exec/vendor_utils.go ./internal/exec/vendor_component_utils.go # Check go-getter configuration and usage patterns ast-grep --pattern 'getter.Client{$$$}'Length of output: 2240
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
Line range hint
22-54: Consider enhancing URI validation for better security.The validation is solid, but could be strengthened further:
- Consider using
path.Cleanfor more robust path traversal prevention- Add validation for encoded path traversal sequences (
%2e%2e)- Document the rationale for the 2048 character limit
func ValidateURI(uri string) error { if uri == "" { return fmt.Errorf("URI cannot be empty") } + // 2048 is a common limit for URIs in many systems and browsers if len(uri) > 2048 { return fmt.Errorf("URI exceeds maximum length of 2048 characters") } - if strings.Contains(uri, "..") { + // Check for both raw and encoded path traversal sequences + if strings.Contains(uri, "..") || strings.Contains(strings.ToLower(uri), "%2e%2e") { return fmt.Errorf("URI cannot contain path traversal sequences") } + // Clean the path to catch more traversal attempts + if cleaned := path.Clean(uri); strings.Contains(cleaned, "..") { + return fmt.Errorf("URI contains invalid path traversal after cleaning") + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/go_getter_utils.go(1 hunks)tests/test-cases/demo-stacks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test-cases/demo-stacks.yaml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [lint] quick-start-advanced
- GitHub Check: [lint] demo-context
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/go_getter_utils.go (1)
64-64: LGTM! Clean addition of file scheme support.The addition of the "file" scheme aligns perfectly with the PR objectives and maintains the existing validation pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/vendor_utils.go (2)
390-390: Consider using filepath.Clean before joining paths.While
filepath.ToSlashhelps with cross-platform compatibility, addingfilepath.Cleanwould provide additional protection against path traversal attempts.- targetPath := filepath.Join(filepath.ToSlash(vendorConfigFilePath), filepath.ToSlash(target)) + targetPath := filepath.Join(filepath.ToSlash(filepath.Clean(vendorConfigFilePath)), filepath.ToSlash(filepath.Clean(target)))
572-573: Consider using path/filepath for consistent path handling.While
filepath.ToSlashworks here, consider usingpath/filepathpackage's functions consistently throughout the path handling logic for better maintainability.- tempDir = filepath.ToSlash(tempDir) - src = filepath.ToSlash(src) + tempDir = filepath.Clean(filepath.ToSlash(tempDir)) + src = filepath.Clean(filepath.ToSlash(src))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/vendor_utils.go (2)
367-372: Good addition of URI validation!The conditional validation ensures that only non-local URIs are validated, which is a secure approach to handling different types of sources.
525-533: Verify file URI handling across platforms.The file URI handling looks good, but we should verify it works correctly on Windows paths. Let's check if the code handles Windows-style paths properly.
✅ Verification successful
File URI handling is properly implemented for cross-platform compatibility 💪
The implementation is robust and platform-agnostic due to:
- Consistent use of
filepath.ToSlashfor path separator normalization- Proper path cleaning with
filepath.Clean- Correct handling of the
file://scheme🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Windows-style paths in the codebase rg -l "file://[A-Za-z]:\\\\" . # Check for any file URI tests rg -l "file://" "**/*_test.go"Length of output: 114
Script:
#!/bin/bash # Search for test files properly fd "_test\.go$" -x rg -l "file://" {} # Look for file URI handling patterns rg -g "*.go" -l "url\.Parse.*file://" # Check for filepath usage patterns in the same file rg -g "vendor_utils.go" "filepath\.(Clean|ToSlash)" -A 2 -B 2Length of output: 1927
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
367-375: LGTM! Good security check for URI validation.The conditional validation and specific handling of path traversal sequences enhance security. Consider making the error message more specific about URI format requirements.
- return fmt.Errorf("Invalid URI '%s': %w. Please ensure the source is a valid local path or a properly formatted URI.", uri, err) + return fmt.Errorf("Invalid URI '%s': %w. URI must be in format scheme://host/path (e.g., git://github.com/org/repo or https://domain.com/path)", uri, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/vendor_utils.go (2)
393-393: LGTM! Robust cross-platform path handling.The use of
filepath.ToSlashensures consistent path separators across different operating systems.
575-576: LGTM! Consistent path normalization.The path normalization maintains consistency with the rest of the codebase and ensures reliable path comparisons across platforms.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/fixtures/scenarios/vendor/test-components/outputs.tf (1)
1-4: Consider adding error handling for the JSON decode operation.The jsondecode function could fail if the response is not valid JSON. Consider wrapping this in a try/can_fail block to handle potential errors gracefully.
output "metadata" { description = "The data retrieved from IPinfo for the specified IP address" - value = jsondecode(data.http.ipinfo.response_body) + value = can(jsondecode(data.http.ipinfo.response_body)) ? jsondecode(data.http.ipinfo.response_body) : null }tests/fixtures/scenarios/vendor/test-components/main.tf (1)
1-7: Strengthen the HTTP data source configuration, brave developer!Consider these battle-tested improvements:
- Add timeout settings to prevent hanging requests
- Consider rate limiting to avoid API throttling
- URL encode the IP address parameter for safety
data "http" "ipinfo" { - url = var.ip_address != "" ? "https://ipinfo.io/${var.ip_address}" : "https://ipinfo.io" + url = var.ip_address != "" ? "https://ipinfo.io/${urlencode(var.ip_address)}" : "https://ipinfo.io" request_headers = { Accept = "application/json" } + retry { + attempts = 2 + max_delay_seconds = 10 + } }tests/fixtures/scenarios/vendor/test-components/README.md (1)
1-13: Enhance the documentation with examples and response format, noble scribe!The documentation is clear but could be strengthened with:
- Add a complete example usage block
- Document the expected response format/schema
- Include rate limiting considerations
Here's a suggested addition:
### Example Usage ```hcl module "ipinfo" { source = "path/to/module" ip_address = "8.8.8.8" # Optional: Google DNS IP } output "ip_details" { value = module.ipinfo.metadata }Response Format
The
metadataoutput contains the following fields:
ip: The IP addresshostname: The hostname (if available)city: The city locationregion: The region/statecountry: The country codeloc: Latitude and longitudeorg: Organization namepostal: Postal codetimezone: TimezoneRate Limiting
IPinfo API has rate limits. Consider implementing appropriate delays between requests.
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~13-~13: Loose punctuation mark. Context: ...empty string. ### Outputs - `metadata`: The data retrieved from IPinfo for the ... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> </blockquote></details> <details> <summary>tests/fixtures/scenarios/vendor/atmos.yaml (1)</summary><blockquote> `19-28`: **Consider documenting the vendor configuration options.** The vendor configuration section provides multiple base path options but only one is active. Adding comments explaining when to use each option would improve maintainability. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 yamllint (1.35.1)</summary> [error] 19-19: trailing spaces (trailing-spaces) --- [error] 22-22: trailing spaces (trailing-spaces) --- [error] 25-25: trailing spaces (trailing-spaces) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2f3690fdc8fe398ccb311a0d4553d738e1209ef7 and 5b36f548efdd8fafd691b8a6a8a7e81721bbe35e. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `internal/exec/vendor_utils.go` (5 hunks) * `tests/fixtures/scenarios/vendor/atmos.yaml` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/README.md` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/main.tf` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/outputs.tf` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/providers.tf` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/variables.tf` (1 hunks) * `tests/fixtures/scenarios/vendor/test-components/versions.tf` (1 hunks) * `tests/fixtures/scenarios/vendor/vendor.yaml` (1 hunks) * `tests/test-cases/demo-stacks.yaml` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * tests/fixtures/scenarios/vendor/test-components/providers.tf * tests/fixtures/scenarios/vendor/test-components/versions.tf </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 yamllint (1.35.1)</summary> <details> <summary>tests/fixtures/scenarios/vendor/atmos.yaml</summary> [error] 19-19: trailing spaces (trailing-spaces) --- [error] 22-22: trailing spaces (trailing-spaces) --- [error] 25-25: trailing spaces (trailing-spaces) --- [warning] 37-37: wrong indentation: expected 2 but found 0 (indentation) --- [warning] 40-40: wrong indentation: expected 4 but found 2 (indentation) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>tests/fixtures/scenarios/vendor/test-components/README.md</summary> [uncategorized] ~13-~13: Loose punctuation mark. Context: ...empty string. ### Outputs - `metadata`: The data retrieved from IPinfo for the ... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (2)</summary> * GitHub Check: Build (windows-latest, windows) * GitHub Check: Summary </details> <details> <summary>🔇 Additional comments (7)</summary><blockquote> <details> <summary>tests/fixtures/scenarios/vendor/test-components/variables.tf (1)</summary> `1-5`: **Well-structured variable definition, warrior!** The variable definition follows Terraform best practices with clear description and appropriate default value. </details> <details> <summary>tests/fixtures/scenarios/vendor/atmos.yaml (1)</summary> `36-40`: **Verify the vendor pull command's behavior.** The `test` command runs `vendor pull --everything` which could potentially overwrite local changes. Consider adding a dry-run flag or warning message. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 yamllint (1.35.1)</summary> [warning] 37-37: wrong indentation: expected 2 but found 0 (indentation) --- [warning] 40-40: wrong indentation: expected 4 but found 2 (indentation) </details> </details> </details> <details> <summary>tests/fixtures/scenarios/vendor/vendor.yaml (2)</summary> `44-53`: **LGTM! Test case for path traversal.** The `vpc-src` component correctly tests path traversal scenarios with `../../demo-library/weather`. This aligns with the PR's objective to fix path traversal handling. --- `24-41`: **LGTM! Comprehensive URI scheme coverage.** The configuration tests multiple URI schemes: - Local file system (`file:///./test-components`) - Git HTTPS (`git::https://github.com/...`) - OCI registry (`oci://public.ecr.aws/...`) This provides good test coverage for the URI handling improvements. </details> <details> <summary>tests/test-cases/demo-stacks.yaml (1)</summary> `162-196`: **LGTM! Comprehensive vendor pull test coverage.** The test case thoroughly verifies the vendor pull functionality by: 1. Checking existence of all vendored files 2. Testing across different component types (github, weather, vpc) 3. Validating the correct file structure This addresses the previous review request for proper testing. </details> <details> <summary>internal/exec/vendor_utils.go (2)</summary> `367-375`: **LGTM! Secure URI validation implementation.** The validation logic correctly: 1. Checks for path traversal sequences 2. Provides clear error messages 3. Only validates non-local filesystem URIs This addresses the security concerns around path traversal. --- `523-539`: **LGTM! Robust URL parsing and schema handling.** The implementation: 1. Properly normalizes paths using `filepath.ToSlash` 2. Safely handles the `file://` schema 3. Correctly processes local file paths This ensures consistent path handling across different URI types. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/vendor_utils.go (2)
370-378: Consider enhancing URI validation error messages.While the validation logic is solid, the error message could be more specific about URI format requirements. Consider providing examples of valid URIs in the error message.
- return fmt.Errorf("Invalid URI '%s': %w. Please ensure the source is a valid local path or a properly formatted URI.", uri, err) + return fmt.Errorf("Invalid URI '%s': %w. Please ensure the source is either a valid local path (e.g., './path/to/file') or a properly formatted URI (e.g., 'git::https://github.com/org/repo.git', 'file:///path/to/file').", uri, err)
Line range hint
516-551: Consider restructuring for better error handling and readability.While the function logic is correct, it could benefit from early returns and clearer error handling for URL parsing.
func determineSourceType(uri *string, vendorConfigFilePath string) (bool, bool, bool, error) { useOciScheme := strings.HasPrefix(*uri, "oci://") if useOciScheme { *uri = strings.TrimPrefix(*uri, "oci://") - return useOciScheme, false, false, nil + return useOciScheme, false, false, nil } useLocalFileSystem := false sourceIsLocalFile := false - if !useOciScheme { - absPath, err := u.JoinAbsolutePathWithPath(filepath.ToSlash(vendorConfigFilePath), *uri) - if err != nil && strings.Contains(*uri, "..") { - return useOciScheme, useLocalFileSystem, sourceIsLocalFile, fmt.Errorf("invalid source path '%s': %w", *uri, err) - } - if err == nil { - uri = &absPath - useLocalFileSystem = true - sourceIsLocalFile = u.FileExists(*uri) - } - parsedURL, err := url.Parse(*uri) - if err != nil { - return useOciScheme, useLocalFileSystem, sourceIsLocalFile, err - } - if err == nil && parsedURL.Scheme != "" { - if parsedURL.Scheme == "file" { - trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/") - *uri = filepath.Clean(trimmedPath) - useLocalFileSystem = true - } - } + // Try to resolve as local path first + absPath, err := u.JoinAbsolutePathWithPath(filepath.ToSlash(vendorConfigFilePath), *uri) + if err != nil && strings.Contains(*uri, "..") { + return useOciScheme, useLocalFileSystem, sourceIsLocalFile, fmt.Errorf("invalid source path '%s': %w", *uri, err) + } + if err == nil { + uri = &absPath + useLocalFileSystem = true + sourceIsLocalFile = u.FileExists(*uri) + return useOciScheme, useLocalFileSystem, sourceIsLocalFile, nil } - return useOciScheme, useLocalFileSystem, sourceIsLocalFile, nil + // Try to parse as URL + parsedURL, err := url.Parse(*uri) + if err != nil { + return useOciScheme, useLocalFileSystem, sourceIsLocalFile, err + } + + if parsedURL.Scheme == "file" { + trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/") + *uri = filepath.Clean(trimmedPath) + useLocalFileSystem = true + } + + return useOciScheme, useLocalFileSystem, sourceIsLocalFile, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/vendor_utils.go(6 hunks)tests/fixtures/scenarios/vendor/vendor.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fixtures/scenarios/vendor/vendor.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/vendor_utils.go (3)
5-5: LGTM! Good addition of net/url package.The import of
net/urlpackage aligns well with the enhanced URI parsing and validation requirements.
396-396: LGTM! Good use of path normalization.The use of
filepath.ToSlashensures consistent path handling across different operating systems.
587-588: LGTM! Consistent path normalization.Good use of
filepath.ToSlashfor ensuring consistent path handling during copy operations.
|
These changes were released in v1.153.2. |
fix vendor bugs and add test for vendor
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
atmosCLI commands.