Repository: jhillyerd/enmime
Pull request #98 (2018-11-27 17:43:21)

Better character set detection behavior
Only attempt to detect character set on `text/*` parts

On failure to detect character set, fall back to part-specified character set (which is what was used prior to charset detection being introduced) and add a warning to the part rather than fail to parse the message.

dcormier: Related to issue #81 and the resolution of that issue in PR #87.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/20318353/badge)](https://coveralls.io/builds/20318353)

Coverage increased (+0.06%) to 86.794% when pulling **616eccf9e915e76b2720704a72275c8f73592e30 on dcormier:dc/charset** into **1b665ca8061634f9f84d86a79c8a5c76c496ec93 on jhillyerd:develop**.


Patch:
diff --git a/encode.go b/encode.go
index dd495f2..7934eb1 100644
--- a/encode.go
+++ b/encode.go
@@ -8,6 +8,7 @@ import (
 	"mime/quotedprintable"
 	"net/textproto"
 	"sort"
+	"time"
 
 	"github.com/jhillyerd/enmime/internal/coding"
 	"github.com/jhillyerd/enmime/internal/stringutil"
@@ -109,6 +110,9 @@ func (p *Part) setupMIMEHeaders() transferEncoding {
 		// Build disposition header.
 		param := make(map[string]string)
 		setParamValue(param, hpFilename, stringutil.ToASCII(p.FileName))
+		if !p.FileModDate.IsZero() {
+			setParamValue(param, hpModDate, p.FileModDate.Format(time.RFC822))
+		}
 		mt := mime.FormatMediaType(p.Disposition, param)
 		if mt == "" {
 			// There was an error, FormatMediaType couldn't encode the params.
diff --git a/header.go b/header.go
index 88872ef..30338b8 100644
--- a/header.go
+++ b/header.go
@@ -54,6 +54,7 @@ const (
 	hpFile     = "file"
 	hpFilename = "filename"
 	hpName     = "name"
+	hpModDate  = "modification-date"
 
 	utf8 = "utf-8"
 )
diff --git a/part.go b/part.go
index 907d524..d210a44 100644
--- a/part.go
+++ b/part.go
@@ -11,6 +11,7 @@ import (
 	"net/textproto"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/gogs/chardet"
 	"github.com/jhillyerd/enmime/internal/coding"
@@ -28,13 +29,14 @@ type Part struct {
 	NextSibling *Part                // NextSibling of this part.
 	Header      textproto.MIMEHeader // Header for this Part.
 
-	Boundary    string // Boundary marker used within this part.
-	ContentID   string // ContentID header for cid URL scheme.
-	ContentType string // ContentType header without parameters.
-	Disposition string // Content-Disposition header without parameters.
-	FileName    string // The file-name from disposition or type header.
-	Charset     string // The content charset encoding, may differ from charset in header.
-	OrigCharset string // The original content charset when a different charset was detected.
+	Boundary    string    // Boundary marker used within this part.
+	ContentID   string    // ContentID header for cid URL scheme.
+	ContentType string    // ContentType header without parameters.
+	Disposition string    // Content-Disposition header without parameters.
+	FileName    string    // The file-name from disposition or type header.
+	FileModDate time.Time // The modification date of the file.
+	Charset     string    // The content charset encoding, may differ from charset in header.
+	OrigCharset string    // The original content charset when a different charset was detected.
 
 	Errors   []*Error // Errors encountered while parsing this part.
 	Content  []byte   // Content after decoding, UTF-8 conversion if applicable.
@@ -153,6 +155,100 @@ func (p *Part) setupContentHeaders(mediaParams map[string]string) {
 	if p.Charset == "" {
 		p.Charset = mediaParams[hpCharset]
 	}
+	if p.FileModDate.IsZero() {
+		p.FileModDate, _ = time.Parse(time.RFC822, mediaParams[hpModDate])
+	}
+}
+
+// convertFromDetectedCharset attempts to detect the character set for the given part, and returns
+// an io.Reader that will convert from that charset to UTF-8. If the charset cannot be detected,
+// this method adds a warning to the part and automatically falls back to using
+// `convertFromStatedCharset` and returns the reader from that method.
+func (p *Part) convertFromDetectedCharset(r io.Reader) (io.Reader, error) {
+	// Attempt to detect character set from part content.
+	var cd *chardet.Detector
+	switch p.ContentType {
+	case "text/html":
+		cd = chardet.NewHtmlDetector()
+	default:
+		cd = chardet.NewTextDetector()
+	}
+
+	buf, err := ioutil.ReadAll(r)
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+
+	cs, err := cd.DetectBest(buf)
+	switch err {
+	case nil:
+		// Carry on
+	case chardet.NotDetectedError:
+		p.addWarning(ErrorCharsetDeclaration, "charset could not be detected: %v", err)
+	default:
+		return nil, errors.WithStack(err)
+	}
+
+	// Restore r.
+	r = bytes.NewReader(buf)
+
+	if cs != nil && cs.Confidence >= minCharsetConfidence {
+		// Confidence exceeded our threshold, use detected character set.
+		if p.Charset != "" && !strings.EqualFold(cs.Charset, p.Charset) {
+			p.addWarning(ErrorCharsetDeclaration,
+				"declared charset %q, detected %q, confidence %d",
+				p.Charset, cs.Charset, cs.Confidence)
+		}
+
+		reader, err := coding.NewCharsetReader(cs.Charset, r)
+		if err != nil {
+			// Failed to get a conversion reader.
+			p.addWarning(ErrorCharsetConversion, err.Error())
+		} else {
+			r = reader
+			p.OrigCharset = p.Charset
+			p.Charset = cs.Charset
+		}
+	} else {
+		// Low confidence, use declared character set.
+		r = p.convertFromStatedCharset(r)
+	}
+
+	return r, nil
+}
+
+// convertFromStatedCharset returns a reader that will convert from the charset specified for the
+// current `*Part` to UTF-8. In case of error, or an unhandled character set, a warning will be
+// added to the `*Part` and the original io.Reader will be returned.
+func (p *Part) convertFromStatedCharset(r io.Reader) io.Reader {
+	if p.Charset == "" {
+		// US-ASCII. Just read.
+		return r
+	}
+
+	reader, err := coding.NewCharsetReader(p.Charset, r)
+	if err != nil {
+		// Failed to get a conversion reader.
+		p.addWarning(ErrorCharsetConversion, "failed to get reader for charset %q: %v", p.Charset, err)
+	} else {
+		return reader
+	}
+
+	// Try to parse charset again here to see if we can salvage some badly formed
+	// ones like charset="charset=utf-8".
+	charsetp := strings.Split(p.Charset, "=")
+	if strings.ToLower(charsetp[0]) == "charset" && len(charsetp) > 1 {
+		p.Charset = charsetp[1]
+		reader, err = coding.NewCharsetReader(p.Charset, r)
+		if err != nil {
+			// Failed to get a conversion reader.
+			p.addWarning(ErrorCharsetConversion, "failed to get reader for charset %q: %v", p.Charset, err)
+		} else {
+			return reader
+		}
+	}
+
+	return r
 }
 
 // decodeContent performs transport decoding (base64, quoted-printable) and charset decoding,
@@ -184,63 +280,11 @@ func (p *Part) decodeContent(r io.Reader) error {
 			encoding)
 	}
 	// Build charset decoding reader.
-	if validEncoding && !detectAttachmentHeader(p.Header) {
-		// Attempt to detect character set from part content.
-		cd := chardet.NewTextDetector()
-		if p.ContentType == "text/html" {
-			cd = chardet.NewHtmlDetector()
-		}
-		buf, err := ioutil.ReadAll(contentReader)
-		if err != nil {
-			return errors.WithStack(err)
-		}
-		cs, err := cd.DetectBest(buf)
+	if validEncoding && strings.HasPrefix(p.ContentType, "text/") {
+		var err error
+		contentReader, err = p.convertFromDetectedCharset(contentReader)
 		if err != nil {
-			return errors.WithStack(err)
-		}
-		contentReader = bytes.NewReader(buf) // Restore contentReader.
-
-		if cs != nil && cs.Confidence >= minCharsetConfidence {
-			// Confidence exceeded our threshold, use detected character set.
-			if p.Charset != "" && !strings.EqualFold(cs.Charset, p.Charset) {
-				p.addWarning(ErrorCharsetDeclaration,
-					fmt.Sprintf("Part %s: declared charset %q, detected %q, confidence %d",
-						p.PartID, p.Charset, cs.Charset, cs.Confidence))
-			}
-			reader, err := coding.NewCharsetReader(cs.Charset, contentReader)
-			if err == nil {
-				contentReader = reader
-				p.OrigCharset = p.Charset
-				p.Charset = cs.Charset
-			} else {
-				// Failed to get a conversion reader.
-				p.addWarning(ErrorCharsetConversion, err.Error())
-			}
-		} else {
-			// Low confidence, use declared character set.
-			if p.Charset != "" {
-				reader, err := coding.NewCharsetReader(p.Charset, contentReader)
-				if err == nil {
-					contentReader = reader
-				} else {
-					// Try to parse charset again here to see if we can salvage some badly formed
-					// ones like charset="charset=utf-8".
-					charsetp := strings.Split(p.Charset, "=")
-					if strings.ToLower(charsetp[0]) == "charset" && len(charsetp) > 1 {
-						p.Charset = charsetp[1]
-						reader, err = coding.NewCharsetReader(p.Charset, contentReader)
-						if err == nil {
-							contentReader = reader
-						} else {
-							// Failed to get a conversion reader.
-							p.addWarning(ErrorCharsetConversion, err.Error())
-						}
-					} else {
-						// Failed to get a conversion reader.
-						p.addWarning(ErrorCharsetConversion, err.Error())
-					}
-				}
-			}
+			return err
 		}
 	}
 	// Decode and store content.


Test patch:
diff --git a/encode_test.go b/encode_test.go
index ca3ec9f..ff48df0 100644
--- a/encode_test.go
+++ b/encode_test.go
@@ -3,6 +3,7 @@ package enmime_test
 import (
 	"bytes"
 	"testing"
+	"time"
 
 	"github.com/jhillyerd/enmime"
 	"github.com/jhillyerd/enmime/internal/test"
@@ -37,6 +38,7 @@ func TestEncodePartDefaultHeaders(t *testing.T) {
 	p.ContentID = "mycontentid"
 	p.Disposition = "attachment"
 	p.FileName = "stuff.zip"
+	p.FileModDate, _ = time.Parse(time.RFC822, "01 Feb 03 04:05 GMT")
 	p.Content = []byte("ZIPZIPZIP")
 
 	b := &bytes.Buffer{}
@@ -54,6 +56,7 @@ func TestEncodePartQuotedHeaders(t *testing.T) {
 	p.ContentID = "mycontentid"
 	p.Disposition = "attachment"
 	p.FileName = `árvíztűrő "x" tükörfúrógép.zip`
+	p.FileModDate, _ = time.Parse(time.RFC822, "01 Feb 03 04:05 GMT")
 	p.Content = []byte("ZIPZIPZIP")
 
 	b := &bytes.Buffer{}
@@ -178,3 +181,17 @@ func TestEncodePartContentBinary(t *testing.T) {
 	}
 	test.DiffGolden(t, b.Bytes(), "testdata", "encode", "part-bin-content.golden")
 }
+
+func TestEncodeFileModDate(t *testing.T) {
+	p := enmime.NewPart("text/plain")
+	p.Content = []byte("¡Hola, señor! Welcome to MIME")
+	p.Disposition = "inline"
+	p.FileModDate, _ = time.Parse(time.RFC822, "01 Feb 03 04:05 GMT")
+
+	b := &bytes.Buffer{}
+	err := p.Encode(b)
+	if err != nil {
+		t.Fatal(err)
+	}
+	test.DiffGolden(t, b.Bytes(), "testdata", "encode", "part-file-mod-date.golden")
+}
diff --git a/part_test.go b/part_test.go
index d39a2d3..e93f1e5 100644
--- a/part_test.go
+++ b/part_test.go
@@ -817,3 +817,58 @@ func TestContentTypeParamUnquotedSpecial(t *testing.T) {
 	}
 	test.ComparePart(t, p, wantp)
 }
+
+func TestChardetFailure(t *testing.T) {
+	const expectedContent = "GIF89ad\x00\x04\x00\x80\x00\x00\x00f\xccf\xff\x99!\xf9\x04\x00\x00\x00\x00\x00,\x00\x00\x00\x00d\x00\x04\x00\x00\x02\x1a\x8c\x8f\xa9\xcb\xed\x0f\xa3\x9c\xb4\xda\xeb\x80\u07bc\xfb\x0f\x86\xe2H\x96扦\xea*\x16\x00;"
+
+	t.Run("text part", func(t *testing.T) {
+		r := test.OpenTestData("parts", "chardet-fail.raw")
+		p, err := enmime.ReadParts(r)
+		if err != nil {
+			t.Fatal(err)
+		}
+		wantp := &enmime.Part{
+			PartID:      "0",
+			ContentType: "text/plain",
+			ContentID:   "part3.E34FF3C4.059DAD00@example.com",
+			FileName:    "rzkly.txt",
+		}
+		test.ComparePart(t, p, wantp)
+		expected := enmime.ErrorCharsetDeclaration
+		satisfied := false
+		for _, perr := range p.Errors {
+			if perr.Name == expected {
+				satisfied = true
+				if perr.Severe {
+					t.Errorf("Expected Severe to be false, got true for %q", perr.Name)
+				}
+			}
+		}
+		if !satisfied {
+			t.Errorf(
+				"Did not find expected error on part. Expected %q, but had: %v",
+				expected,
+				p.Errors)
+		}
+		test.ContentEqualsString(t, p.Content, expectedContent)
+	})
+
+	t.Run("non-text part", func(t *testing.T) {
+		r := test.OpenTestData("parts", "chardet-fail-non-txt.raw")
+		p, err := enmime.ReadParts(r)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(p.Errors) > 0 {
+			t.Errorf("Errors encountered while processing part: %v", p.Errors)
+		}
+		wantp := &enmime.Part{
+			PartID:      "0",
+			ContentType: "image/gif",
+			ContentID:   "part3.E34FF3C4.059DAD00@example.com",
+			FileName:    "rzkly.gif",
+		}
+		test.ComparePart(t, p, wantp)
+		test.ContentEqualsString(t, p.Content, expectedContent)
+	})
+}
diff --git a/testdata/encode/part-default-headers.golden b/testdata/encode/part-default-headers.golden
index b34c422..72114b1 100644
--- a/testdata/encode/part-default-headers.golden
+++ b/testdata/encode/part-default-headers.golden
@@ -1,4 +1,5 @@
-Content-Disposition: attachment; filename=stuff.zip
+Content-Disposition: attachment; filename=stuff.zip; modification-date="01
+ Feb 03 04:05 GMT"
 Content-Id: <mycontentid>
 Content-Transfer-Encoding: base64
 Content-Type: application/zip; boundary=enmime-abcdefg0123456789;
diff --git a/testdata/encode/part-file-mod-date.golden b/testdata/encode/part-file-mod-date.golden
new file mode 100644
index 0000000..2e58916
--- /dev/null
+++ b/testdata/encode/part-file-mod-date.golden
@@ -0,0 +1,5 @@
+Content-Disposition: inline; modification-date="01 Feb 03 04:05 GMT"
+Content-Transfer-Encoding: quoted-printable
+Content-Type: text/plain; charset=utf-8
+
+=C2=A1Hola, se=C3=B1or! Welcome to MIME
diff --git a/testdata/encode/part-quoted-headers.golden b/testdata/encode/part-quoted-headers.golden
index 71cf847..34a4a5d 100644
--- a/testdata/encode/part-quoted-headers.golden
+++ b/testdata/encode/part-quoted-headers.golden
@@ -1,5 +1,5 @@
 Content-Disposition: attachment; filename="arvizturo \"x\"
- tukorfurogep.zip"
+ tukorfurogep.zip"; modification-date="01 Feb 03 04:05 GMT"
 Content-Id: <mycontentid>
 Content-Transfer-Encoding: base64
 Content-Type: application/zip; boundary=enmime-abcdefg0123456789;
diff --git a/testdata/parts/chardet-fail-non-txt.raw b/testdata/parts/chardet-fail-non-txt.raw
new file mode 100644
index 0000000..0e8bc0d
--- /dev/null
+++ b/testdata/parts/chardet-fail-non-txt.raw
@@ -0,0 +1,7 @@
+Content-Type: image/gif;
+	name="rzkly.gif"
+Content-Transfer-Encoding: base64
+Content-ID: <part3.E34FF3C4.059DAD00@example.com>
+
+R0lGODlhZAAEAIAAAABmzGb/mSH5BAAAAAAALAAAAABkAAQAAAIajI+py+0Po5y02uuA3rz7D4bi
+SJbmiabqKhYAOw==
diff --git a/testdata/parts/chardet-fail.raw b/testdata/parts/chardet-fail.raw
new file mode 100644
index 0000000..ae0bd91
--- /dev/null
+++ b/testdata/parts/chardet-fail.raw
@@ -0,0 +1,7 @@
+Content-Type: text/plain;
+	name="rzkly.txt"
+Content-Transfer-Encoding: base64
+Content-ID: <part3.E34FF3C4.059DAD00@example.com>
+
+R0lGODlhZAAEAIAAAABmzGb/mSH5BAAAAAAALAAAAABkAAQAAAIajI+py+0Po5y02uuA3rz7D4bi
+SJbmiabqKhYAOw==