Skip to content

fix: validate artifact host for upload and download to address codeql ssrf alerts #207

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

Merged
gmegidish merged 1 commit into from
Apr 27, 2026
Merged

fix: validate artifact host for upload and download to address codeql ssrf alerts #207

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 9 additions & 5 deletions devices/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"github.com/mobile-next/mobilecli/utils"
)

const artifactsHost = "mobilenexthq-artifacts.s3.us-west-2.amazonaws.com"

type params map[string]any

type RemoteDevice struct {
Expand Down Expand Up @@ -229,8 +231,8 @@ func uploadFileToURL(filePath, uploadURL string) error {
if err != nil {
return fmt.Errorf("invalid upload URL: %w", err)
}
if u.Scheme != "https" || u.Host != "mobilenexthq-artifacts.s3.us-west-2.amazonaws.com" {
return fmt.Errorf("upload URL must be https://mobilenexthq-artifacts.s3.us-west-2.amazonaws.com/..., got: %s", uploadURL)
if u.Scheme != "https" || u.Hostname() != artifactsHost {
return fmt.Errorf("upload URL must be https://%s/..., got: %s", artifactsHost, uploadURL)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Do not log or echo full presigned artifact URLs.

Line 235, Line 285, and Line 288 can expose signed query params in logs/errors. That leaks temporary credentials and weakens the security fix.

🔐 Proposed fix (redact URL details) 
- return fmt.Errorf("upload URL must be https://%s/..., got: %s", artifactsHost, uploadURL)
+ return fmt.Errorf("upload URL must be https://%s/...", artifactsHost)

- return fmt.Errorf("download URL must be https://%s/..., got: %s", artifactsHost, downloadURL)
+ return fmt.Errorf("download URL must be https://%s/...", artifactsHost)

- utils.Verbose("downloading from %v", parsed)
+ utils.Verbose("downloading from host=%s path=%s", parsed.Hostname(), parsed.EscapedPath())

Also applies to: 285-285, 288-288

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@devices/remote.go` at line 235, The error message currently echoes the full
presigned uploadURL (see the fmt.Errorf that references artifactsHost and
uploadURL) which leaks signed query params; change the code to avoid logging or
returning the full URL—parse uploadURL (using url.Parse) and either log only
safe parts (scheme and host and/or path) or redact the query/fragment before
including it in the error, or simply reference artifactsHost and a short
indicator like "<redacted-presigned-url>" instead of the full uploadURL; apply
the same redaction approach to the other occurrences that reference uploadURL
around the fmt.Errorf calls at the same code sites.

}

f, err := os.Open(filePath)
Expand Down Expand Up @@ -278,11 +280,13 @@ func downloadFile(downloadURL, outputPath string, cb *ScreenRecordCallbacks) err
if err != nil {
return fmt.Errorf("invalid download URL: %w", err)
}
if parsed.Scheme != "https" {
return fmt.Errorf("download URL must use HTTPS scheme, got %q", parsed.Scheme)

if parsed.Scheme != "https" || parsed.Hostname() != artifactsHost {
return fmt.Errorf("download URL must be https://%s/..., got: %s", artifactsHost, downloadURL)
}

resp, err := http.Get(parsed.String()) //nolint:gosec // URL is validated above
utils.Verbose("downloading from %v", parsed)
resp, err := http.Get(parsed.String())
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify unbounded GET usage in Go files and confirm timeout-backed clients.
# Expected:
# 1) No remaining production-path `http.Get(...)` in download/upload flows.
# 2) Download and upload both use `http.Client{Timeout: ...}`.

rg -nP --type=go -C2 '\bhttp\.Get\s*\('
rg -nP --type=go -C2 'var\s+\w+HTTPClient\s*=\s*&http\.Client\s*\{\s*Timeout:'

Repository: mobile-next/mobilecli

Length of output: 1360

🏁 Script executed:

sed -n '220,295p' devices/remote.go

Repository: mobile-next/mobilecli

Length of output: 2213

Use an HTTP client with explicit timeout for the download request.

Line 289 uses http.Get, which uses the default client without a request timeout. A stalled connection can block this flow indefinitely. This mirrors the pattern already established in the file where uploadFileToURL uses uploadHTTPClient with a 5-minute timeout.

⏱️ Proposed fix (bounded download request) 
+var downloadHTTPClient = &http.Client{Timeout: 5 * time.Minute}
+
 func downloadFile(downloadURL, outputPath string, cb *ScreenRecordCallbacks) error {
 	parsed, err := url.Parse(downloadURL)
 	if err != nil {
 		return fmt.Errorf("invalid download URL: %w", err)
 	}

 	if parsed.Scheme != "https" || parsed.Hostname() != artifactsHost {
 		return fmt.Errorf("download URL must be https://%s/..., got: %s", artifactsHost, downloadURL)
 	}

 	utils.Verbose("downloading from %v", parsed)
-	resp, err := http.Get(parsed.String())
+	resp, err := downloadHTTPClient.Get(parsed.String())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resp, err := http.Get(parsed.String())
var downloadHTTPClient = &http.Client{Timeout: 5 * time.Minute}
func downloadFile(downloadURL, outputPath string, cb *ScreenRecordCallbacks) error {
parsed, err := url.Parse(downloadURL)
if err != nil {
return fmt.Errorf("invalid download URL: %w", err)
}
if parsed.Scheme != "https" || parsed.Hostname() != artifactsHost {
return fmt.Errorf("download URL must be https://%s/..., got: %s", artifactsHost, downloadURL)
}
utils.Verbose("downloading from %v", parsed)
resp, err := downloadHTTPClient.Get(parsed.String())
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@devices/remote.go` at line 289, The download call currently uses http.Get
(resp, err := http.Get(parsed.String())) which has no timeout; replace it with a
dedicated HTTP client that enforces a timeout (e.g., reuse or create a
downloadHTTPClient with Timeout: 5*time.Minute consistent with uploadHTTPClient)
and call downloadHTTPClient.Get(parsed.String()) instead, ensuring any client
variable (downloadHTTPClient or uploadHTTPClient) is defined at package scope
like uploadFileToURL uses and that error handling and resp.Body handling remain
unchanged.

if err != nil {
return fmt.Errorf("HTTP GET failed: %w", err)
}
Expand Down
Loading