Repository: microcosm-cc/bluemonday
Pull request #103 (2020-08-13 11:47:29)

Fix href sanitization bugs
This PR fixes two issues.

The first issue is that if you have two query params with the same name, only the last param is included in the sanitized result. See the following test case for an example.

```
    TestLinks: sanitize_test.go:171: test 9 failed;
        input   : <a href="?q=1&q=2">
        output  : <a href="?q=2" rel="nofollow">
        expected: <a href="?q=1&q=2" rel="nofollow">
```

The second issue this PR fixes is that characters inside the query params are HTML encoded. Take the following query.

?json=%7B%22value%22%3A%22a%22%7D

| key | value |
| ----|------|
| json | {"value":"a"} |

**After sanitization the query becomes the following, which is a bug as the query is no longer valid JSON.:**

?json=%7B%26%2334%3Bvalue%26%2334%3B%3A%26%2334%3Ba%26%2334%3B%7D

| key | value |
| ----|------|
| json | {&#x26;#34;value&#x26;#34;:&#x26;#34;a&#x26;#34;} |



Patch:
diff --git a/sanitize.go b/sanitize.go
index 771fc2c..103f39f 100644
--- a/sanitize.go
+++ b/sanitize.go
@@ -133,8 +133,8 @@ func sanitizedUrl(val string) (string, error) {
 	for k, vals := range queryValues {
 		sk := html.EscapeString(k)
 		for _, v := range vals {
-			sv := escapeUrlComponent(v)
-			sanitizedQueryValues.Set(sk, sv)
+			sv := v
+			sanitizedQueryValues.Add(sk, sv)
 		}
 	}
 	u.RawQuery = sanitizedQueryValues.Encode()
@@ -390,10 +390,10 @@ func (p *Policy) sanitizeAttrs(
 		hasStylePolicies = true
 	}
 	// no specific element policy found, look for a pattern match
-	if !hasStylePolicies{
-		for k, v := range p.elsMatchingAndStyles{
+	if !hasStylePolicies {
+		for k, v := range p.elsMatchingAndStyles {
 			if k.MatchString(elementName) {
-				if len(v) > 0{
+				if len(v) > 0 {
 					hasStylePolicies = true
 					break
 				}
@@ -669,14 +669,14 @@ func (p *Policy) sanitizeAttrs(
 
 func (p *Policy) sanitizeStyles(attr html.Attribute, elementName string) html.Attribute {
 	sps := p.elsAndStyles[elementName]
-	if len(sps) == 0{
+	if len(sps) == 0 {
 		sps = map[string]stylePolicy{}
 		// check for any matching elements, if we don't already have a policy found
 		// if multiple matches are found they will be overwritten, it's best
 		// to not have overlapping matchers
-		for regex, policies :=range p.elsMatchingAndStyles{
-			if regex.MatchString(elementName){
-				for k, v := range policies{
+		for regex, policies := range p.elsMatchingAndStyles {
+			if regex.MatchString(elementName) {
+				for k, v := range policies {
 					sps[k] = v
 				}
 			}
@@ -874,7 +874,7 @@ func removeUnicode(value string) string {
 	return substitutedValue
 }
 
-func (p *Policy) matchRegex(elementName string ) (map[string]attrPolicy, bool) {
+func (p *Policy) matchRegex(elementName string) (map[string]attrPolicy, bool) {
 	aps := make(map[string]attrPolicy, 0)
 	matched := false
 	for regex, attrs := range p.elsMatchingAndAttrs {


Test patch:
diff --git a/sanitize_test.go b/sanitize_test.go
index 3018efe..5f3c049 100644
--- a/sanitize_test.go
+++ b/sanitize_test.go
@@ -131,6 +131,14 @@ func TestLinks(t *testing.T) {
 			in:       `<a href="?q=1&r=2">`,
 			expected: `<a href="?q=1&r=2" rel="nofollow">`,
 		},
+		{
+			in:       `<a href="?q=1&q=2">`,
+			expected: `<a href="?q=1&q=2" rel="nofollow">`,
+		},
+		{
+			in:       `<a href="?q=%7B%22value%22%3A%22a%22%7D">`,
+			expected: `<a href="?q=%7B%22value%22%3A%22a%22%7D" rel="nofollow">`,
+		},
 		{
 			in:       `<a href="?q=1&r=2&s=:foo@">`,
 			expected: `<a href="?q=1&r=2&s=%3Afoo%40" rel="nofollow">`,