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
+227 −8
Open

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
063f955
push reset to git reset --soft 15bad9a7f6409cf5abf3762643ae2cd65c026007
haitham911 Jan 20, 2025
201b521
fix build
haitham911 Jan 20, 2025
88d043e
fix test
haitham911 Jan 20, 2025
46edee4
fix test vendor pull
haitham911 Jan 21, 2025
8c6d2e0
remove validateURI
haitham911 Jan 21, 2025
bced3b5
fix URI cannot contain path traversal
haitham911 Jan 21, 2025
d0490de
add relative path test
haitham911 Jan 21, 2025
4b3c662
fix ValidateURI
haitham911 Jan 21, 2025
2f3690f
improve log
haitham911 Jan 21, 2025
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
40 changes: 40 additions & 0 deletions examples/tests/test-vendor/atmos.yaml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This directory has moved to tests/fixtures/scenarios

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
base_path: "./"

components:
terraform:
base_path: "components/terraform"
apply_auto_approve: false
deploy_run_init: true
init_run_reconfigure: true
auto_generate_backend_file: false

stacks:
base_path: "stacks"
included_paths:
- "deploy/**/*"
excluded_paths:
- "**/_defaults.yaml"
name_pattern: "{stage}"

vendor:
# Single file
base_path: "./vendor.yaml"

# Directory with multiple files
#base_path: "./vendor"

# Absolute path
#base_path: "vendor.d/vendor1.yaml"

logs:
file: "/dev/stderr"
level: Info

# Custom CLI commands

# No arguments or flags are required
commands:
- name: "test"
description: "Run all tests"
steps:
- atmos vendor pull --everything
Comment on lines +36 to +40
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

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 demo to 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)

13 changes: 13 additions & 0 deletions examples/tests/test-vendor/test-components/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Example Terraform IPinfo Component

This Terraform module retrieves data from the IPinfo API for a specified IP address. If no IP address is specified, it retrieves data for the requester's IP address.

## Usage

Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add usage examples and prerequisites.

The Usage section would benefit from:

  • A complete example showing how to use this module
  • Prerequisites (e.g., required provider configuration)
  • Information about IPinfo API rate limits and authentication requirements
  • Version compatibility information

Here's a suggested structure:

## Usage

### Prerequisites
- Terraform >= 0.13
- IPinfo API token (if required)

### Provider Configuration
```hcl
provider "http" {}

Example Usage

module "ipinfo" {
  source = "path/to/module"
  
  ip_address = "8.8.8.8"  # Optional: Specify an IP address
}

output "ip_metadata" {
  value = module.ipinfo.metadata
}

<!-- This is an auto-generated comment by CodeRabbit -->

### Inputs

- `ip_address` (optional): The IP address to retrieve information for. If not specified, the requester's IP address will be used. The default value is an empty string.

### Outputs

- `metadata`: The data retrieved from IPinfo for the specified IP address, in JSON format.
7 changes: 7 additions & 0 deletions examples/tests/test-vendor/test-components/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
data "http" "ipinfo" {
url = var.ip_address != "" ? "https://ipinfo.io/${var.ip_address}" : "https://ipinfo.io"

request_headers = {
Accept = "application/json"
}
}
osterman marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions examples/tests/test-vendor/test-components/outputs.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
output "metadata" {
description = "The data retrieved from IPinfo for the specified IP address"
value = jsondecode(data.http.ipinfo.response_body)
}
1 change: 1 addition & 0 deletions examples/tests/test-vendor/test-components/providers.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
provider "http" {}
5 changes: 5 additions & 0 deletions examples/tests/test-vendor/test-components/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
variable "ip_address" {
description = "The IP address to retrieve information for (optional)"
type = string
default = ""
}
5 changes: 5 additions & 0 deletions examples/tests/test-vendor/test-components/versions.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
terraform {
required_version = ">= 1.0.0"

required_providers {}
}
53 changes: 53 additions & 0 deletions examples/tests/test-vendor/vendor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
apiVersion: atmos/v1
kind: AtmosVendorConfig
metadata:
name: demo-vendoring
description: Atmos vendoring manifest for Atmos demo component library
spec:
# Import other vendor manifests, if necessary
imports: []

sources:
- component: "github/stargazers"
source: "github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
included_paths:
- "**/*.tf"
- "**/*.tfvars"
- "**/*.md"
tags:
- demo
- github

- component: "test-components"
source: "file:///./test-components"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
tags:
- demo

- component: "weather"
source: "git::https://github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
tags:
- demo
- component: "my-vpc1"
source: "oci://public.ecr.aws/cloudposse/components/terraform/stable/aws/vpc:{{.Version}}"
version: "latest"
targets:
- "components/terraform/infra/my-vpc1"
osterman marked this conversation as resolved.
Show resolved Hide resolved
- component: "vpc-src"
source: "../../demo-library/weather" # required for test URI contain path traversal
targets:
- "components/terraform/vpc-src"
included_paths:
- "**/*.tf"
- "**/*.md"
- "**/*.tftmpl"
- "**/modules/**"
excluded_paths: []
9 changes: 9 additions & 0 deletions internal/exec/vendor_component_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"net/url"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -248,6 +249,14 @@ func ExecuteComponentVendorInternal(
sourceIsLocalFile = true
}
}
u, err := url.Parse(uri)
if err == nil && u.Scheme != "" {
if u.Scheme == "file" {
trimmedPath := strings.TrimPrefix(filepath.ToSlash(u.Path), "/")
uri = filepath.Clean(trimmedPath)
useLocalFileSystem = true
}
}
}

var pType pkgType
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 @@ -248,7 +248,7 @@ func downloadAndInstall(p *pkgAtmosVendor, dryRun bool, atmosConfig schema.Atmos
}
}
// Create temp directory
tempDir, err := os.MkdirTemp("", fmt.Sprintf("atmos-vendor-%d-*", time.Now().Unix()))
tempDir, err := os.MkdirTemp("", "atmos-vendor")
if err != nil {
return installedPkgMsg{
err: fmt.Errorf("failed to create temp directory: %w", err),
Expand Down
27 changes: 21 additions & 6 deletions internal/exec/vendor_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package exec

import (
"fmt"
"net/url"
"os"
"path/filepath"
"sort"
Expand Down Expand Up @@ -361,12 +362,14 @@ func ExecuteAtmosVendorInternal(
if err != nil {
return err
}
err = ValidateURI(uri)
if err != nil {
return err
}

useOciScheme, useLocalFileSystem, sourceIsLocalFile := determineSourceType(&uri, vendorConfigFilePath)
if !useLocalFileSystem {
err = ValidateURI(uri)
if err != nil {
return err
}
}

// Determine package type
var pType pkgType
Expand All @@ -384,7 +387,7 @@ func ExecuteAtmosVendorInternal(
if err != nil {
return err
}
targetPath := filepath.Join(vendorConfigFilePath, target)
targetPath := filepath.Join(filepath.ToSlash(vendorConfigFilePath), filepath.ToSlash(target))
pkgName := s.Component
if pkgName == "" {
pkgName = uri
Expand Down Expand Up @@ -514,12 +517,22 @@ func determineSourceType(uri *string, vendorConfigFilePath string) (bool, bool,
useLocalFileSystem := false
sourceIsLocalFile := false
if !useOciScheme {
if absPath, err := u.JoinAbsolutePathWithPath(vendorConfigFilePath, *uri); err == nil {
if absPath, err := u.JoinAbsolutePathWithPath(filepath.ToSlash(vendorConfigFilePath), *uri); err == nil {
uri = &absPath
useLocalFileSystem = true
sourceIsLocalFile = u.FileExists(*uri)
}
parsedURL, err := url.Parse(*uri)
if err == nil && parsedURL.Scheme != "" {
if parsedURL.Scheme == "file" {
trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/")
*uri = filepath.Clean(trimmedPath)
useLocalFileSystem = true
}
}

Comment on lines +523 to +536
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider handling URL parsing errors.

While the URL parsing and file schema handling is good, the error from url.Parse is silently ignored. This could mask potential issues.

 		parsedURL, err := url.Parse(*uri)
-		if err == nil && parsedURL.Scheme != "" {
+		if err != nil {
+			return useOciScheme, useLocalFileSystem, sourceIsLocalFile
+		}
+		if parsedURL.Scheme != "" {
 			if parsedURL.Scheme == "file" {
 				trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/")
 				*uri = filepath.Clean(trimmedPath)

Committable suggestion skipped: line range outside the PR's diff.

}

return useOciScheme, useLocalFileSystem, sourceIsLocalFile
}

Expand Down Expand Up @@ -556,6 +569,8 @@ func generateSkipFunction(atmosConfig schema.AtmosConfiguration, tempDir string,
if filepath.Base(src) == ".git" {
return true, nil
}
tempDir = filepath.ToSlash(tempDir)
src = filepath.ToSlash(src)

trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src)

Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/scenarios/complete/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}}"
version: "1.323.0"
targets:
- "components/terraform/infra/vpc-flow-logs-bucket/{{.Version}}"
Expand Down
64 changes: 64 additions & 0 deletions tests/test-cases/demo-stacks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,67 @@ tests:
stderr:
- "^$"
exit_code: 0

- name: atmos greet with args
enabled: true
description: "Validate atmos custom command greet runs with argument provided."
workdir: "../examples/demo-custom-command/"
command: "atmos"
args:
- "greet"
- "Andrey"
expect:
stdout:
- "Hello, Andrey\n"
stderr:
- "^$"
exit_code: 0

- name: atmos greet without args
enabled: true
description: "Validate atmos custom command greet runs without argument provided."
workdir: "../examples/demo-custom-command/"
command: "atmos"
args:
- "greet"
expect:
stdout:
- "Hello, John Doe\n"
stderr:
- "^$"
exit_code: 0
- name: atmos_vendor_pull
enabled: true
description: "Ensure atmos vendor pull command executes without errors and files are present."
workdir: "../examples/tests/test-vendor/"
Copy link
Member

Choose a reason for hiding this comment

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

Use test fixtures instead

command: "atmos"
args:
- "vendor"
- "pull"
expect:
file_exists:
- "./components/terraform/github/stargazers/main/main.tf"
- "./components/terraform/github/stargazers/main/outputs.tf"
- "./components/terraform/github/stargazers/main/providers.tf"
- "./components/terraform/github/stargazers/main/variables.tf"
- "./components/terraform/github/stargazers/main/versions.tf"
- "./components/terraform/infra/my-vpc1/main.tf"
- "./components/terraform/infra/my-vpc1/outputs.tf"
- "./components/terraform/infra/my-vpc1/providers.tf"
- "./components/terraform/infra/my-vpc1/variables.tf"
- "./components/terraform/infra/my-vpc1/versions.tf"
- "./components/terraform/test-components/main/main.tf"
- "./components/terraform/test-components/main/outputs.tf"
- "./components/terraform/test-components/main/providers.tf"
- "./components/terraform/test-components/main/variables.tf"
- "./components/terraform/test-components/main/versions.tf"
- "./components/terraform/weather/main/main.tf"
- "./components/terraform/weather/main/outputs.tf"
- "./components/terraform/weather/main/providers.tf"
- "./components/terraform/weather/main/variables.tf"
- "./components/terraform/weather/main/versions.tf"
- "./components/terraform/vpc-src/main.tf"
- "./components/terraform/vpc-src/outputs.tf"
- "./components/terraform/vpc-src/variables.tf"
- "./components/terraform/vpc-src/versions.tf"
exit_code: 0
Loading