fix: use urlencode instead of htmlencode to sanitize url (#9538)

fix: use urlencode instead of html encode to validate url Signed-off-by: Tianchu Zhao <evantczhao@gmail.com> Signed-off-by: Tianchu Zhao <evantczhao@gmail.com> Co-authored-by: Saravanan Balasubramanian <33908564+sarabala1979@users.noreply.github.com>
argoproj · Sep 9, 2022 · ec7c210 · ec7c210
1 parent 3a3f159
commit ec7c210
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 18 deletions.
diff --git a/config/config.go b/config/config.go
@@ -2,7 +2,6 @@ package config
 
 import (
 	"fmt"
-	"html"
 	"math"
 	"net/url"
 	"time"
@@ -132,33 +131,28 @@ func (c Config) GetPodGCDeleteDelayDuration() time.Duration {
 	return c.PodGCDeleteDelayDuration.Duration
 }
 
-func (c Config) ValidateProtocol(inputURL string, allowedProtocol []string) error {
-	u, err := url.Parse(inputURL)
-	if err != nil {
-		return err
-	}
+func (c Config) ValidateProtocol(inputProtocol string, allowedProtocol []string) error {
 	for _, protocol := range allowedProtocol {
-		if u.Scheme == protocol {
+		if inputProtocol == protocol {
 			return nil
 		}
 	}
-	return fmt.Errorf("detect javascript link: %s", inputURL)
-}
-
-func (c Config) HTMLEscape(unescaped string) string {
-	return html.EscapeString(unescaped)
+	return fmt.Errorf("protocol %s is not allowed", inputProtocol)
 }
 
 func (c *Config) Sanitize(allowedProtocol []string) error {
 	links := c.Links
 
 	for _, link := range links {
-		err := c.ValidateProtocol(link.URL, allowedProtocol)
+		u, err := url.Parse(link.URL)
+		if err != nil {
+			return err
+		}
+		err = c.ValidateProtocol(u.Scheme, allowedProtocol)
 		if err != nil {
 			return err
-		} else {
-			link.URL = c.HTMLEscape(link.URL)
 		}
+		link.URL = u.String() // reassembles the URL into a valid URL string
 	}
 	return nil
 }

diff --git a/config/config_test.go b/config/config_test.go
@@ -18,8 +18,8 @@ func TestSanitize(t *testing.T) {
 		c   Config
 		err string
 	}{
-		{Config{Links: []*wfv1.Link{{URL: "javascript:foo"}}}, "detect javascript link: javascript:foo"},
-		{Config{Links: []*wfv1.Link{{URL: "javASCRipt: //foo"}}}, "detect javascript link: javASCRipt: //foo"},
+		{Config{Links: []*wfv1.Link{{URL: "javascript:foo"}}}, "protocol javascript is not allowed"},
+		{Config{Links: []*wfv1.Link{{URL: "javASCRipt: //foo"}}}, "protocol javascript is not allowed"},
 		{Config{Links: []*wfv1.Link{{URL: "http://foo.bar/?foo=<script>abc</script>bar"}}}, ""},
 	}
 	for _, tt := range tests {

diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go
@@ -82,7 +82,7 @@ func (s *ArgoServerSuite) TestInfo() {
 			Equal("workflow")
 		json.
 			Path("$.links[0].url").
-			Equal("http://logging-facility?namespace=${metadata.namespace}&amp;workflowName=${metadata.name}&amp;startedAt=${status.startedAt}&amp;finishedAt=${status.finishedAt}")
+			Equal("http://logging-facility?namespace=${metadata.namespace}&workflowName=${metadata.name}&startedAt=${status.startedAt}&finishedAt=${status.finishedAt}")
 	})
 }