fix bug atmos vendor pull URI cannot contain path traversal sequences and git schema #899
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to URI validation and error handling in the Atmos vendoring system. The changes primarily focus on relaxing URI validation constraints in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 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 (1)
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
| if strings.Contains(uri, "..") { | ||
| return fmt.Errorf("URI cannot contain path traversal sequences") | ||
| } |
There was a problem hiding this comment.
Please add a test for vendoring from:
../../demo-library/weather
There was a problem hiding this comment.
@osterman @aknysh
To test the vendor functionality, I reviewed the process and observed that it does not break even if there are errors. However, simply running the vendor pull command is not sufficient as a test.
Instead, there should be specific test cases that validate the process, including counting the vendor files to ensure accuracy. For example, we should test the vendor process on Windows to confirm it does not fail or break. Currently, the process only logs the outcome without actually vendoring any files, which is not reliable for testing purposes
There was a problem hiding this comment.
Ok, can you add some proper tests for this then?
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:
osterman
added
patch
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
examples/tests/test-vendor/atmos.yaml (1)
19-28: Consider using absolute paths for vendor configuration.While relative paths work, using absolute paths for vendor configurations is generally more secure and explicit. This prevents potential path traversal issues when the configuration is loaded from different working directories.
🧰 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)
tests/test_cases.yaml (1)
171-205: Consider adding negative test cases for path traversal preventionWhile the test case verifies successful component vendoring, it would be beneficial to add negative test cases that verify the path traversal fix. This ensures the security fix is working as intended.
Consider adding test cases that:
- Attempt to use URIs with path traversal sequences (
../,./, etc.)- Verify that such attempts are properly rejected
- Confirm appropriate error messages are displayed
Would you like me to help draft these additional test cases?
🧰 Tools
🪛 yamllint (1.35.1)
[error] 171-171: trailing spaces
(trailing-spaces)
[warning] 181-181: wrong indentation: expected 8 but found 10
(indentation)
[warning] 182-182: wrong indentation: expected 8 but found 10
(indentation)
[warning] 183-183: wrong indentation: expected 8 but found 10
(indentation)
[warning] 184-184: wrong indentation: expected 8 but found 10
(indentation)
[warning] 185-185: wrong indentation: expected 8 but found 10
(indentation)
[warning] 187-187: wrong indentation: expected 8 but found 10
(indentation)
[warning] 188-188: wrong indentation: expected 8 but found 10
(indentation)
[warning] 189-189: wrong indentation: expected 8 but found 10
(indentation)
[warning] 190-190: wrong indentation: expected 8 but found 10
(indentation)
[warning] 191-191: wrong indentation: expected 8 but found 10
(indentation)
[warning] 193-193: wrong indentation: expected 8 but found 10
(indentation)
[warning] 194-194: wrong indentation: expected 8 but found 10
(indentation)
[warning] 195-195: wrong indentation: expected 8 but found 10
(indentation)
[warning] 196-196: wrong indentation: expected 8 but found 10
(indentation)
[warning] 197-197: wrong indentation: expected 8 but found 10
(indentation)
[warning] 199-199: wrong indentation: expected 8 but found 10
(indentation)
[warning] 200-200: wrong indentation: expected 8 but found 10
(indentation)
[warning] 201-201: wrong indentation: expected 8 but found 10
(indentation)
[warning] 202-202: wrong indentation: expected 8 but found 10
(indentation)
[warning] 203-203: wrong indentation: expected 8 but found 10
(indentation)
examples/tests/test-vendor/test-components/README.md (1)
1-13: Enhance documentation with additional sectionsThe README provides good basic information but could be more comprehensive.
Consider adding these sections:
- Requirements (Terraform version, provider versions)
- Example usage block showing the module in action
- Notes about API rate limits and any authentication requirements
- Link to IPinfo API documentation
Would you like me to help draft these additional sections?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ...empty string. ### Outputs -metadata: The data retrieved from IPinfo for the ...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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)tests/cli_test.go(5 hunks)tests/test_cases.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/tests/test-vendor/test-components/providers.tf
- examples/tests/test-vendor/test-components/versions.tf
🧰 Additional context used
🪛 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)
tests/test_cases.yaml
[error] 171-171: trailing spaces
(trailing-spaces)
[warning] 181-181: wrong indentation: expected 8 but found 10
(indentation)
[warning] 182-182: wrong indentation: expected 8 but found 10
(indentation)
[warning] 183-183: wrong indentation: expected 8 but found 10
(indentation)
[warning] 184-184: wrong indentation: expected 8 but found 10
(indentation)
[warning] 185-185: wrong indentation: expected 8 but found 10
(indentation)
[warning] 187-187: wrong indentation: expected 8 but found 10
(indentation)
[warning] 188-188: wrong indentation: expected 8 but found 10
(indentation)
[warning] 189-189: wrong indentation: expected 8 but found 10
(indentation)
[warning] 190-190: wrong indentation: expected 8 but found 10
(indentation)
[warning] 191-191: wrong indentation: expected 8 but found 10
(indentation)
[warning] 193-193: wrong indentation: expected 8 but found 10
(indentation)
[warning] 194-194: wrong indentation: expected 8 but found 10
(indentation)
[warning] 195-195: wrong indentation: expected 8 but found 10
(indentation)
[warning] 196-196: wrong indentation: expected 8 but found 10
(indentation)
[warning] 197-197: wrong indentation: expected 8 but found 10
(indentation)
[warning] 199-199: wrong indentation: expected 8 but found 10
(indentation)
[warning] 200-200: wrong indentation: expected 8 but found 10
(indentation)
[warning] 201-201: wrong indentation: expected 8 but found 10
(indentation)
[warning] 202-202: wrong indentation: expected 8 but found 10
(indentation)
[warning] 203-203: wrong indentation: expected 8 but found 10
(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)
🔇 Additional comments (12)
tests/cli_test.go (5)
7-8: Good migration fromioutiltoio.
This change aligns with modern Go practices.
43-49: Properly handling file opening and reading.
Shifting fromReadFileto explicit opens withdefer f.Close()is elegant and safer for error diagnostics.
179-179: Enhanced visibility into command failures.
Including stderr in test logs provides valuable debugging info.
225-232: Better file existence checking with absolute paths.
This approach avoids ambiguity if relative paths shift.
242-248: Reading file contents viaio.ReadAll.
Consistency in file operations is beneficial and clarifies errors.internal/exec/vendor_component_utils.go (2)
8-8: Added dependency onnet/url.
This is necessary for robust URI parsing.
248-255: Support for file scheme URLs.
Discarding the leading slash and marking the flag is a nice refinement for local files.examples/tests/test-vendor/test-components/variables.tf (1)
1-5: New optional IP address variable.
This variable provides flexible input for external queries. Great addition.examples/tests/test-vendor/test-components/outputs.tf (1)
1-4: Added metadata output.
The JSON decode step is a convenient approach for leveraging external data.examples/tests/test-vendor/vendor.yaml (2)
32-38: LGTM: Proper version pinning in Git URL.The Git URL correctly uses
?ref={{.Version}}to pin the version, which is a good security practice to prevent supply chain attacks.
24-26:⚠️ Potential issueValidate local file paths for potential security risks.
The
file:///scheme could be vulnerable to path traversal attacks. Ensure that the path is properly validated and normalized before use.✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the context better and check for any potential security measures around file paths.
Let's examine how the codebase handles file:/// URIs and component sources to understand the security measures in place.
Let's look at how the config sources are processed to understand the security measures for file:/// URIs.
Let's examine how components and their sources are validated to understand the security measures in place.
Let's examine how URIs are validated and if there are any path sanitization measures in place.
File paths are properly validated and sanitized
After examining the codebase, I can confirm that the
file:///scheme in the vendor configuration is properly handled with multiple security measures:
- The URI is validated through
validateURIfunction which checks for malformed inputs- File paths are sanitized using
sanitizeFileNamewhich handles unsafe characters- Local file paths are processed through
determineSourceTypewhich properly normalizes paths- The implementation uses
filepath.Cleanandfilepath.ToSlashfor path normalization- File operations are restricted to the configured base paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the file path contains any suspicious patterns rg -l "file:///(\.\.|\~)" .Length of output: 29
Script:
#!/bin/bash # Let's check for any file:/// URIs in the codebase to analyze their usage rg -p "file:///" . # Also check for path validation or sanitization logic rg -p -g '*.{go,rs,js,ts,py}' "(sanitize|validate|normalize).*path" .Length of output: 1952
Script:
#!/bin/bash # Let's check the implementation of component source handling ast-grep --pattern 'func $$$($$$) { $$$ file:///$$$ $$$ }' # Also check for any URI/path processing logic rg -p "func.*[Pp]rocess.*(?:URI|Source|Path)" .Length of output: 333
Script:
#!/bin/bash # Let's examine the config sources processing implementation cat ./internal/exec/config_sources_utils.go # And check for any component source validation rg -p "func.*ValidateComponent" .Length of output: 17076
Script:
#!/bin/bash # Let's check the component validation implementation cat ./internal/exec/validate_component.go # And check for any URI handling or source validation rg -p "func.*[Vv]alidate.*(?:URI|Source|Path)" .Length of output: 7071
Script:
#!/bin/bash # Let's check the URI validation implementation cat ./internal/exec/vendor_utils.go # And check for any file path handling utilities rg -p "func.*(?:NormalizePath|CleanPath|SanitizePath)" .Length of output: 21392
examples/tests/test-vendor/test-components/README.md (1)
Line range hint
171-205: Verify test coverage for URI validation changesThe PR aims to fix path traversal in URIs, but the current test only verifies successful cases. To ensure comprehensive coverage:
- Add test cases that explicitly verify URI validation
- Include examples from issue Support Path Traversals in Vendoring #888 in the test suite
Let's check the test coverage:
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ...empty string. ### Outputs -metadata: The data retrieved from IPinfo for the ...(UNLIKELY_OPENING_PUNCTUATION)
| commands: | ||
| - name: "test" | ||
| description: "Run all tests" | ||
| steps: | ||
| - atmos vendor pull --everything |
There was a problem hiding this comment.
💡 Codebase verification
Consider using explicit component selection instead of --everything flag
The --everything flag in the test command will pull all defined components:
- github/stargazers (demo, github tags)
- test-components (demo tag)
- weather (demo tag)
- my-vpc1 (no tags)
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.
🔗 Analysis chain
Review security implications of the --everything flag.
The --everything flag in the test command could potentially pull all vendor components without discrimination. Consider being more explicit about which components to test.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check what components would be pulled with --everything flag
atmos vendor pull --dry-run --everything
Length of output: 138
Script:
#!/bin/bash
# Let's check the vendor configuration and related files
fd vendor -t f
# Check for any documentation or configuration related to vendor components
rg -i "vendor" -g "*.{md,yaml,yml}" -A 3 -B 3
Length of output: 34075
Script:
#!/bin/bash
# Let's check the vendor.yaml file in the test-vendor directory to understand the configuration
cat examples/tests/test-vendor/vendor.yaml
# Let's also check if there are any specific components or tags defined
rg "component:|tags:" examples/tests/test-vendor/vendor.yaml -A 2
Length of output: 2092
🧰 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)
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
fix vendor bugs and add test for vendor
references
Summary by CodeRabbit
Security Changes
Improvements
Configuration Updates
Documentation
Tests
atmos vendor pullcommand