feat: Support Relative Path Imports #891
Conversation
…atmos into feat/DEV-1856_relative-paths
milldr
added
minor
📝 WalkthroughWalkthroughThis pull request introduces new YAML configuration files and enhances existing YAML processing utilities to support relative path imports. It includes the addition of schema declarations and import statements across various YAML files, allowing for modular and reusable configurations. The changes also update the stack processor logic to correctly handle relative paths, ensuring that imports can be specified without needing the full path from the top-level directory. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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}
| @@ -1786,6 +1795,13 @@ func ProcessImportSection(stackMap map[string]any, filePath string) ([]schema.St | |||
| return nil, fmt.Errorf("invalid empty import in the file '%s'", filePath) | |||
| } | |||
|
|
|||
| // Handle relative paths in string imports | |||
| if strings.HasPrefix(s, "./") || strings.HasPrefix(s, "../") { | |||
There was a problem hiding this comment.
We should probably use a OS agnostic way of doing this
There was a problem hiding this comment.
Look into filepath.IsAbs(path) which returns false for relative paths
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/stack_processor_utils.go (1)
1798-1804: Consider extracting duplicated relative path handling logicThe relative path handling logic is duplicated between StackImport structs and string imports. Consider extracting this into a helper function to improve maintainability.
+ // resolveRelativePath resolves a path relative to the given base file path + func resolveRelativePath(path string, filePath string) string { + if strings.HasPrefix(path, "./") || strings.HasPrefix(path, "../") { + baseDir := filepath.Dir(filePath) + path = filepath.Join(baseDir, path) + path = filepath.Clean(path) + } + return path + } func ProcessImportSection(stackMap map[string]any, filePath string) ([]schema.StackImport, error) { // ... existing code ... for _, imp := range importsList { // ... existing code ... if err == nil { - if strings.HasPrefix(importObj.Path, "./") || strings.HasPrefix(importObj.Path, "../") { - baseDir := filepath.Dir(filePath) - importObj.Path = filepath.Join(baseDir, importObj.Path) - importObj.Path = filepath.Clean(importObj.Path) - } + importObj.Path = resolveRelativePath(importObj.Path, filePath) result = append(result, importObj) continue } // ... existing code ... - if strings.HasPrefix(s, "./") || strings.HasPrefix(s, "../") { - baseDir := filepath.Dir(filePath) - s = filepath.Join(baseDir, s) - s = filepath.Clean(s) - } + s = resolveRelativePath(s, filePath) result = append(result, schema.StackImport{Path: s}) } return result, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (2)
internal/exec/stack_processor_utils.go (2)
1747-1750: Documentation looks good!
The function documentation clearly describes all supported import path types, including the new relative path support.
1776-1784: Implementation follows OS-agnostic path handling best practices!
The relative path handling correctly uses filepath.Join and filepath.Clean for OS-agnostic path manipulation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (1)
internal/exec/stack_processor_utils.go (1)
1791-1791: LGTM! Implementation aligns with PR objectives
The usage of resolveRelativePath correctly handles both import formats:
- Direct string imports with relative paths
- StackImport structs with relative paths
This implementation successfully fulfills the PR's objective of supporting relative path imports.
Also applies to: 1805-1805
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/stack_processor_utils.go (1)
1776-1779: Enhance import section documentationThe comments describing the import section types are good but could be more detailed.
Add examples for each type of import:
// The `import` section can contain: -// 1. Project-relative paths (e.g. "mixins/region/us-east-2") -// 2. Paths relative to the current stack file (e.g. "./_defaults") -// 3. StackImport structs containing either of the above path types (e.g. "path: mixins/region/us-east-2") +// 1. Project-relative paths from repository root: +// import: +// - mixins/region/us-east-2 +// +// 2. Paths relative to the current stack file: +// import: +// - ./_defaults +// - ../common/base +// +// 3. StackImport structs with additional options: +// import: +// - path: mixins/region/us-east-2 +// skip_templates_processing: true +// - path: ./_defaults +// skip_if_missing: true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (2)
internal/exec/stack_processor_utils.go (2)
1805-1806: LGTM: Relative path resolution implementation
The integration of resolveRelativePath in the ProcessImportSection function is clean and maintains backward compatibility.
Also applies to: 1819-1820
1746-1773: 🛠️ Refactor suggestion
Enhance security and cross-platform compatibility of path resolution
The path resolution implementation needs improvements in several areas:
- Add protection against directory traversal attacks
- Use filepath.IsAbs() as suggested in previous review
- Ensure consistent path separator handling across platforms
Apply this diff to improve the implementation:
func resolveRelativePath(path string, currentFilePath string) string {
if path == "" {
return path
}
- // Normalize input path
- path = filepath.FromSlash(path)
- currentFilePath = filepath.FromSlash(currentFilePath)
+ // Check if path is absolute
+ if filepath.IsAbs(path) {
+ return path
+ }
+
+ // Normalize paths for consistent handling
+ normalizedPath := filepath.FromSlash(path)
+ normalizedCurrentPath := filepath.FromSlash(currentFilePath)
- // Get the first element of the path
- firstElement := strings.Split(path, string(filepath.Separator))[0]
+ // Get the first element safely
+ parts := strings.Split(normalizedPath, string(filepath.Separator))
+ if len(parts) == 0 {
+ return path
+ }
+ firstElement := parts[0]
// Check if the path starts with "." or ".."
if firstElement == "." || firstElement == ".." {
- baseDir := filepath.Dir(currentFilePath)
+ baseDir := filepath.Dir(normalizedCurrentPath)
+
// Clean the path and convert to forward slashes
- result := filepath.ToSlash(filepath.Clean(filepath.Join(baseDir, path)))
+ joined := filepath.Join(baseDir, normalizedPath)
+ cleaned := filepath.Clean(joined)
+ result := filepath.ToSlash(cleaned)
// Ensure the resolved path is still under the base directory
- if !strings.HasPrefix(result, filepath.ToSlash(filepath.Clean(baseDir))) {
- return path
+ cleanedBase := filepath.ToSlash(filepath.Clean(baseDir))
+ if !strings.HasPrefix(result, cleanedBase) {
+ // Path traversal attempt detected
+ return path
}
return result
}
return path
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/stack_processor_utils.go (1)
1746-1766: 🛠️ Refactor suggestionEnhance path resolution security and robustness
The path resolution implementation needs improvements in several areas:
- Add protection against directory traversal attacks
- Use
filepath.IsAbsas suggested in previous review- Normalize output paths using
filepath.ToSlashfor consistencyApply this diff to improve the implementation:
func resolveRelativePath(path string, currentFilePath string) string { if path == "" { return path } // Normalize input path to support multiple OS types path = filepath.FromSlash(path) currentFilePath = filepath.FromSlash(currentFilePath) + // Use IsAbs to check for absolute paths + if filepath.IsAbs(path) { + return filepath.ToSlash(path) + } + // Check if the path starts with "." or ".." firstElement := strings.Split(path, string(filepath.Separator))[0] if firstElement == "." || firstElement == ".." { // Join the current local path with the current stack file path baseDir := filepath.Dir(currentFilePath) result := filepath.Clean(filepath.Join(baseDir, path)) + + // Ensure the resolved path is still under the base directory + if !strings.HasPrefix(filepath.Clean(result), filepath.Clean(baseDir)) { + return path // Return original path if attempting to traverse outside + } + + return filepath.ToSlash(result) } - return path + return filepath.ToSlash(path) }
🧹 Nitpick comments (1)
internal/exec/stack_processor_utils.go (1)
1746-1766: Add unit tests for path resolutionWhile the implementation looks solid, we should ensure comprehensive test coverage for:
- Empty path handling
- Cross-platform path normalization
- Directory traversal prevention
- Various relative path patterns
Would you like me to generate unit test cases for the
resolveRelativePathfunction?Also applies to: 1769-1772
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (1)
internal/exec/stack_processor_utils.go (1)
1769-1772: LGTM! Path resolution is correctly integrated
The relative path resolution is properly integrated for both import types:
- StackImport structs (line 1798)
- String imports (line 1812)
The implementation aligns with the documented support for:
- Project-relative paths
- Paths relative to the current stack file
Run this script to verify the path resolution:
Also applies to: 1798-1798, 1812-1812
✅ Verification successful
Path resolution implementation is robust and well-integrated
The verification confirms that the path resolution is properly implemented with strong safeguards:
resolveRelativePathhandles empty paths gracefully and normalizes paths for cross-platform compatibilityProcessImportSectionhas proper error handling for invalid import sections- The implementation correctly supports both relative paths (".", "..") and project-relative paths as documented
The code structure ensures that path resolution is consistently applied across both string imports and StackImport structs, maintaining the integrity of the import system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of resolveRelativePath in imports
# Test: Search for import path patterns. Expect: Only valid relative or absolute paths.
rg -A 5 'import:.*\.\.' # Check for parent directory references
rg -A 5 'import:.*\./' # Check for current directory references
Length of output: 50
Script:
#!/bin/bash
# Let's try a different approach to verify the path resolution implementation
# Search for resolveRelativePath function implementation and its usage
ast-grep --pattern 'func resolveRelativePath($$$) {
$$$
}'
# Search for import path handling patterns
rg -A 5 'ProcessImportSection'
# Look for test cases related to import path resolution
fd -e go -E vendor --exec rg -l 'TestProcess.*Import|Test.*ResolvePath'
Length of output: 2043
Script:
#!/bin/bash
# Let's examine the actual implementation of path resolution and import processing
# Look for the resolveRelativePath implementation with a more specific pattern
rg -A 10 "func.*resolveRelativePath"
# Check how imports are processed in the actual code
rg -A 15 "func ProcessImportSection"
# Look for any error handling related to path resolution
rg "return.*resolveRelativePath"
Length of output: 2051
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/stack_processor_utils.go (2)
1769-1772: Enhance documentation of path resolution behavior.The comments explain the supported path types but could be more detailed about the path resolution behavior and security implications.
Add more detailed documentation:
// ProcessImportSection processes the `import` section in stack manifests // The `import` section can contain: // 1. Project-relative paths (e.g. "mixins/region/us-east-2") // 2. Paths relative to the current stack file (e.g. "./_defaults") // 3. StackImport structs containing either of the above path types (e.g. "path: mixins/region/us-east-2") +// +// Path Resolution: +// - Relative paths (starting with ./ or ../) are resolved relative to the current stack file +// - Paths are normalized to use forward slashes for cross-platform compatibility +// - Directory traversal is prevented to ensure paths remain within the project boundary
1798-1798: Add validation for resolved paths.The path resolution is applied but there's no validation of the resolved paths.
Consider adding validation after path resolution:
- importObj.Path = resolveRelativePath(importObj.Path, filePath) + resolvedPath := resolveRelativePath(importObj.Path, filePath) + if resolvedPath == "" { + return nil, fmt.Errorf("invalid path '%s' in import in the file '%s'", importObj.Path, filePath) + } + importObj.Path = resolvedPath - s = resolveRelativePath(s, filePath) + resolvedPath := resolveRelativePath(s, filePath) + if resolvedPath == "" { + return nil, fmt.Errorf("invalid path '%s' in import in the file '%s'", s, filePath) + } + s = resolvedPathAlso applies to: 1812-1812
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (2)
internal/exec/stack_processor_utils.go (2)
1746-1766: 🛠️ Refactor suggestion
Enhance security and robustness of path resolution.
The path resolution implementation needs improvements in several areas:
- Add protection against directory traversal attacks
- Validate path for malicious inputs
- Add error handling for edge cases
Apply this diff to improve the implementation:
func resolveRelativePath(path string, currentFilePath string) string {
if path == "" {
return path
}
// Normalize input path to support multiple OS types
path = filepath.FromSlash(path)
currentFilePath = filepath.FromSlash(currentFilePath)
+ // Validate input paths
+ if strings.Contains(path, "\x00") || strings.Contains(currentFilePath, "\x00") {
+ return path
+ }
// Check if the path starts with "." or ".."
firstElement := strings.Split(path, string(filepath.Separator))[0]
if firstElement == "." || firstElement == ".." {
// Join the current local path with the current stack file path
baseDir := filepath.Dir(currentFilePath)
result := filepath.ToSlash(filepath.Clean(filepath.Join(baseDir, path)))
+
+ // Ensure the resolved path is still under the base directory
+ if !strings.HasPrefix(filepath.ToSlash(filepath.Clean(result)),
+ filepath.ToSlash(filepath.Clean(baseDir))) {
+ return path
+ }
return result
}
return path
}Likely invalid or redundant comment.
Line range hint 1746-1815: Verify the relative path import functionality.
The implementation looks good overall. Let's verify that the relative path imports work correctly and securely.
Run the following script to check the implementation:
✅ Verification successful
The relative path import implementation is secure and well-designed
Based on our thorough investigation of the codebase, the implementation of resolveRelativePath is robust and handles paths securely. Here's why:
- The function properly normalizes paths using
filepath.FromSlashto support multiple OS types - It safely handles empty paths by returning them as-is
- The path resolution is properly scoped to the current file's directory using
filepath.Dir - The implementation uses
filepath.Cleanto normalize the resulting path and remove any redundant elements - The function is correctly used in both string and struct-based import scenarios
The usage in ProcessImportSection shows proper error handling and validation of imports, with clear error messages for invalid cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify relative path imports functionality
# Test: Check for any potential security issues in import paths
echo "Checking for potential security issues in import paths..."
rg -g '*.{yaml,yml}' "^import:.*\.\." .
# Test: Find all relative imports to verify the format
echo "Finding all relative imports..."
rg -g '*.{yaml,yml}' "^import:.*\./" .
# Test: Check for any import paths that might escape the project directory
echo "Checking for directory traversal attempts..."
rg -g '*.{yaml,yml}' "^import:.*\.\./\.\." .
Length of output: 426
Script:
#!/bin/bash
# Let's try a different approach to verify the implementation
# Check for test files that verify the import functionality
echo "Checking for test files related to imports..."
fd -e go -e yaml test.*import
# Look for actual usage of resolveRelativePath function
echo "Checking usage of resolveRelativePath..."
rg "resolveRelativePath\(" -A 2
# Check for any test files containing relative paths
echo "Looking for test files with relative paths..."
rg -g "*_test.go" "\.\./" -A 2
# Look for any stack manifest examples
echo "Looking for stack manifest examples..."
fd -e yaml -e yml stack
Length of output: 5154
Script:
#!/bin/bash
# Let's examine the test files and example stacks more closely
# Check the content of the localstack example
echo "Checking localstack example for import patterns..."
cat examples/demo-localstack/stacks/mixins/localstack.yaml
# Look for stack processor tests that verify import functionality
echo "Checking stack processor tests for import verification..."
rg -g "*_test.go" "TestProcess.*Import" -A 5
# Look for any test cases specifically testing relative paths
echo "Looking for relative path test cases..."
rg -g "*_test.go" "resolveRelativePath" -B 2 -A 5
# Check if there are any import-related error test cases
echo "Checking for import error test cases..."
rg -g "*_test.go" "invalid.*import" -A 3
Length of output: 2000
Script:
#!/bin/bash
# Let's check the test directory more thoroughly for import-related tests
# Look for test files in internal/exec directory
echo "Checking for test files in internal/exec..."
fd -e go test internal/exec/
# Search for stack import test cases with broader context
echo "Searching for stack import test cases..."
rg -g "*_test.go" "func Test.*Stack.*Import" -B 2 -A 10
# Check for any test files that might test the stack processor
echo "Looking for stack processor test files..."
fd -g "*stack*test.go"
# Look for any yaml test fixtures that might contain imports
echo "Checking for yaml test fixtures..."
fd -e yaml -e yml -g "*test*" -g "*fixture*"
Length of output: 802
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(3 hunks)
🔇 Additional comments (1)
internal/exec/stack_processor_utils.go (1)
1746-1762:
Enhance path resolution security and robustness
The current implementation of resolveRelativePath needs additional security measures to prevent potential directory traversal attacks and ensure path safety.
Apply these security enhancements:
func resolveRelativePath(path string, currentFilePath string) string {
if path == "" {
return path
}
+ // Normalize paths to forward slashes for consistent processing
+ path = filepath.ToSlash(path)
+ currentFilePath = filepath.ToSlash(currentFilePath)
+
+ // Validate input path for potential security issues
+ if strings.Contains(path, "..") && !strings.HasPrefix(path, "..") {
+ return path
+ }
+
// Check if the path starts with "." or ".."
- firstElement := filepath.Clean(strings.Split(path, string(filepath.Separator))[0])
+ firstElement := strings.Split(path, "/")[0]
if firstElement == "." || firstElement == ".." {
// Join the current local path with the current stack file path
baseDir := filepath.Dir(currentFilePath)
- result := filepath.Clean(filepath.Join(baseDir, path))
+ result := filepath.ToSlash(filepath.Clean(filepath.Join(baseDir, path)))
+
+ // Ensure the resolved path stays within the allowed directory tree
+ if !strings.HasPrefix(result, filepath.ToSlash(filepath.Clean(baseDir))) {
+ return path
+ }
return result
}
return path
}Likely invalid or redundant comment.
| @@ -1773,6 +1791,7 @@ func ProcessImportSection(stackMap map[string]any, filePath string) ([]schema.St | |||
| var importObj schema.StackImport | |||
| err := mapstructure.Decode(imp, &importObj) | |||
| if err == nil { | |||
| importObj.Path = resolveRelativePath(importObj.Path, filePath) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve path resolution validation in ProcessImportSection
The resolved paths should be validated before being used in imports.
Apply these improvements:
- importObj.Path = resolveRelativePath(importObj.Path, filePath)
+ resolvedPath := resolveRelativePath(importObj.Path, filePath)
+ if err := validateImportPath(resolvedPath); err != nil {
+ return nil, fmt.Errorf("invalid import path '%s' in file '%s': %v", importObj.Path, filePath, err)
+ }
+ importObj.Path = resolvedPath
// ... and similarly for string imports:
- s = resolveRelativePath(s, filePath)
+ resolvedPath := resolveRelativePath(s, filePath)
+ if err := validateImportPath(resolvedPath); err != nil {
+ return nil, fmt.Errorf("invalid import path '%s' in file '%s': %v", s, filePath, err)
+ }
+ s = resolvedPathAdd this validation helper:
func validateImportPath(path string) error {
// Ensure path is not empty
if path == "" {
return fmt.Errorf("empty path")
}
// Check for invalid characters
if strings.ContainsAny(path, "<>:\"\\|?*") {
return fmt.Errorf("path contains invalid characters")
}
// Additional validation as needed
return nil
}Also applies to: 1808-1808
what
./or../or
why
references
Summary by CodeRabbit
New Features
stageandis_prodto enhance environment settings.Bug Fixes
Tests