Merge pull request #105 from lunny/lunny/fix_sanitizedURL

Fix the bug that sanitizedUrl will change the order of the link

sanitize.go

@@ -122,22 +122,79 @@ func escapeUrlComponent(val string) string {
 	return w.String()
 }
 
-func sanitizedUrl(val string) (string, error) {
+// Query represents a query
+type Query struct {
+	Key   string
+	Value string
+}
+
+func parseQuery(query string) (values []Query, err error) {
+	for query != "" {
+		key := query
+		if i := strings.IndexAny(key, "&;"); i >= 0 {
+			key, query = key[:i], key[i+1:]
+		} else {
+			query = ""
+		}
+		if key == "" {
+			continue
+		}
+		value := ""
+		if i := strings.Index(key, "="); i >= 0 {
+			key, value = key[:i], key[i+1:]
+		}
+		key, err1 := url.QueryUnescape(key)
+		if err1 != nil {
+			if err == nil {
+				err = err1
+			}
+			continue
+		}
+		value, err1 = url.QueryUnescape(value)
+		if err1 != nil {
+			if err == nil {
+				err = err1
+			}
+			continue
+		}
+		values = append(values, Query{
+			Key:   key,
+			Value: value,
+		})
+	}
+	return values, err
+}
+
+func encodeQueries(queries []Query) string {
+	var b strings.Builder
+	for i, query := range queries {
+		b.WriteString(url.QueryEscape(query.Key))
+		b.WriteString("=")
+		b.WriteString(url.QueryEscape(query.Value))
+		if i < len(queries)-1 {
+			b.WriteString("&")
+		}
+	}
+	return b.String()
+}
+
+func sanitizedURL(val string) (string, error) {
 	u, err := url.Parse(val)
 	if err != nil {
 		return "", err
 	}
+
+	// we use parseQuery but not u.Query to keep the order not change because
+	// url.Values is a map which has a random order.
+	queryValues, err := parseQuery(u.RawQuery)
+	if err != nil {
+		return "", err
+	}
 	// sanitize the url query params
-	sanitizedQueryValues := make(url.Values, 0)
-	queryValues := u.Query()
-	for k, vals := range queryValues {
-		sk := html.EscapeString(k)
-		for _, v := range vals {
-			sv := v
-			sanitizedQueryValues.Add(sk, sv)
-		}
+	for i, query := range queryValues {
+		queryValues[i].Key = html.EscapeString(query.Key)
 	}
-	u.RawQuery = sanitizedQueryValues.Encode()
+	u.RawQuery = encodeQueries(queryValues)
 	// u.String() will also sanitize host/scheme/user/pass
 	return u.String(), nil
 }
@@ -158,7 +215,7 @@ func (p *Policy) writeLinkableBuf(buff *bytes.Buffer, token *html.Token) {
 				tokenBuff.WriteString(html.EscapeString(attr.Val))
 				continue
 			}
-			u, err := sanitizedUrl(u)
+			u, err := sanitizedURL(u)
 			if err == nil {
 				tokenBuff.WriteString(u)
 			} else {

sanitize_test.go

@@ -1679,7 +1679,41 @@ func TestIssue85NoReferrer(t *testing.T) {
 	wg.Wait()
 }
 
+func TestSanitizedURL(t *testing.T) {
+	tests := []test{
+		{
+			in:       `http://abc.com?d=1&a=2&a=3`,
+			expected: `http://abc.com?d=1&a=2&a=3`,
+		},
+		{
+			in:       `http://abc.com?d=1 2&a=2&a=3`,
+			expected: `http://abc.com?d=1+2&a=2&a=3`,
+		},
+		{
+			in:       `http://abc.com?d=1/2&a=2&a=3`,
+			expected: `http://abc.com?d=1%2F2&a=2&a=3`,
+		},
+		{
+			in:       `http://abc.com?<d>=1&a=2&a=3`,
+			expected: `http://abc.com?%26lt%3Bd%26gt%3B=1&a=2&a=3`,
+		},
+	}
 
+	for _, theTest := range tests {
+		res, err := sanitizedURL(theTest.in)
+		if err != nil {
+			t.Errorf("sanitizedURL returned error: %v", err)
+		}
+		if theTest.expected != res {
+			t.Errorf(
+				"test failed;\ninput   : %s\nexpected: %s, actual: %s",
+				theTest.in,
+				theTest.expected,
+				res,
+			)
+		}
+	}
+}
 
 func TestIssue111ScriptTags(t *testing.T) {
 	p1 := NewPolicy()
@@ -1719,4 +1753,4 @@ func TestIssue111ScriptTags(t *testing.T) {
 			expected,
 		)
 	}
-}
+}