OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

olucasfreitas · 2025-10-21T13:18:42Z

Details

This PR fixes a usability issue where rosa create cluster --http-proxy was displaying unhelpful error messages when proxy passwords contained special characters like forward slashes, making it difficult for users to understand what was wrong with their input.

Fixed Behavior

Run the command with a proxy password containing a forward slash:

./rosa create cluster --http-proxy "http://proxyuser:QvoZjyy/[email protected]:8080"

Now shows clear error message:

E: Invalid http-proxy value 'http://proxyuser:QvoZjyy/[email protected]:8080': 
   password contains special characters that need URL encoding

Additional Validations Added

Spaces in URL - Detects and rejects URLs with spaces:

./rosa create cluster --http-proxy "http://proxy.example.com :8080"

Output:

E: Invalid http-proxy value 'http://proxy.example.com :8080': URL cannot contain spaces

Invalid Port Range - Validates port numbers are between 1 and 65535:

./rosa create cluster --http-proxy "http://proxy.example.com:99999"

Output:

E: Invalid http-proxy value 'http://proxy.example.com:99999': port must be between 1 and 65535

Multiple @ Symbols - Detects unencoded @ in passwords:

./rosa create cluster --http-proxy "http://user:pass@[email protected]:8080"

Output:

E: Invalid http-proxy value 'http://user:pass@[email protected]:8080': 
   password contains special characters that need URL encoding

The --https-proxy Flag Behavior

Using HTTPS scheme with --http-proxy continues to show the existing error:

./rosa create cluster --http-proxy "https://proxy.example.com:8080"

Output:

E: Expected http-proxy to have an http:// scheme

Ticket

Closes OCM-18900

pkg/ocm/helpers_test.go

codecov · 2025-10-21T13:54:05Z

Codecov Report

❌ Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.07%. Comparing base (5f21d22) to head (a50904c).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/ocm/helpers.go	75.00%	5 Missing and 2 partials ⚠️
pkg/interactive/validation.go	90.90%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3052      +/-   ##
==========================================
+ Coverage   27.91%   28.07%   +0.15%     
==========================================
  Files         316      316              
  Lines       35008    35043      +35     
==========================================
+ Hits         9774     9837      +63     
+ Misses      24582    24551      -31     
- Partials      652      655       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cmd/list/dnsdomains/cmd.go

olucasfreitas · 2025-11-03T11:37:00Z

/retest-required

cmd/list/dnsdomains/cmd.go

tests/utils/exec/rosacli/upgrade_service.go

pkg/ocm/helpers.go

olucasfreitas · 2025-11-03T15:15:52Z

/retest-required

olucasfreitas · 2025-11-03T15:22:59Z

/retest-required

hunterkepley

The test case in the bug ticket does not seem to pass:

E: parse "http://proxyuser:QvoZjyy/[email protected]:8080": invalid port ":QvoZjyy" after host

I think there may be a bug in the parse code

olucasfreitas · 2025-11-03T15:58:28Z

@hunterkepley I just did some tests here, and it seems to be working after the fix, did you test it with the branch locally ?

olucasfreitas · 2025-11-04T11:08:34Z

/retest-required

olucasfreitas · 2025-11-04T11:16:05Z

/retest-required

tests/e2e/test_rosacli_cluster.go

pkg/ocm/helpers_test.go

pkg/ocm/helpers.go

olucasfreitas · 2025-11-11T23:57:58Z

@hunterkepley My bad then, I understood this more as as a suggestions that a thing to initiate a discussion. Answering what you said, I think we can show the password, because this way the user can just do a quick comparison and figure out what's wrong, WDYT ?

I'm also working on the test cases you commented

openshift-ci · 2025-11-12T19:08:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: olucasfreitas
Once this PR has been reviewed and has the lgtm label, please assign yuwang-rh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hunterkepley · 2025-11-13T20:51:13Z

/test all

hunterkepley · 2025-11-13T20:52:08Z

/test e2e-presubmits-pr-rosa-sts-advanced

hunterkepley · 2025-11-13T20:53:09Z

/test e2e-presubmits-pr-rosa-hcp-advanced

hunterkepley · 2025-11-13T20:53:59Z

Some reason the e2e tests didn't run, I manually ran them so we can verify this MR didn't break any

pkg/helper/helpers.go

pkg/interactive/validation.go

pkg/ocm/helpers.go

pkg/ocm/helpers_test.go

pkg/ocm/registry_config_test.go

olucasfreitas · 2025-11-18T20:38:19Z

/retest

olucasfreitas · 2025-11-18T20:47:09Z

/retest

olucasfreitas · 2025-11-19T11:42:09Z

/retest

hunterkepley · 2025-11-18T20:53:04Z

pkg/interactive/validation.go

+	if parsedUri == nil {
+		return nil
+	}


I think I've left 2 comments now asking about this 🤔 ; but why did you add this?

I added that because _isUrl can return nil when it gets an empty string or nil input, and without that check we'd maybe get a panic, so I wanted to be sure, sorry about this, as I got a lot of comments I must have let this one pass, just removed it on the last commit

@olucasfreitas Maybe it would be helpful to respond + resolve to comments one by one 🙂

But, I feel like something was misunderstood about the function based on the implementation of the nil check here

Here is the function you added this to:

func IsURLHttps(val interface{}) error { parsedUri, err := _isUrl(val) if err != nil { return err } if parsedUri.Scheme != helper.ProtocolHttps { return fmt.Errorf("expect URL '%s' to use an 'https://' scheme", val.(string)) } return nil }

So, the point of this, is to return an error when it's not valid, and return nil when it is valid.

Lets review where this function is called:

issuerUrl, err := interactive.GetString(interactive.Input{ Question: "Issuer URL (please include 'https://')", Help: cmd.Flags().Lookup(IssuerUrlFlag).Usage, Required: true, Validators: []interactive.Validator{interactive.IsURLHttps}, })

Here it is used as a validator. The value is Required, meaning we cannot pass in an empty string.

The second place it's called is when validating another interactive prompt, but in a different way

If we look at the implementation of _isUrl, we can see the following:

func _isUrl(val interface{}) (*url.URL, error) { if val == nil { return nil, nil } s, ok := val.(string) if !ok { return nil, fmt.Errorf("can only validate strings, got %v", val) } if s == "" { return nil, nil } parsedUri, err := url.ParseRequestURI(fmt.Sprintf("%v", val)) return parsedUri, err }

So, which cases does this return nil?

When the URL passed in is nil (impossible for strings! it's possible with pointers to strings, but not strings)

When the URL is empty (not possible with the shown implementation)

When there is a non-string passed in (this will be caught by the function already, and returned, before it checks if it's nil, due to it returning an actual error)

All implementations have been around since 2023, and are not related to this feature 🙂 so we should leave those functions alone. They only deal with OIDC providers, not create/cluster like this ticket is supposed to

But, I feel like something was misunderstood about the function based on the implementation of the nil check here

To circle it back to my original statement; If the URL was nil, I don't think we should return nil as the error. I think we should inform that the URL passed in was nil, and therefore incorrect. But again, we don't need any of it for this feature 🙂

Sorry about the comments, I had understood that the person that commented would resolve the comments if in the next review they saw that the issues we're resolved

also sorry about not getting this at the first place, thanks for the run down, I think my last commit is now correct

@olucasfreitas

Sorry about the comments, I had understood that the person that commented would resolve the comments if in the next review they saw that the issues we're resolved

I was replying to your original reply: as I got a lot of comments I must have let this one pass, just removed it on the last commit

it's all good if you leave the comments for the reviewer to resolve, in fact that's a good idea. What I was getting at was that it may be helpful to take them one at a time, and reply to it after fixing it. I see usually that you respond to all of them after fixing everything, then post the commit along with the replies is the reason I suggested it 🙂 might help break up the context of each review comment some

pkg/ocm/helpers.go

pkg/interactive/validation.go

pkg/ocm/helpers.go

hunterkepley · 2025-11-19T14:35:39Z

/test e2e-presubmits-pr-rosa-hcp-advanced

hunterkepley · 2025-11-19T14:35:45Z

/test e2e-presubmits-pr-rosa-sts-advanced

openshift-ci · 2025-11-19T20:05:45Z

@olucasfreitas: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/coverage	`2637639`	link	true	`/test coverage`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

olucasfreitas · 2025-11-19T20:22:04Z

/retest

…al characters

hunterkepley · 2025-12-02T20:13:24Z

pkg/helper/helpers.go

+	Field       string
+	InvalidChar rune
+	HasInvalid  bool
+}


we should define structs at the top of files to be consistent with other files

I also suggest maybe putting this + the function below into a more specific file for its purpose (maybe pkg/helpers/url/validation.go would suite this nicely, or some variation of this)

hunterkepley · 2025-12-02T20:14:25Z

pkg/helper/helpers.go

+			return &URLCredentialValidation{
+				Field:       field,
+				InvalidChar: char,
+				HasInvalid:  true,


we could drop this field, and instead just check if InvalidChar is an empty rune or not

hunterkepley · 2025-12-02T20:15:20Z

pkg/helper/helpers.go

+	return checkForInvalidChars(password, "password")
+}
+
+func checkForInvalidChars(value, field string) *URLCredentialValidation {


I think this should also go in that new file dedicated to validating URLs

hunterkepley · 2025-12-02T20:15:28Z

pkg/helper/helpers.go

+	HasInvalid  bool
+}
+
+func ValidateURLCredentials(urlString string) *URLCredentialValidation {


same for this

hunterkepley · 2025-12-02T20:17:43Z

pkg/helper/helpers.go

+
+	schemeIdx := strings.Index(urlString, "://")
+	if schemeIdx == -1 {
+		return &URLCredentialValidation{HasInvalid: false}


We should probably tell the user what is wrong here, right? This only says there is an invalid character somewhere

hunterkepley · 2025-12-02T20:21:11Z

pkg/ocm/helpers.go

+	if parsedURL.Scheme == "" {
+		return fmt.Errorf("invalid %s value: URL scheme is missing (e.g., %s://)", proxyType, expectedScheme)
+	}
+	if parsedURL.Scheme != expectedScheme {
+		return errors.Errorf("%s", fmt.Sprintf("expected %s to have an %s:// scheme", proxyType, expectedScheme))
+	}


We should encase the %s's in single quotes, and get rid of this first "empty check" validation. They are one and the same if we use single quotes. WDYT? Does that make sense?

hunterkepley · 2025-12-02T20:22:36Z

pkg/ocm/helpers.go

+
+	// Proxy URLs shouldn't have a path
+	if parsedURL.Path != "" && parsedURL.Path != "/" {
+		return fmt.Errorf("invalid %s value: proxy URL should not contain a path", proxyType)


Maybe we should let the user know what the value was in case they are confused?

hunterkepley · 2025-12-02T20:23:54Z

pkg/ocm/helpers.go

+	host, port, err := net.SplitHostPort(parsedURL.Host)
+	if err != nil {
+		// No port specified, just validate the host
+		host = parsedURL.Host


Are we sure that the only reason this can return an error is due to not having a port specified? I would suggest looking at the source code for that net pkg func

hunterkepley · 2025-12-02T20:39:47Z

pkg/ocm/helpers.go

+		portNum, _ := strconv.Atoi(port)
+		if portNum < 1 || portNum > 65535 {
+			return fmt.Errorf("invalid %s value: port must be between 1 and 65535", proxyType)


I don't think we need this validation specifically- the user shouldn't be inputting a giant or negative port number if they have a proxy URL they know they want to use. Having it result in a 404 eventually would be fine UX. WDYT?

though there's no harm in keeping it specifically, it just strays some from the original goal

hunterkepley · 2025-12-02T20:42:56Z

pkg/ocm/helpers.go

+			// Not an IP, check if it's a valid hostname
+			if strings.Contains(host, "/") {
+				return fmt.Errorf("invalid %s value: invalid host", proxyType)
+			}


is this needed? How do we know it was a slash that caused this? What happens if it wasn't a slash?

openshift-ci bot requested review from ciaranRoche and hunterkepley October 21, 2025 13:19

gdbranco reviewed Oct 21, 2025

View reviewed changes

pkg/ocm/helpers_test.go Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch from fc09c54 to aa528ca Compare October 21, 2025 15:11

olucasfreitas requested a review from gdbranco October 21, 2025 17:03

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2025

gdbranco reviewed Oct 28, 2025

View reviewed changes

cmd/list/dnsdomains/cmd.go Outdated Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 00638f4 to 120a803 Compare November 3, 2025 11:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2025

olucasfreitas requested a review from gdbranco November 3, 2025 11:36

hunterkepley reviewed Nov 3, 2025

View reviewed changes

cmd/list/dnsdomains/cmd.go Outdated Show resolved Hide resolved

hunterkepley reviewed Nov 3, 2025

View reviewed changes

tests/utils/exec/rosacli/upgrade_service.go Show resolved Hide resolved

hunterkepley reviewed Nov 3, 2025

View reviewed changes

pkg/ocm/helpers.go Outdated Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 2260456 to c6b94b4 Compare November 3, 2025 14:46

olucasfreitas requested a review from hunterkepley November 3, 2025 15:16

hunterkepley reviewed Nov 3, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 96029c9 to a50904c Compare November 3, 2025 19:49

hunterkepley reviewed Nov 4, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 9502aa4 to 2637639 Compare November 5, 2025 15:38

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from dca6669 to 012f289 Compare November 12, 2025 19:06

olucasfreitas force-pushed the OCM-18900 branch 4 times, most recently from 560e392 to 3df4265 Compare November 13, 2025 19:27

olucasfreitas requested a review from hunterkepley November 17, 2025 20:49

hunterkepley reviewed Nov 17, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch from 3df4265 to 055b821 Compare November 18, 2025 20:24

hunterkepley reviewed Nov 19, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 34c6edf to 875a547 Compare November 19, 2025 19:47

olucasfreitas force-pushed the OCM-18900 branch from 875a547 to 4fe55d7 Compare November 19, 2025 20:37

olucasfreitas requested a review from hunterkepley November 19, 2025 21:11

OCM-18900 | fix: Improve error message for proxy passwords with speci…

a51e1b8

…al characters

olucasfreitas force-pushed the OCM-18900 branch from 4fe55d7 to a51e1b8 Compare December 1, 2025 17:17

hunterkepley reviewed Dec 2, 2025

View reviewed changes

OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

Are you sure you want to change the base?

OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

Uh oh!

Conversation

olucasfreitas commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Behavior

Additional Validations Added

The --https-proxy Flag Behavior

Ticket

Uh oh!

Uh oh!

codecov bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

hunterkepley left a comment

Choose a reason for hiding this comment

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

olucasfreitas commented Nov 4, 2025

Uh oh!

olucasfreitas commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 12, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Nov 18, 2025

Uh oh!

olucasfreitas commented Nov 18, 2025

Uh oh!

olucasfreitas commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hunterkepley Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olucasfreitas commented Oct 21, 2025 •

edited

Loading

codecov bot commented Oct 21, 2025 •

edited

Loading

olucasfreitas commented Nov 11, 2025 •

edited

Loading

hunterkepley Nov 19, 2025 •

edited

Loading

openshift-ci bot commented Nov 19, 2025 •

edited

Loading

hunterkepley Dec 2, 2025 •

edited

Loading