OCM-18900 | fix: Improve error message for proxy passwords with special characters

Author: olucasfreitas

cmd/create/cluster/cmd.go

@@ -3047,15 +3047,15 @@ func run(cmd *cobra.Command, _ []string) {
 			Help:     cmd.Flags().Lookup("https-proxy").Usage,
 			Default:  httpsProxy,
 			Validators: []interactive.Validator{
-				interactive.IsURL,
+				ocm.ValidateHTTPSProxy,
 			},
 		})
 		if err != nil {
 			r.Reporter.Errorf("Expected a valid https proxy: %s", err)
 			os.Exit(1)
 		}
 	}
-	err = interactive.IsURL(httpsProxy)
+	err = ocm.ValidateHTTPSProxy(httpsProxy)
 	if err != nil {
 		r.Reporter.Errorf("%s", err)
 		os.Exit(1)

cmd/edit/cluster/cmd.go

@@ -508,7 +508,7 @@ func run(cmd *cobra.Command, _ []string) {
 		}
 	}
 	if httpsProxy != nil && *httpsProxy != input.DoubleQuotesToRemove {
-		err = interactive.IsURL(*httpsProxy)
+		err = ocm.ValidateHTTPSProxy(*httpsProxy)
 		if err != nil {
 			r.Reporter.Errorf("%s", err)
 			os.Exit(1)

pkg/helper/helpers.go

@@ -268,3 +268,65 @@ func FilterEmptyStrings(strings []string) []string {
 	}
 	return filteredResult
 }
+
+type URLCredentialValidation struct {
+	Field       string
+	InvalidChar rune
+	HasInvalid  bool
+}
+
+func ValidateURLCredentials(urlString string) *URLCredentialValidation {
+	if !strings.Contains(urlString, "@") || !strings.Contains(urlString, "://") {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	schemeIdx := strings.Index(urlString, "://")
+	if schemeIdx == -1 {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	rest := urlString[schemeIdx+3:]
+	atIdx := strings.LastIndex(rest, "@")
+	if atIdx == -1 {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	userinfo := rest[:atIdx]
+
+	if strings.Count(urlString, "@") > 1 {
+		return &URLCredentialValidation{
+			Field:       "password",
+			InvalidChar: '@',
+			HasInvalid:  true,
+		}
+	}
+
+	colonIdx := strings.Index(userinfo, ":")
+	if colonIdx == -1 {
+		return checkForInvalidChars(userinfo, "username")
+	}
+
+	username := userinfo[:colonIdx]
+	if result := checkForInvalidChars(username, "username"); result.HasInvalid {
+		return result
+	}
+
+	password := userinfo[colonIdx+1:]
+	return checkForInvalidChars(password, "password")
+}
+
+func checkForInvalidChars(value, field string) *URLCredentialValidation {
+	invalidChars := "/:#?[]@!$&'()*+,;="
+
+	for _, char := range value {
+		if strings.ContainsRune(invalidChars, char) {
+			return &URLCredentialValidation{
+				Field:       field,
+				InvalidChar: char,
+				HasInvalid:  true,
+			}
+		}
+	}
+
+	return &URLCredentialValidation{HasInvalid: false}
+}

pkg/interactive/validation_test.go

@@ -131,7 +131,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("The number of subnets for a public hosted " +
+			Expect(err.Error()).To(ContainSubstring("the number of subnets for a public hosted " +
 				"cluster should be at least two"))
 		})
 
@@ -153,7 +153,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("Must have at least one public subnet when not using both " +
+			Expect(err.Error()).To(ContainSubstring("must have at least one public subnet when not using both " +
 				"'private API' and 'private ingress'"))
 		})
 
@@ -169,7 +169,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("The number of subnets for a 'multi-AZ'"+
+			Expect(err.Error()).To(ContainSubstring("the number of subnets for a 'multi-AZ'"+
 				" 'private link cluster' should be '3', instead received: '%d'", len(answers)))
 		})
 

pkg/ocm/helpers.go

@@ -108,9 +108,9 @@ func ClusterNameValidator(name interface{}) error {
 	if str, ok := name.(string); ok {
 		str := strings.Trim(str, " \t")
 		if !IsValidClusterName(str) {
-			return fmt.Errorf("Cluster name must consist of no more than %d lowercase "+
+			return fmt.Errorf("cluster name must consist of no more than %d lowercase "+
 				"alphanumeric characters or '-', start with a letter, and end with an "+
-				"alphanumeric character.", MaxClusterNameLength)
+				"alphanumeric character", MaxClusterNameLength)
 		}
 		return nil
 	}
@@ -125,30 +125,88 @@ func ClusterDomainPrefixValidator(domainPrefix interface{}) error {
 	if str, ok := domainPrefix.(string); ok {
 		str := strings.Trim(str, " \t")
 		if str != "" && !IsValidClusterDomainPrefix(str) {
-			return fmt.Errorf("Cluster domain prefix must consist of no more than %d lowercase "+
+			return fmt.Errorf("cluster domain prefix must consist of no more than %d lowercase "+
 				"alphanumeric characters or '-', start with a letter, and end with an "+
-				"alphanumeric character.", MaxClusterDomainPrefixLength)
+				"alphanumeric character", MaxClusterDomainPrefixLength)
 		}
 		return nil
 	}
 	return fmt.Errorf("can only validate strings, got '%v'", domainPrefix)
 }
 
-func ValidateHTTPProxy(val interface{}) error {
-	if httpProxy, ok := val.(string); ok {
-		if httpProxy == "" {
-			return nil
-		}
-		url, err := url.ParseRequestURI(httpProxy)
-		if err != nil {
-			return fmt.Errorf("Invalid http-proxy value '%s'", httpProxy)
+// validateProxyURL validates common proxy URL requirements
+func validateProxyURL(proxyURL string, proxyType string, expectedScheme string) error {
+	if strings.Contains(proxyURL, " ") {
+		return fmt.Errorf("invalid %s value: URL cannot contain spaces", proxyType)
+	}
+
+	if credValidation := helper.ValidateURLCredentials(proxyURL); credValidation.HasInvalid {
+		return fmt.Errorf("invalid %s value: %s contains invalid character '%c'",
+			proxyType, credValidation.Field, credValidation.InvalidChar)
+	}
+
+	parsedURL, err := url.Parse(proxyURL)
+	if err != nil {
+		return fmt.Errorf("invalid %s value: %v", proxyType, err)
+	}
+
+	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))
+	}
+
+	if parsedURL.Host == "" {
+		return fmt.Errorf("invalid %s value: host is missing", proxyType)
+	}
+
+	// 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)
+	}
+
+	// Validate hostname/IP and port
+	host, port, err := net.SplitHostPort(parsedURL.Host)
+	if err != nil {
+		// No port specified, just validate the host
+		host = parsedURL.Host
+	} else {
+		// Validate port range (url.Parse already validated it's numeric)
+		portNum, _ := strconv.Atoi(port)
+		if portNum < 1 || portNum > 65535 {
+			return fmt.Errorf("invalid %s value: port must be between 1 and 65535", proxyType)
 		}
-		if url.Scheme != "http" {
-			return errors.Errorf("%s", "Expected http-proxy to have an http:// scheme")
+	}
+
+	// Validate host is a valid hostname or IP address
+	if host != "" {
+		// Check if it's an IP address
+		if ip := net.ParseIP(host); ip == nil {
+			// Not an IP, check if it's a valid hostname
+			if strings.Contains(host, "/") {
+				return fmt.Errorf("invalid %s value: invalid host", proxyType)
+			}
 		}
+	}
+
+	return nil
+}
+
+func ValidateHTTPProxy(val interface{}) error {
+	httpProxy := val.(string)
+	if httpProxy == "" {
 		return nil
 	}
-	return fmt.Errorf("can only validate strings, got '%v'", val)
+	return validateProxyURL(httpProxy, "http-proxy", "http")
+}
+
+func ValidateHTTPSProxy(val interface{}) error {
+	httpsProxy := val.(string)
+	if httpsProxy == "" {
+		return nil
+	}
+	return validateProxyURL(httpsProxy, "https-proxy", "https")
 }
 
 func ValidateRegistryAdditionalCa(input map[string]string) error {
@@ -174,9 +232,9 @@ func ValidateAllowedRegistriesForImport(input interface{}) error {
 		registries = strings.Split(input.(string), ",")
 		for _, registry := range registries {
 			if !idRE.MatchString(registry) {
-				return fmt.Errorf("invalid identifier '%s' for 'allowed registries for import.' "+
+				return fmt.Errorf("invalid identifier '%s' for 'allowed registries for import'. "+
 					"Should be in a <registry>:<boolean> format. "+
-					"The boolean indicates whether the registry is secure or not.", registry)
+					"The boolean indicates whether the registry is secure or not", registry)
 			}
 		}
 	case reflect.Slice:
@@ -745,7 +803,7 @@ func ValidateSubnetsCount(multiAZ bool, privateLink bool, subnetsInputCount int)
 		if multiAZ {
 			azPrefix = "multi-AZ"
 		}
-		return fmt.Errorf("The number of subnets for a '%s' '%s' should be '%d', "+
+		return fmt.Errorf("the number of subnets for a '%s' '%s' should be '%d', "+
 			"instead received: '%d'", azPrefix, clusterPrefix, expected, subnetsInputCount)
 	}
 	return nil
@@ -755,10 +813,10 @@ func ValidateHostedClusterSubnets(awsClient aws.Client, isPrivate bool, subnetID
 	privateIngress bool) (int, error) {
 
 	if isPrivate && len(subnetIDs) < 1 {
-		return 0, fmt.Errorf("The number of subnets for a private hosted cluster should be at least one")
+		return 0, fmt.Errorf("the number of subnets for a private hosted cluster should be at least one")
 	}
 	if !isPrivate && len(subnetIDs) < 2 {
-		return 0, fmt.Errorf("The number of subnets for a public hosted cluster should be at least two")
+		return 0, fmt.Errorf("the number of subnets for a public hosted cluster should be at least two")
 	}
 	vpcSubnets, vpcSubnetsErr := awsClient.GetVPCSubnets(subnetIDs[0])
 	if vpcSubnetsErr != nil {
@@ -785,11 +843,11 @@ func ValidateHostedClusterSubnets(awsClient aws.Client, isPrivate bool, subnetID
 
 	if isPrivate && privateIngress {
 		if publicSubnetsCount > 0 {
-			return 0, fmt.Errorf("The number of public subnets for a private hosted cluster should be zero")
+			return 0, fmt.Errorf("the number of public subnets for a private hosted cluster should be zero")
 		}
 	} else {
 		if publicSubnetsCount == 0 {
-			return 0, fmt.Errorf("Must have at least one public subnet when not using both " +
+			return 0, fmt.Errorf("must have at least one public subnet when not using both " +
 				"'private API' and 'private ingress'")
 		}
 	}
@@ -1106,8 +1164,8 @@ func ValidateClaimValidationRules(input interface{}) error {
 		inputRules = strings.Split(input.(string), ",")
 		for _, inputRule := range inputRules {
 			if !idRE.MatchString(inputRule) {
-				return fmt.Errorf("invalid identifier '%s' for 'claim validation rule. '"+
-					"Should be in a <claim>:<required_value> format.", inputRule)
+				return fmt.Errorf("invalid identifier '%s' for 'claim validation rule'. "+
+					"Should be in a <claim>:<required_value> format", inputRule)
 			}
 		}
 	case reflect.Slice: