Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug atmos vendor pull URI cannot contain path traversal sequences and git schema #899

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+2 −20
Open

fix bug atmos vendor pull URI cannot contain path traversal sequences and git schema #899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5900ef3
allow URI contain path traversal
haitham911 Dec 28, 2024
71fff29
allow scheme: git::
haitham911 Dec 28, 2024
4e8c172
remove max length check url
haitham911 Dec 28, 2024
26fb6f6
remove character url validate
haitham911 Dec 28, 2024
dcd0086
fix log error
haitham911 Dec 28, 2024
ee879d9
add git schema to source on vendor yaml
haitham911 Dec 29, 2024
f10db51
modify source on vendor yaml
haitham911 Dec 29, 2024
4ebd799
use git on vendor test
haitham911 Dec 29, 2024
521ba3b
Merge branch 'main' into issues-888
osterman Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/tests/vendor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ spec:
- test
- networking
- component: "vpc-flow-logs-bucket"
source: "github.com/cloudposse/terraform-aws-components.git//modules/vpc-flow-logs-bucket?ref={{.Version}}"
source: "git::https://github.com/cloudposse/terraform-aws-components.git//modules/vpc-flow-logs-bucket?ref={{.Version}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Git URL format needs to be updated across example files! 🔄

The codebase shows inconsistent URL formats in example and test files. While some files use the secure git:: prefix, others don't. Based on the code analysis:

  1. The validation logic in determineSourceType function supports multiple source types (OCI, local, remote) but doesn't enforce the git:: prefix
  2. Several example files still use the old format without the git:: prefix:
    • examples/quick-start-advanced/vendor.yaml
    • examples/tests/vendor/*.yaml
    • examples/demo-vendoring/**/*.yaml
    • examples/demo-component-versions/vendor.yaml

These files should be updated to use the secure git:: prefix to maintain consistency with the security improvements and to serve as proper examples for users.

🔗 Analysis chain

Strong approval: Proper Git URL format enhances security! 💪

The addition of the git:: prefix is a robust improvement that explicitly declares the protocol, making the source URL more secure and resistant to path traversal attacks. This change aligns perfectly with the PR's objective.

Let's verify other Git URLs in the codebase follow this pattern:

Git URL format needs standardization across example files! 🛡️

Several example and test files are using potentially unsafe Git URLs without the git:: prefix. This inconsistency could lead to confusion and potential security risks.

Locations requiring attention:

  • examples/quick-start-advanced/vendor.yaml
  • examples/tests/vendor/*.yaml files
  • examples/demo-vendoring/**/*.yaml files
  • examples/demo-component-versions/vendor.yaml

Let's verify if this is a test requirement or if these files need to be updated to match the secure pattern:

Let's check the code to see if there's any validation logic that treats these URLs differently:

Let's check the vendor-related code specifically:

Let's check the specific validation logic for source URLs:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potentially unsafe Git URLs without the git:: prefix
rg -g '*.{yaml,yml}' 'source:.*github\.com.*git.*ref=' --no-line-number | grep -v 'git::'

Length of output: 1932

Script:

#!/bin/bash
# Check if these files are part of test cases for invalid formats
rg -g '*.{yaml,yml}' -B 2 -A 2 'source:.*github\.com.*git.*ref=' | grep -i -E 'test|invalid|bad|wrong|error'

Length of output: 1909

Script:

#!/bin/bash
# Look for URL validation logic in the codebase
ast-grep --pattern 'func $_(source string) $_ {
  $$$
}'

# Also check for any constants or documentation about URL formats
rg -g '*.go' -i 'git.*url|source.*url'

Length of output: 832

Script:

#!/bin/bash
# Look for vendor-related URL handling
rg -g '*.go' -A 5 'func.*[Vv]endor' 
rg -g '*.go' -A 5 'ParseVendorSource|ValidateVendorSource'

Length of output: 13623

Script:

#!/bin/bash
# Search for source URL validation in vendor-related files
rg -g 'vendor*.go' -B 3 -A 10 'source.*=|validateSource|parseSource|Source.*string'

# Also check for any URL-related constants or validation patterns
rg -g '*.go' 'const.*source.*=|var.*source.*=' -A 2

Length of output: 20412

version: "1.323.0"
targets:
- "components/terraform/infra/vpc-flow-logs-bucket/{{.Version}}"
Expand Down
2 changes: 1 addition & 1 deletion internal/exec/vendor_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func (m *modelVendor) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
}
version := grayColor.Render(version)
return m, tea.Sequence(
tea.Printf("%s %s %s", mark, pkg.name, version),
tea.Printf("%s %s %s %s", mark, pkg.name, version, errMsg),
tea.Quit,
)
}
Expand Down
18 changes: 0 additions & 18 deletions internal/exec/vendor_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,32 +637,14 @@ func validateURI(uri string) error {
if uri == "" {
return fmt.Errorf("URI cannot be empty")
}
// Maximum length check
if len(uri) > 2048 {
return fmt.Errorf("URI exceeds maximum length of 2048 characters")
}
// Add more validation as needed
// Validate URI format
if strings.Contains(uri, "..") {
return fmt.Errorf("URI cannot contain path traversal sequences")
}
Comment on lines -646 to -648
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for vendoring from:

../../demo-library/weather

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add it to example/tests

if strings.Contains(uri, " ") {
return fmt.Errorf("URI cannot contain spaces")
}
// Validate characters
if strings.ContainsAny(uri, "<>|&;$") {
return fmt.Errorf("URI contains invalid characters")
}
// Validate scheme-specific format
if strings.HasPrefix(uri, "oci://") {
if !strings.Contains(uri[6:], "/") {
return fmt.Errorf("invalid OCI URI format")
}
} else if strings.Contains(uri, "://") {
scheme := strings.Split(uri, "://")[0]
if !isValidScheme(scheme) {
return fmt.Errorf("unsupported URI scheme: %s", scheme)
}
}
return nil
}
Expand Down
Loading