Conversation
WalkthroughThe change modifies URL validation logic in the remote handler for artifact uploads and downloads. A new file-local constant 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/remote.go`:
- 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.
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| 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) |
There was a problem hiding this comment.
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.
|
|
||
| resp, err := http.Get(parsed.String()) //nolint:gosec // URL is validated above | ||
| utils.Verbose("downloading from %v", parsed) | ||
| resp, err := http.Get(parsed.String()) |
There was a problem hiding this comment.
🧩 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.goRepository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.