Repository navigation
fix(import-url): validate artifact URL scheme - #560
Conversation
Signed-off-by: Dipak Dhangar <[email protected]>
|
Welcome to the Microcks community! 💖 Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly. Hope you have a great time there! |
|
LGTM, this matches what I proposed in #534. Thanks for picking it up, @dipak0000812 |
| f, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this way it gives to many responsibilities to this function.
Cobra offer pre-hook Args which is the right place for this kind of validation
this is also valid for the if len(arg) == 0 check
There was a problem hiding this comment.
Yes, I totally agree here. We can use that functionality to shift our validation much closer to the actual user input.
Signed-off-by: Dipak Dhangar <[email protected]>
|
Thanks for the feedback. I've updated the implementation:
The targeted tests, I've pushed the changes in |
| // Check if URL starts with https or http | ||
| if strings.HasPrefix(f, "https://") || strings.HasPrefix(f, "http://") { | ||
| lower := strings.ToLower(f) | ||
| if strings.HasPrefix(lower, "https://") || strings.HasPrefix(lower, "http://") { |
There was a problem hiding this comment.
@dipak0000812 I think this validation can be removed from parseImportURLArg, since the Cobra Args validator already checks the URL scheme before RunE is called.
There was a problem hiding this comment.
Good point. I've removed the redundant scheme check from parseImportURLArg; URL scheme validation now happens in Cobra's Args validator, before RunE. The parser is focused on suffix parsing, and the existing regression tests pass. Pushed the update.
Signed-off-by: Dipak Dhangar <[email protected]>
Vaishnav88sk
left a comment
Vaishnav88sk
left a comment
There was a problem hiding this comment.
Thanks @dipak0000812, the case-insensitive scheme handling and the Args-hook validation both look good now.
Nit, optional: treat bare http:// and https:// as invalid.
Signed-off-by: Dipak Dhangar <[email protected]>
|
Verified the merge result (build, Disclosure: this comment was drafted with AI assistance, and the results were run locally. |
yada
left a comment
yada
left a comment
There was a problem hiding this comment.
Verified locally. Disclosure: AI-assisted verification.
|
You are now a Microcks community contributor! 💖 Thanks and congrats 🚀 on merging your first pull request! We are delighted and very proud of you! 👏 📢 If you're using Microcks in your organization, please add your company name to this list. 🙏 It really helps the project to gain momentum and credibility. It's a small contribution back to the project with a big impact. If you need to know why and how to add yourself to the list, please read the blog post "Join the Microcks Adopters list and Empower the vibrant open source Community 🙌" Kudos and please keep going, we need you 🙌 |
Description
Fixes #534
When
import-urlreceives an artifact argument without anhttp://orhttps://prefix,parseImportURLArgpreviously returned the input unchanged and it was passed tomc.DownloadArtifact, resulting in a misleading server or connection error.This change:
parseImportURLArg.errors.KindUsageerror when the input does not start withhttp://orhttps://.import-url'sRunE.ftp://,file:///), empty input, and the reported issue syntax.Testing
go test -v ./cmd -run "TestImportURL|TestParseImportURLArg"go test ./...go vet ./...git diff --checkAI Assistance Disclosure
AI assistance was used minimally during development for codebase/policy lookup and review. The implementation, tests, and final verification were performed and reviewed by the author.