Skip to content

fix(import-url): validate artifact URL scheme - #560

Open
dipak0000812 wants to merge 2 commits into
microcks:masterfrom
dipak0000812:fix/534-import-url-scheme-validation
Open

dipak0000812 wants to merge 2 commits into
microcks:masterfrom
dipak0000812:fix/534-import-url-scheme-validation

Conversation

@dipak0000812

Copy link
Copy Markdown

Description

Fixes #534

When import-url receives an artifact argument without an http:// or https:// prefix, parseImportURLArg previously returned the input unchanged and it was passed to mc.DownloadArtifact, resulting in a misleading server or connection error.

This change:

  • Validates the artifact URL scheme in parseImportURLArg.
  • Returns an errors.KindUsage error when the input does not start with http:// or https://.
  • Propagates the validation error from import-url's RunE.
  • Adds regression tests for relative paths, unsupported schemes (ftp://, file:///), empty input, and the reported issue syntax.

Testing

  • go test -v ./cmd -run "TestImportURL|TestParseImportURLArg"
  • go test ./...
  • go vet ./...
  • git diff --check

AI 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.

Signed-off-by: Dipak Dhangar <dhangardip09@gmail.com>
@github-actions

Copy link
Copy Markdown

👋 @dipak0000812

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!

@Syedowais312

Copy link
Copy Markdown
Contributor

LGTM, this matches what I proposed in #534. Thanks for picking it up, @dipak0000812

Comment thread cmd/importURL.go
f,
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I totally agree here. We can use that functionality to shift our validation much closer to the actual user input.

@Harsh4902 Harsh4902 added this to the Next milestone Oct 9, 2026
@Harsh4902 Harsh4902 added component/cli go Pull requests that update go code labels Oct 9, 2026
Comment thread cmd/importURL.go Outdated
Signed-off-by: Dipak Dhangar <dhangardip09@gmail.com>
@dipak0000812

Copy link
Copy Markdown
Author

Thanks for the feedback. I've updated the implementation:

  • Moved missing-argument and URL-scheme validation into Cobra's Args validator.
  • Validated all comma-separated URLs before RunE executes, preventing partial imports caused by invalid entries.
  • Made scheme validation case-insensitive while preserving the original URL.
  • Added regression tests for mixed-case schemes and upfront validation.

The targeted tests, go vet ./cmd/..., and git diff --check pass. I also verified that the full-suite Windows path-related failures reproduce on clean master.

I've pushed the changes in 2806c15. Thanks for the review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/cli go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: import-url doesn't validate URL scheme, fails with misleading error instead

5 participants