Skip to content

Commit 2637639

Browse files
olucasfreitasolucasfreitas
committed
OCM-18900 | fix: Improve error message for proxy passwords with special characters
1 parent c08ef48 commit 2637639

File tree

4 files changed

+340
-5
lines changed

4 files changed

+340
-5
lines changed

‎pkg/interactive/validation.go‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,9 @@ func IsURLHttps(val interface{}) error {
8989
if err != nil {
9090
return err
9191
}
92+
if parsedUri == nil {
93+
return nil
94+
}
9295
if parsedUri.Scheme != helper.ProtocolHttps {
9396
return fmt.Errorf("Expect URL '%s' to use an 'https://' scheme", val.(string))
9497
}
@@ -106,8 +109,40 @@ func _isUrl(val interface{}) (*url.URL, error) {
106109
if s == "" {
107110
return nil, nil
108111
}
109-
parsedUri, err := url.ParseRequestURI(fmt.Sprintf("%v", val))
110-
return parsedUri, err
112+
113+
// Check for multiple @ symbols which indicates unencoded @ in password
114+
if strings.Count(s, "@") > 1 {
115+
return nil, fmt.Errorf("Invalid URL '%s': password contains special characters that need URL encoding", s)
116+
}
117+
118+
// Check if the URL contains user credentials with special characters before parsing
119+
if strings.Contains(s, "@") && strings.Contains(s, "://") {
120+
idx := strings.Index(s, "://")
121+
rest := s[idx+3:]
122+
atIdx := strings.LastIndex(rest, "@")
123+
if atIdx != -1 {
124+
userinfo := rest[:atIdx]
125+
if colonIdx := strings.Index(userinfo, ":"); colonIdx != -1 {
126+
password := userinfo[colonIdx+1:]
127+
// Check if password contains special characters that need encoding
128+
specialChars := "/:#?[]!$&'()*+,;="
129+
for _, char := range specialChars {
130+
if strings.ContainsRune(password, char) {
131+
return nil, fmt.Errorf("Invalid URL '%s': password contains special characters that need URL encoding", s)
132+
}
133+
}
134+
}
135+
}
136+
}
137+
138+
parsedUri, err := url.Parse(fmt.Sprintf("%v", val))
139+
if err != nil {
140+
return nil, err
141+
}
142+
if parsedUri.Scheme == "" || parsedUri.Host == "" {
143+
return nil, fmt.Errorf("URL must have a scheme and host")
144+
}
145+
return parsedUri, nil
111146
}
112147

113148
// IsCert validates whether the given filepath is a valid cert file

‎pkg/interactive/validation_test.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,131 @@ var _ = Describe("Validation", func() {
108108
})
109109
})
110110

111+
Context("IsURL", func() {
112+
It("accepts empty string", func() {
113+
err := IsURL("")
114+
Expect(err).NotTo(HaveOccurred())
115+
})
116+
117+
It("accepts nil", func() {
118+
err := IsURL(nil)
119+
Expect(err).NotTo(HaveOccurred())
120+
})
121+
122+
It("accepts valid http URL", func() {
123+
err := IsURL("http://example.com")
124+
Expect(err).NotTo(HaveOccurred())
125+
})
126+
127+
It("accepts valid https URL", func() {
128+
err := IsURL("https://example.com")
129+
Expect(err).NotTo(HaveOccurred())
130+
})
131+
132+
It("accepts URL with authentication", func() {
133+
err := IsURL("http://user:[email protected]:8080")
134+
Expect(err).NotTo(HaveOccurred())
135+
})
136+
137+
It("rejects URL with unencoded special characters in password", func() {
138+
err := IsURL("http://proxyuser:QvoZjyy/[email protected]:8080")
139+
Expect(err).To(HaveOccurred())
140+
})
141+
142+
It("accepts URL with percent-encoded special characters in password", func() {
143+
err := IsURL("http://proxyuser:QvoZjyy%[email protected]:8080")
144+
Expect(err).NotTo(HaveOccurred())
145+
})
146+
147+
It("rejects URL with unencoded @ in password", func() {
148+
err := IsURL("http://user:pass@[email protected]:8080")
149+
Expect(err).To(HaveOccurred())
150+
})
151+
152+
It("accepts URL with percent-encoded @ in password", func() {
153+
err := IsURL("http://user:pass%[email protected]:8080")
154+
Expect(err).NotTo(HaveOccurred())
155+
})
156+
157+
It("rejects URL with extra colon in password", func() {
158+
err := IsURL("http://user:pass:[email protected]:8080")
159+
Expect(err).To(HaveOccurred())
160+
})
161+
162+
It("accepts URL with percent-encoded colon in password", func() {
163+
err := IsURL("http://user:pass%[email protected]:8080")
164+
Expect(err).NotTo(HaveOccurred())
165+
})
166+
167+
It("rejects URL without scheme", func() {
168+
err := IsURL("example.com")
169+
Expect(err).To(HaveOccurred())
170+
Expect(err.Error()).To(ContainSubstring("URL must have a scheme and host"))
171+
})
172+
173+
It("rejects URL without host", func() {
174+
err := IsURL("http://")
175+
Expect(err).To(HaveOccurred())
176+
Expect(err.Error()).To(ContainSubstring("URL must have a scheme and host"))
177+
})
178+
179+
It("rejects non-string input", func() {
180+
err := IsURL(123)
181+
Expect(err).To(HaveOccurred())
182+
Expect(err.Error()).To(ContainSubstring("can only validate strings"))
183+
})
184+
})
185+
186+
Context("IsURLHttps", func() {
187+
It("accepts empty string", func() {
188+
err := IsURLHttps("")
189+
Expect(err).NotTo(HaveOccurred())
190+
})
191+
192+
It("accepts nil", func() {
193+
err := IsURLHttps(nil)
194+
Expect(err).NotTo(HaveOccurred())
195+
})
196+
197+
It("accepts valid https URL", func() {
198+
err := IsURLHttps("https://example.com")
199+
Expect(err).NotTo(HaveOccurred())
200+
})
201+
202+
It("accepts https URL with authentication", func() {
203+
err := IsURLHttps("https://user:[email protected]:8080")
204+
Expect(err).NotTo(HaveOccurred())
205+
})
206+
207+
It("rejects https URL with unencoded special characters in password", func() {
208+
err := IsURLHttps("https://proxyuser:QvoZjyy/[email protected]:8080")
209+
Expect(err).To(HaveOccurred())
210+
})
211+
212+
It("accepts https URL with percent-encoded special characters in password", func() {
213+
err := IsURLHttps("https://proxyuser:QvoZjyy%[email protected]:8080")
214+
Expect(err).NotTo(HaveOccurred())
215+
})
216+
217+
It("rejects http URL", func() {
218+
err := IsURLHttps("http://example.com")
219+
Expect(err).To(HaveOccurred())
220+
Expect(err.Error()).To(ContainSubstring("Expect URL 'http://example.com' to use an 'https://' scheme"))
221+
})
222+
223+
It("rejects URL without scheme", func() {
224+
err := IsURLHttps("example.com")
225+
Expect(err).To(HaveOccurred())
226+
Expect(err.Error()).To(ContainSubstring("URL must have a scheme and host"))
227+
})
228+
229+
It("rejects non-string input", func() {
230+
err := IsURLHttps(123)
231+
Expect(err).To(HaveOccurred())
232+
Expect(err.Error()).To(ContainSubstring("can only validate strings"))
233+
})
234+
})
235+
111236
})
112237

113238
var _ = Describe("SubnetsValidator", func() {

‎pkg/ocm/helpers.go‎

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,13 +139,89 @@ func ValidateHTTPProxy(val interface{}) error {
139139
if httpProxy == "" {
140140
return nil
141141
}
142-
url, err := url.ParseRequestURI(httpProxy)
142+
143+
// Check for spaces
144+
if strings.Contains(httpProxy, " ") {
145+
return fmt.Errorf("Invalid http-proxy value '%s': URL cannot contain spaces", httpProxy)
146+
}
147+
148+
// Check for multiple @ symbols which indicates unencoded @ in password
149+
if strings.Count(httpProxy, "@") > 1 {
150+
return fmt.Errorf("Invalid http-proxy value '%s': password contains special characters that need URL encoding", httpProxy)
151+
}
152+
153+
// Check if the URL contains user credentials with special characters before parsing
154+
if strings.Contains(httpProxy, "@") && strings.Contains(httpProxy, "://") {
155+
idx := strings.Index(httpProxy, "://")
156+
rest := httpProxy[idx+3:]
157+
atIdx := strings.LastIndex(rest, "@")
158+
if atIdx != -1 {
159+
userinfo := rest[:atIdx]
160+
if colonIdx := strings.Index(userinfo, ":"); colonIdx != -1 {
161+
password := userinfo[colonIdx+1:]
162+
// Check if password contains special characters that need encoding
163+
specialChars := "/:#?[]!$&'()*+,;="
164+
for _, char := range specialChars {
165+
if strings.ContainsRune(password, char) {
166+
return fmt.Errorf("Invalid http-proxy value '%s': password contains special characters that need URL encoding", httpProxy)
167+
}
168+
}
169+
}
170+
}
171+
}
172+
173+
parsedURL, err := url.Parse(httpProxy)
143174
if err != nil {
144-
return fmt.Errorf("Invalid http-proxy value '%s'", httpProxy)
175+
// Check for invalid port in error message
176+
if strings.Contains(err.Error(), "invalid port") {
177+
return fmt.Errorf("Invalid http-proxy value '%s': invalid port number", httpProxy)
178+
}
179+
return fmt.Errorf("Invalid http-proxy value '%s': %v", httpProxy, err)
145180
}
146-
if url.Scheme != "http" {
181+
182+
if parsedURL.Scheme == "" {
183+
return fmt.Errorf("Invalid http-proxy value '%s': URL scheme is missing (e.g., http://)", httpProxy)
184+
}
185+
if parsedURL.Scheme != "http" {
147186
return errors.Errorf("%s", "Expected http-proxy to have an http:// scheme")
148187
}
188+
189+
if parsedURL.Host == "" {
190+
return fmt.Errorf("Invalid http-proxy value '%s': host is missing", httpProxy)
191+
}
192+
193+
// Proxy URLs shouldn't have a path
194+
if parsedURL.Path != "" && parsedURL.Path != "/" {
195+
return fmt.Errorf("Invalid http-proxy value '%s': proxy URL should not contain a path", httpProxy)
196+
}
197+
198+
// Validate hostname/IP and port
199+
host, port, err := net.SplitHostPort(parsedURL.Host)
200+
if err != nil {
201+
// No port specified, just validate the host
202+
host = parsedURL.Host
203+
} else {
204+
// Validate port
205+
portNum, err := strconv.Atoi(port)
206+
if err != nil {
207+
return fmt.Errorf("Invalid http-proxy value '%s': invalid port number", httpProxy)
208+
}
209+
if portNum < 1 || portNum > 65535 {
210+
return fmt.Errorf("Invalid http-proxy value '%s': port must be between 1 and 65535", httpProxy)
211+
}
212+
}
213+
214+
// Validate host is a valid hostname or IP address
215+
if host != "" {
216+
// Check if it's an IP address
217+
if ip := net.ParseIP(host); ip == nil {
218+
// Not an IP, check if it's a valid hostname
219+
if strings.Contains(host, "/") {
220+
return fmt.Errorf("Invalid http-proxy value '%s': invalid host", httpProxy)
221+
}
222+
}
223+
}
224+
149225
return nil
150226
}
151227
return fmt.Errorf("can only validate strings, got '%v'", val)

‎pkg/ocm/helpers_test.go‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,3 +695,102 @@ var _ = Describe("GetAutoNodeRoleArn", func() {
695695
Expect(exists).To(BeTrue())
696696
})
697697
})
698+
699+
var _ = Describe("ValidateHTTPProxy", func() {
700+
It("accepts empty string", func() {
701+
err := ValidateHTTPProxy("")
702+
Expect(err).NotTo(HaveOccurred())
703+
})
704+
705+
It("accepts valid http proxy without authentication", func() {
706+
err := ValidateHTTPProxy("http://proxy.example.com:8080")
707+
Expect(err).NotTo(HaveOccurred())
708+
})
709+
710+
It("accepts valid http proxy with basic authentication", func() {
711+
err := ValidateHTTPProxy("http://user:[email protected]:8080")
712+
Expect(err).NotTo(HaveOccurred())
713+
})
714+
715+
It("rejects http proxy with unencoded forward slash in password and provides helpful error", func() {
716+
// This is the exact example from the bug report OCM-18900
717+
err := ValidateHTTPProxy("http://proxyuser:QvoZjyy/[email protected]:8080")
718+
Expect(err).To(HaveOccurred())
719+
Expect(err.Error()).To(ContainSubstring("password contains special characters that need URL encoding"))
720+
})
721+
722+
It("accepts http proxy with percent-encoded forward slash in password", func() {
723+
err := ValidateHTTPProxy("http://proxyuser:QvoZjyy%[email protected]:8080")
724+
Expect(err).NotTo(HaveOccurred())
725+
})
726+
727+
It("rejects http proxy with unencoded @ in password and provides helpful error", func() {
728+
err := ValidateHTTPProxy("http://user:pass@[email protected]:8080")
729+
Expect(err).To(HaveOccurred())
730+
Expect(err.Error()).To(ContainSubstring("Invalid http-proxy value"))
731+
})
732+
733+
It("accepts http proxy with percent-encoded @ in password", func() {
734+
err := ValidateHTTPProxy("http://user:pass%[email protected]:8080")
735+
Expect(err).NotTo(HaveOccurred())
736+
})
737+
738+
It("rejects http proxy with unencoded special characters in password", func() {
739+
err := ValidateHTTPProxy("http://user:pass#[email protected]:8080")
740+
Expect(err).To(HaveOccurred())
741+
Expect(err.Error()).To(ContainSubstring("password contains special characters that need URL encoding"))
742+
})
743+
744+
It("accepts http proxy with percent-encoded colon in password", func() {
745+
err := ValidateHTTPProxy("http://user:pass%[email protected]:8080")
746+
Expect(err).NotTo(HaveOccurred())
747+
})
748+
749+
It("rejects proxy with https scheme", func() {
750+
err := ValidateHTTPProxy("https://proxy.example.com:8080")
751+
Expect(err).To(HaveOccurred())
752+
Expect(err.Error()).To(Equal("Expected http-proxy to have an http:// scheme"))
753+
})
754+
755+
It("rejects proxy without scheme", func() {
756+
err := ValidateHTTPProxy("proxy.example.com:8080")
757+
Expect(err).To(HaveOccurred())
758+
Expect(err.Error()).To(ContainSubstring("Expected http-proxy to have an http:// scheme"))
759+
})
760+
761+
It("rejects proxy without host", func() {
762+
err := ValidateHTTPProxy("http://")
763+
Expect(err).To(HaveOccurred())
764+
Expect(err.Error()).To(ContainSubstring("host is missing"))
765+
})
766+
767+
It("rejects non-string input", func() {
768+
err := ValidateHTTPProxy(123)
769+
Expect(err).To(HaveOccurred())
770+
Expect(err.Error()).To(ContainSubstring("can only validate strings"))
771+
})
772+
773+
It("rejects proxy URL with spaces", func() {
774+
err := ValidateHTTPProxy("http://proxy.example.com :8080")
775+
Expect(err).To(HaveOccurred())
776+
Expect(err.Error()).To(ContainSubstring("URL cannot contain spaces"))
777+
})
778+
779+
It("rejects proxy with invalid port", func() {
780+
err := ValidateHTTPProxy("http://proxy.example.com:99999")
781+
Expect(err).To(HaveOccurred())
782+
Expect(err.Error()).To(ContainSubstring("port must be between 1 and 65535"))
783+
})
784+
785+
It("rejects proxy with invalid IP and port", func() {
786+
err := ValidateHTTPProxy("http://10/.0.0.161:9999999")
787+
Expect(err).To(HaveOccurred())
788+
Expect(err.Error()).To(ContainSubstring("proxy URL should not contain a path"))
789+
})
790+
791+
It("rejects proxy URL with path", func() {
792+
err := ValidateHTTPProxy("http://proxy.example.com:8080/path")
793+
Expect(err).To(HaveOccurred())
794+
Expect(err.Error()).To(ContainSubstring("proxy URL should not contain a path"))
795+
})
796+
})

0 commit comments

Comments
 (0)