Conversation
…s. Halt the execution if any is found with a dedicated error message. Signed-off-by: Yannick Alexander <yannick@alexanderdev.io>
Signed-off-by: Yannick Alexander <yannick@alexanderdev.io>
There was a problem hiding this comment.
Pull request overview
Adds upfront validation for helm registry login host input to reject scheme and path components early, improving error clarity and avoiding later ORAS parsing failures.
Changes:
- Replaced the previous warning-only “host has path” check with an erroring
validateHostpre-check inClient.Login. - Introduced a regex-based host parser to detect scheme/path and return dedicated error messages.
- Updated unit tests to cover scheme/path rejection cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/registry/client.go | Adds validateHost + regex-based parsing; enforces no scheme/path before remote.NewRegistry. |
| pkg/registry/client_test.go | Replaces warning test with validateHost table-driven tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Promptless prepared a documentation update related to this change. This PR adds stricter host validation to Review at helm/helm-www#2057 |
…e pattern access Signed-off-by: Yannick Alexander <yannick@alexanderdev.io>
| // While ORAS will also validate some of these things, the current errors are a bit opaque. | ||
| // By validating these things upfront, we can provide clearer error messages to users when they attempt to login with an invalid host string. | ||
| func validateHost(host string) error { | ||
| matches := hostRegex.FindStringSubmatch(host) |
There was a problem hiding this comment.
Copilot is correct here, and alluding to a larger problem. This regex will reject many valid "hosts". A "host" here can mean anything from an IP address to a full domain name. Anything which is "resolvable" by the user's OS's network stack, effectively. And Helm can't attempt to encode these rules.
For a check like "Is the "host" actually a URL with a scheme or path?", url.Parse() can be used (One caveat with url.Parse(..) is that a "host" name is not really a URL. But if a URL is supplied, then url.Parse(..) will identify any scheme or path)
There was a problem hiding this comment.
I get the concern - the current regex is kept very broad (I updated it after Copilots comment) and should let through the vast majority of hosts but there will likely be cases that it still might not match (one I can think of are URLs with query parameters and co. - however those are anyway not allowed, right?).
Using url.Parse(...) though has some caveats. The main issue is, that without a scheme it will / might interpret the entire url as just a relative path. Because users have to provide the host without a scheme (users not doing that is exactly what we want to catch here), we would need to do an up-front check and prepend a dummy://-scheme for Parse(...) to not get confused and interpret it as a relative path. Doing so would require a regex anyway and then conditionally we would need to prepend a scheme if not match was found.
I am not opposed to changing the logic to use the regex as a scheme check only and then use url.Parse(...) to validate the rest of the URL - would you be good with this approach then?
What this PR does / why we need it:
This PR adds host validation on login to check if a scheme or path is present. If yes, the login is cancelled with a dedicated error message, informing the user what went wrong.
The context of this PR is the issue #31962, where we discussed the handling of the URL scheme in Helm v4 and the change from v3 (also refer to #30873). Currently, providing a scheme does raise a non-specific error, therefore the goal of this change is to provide clearer error messages and halt execution early (even if ORAS would technically also catch these errors later down the line). Providing a path did already produce a warning, however I decided to remove the warning and integrate the check into the new validation.
Validation is implemented using a naive regex to check if any scheme or path is present, as built-in things like
url.Parsedon't behave well when no scheme is present, interpreting the entire host as a path.Closes #31962
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)