Repository: jhillyerd/enmime
Pull request #97 (2018-11-26 19:51:05)

 Fix content-type params with unquoted specials per RFC 2045, section 5.1
This allowed handling Content-Type headers with parameters that contain unquoted special characters, like this:

	Content-Type: text/calendar; method=text/calendar

In that example, the `method` parameter contains a character that needs to be quoted (`/`), but is not.

This change reformats such parameters so they are quoted properly.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/20298122/badge)](https://coveralls.io/builds/20298122)

Coverage increased (+0.9%) to 86.728% when pulling **666606f30d43e40c8cd6dc155513dbd20ed4a380 on dcormier:dc/quote** into **4b02260e35dcdb464660222fe78272114888210a on jhillyerd:develop**.

dcormier: Yeah, it was a bigger project than I would've liked, given what the problem was. But this should be pretty resilient.

Code review comments addressed.
jhillyerd: Looks great, thanks

Patch:
diff --git a/header.go b/header.go
index 6759b66..88872ef 100644
--- a/header.go
+++ b/header.go
@@ -4,7 +4,6 @@ import (
 	"bufio"
 	"bytes"
 	stderrors "errors"
-	"fmt"
 	"io"
 	"mime"
 	"net/textproto"
@@ -270,16 +269,184 @@ func fixMangledMediaType(mtype, sep string) string {
 	return mtype
 }
 
-// fixUnquotedSpecials as defined in https://www.w3.org/Protocols/rfc1341/4_Content-Type.html
+// consumeParam takes the the parameter part of a Content-Type header, returns a clean version of
+// the first parameter (quoted as necessary), and the remainder of the parameter part of the
+// Content-Type header.
+//
+// Given this this header:
+//     `Content-Type: text/calendar; charset=utf-8; method=text/calendar`
+// `consumeParams` should be given this part:
+//     ` charset=utf-8; method=text/calendar`
+// And returns (first pass):
+//     `consumed = "charset=utf-8;"`
+//     `rest     = " method=text/calendar"`
+// Capture the `consumed` value (to build a clean Content-Type header value) and pass the value of
+// `rest` back to `consumeParam`. That second call will return:
+//     `consumed = " method=\"text/calendar\""`
+//     `rest     = ""`
+// Again, use the value of `consumed` to build a clean Content-Type header value. Given that `rest`
+// is empty, all of the parameters have been consumed successfully.
+//
+// If `consumed` is returned empty and `rest` is not empty, then the value of `rest` does not
+// begin with a parsable parameter. This does not necessarily indicate a problem. For example,
+// if there is trailing whitespace, it would be returned here.
+func consumeParam(s string) (consumed, rest string) {
+	i := strings.IndexByte(s, '=')
+	if i < 0 {
+		return "", s
+	}
+
+	param := strings.Builder{}
+	param.WriteString(s[:i+1])
+	s = s[i+1:]
+
+	value := strings.Builder{}
+	valueQuotedOriginally := false
+	valueQuoteAdded := false
+	valueQuoteNeeded := false
+
+	var r rune
+findValueStart:
+	for i, r = range s {
+		switch r {
+		case ' ', '\t':
+			param.WriteRune(r)
+
+		case '"':
+			valueQuotedOriginally = true
+			valueQuoteAdded = true
+			value.WriteRune(r)
+
+			break findValueStart
+
+		default:
+			valueQuotedOriginally = false
+			valueQuoteAdded = false
+			value.WriteRune(r)
+
+			break findValueStart
+		}
+	}
+
+	s = s[i+1:]
+
+	quoteIfUnquoted := func() {
+		if !valueQuoteNeeded {
+			if !valueQuoteAdded {
+				param.WriteByte('"')
+
+				valueQuoteAdded = true
+			}
+
+			valueQuoteNeeded = true
+		}
+	}
+
+findValueEnd:
+	for len(s) > 0 {
+		switch s[0] {
+		case ';', ' ', '\t':
+			if valueQuotedOriginally {
+				// We're in a quoted string, so whitespace is allowed.
+				value.WriteByte(s[0])
+				s = s[1:]
+				break
+			}
+
+			// Otherwise, we've reached the end of an unquoted value.
+
+			param.WriteString(value.String())
+			value.Reset()
+
+			if valueQuoteNeeded {
+				param.WriteByte('"')
+			}
+
+			param.WriteByte(s[0])
+			s = s[1:]
+
+			break findValueEnd
+
+		case '"':
+			if valueQuotedOriginally {
+				// We're in a quoted value. This is the end of that value.
+				param.WriteString(value.String())
+				value.Reset()
+
+				param.WriteByte(s[0])
+				s = s[1:]
+
+				break findValueEnd
+			}
+
+			quoteIfUnquoted()
+
+			value.WriteByte('\\')
+			value.WriteByte(s[0])
+			s = s[1:]
+
+		case '\\':
+			if len(s) > 1 {
+				value.WriteByte(s[0])
+				s = s[1:]
+
+				// Backslash escapes the next char. Consume that next char.
+				value.WriteByte(s[0])
+
+				quoteIfUnquoted()
+			}
+			// Else there is no next char to consume.
+			s = s[1:]
+
+		case '(', ')', '<', '>', '@', ',', ':', '/', '[', ']', '?', '=':
+			quoteIfUnquoted()
+
+			fallthrough
+
+		default:
+			value.WriteByte(s[0])
+			s = s[1:]
+		}
+	}
+
+	if value.Len() > 0 {
+		// There is a value that ends with the string. Capture it.
+		param.WriteString(value.String())
+
+		if !valueQuotedOriginally && valueQuoteNeeded {
+			param.WriteByte('"')
+		}
+	}
+
+	return param.String(), s
+}
+
+// fixUnquotedSpecials as defined in RFC 2045, section 5.1:
+// https://tools.ietf.org/html/rfc2045#section-5.1
 func fixUnquotedSpecials(s string) string {
-	if strings.Contains(s, "name=") {
-		nameSplit := strings.SplitAfter(s, "name=")
-		if strings.ContainsAny(nameSplit[1], "()<>@,;:\\/[]?.=") &&
-			!strings.HasSuffix(nameSplit[1], "\"") {
-			return fmt.Sprintf("%s\"%s\"", nameSplit[0], nameSplit[1])
+	idx := strings.IndexByte(s, ';')
+	if idx < 0 || idx == len(s) {
+		// No parameters
+		return s
+	}
+
+	clean := strings.Builder{}
+	clean.WriteString(s[:idx+1])
+	s = s[idx+1:]
+
+	for len(s) > 0 {
+		var consumed string
+		consumed, s = consumeParam(s)
+
+		if len(consumed) == 0 {
+			clean.WriteString(s)
+			return clean.String()
 		}
+
+		clean.WriteString(consumed)
 	}
-	return s
+
+	return clean.String()
 }
 
 // Detects a RFC-822 linear-white-space, passed to strings.FieldsFunc.


Test patch:
diff --git a/header_test.go b/header_test.go
index bac1a27..afa56a3 100644
--- a/header_test.go
+++ b/header_test.go
@@ -207,6 +207,105 @@ func TestFixMangledMediaType(t *testing.T) {
 	}
 }
 
+func TestFixUnquotedSpecials(t *testing.T) {
+	testCases := []struct {
+		input, want string
+	}{
+		{
+			input: "",
+			want:  "",
+		},
+		{
+			input: "application/octet-stream",
+			want:  "application/octet-stream",
+		},
+		{
+			input: "application/octet-stream;",
+			want:  "application/octet-stream;",
+		},
+		{
+			input: "application/octet-stream; param1=\"value1\"",
+			want:  "application/octet-stream; param1=\"value1\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"value1\"\\",
+			want:  "application/octet-stream; param1=\"value1\"\\",
+		},
+		{
+			input: "application/octet-stream; param1=value1",
+			want:  "application/octet-stream; param1=value1",
+		},
+		{
+			input: "application/octet-stream; param1=value1\\",
+			want:  "application/octet-stream; param1=value1",
+		},
+		{
+			input: "application/octet-stream; param1=value1\\\"",
+			want:  "application/octet-stream; param1=\"value1\\\"\"",
+		},
+		{
+			input: "application/octet-stream; param1=value\"1\"",
+			want:  "application/octet-stream; param1=\"value\\\"1\\\"\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"value\\\"1\\\"\"",
+			want:  "application/octet-stream; param1=\"value\\\"1\\\"\"",
+		},
+		{
+			input: "application/octet-stream; param1= value1",
+			want:  "application/octet-stream; param1= value1",
+		},
+		{
+			input: "application/octet-stream; param1=\tvalue1",
+			want:  "application/octet-stream; param1=\tvalue1",
+		},
+		{
+			input: "application/octet-stream; param1=\"value1;\"",
+			want:  "application/octet-stream; param1=\"value1;\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"value 1\"",
+			want:  "application/octet-stream; param1=\"value 1\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"value\t1\"",
+			want:  "application/octet-stream; param1=\"value\t1\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"value(1).pdf\"",
+			want:  "application/octet-stream; param1=\"value(1).pdf\"",
+		},
+		{
+			input: "application/octet-stream; param1=value(1).pdf",
+			want:  "application/octet-stream; param1=\"value(1).pdf\"",
+		},
+		{
+			input: "application/octet-stream; param1=value(1).pdf; param2=value(2).pdf",
+			want:  "application/octet-stream; param1=\"value(1).pdf\"; param2=\"value(2).pdf\"",
+		},
+		{
+			input: "application/octet-stream; param1=value(1).pdf;\tparam2=value2.pdf;",
+			want:  "application/octet-stream; param1=\"value(1).pdf\";\tparam2=value2.pdf;",
+		},
+		{
+			input: "application/octet-stream; param1=value(1).pdf;param2=value2.pdf;",
+			want:  "application/octet-stream; param1=\"value(1).pdf\";param2=value2.pdf;",
+		},
+		{
+			input: "application/octet-stream; param1=value/1",
+			want:  "application/octet-stream; param1=\"value/1\"",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.input, func(t *testing.T) {
+			got := fixUnquotedSpecials(tc.input)
+			if got != tc.want {
+				t.Errorf("\ngot:  %s\nwant: %s", got, tc.want)
+			}
+		})
+	}
+}
+
 func TestReadHeader(t *testing.T) {
 	prefix := "From: hooman\n \n being\n"
 	suffix := "Subject: hi\n\nPart body\n"
diff --git a/part_test.go b/part_test.go
index 679f9de..d39a2d3 100644
--- a/part_test.go
+++ b/part_test.go
@@ -748,29 +748,16 @@ func TestBarrenContentType(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	want := ""
-	if p.ContentType != want {
-		t.Errorf("ContentType %q, want %q", p.ContentType, want)
-	}
-	want = ""
-	if p.FileName != want {
-		t.Errorf("FileName %q, want %q", p.FileName, want)
-	}
-	want = ""
-	if p.Charset != want {
-		t.Errorf("Charset %q, want %q", p.Charset, want)
-	}
-	want = "attachment"
-	if p.Disposition != want {
-		t.Errorf("Disposition %q, want %q", p.Disposition, want)
-	}
-	want = ""
-	if p.ContentID != want {
-		t.Errorf("ContentID %q, want %q", p.ContentID, want)
+	wantp := &enmime.Part{
+		PartID:      "0",
+		Disposition: "attachment",
 	}
+	test.ComparePart(t, p, wantp)
+
+	expected := enmime.ErrorMissingContentType
 	satisfied := false
 	for _, perr := range p.Errors {
-		if perr.Name == enmime.ErrorMissingContentType {
+		if perr.Name == expected {
 			satisfied = true
 			if perr.Severe {
 				t.Errorf("Expected Severe to be false, got true for %q", perr.Name)
@@ -780,7 +767,7 @@ func TestBarrenContentType(t *testing.T) {
 	if !satisfied {
 		t.Errorf(
 			"Did not find expected error on part. Expected %q, but had: %v",
-			enmime.ErrorMissingContentType,
+			expected,
 			p.Errors)
 	}
 }
@@ -789,16 +776,18 @@ func TestMalformedContentTypeParams(t *testing.T) {
 	r := test.OpenTestData("parts", "malformed-content-type-params.raw")
 	p, err := enmime.ReadParts(r)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("%+v", err)
 	}
 	wantp := &enmime.Part{
 		PartID:      "0",
 		ContentType: "text/html",
 	}
 	test.ComparePart(t, p, wantp)
+
+	expected := enmime.ErrorMalformedHeader
 	satisfied := false
 	for _, perr := range p.Errors {
-		if perr.Name == enmime.ErrorMalformedHeader {
+		if perr.Name == expected {
 			satisfied = true
 			if perr.Severe {
 				t.Errorf("Expected Severe to be false, got true for %q", perr.Name)
@@ -808,7 +797,23 @@ func TestMalformedContentTypeParams(t *testing.T) {
 	if !satisfied {
 		t.Errorf(
 			"Did not find expected error on part. Expected %q, but had: %v",
-			enmime.ErrorMalformedHeader,
+			expected,
 			p.Errors)
 	}
 }
+
+func TestContentTypeParamUnquotedSpecial(t *testing.T) {
+	r := test.OpenTestData("parts", "unquoted-ctype-param-special.raw")
+	p, err := enmime.ReadParts(r)
+	if err != nil {
+		t.Fatalf("%+v", err)
+	}
+
+	wantp := &enmime.Part{
+		PartID:      "0",
+		ContentType: "text/calendar",
+		Disposition: "attachment",
+		FileName:    "calendar.ics",
+	}
+	test.ComparePart(t, p, wantp)
+}
diff --git a/testdata/parts/unquoted-ctype-param-special.raw b/testdata/parts/unquoted-ctype-param-special.raw
new file mode 100644
index 0000000..47552cb
--- /dev/null
+++ b/testdata/parts/unquoted-ctype-param-special.raw
@@ -0,0 +1,3 @@
+Content-Type: text/calendar; method=text/calendar
+Content-Disposition: attachment; filename=calendar.ics
+