fix(vendor): Remove excessive URL validation #1024
Conversation
📝 WalkthroughWalkthroughThe update modifies the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
34-36: Consider removing space validation.Since go-getter handles URL validation and we're operating in a trusted environment, this check might be redundant.
- if strings.Contains(uri, " ") { - return fmt.Errorf("URI cannot contain spaces: %v", uri) - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/go_getter_utils.go (1)
38-47: Verify if scheme validation is necessary.The scheme validation might be redundant since go-getter already validates URLs. However, if you decide to keep it, consider updating the valid schemes to match go-getter's supported schemes (documented in lines 193-241).
Let's check if go-getter performs scheme validation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for scheme validation in go-getter rg -A 5 'scheme|protocol' $(fd -t f -e go . | grep go-getter)Length of output: 39042
Scheme validation is custom and not entirely redundant—but the valid schemes list needs review
Our investigation confirms that the code ininternal/exec/go_getter_utils.goperforms its own scheme validation (via theIsValidSchemefunction) rather than relying solely on go-getter’s internal URL checks. This extra validation catches unsupported schemes early with explicit error messages. However, the current validation only accepts"http","https","git", and"ssh", while the documentation (and go-getter’s own supported schemes) include additional protocols (e.g.,file,dir,tar,zip, etc.). Consider updating the valid schemes list if you decide to keep this extra layer of validation.
Good point, I will remove that again. It would be helpful if the |
Ok, I found a quick way to add that. |
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/vendor_utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/vendor_utils.go (1)
373-374: LGTM: Improved error message for path traversal.The simplified error message for path traversal cases is clearer and more focused.
I’m on mobile, so I can’t easily verify. Since we return an error, it will propagate up the call stack to a point where we also have access to the component name. At that level, we can log it with structured key-value pairs. We’ve also introduced a global log instance with contextual support, making it easier to capture and enrich logs with relevant metadata. |
|
These changes were released in v1.160.1. |
* fix(vendor): Remove excessive URL validation Fixes #1019 * feat(vendor): Add component to error message --------- Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
what
why
depthwhile also specifying aref(and other features supported by go-getter).url.Parsewas added.references
Summary by CodeRabbit