Repository: jhillyerd/enmime
Pull request #141 (2019-12-23 16:07:03)

Coverage and cleanup

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

Coverage increased (+0.9%) to 93.044% when pulling **1b72231352030caec1a32b39163f624f6fb728da on requaos:requaos/cover** into **3f81043bfbd775f04c25cb20d648788309253ef4 on jhillyerd:develop**.

requaos: @jhillyerd This also cleans up a fabricated error case that was designed to catch malformed header boundaries with the body in conjunction with an `UnreadByte()` error of `io.UnexpectedEOF`. Since Go 1.13 release, when we refactored this code, the UnreadByte error is gone and so we no longer need this custom error exchange between the boundaryreader and the other code. Plus, removing unreachable code increases coverage 👍 
requaos: @jhillyerd I guess this turned out to be a solid cleanup sweep. Getting that last 4+ percent is going to be quite challenging, so maybe sometime next year we can get around to it.
jhillyerd: Lot of nice simplifications in here, hopefully it makes the code easier to follow.

Patch:
diff --git a/detect.go b/detect.go
index 82bc220..12587d6 100644
--- a/detect.go
+++ b/detect.go
@@ -10,12 +10,10 @@ func detectMultipartMessage(root *Part) bool {
 	// Parse top-level multipart
 	ctype := root.Header.Get(hnContentType)
 	mediatype, _, _, err := ParseMediaType(ctype)
-	if err != nil {
-		return false
-	}
+
 	// According to rfc2046#section-5.1.7 all other multipart should
 	// be treated as multipart/mixed
-	return strings.HasPrefix(mediatype, ctMultipartPrefix)
+	return err == nil && strings.HasPrefix(mediatype, ctMultipartPrefix)
 }
 
 // detectAttachmentHeader returns true, if the given header defines an attachment. First it checks
@@ -48,13 +46,11 @@ func detectTextHeader(header textproto.MIMEHeader, emptyContentTypeIsText bool)
 		return true
 	}
 
-	mediatype, _, _, err := ParseMediaType(ctype)
-	if err != nil {
-		return false
-	}
-	switch mediatype {
-	case ctTextPlain, ctTextHTML:
-		return true
+	if mediatype, _, _, err := ParseMediaType(ctype); err == nil {
+		switch mediatype {
+		case ctTextPlain, ctTextHTML:
+			return true
+		}
 	}
 
 	return false
diff --git a/encode.go b/encode.go
index b1e90a2..2b408c8 100644
--- a/encode.go
+++ b/encode.go
@@ -103,12 +103,10 @@ func (p *Part) setupMIMEHeaders() transferEncoding {
 		setParamValue(param, hpCharset, p.Charset)
 		setParamValue(param, hpName, stringutil.ToASCII(p.FileName))
 		setParamValue(param, hpBoundary, p.Boundary)
-		mt := mime.FormatMediaType(p.ContentType, param)
-		if mt == "" {
-			// There was an error, FormatMediaType couldn't encode the params.
-			mt = p.ContentType
+		if mt := mime.FormatMediaType(p.ContentType, param); mt != "" {
+			p.ContentType = mt
 		}
-		p.Header.Set(hnContentType, mt)
+		p.Header.Set(hnContentType, p.ContentType)
 	}
 	if p.Disposition != "" {
 		// Build disposition header.
@@ -117,12 +115,10 @@ func (p *Part) setupMIMEHeaders() transferEncoding {
 		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.
-			mt = p.Disposition
+		if mt := mime.FormatMediaType(p.Disposition, param); mt != "" {
+			p.Disposition = mt
 		}
-		p.Header.Set(hnContentDisposition, mt)
+		p.Header.Set(hnContentDisposition, p.Disposition)
 	}
 	return cte
 }
diff --git a/envelope.go b/envelope.go
index f73e8a7..28273b3 100644
--- a/envelope.go
+++ b/envelope.go
@@ -111,23 +111,18 @@ func (e *Envelope) AddressList(key string) ([]*mail.Address, error) {
 	}
 
 	str := decodeToUTF8Base64Header(e.header.Get(key))
-	if str == "" {
-		return nil, mail.ErrHeaderNotPresent
-	}
 
 	// These statements are handy for debugging ParseAddressList errors
 	// fmt.Println("in:  ", m.header.Get(key))
 	// fmt.Println("out: ", str)
 	ret, err := mail.ParseAddressList(str)
-	switch {
-	case err == nil:
-		// carry on
-	case err.Error() == "mail: expected comma":
-		ret, err = mail.ParseAddressList(ensureCommaDelimitedAddresses(str))
-		if err != nil {
-			return nil, err
+	if err != nil {
+		switch err.Error() {
+		case "mail: expected comma":
+			return mail.ParseAddressList(ensureCommaDelimitedAddresses(str))
+		case "mail: no address":
+			return nil, mail.ErrHeaderNotPresent
 		}
-	default:
 		return nil, err
 	}
 	return ret, nil
@@ -203,13 +198,9 @@ func EnvelopeFromPart(root *Part) (*Envelope, error) {
 			"Message did not contain a text/plain part")
 		var err error
 		if e.Text, err = html2text.FromString(e.HTML); err != nil {
-			// Downcoversion shouldn't fail
-			e.Text = ""
+			e.Text = "" // Down-conversion shouldn't fail
 			p := e.Root.BreadthMatchFirst(matchHTMLBodyPart)
-			p.addError(
-				ErrorPlainTextFromHTML,
-				"Failed to downconvert HTML: %v",
-				err)
+			p.addError(ErrorPlainTextFromHTML, "Failed to downconvert HTML: %v", err)
 		}
 	}
 
diff --git a/header.go b/header.go
index a36a0af..09a4f30 100644
--- a/header.go
+++ b/header.go
@@ -3,9 +3,7 @@ package enmime
 import (
 	"bufio"
 	"bytes"
-	stderrors "errors"
 	"fmt"
-	"io"
 	"mime"
 	"net/textproto"
 	"strings"
@@ -62,8 +60,6 @@ const (
 	utf8 = "utf-8"
 )
 
-var errEmptyHeaderBlock = stderrors.New("empty header block")
-
 // AddressHeaders is the set of SMTP headers that contain email addresses, used by
 // Envelope.AddressList().  Key characters must be all lowercase.
 var AddressHeaders = map[string]bool{
@@ -99,14 +95,8 @@ func readHeader(r *bufio.Reader, p *Part) (textproto.MIMEHeader, error) {
 		// Pull out each line of the headers as a temporary slice s
 		s, err := tp.ReadLineBytes()
 		if err != nil {
-			cause := errors.Cause(err)
-			if cause == io.ErrUnexpectedEOF && buf.Len() == 0 {
-				return nil, errors.WithStack(errEmptyHeaderBlock)
-			} else if cause == io.EOF {
-				buf.Write([]byte{'\r', '\n'})
-				break
-			}
-			return nil, err
+			buf.Write([]byte{'\r', '\n'})
+			break
 		}
 		firstColon := bytes.IndexByte(s, ':')
 		firstSpace := bytes.IndexAny(s, " \t\n\r")
@@ -231,30 +221,12 @@ func quotedDisplayName(s string) string {
 //   * Repeating media parameters
 //   * Unquoted values in media parameters containing 'tspecials' characters
 func ParseMediaType(ctype string) (mtype string, params map[string]string, invalidParams []string, err error) {
-	mtype, params, err = mime.ParseMediaType(ctype)
+	mtype, params, err = mime.ParseMediaType(fixUnescapedQuotes(fixUnquotedSpecials(fixMangledMediaType(ctype, ";"))))
 	if err != nil {
 		if err.Error() == "mime: no media type" {
 			return "", nil, nil, nil
 		}
-		// Small hack to remove harmless charset duplicate params.
-		mctype := fixMangledMediaType(ctype, ";")
-		mtype, params, err = mime.ParseMediaType(mctype)
-		if err != nil {
-			// If the media parameter has special characters, ensure that
-			// it is quoted and that any existing quotes are escaped.
-			mtype, params, err = mime.ParseMediaType(fixUnescapedQuotes(fixUnquotedSpecials(mctype)))
-			if err != nil {
-				// Some badly formed media types forget to send ; between fields.
-				mctype := fixMangledMediaType(ctype, " ")
-				if strings.Contains(mctype, `name=""`) {
-					mctype = strings.Replace(mctype, `name=""`, `name=" "`, -1)
-				}
-				mtype, params, err = mime.ParseMediaType(mctype)
-				if err != nil {
-					return "", nil, nil, errors.WithStack(err)
-				}
-			}
-		}
+		return "", nil, nil, errors.WithStack(err)
 	}
 	if mtype == ctPlaceholder {
 		mtype = ""
@@ -572,10 +544,13 @@ func fixUnescapedQuotes(hvalue string) string {
 		// Check if only one quote was found in the string.
 		if closingQuote == startingQuote {
 			// Append the next chunk of params here in case of a semicolon mid string.
-			param = fmt.Sprintf("%s%s", param, params[i+1])
+			if len(params) > i+1 {
+				param = fmt.Sprintf("%s%s", param, params[i+1])
+			}
 			closingQuote = strings.LastIndexByte(param, '"')
 			i++
 			if closingQuote == startingQuote {
+				sb.WriteString("=\"\"")
 				return sb.String()
 			}
 		}
diff --git a/inspect.go b/inspect.go
index 7e0d371..591dbf0 100644
--- a/inspect.go
+++ b/inspect.go
@@ -5,11 +5,9 @@ import (
 	"bytes"
 	"fmt"
 	"io"
-	"mime"
 	"net/textproto"
 	"strings"
 
-	"github.com/jhillyerd/enmime/internal/coding"
 	"github.com/pkg/errors"
 )
 
@@ -123,16 +121,9 @@ func rfc2047recurse(s string) (string, error) {
 		return s, io.EOF
 	}
 
-	val, err := decodeHeaderWithError(s)
-	if err != nil {
-		return val, err
-	}
-	if val == s {
-		val, err = decodeHeaderWithError(fixRFC2047String(val))
-		if err != nil {
-			return val, err
-		}
-		if val == s {
+	var val string
+	if val = decodeHeader(s); val == s {
+		if val = decodeHeader(fixRFC2047String(val)); val == s {
 			return val, io.EOF
 		}
 	}
@@ -140,17 +131,6 @@ func rfc2047recurse(s string) (string, error) {
 	return val, nil
 }
 
-// decodeHeaderWithError decodes a single line (per RFC 2047) using Golang's mime.WordDecoder
-func decodeHeaderWithError(input string) (string, error) {
-	dec := new(mime.WordDecoder)
-	dec.CharsetReader = coding.NewCharsetReader
-	header, err := dec.DecodeHeader(input)
-	if err != nil {
-		return input, err
-	}
-	return header, nil
-}
-
 // fixRFC2047String removes the following characters from charset and encoding segments of an RFC2047 string:
 //  '\n', '\r' and ' '
 func fixRFC2047String(s string) string {
diff --git a/internal/coding/charsets.go b/internal/coding/charsets.go
index 2481ecf..403b048 100644
--- a/internal/coding/charsets.go
+++ b/internal/coding/charsets.go
@@ -270,9 +270,6 @@ func init() {
 // ConvertToUTF8String uses the provided charset to decode a slice of bytes into a normal
 // UTF-8 string.
 func ConvertToUTF8String(charset string, textBytes []byte) (string, error) {
-	if strings.ToLower(charset) == utf8 {
-		return string(textBytes), nil
-	}
 	csentry, ok := encodings[strings.ToLower(charset)]
 	if !ok {
 		return "", fmt.Errorf("Unsupported charset %q", charset)
diff --git a/internal/coding/idheader.go b/internal/coding/idheader.go
index 9db02d1..5293731 100644
--- a/internal/coding/idheader.go
+++ b/internal/coding/idheader.go
@@ -13,11 +13,10 @@ func FromIDHeader(v string) string {
 	}
 	v = strings.TrimLeft(v, "<")
 	v = strings.TrimRight(v, ">")
-	r, err := url.QueryUnescape(v)
-	if err != nil {
-		return v
+	if r, err := url.QueryUnescape(v); err == nil {
+		v = r
 	}
-	return r
+	return v
 }
 
 // ToIDHeader encodes a Content-ID or Message-ID header value (RFC 2392) from a utf-8 string.
diff --git a/internal/coding/quotedprint.go b/internal/coding/quotedprint.go
index bf861bd..80400d2 100644
--- a/internal/coding/quotedprint.go
+++ b/internal/coding/quotedprint.go
@@ -68,10 +68,7 @@ func validHexByte(b byte) bool {
 }
 
 func validHexBytes(v []byte) bool {
-	if len(v) < 1 {
-		return false
-	}
-	if v[0] == '\n' {
+	if len(v) > 0 && v[0] == '\n' {
 		// Soft line break.
 		return true
 	}
diff --git a/part.go b/part.go
index 18f2361..05a8ef3 100644
--- a/part.go
+++ b/part.go
@@ -4,7 +4,6 @@ import (
 	"bufio"
 	"bytes"
 	"encoding/base64"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"mime/quotedprintable"
@@ -120,11 +119,6 @@ func (p *Part) setupHeaders(r *bufio.Reader, defaultContentType string) error {
 	if err != nil {
 		return err
 	}
-	if mtype == "" && len(mparams) > 0 {
-		p.addWarning(
-			ErrorMissingContentType,
-			"Content-Type header has parameters but no content type")
-	}
 	for i := range minvalidParams {
 		p.addWarning(
 			ErrorMalformedHeader,
@@ -186,10 +180,8 @@ func (p *Part) convertFromDetectedCharset(r io.Reader) (io.Reader, error) {
 	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)
+		p.addWarning(ErrorCharsetDeclaration, "charset could not be detected: %v", err)
 	}
 
 	// Restore r.
@@ -207,11 +199,7 @@ func (p *Part) convertFromDetectedCharset(r io.Reader) (io.Reader, error) {
 			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 {
+	if reader, err := coding.NewCharsetReader(cs.Charset, r); err == nil {
 		r = reader
 		p.OrigCharset = p.Charset
 		p.Charset = cs.Charset
@@ -405,15 +393,7 @@ func parseParts(parent *Part, reader *bufio.Reader) error {
 		// Look for part header.
 		bbr := bufio.NewReader(br)
 		err = p.setupHeaders(bbr, "")
-		if errors.Cause(err) == errEmptyHeaderBlock {
-			// Empty header probably means the part didn't use the correct trailing "--" syntax to
-			// close its boundary.
-			if _, err = br.Next(); err != nil {
-				// The error is already wrapped with a stack, so only adding a message here.
-				// TODO: Once `errors` releases a version > v0.8.0, change to use errors.WithMessagef()
-				return errors.WithMessage(err, fmt.Sprintf("error at boundary %v", parent.Boundary))
-			}
-		} else if err != nil {
+		if err != nil {
 			return err
 		}
 		// Insert this Part into the MIME tree.


Test patch:
diff --git a/envelope_test.go b/envelope_test.go
index b25f70d..420f7d9 100644
--- a/envelope_test.go
+++ b/envelope_test.go
@@ -738,7 +738,7 @@ func TestEnvelopeAddressList(t *testing.T) {
 	e := &enmime.Envelope{}
 	_, err := e.AddressList("To")
 	if err == nil {
-		t.Error("AddressList(\"Subject\") should have returned err, got nil")
+		t.Error("AddressList(\"To\") should have returned err, got nil")
 	}
 
 	r := test.OpenTestData("mail", "qp-utf8-header.raw")
@@ -747,6 +747,11 @@ func TestEnvelopeAddressList(t *testing.T) {
 		t.Fatal("Failed to parse MIME:", err)
 	}
 
+	_, err = e.AddressList("BCC")
+	if err == nil {
+		t.Error("AddressList(\"BCC\") should have returned err, got nil")
+	}
+
 	_, err = e.AddressList("Subject")
 	if err == nil {
 		t.Error("AddressList(\"Subject\") should have returned err, got nil")
diff --git a/header_test.go b/header_test.go
index 1c9e740..f4978e7 100644
--- a/header_test.go
+++ b/header_test.go
@@ -179,6 +179,16 @@ func TestFixMangledMediaType(t *testing.T) {
 			sep:   ";",
 			want:  "text/html;charset=",
 		},
+		{
+			input: "text/;charset=",
+			sep:   ";",
+			want:  "text/plain;charset=",
+		},
+		{
+			input: "multipart/;charset=",
+			sep:   ";",
+			want:  "multipart/mixed;charset=",
+		},
 		{
 			input: "application/octet-stream;=?UTF-8?B?bmFtZT0iw7DCn8KUwoo=?=You've got a new voice miss call.msg",
 			sep:   ";",
@@ -199,6 +209,11 @@ func TestFixMangledMediaType(t *testing.T) {
 			sep:   ";",
 			want:  "one/two; name=\"file.two\"",
 		},
+		{
+			input: "one/; name=\"file.two\"; name=\"file.two\"",
+			sep:   ";",
+			want:  "application/octet-stream; name=\"file.two\"",
+		},
 		{
 			input: "application/octet-stream; =?UTF-8?B?bmFtZT3DsMKfwpTCii5tc2c=?=",
 			sep:   " ",
@@ -367,6 +382,14 @@ func TestFixUnEscapedQuotes(t *testing.T) {
 			input: "application/rtf; charset=iso-8859-1; name=\"\"V047411.rtf\".rtf\"",
 			want:  "application/rtf; charset=iso-8859-1; name=\"\\\"V047411.rtf\\\".rtf\"",
 		},
+		{
+			input: "application/octet-stream; param1=\"",
+			want:  "application/octet-stream; param1=\"\"",
+		},
+		{
+			input: "application/octet-stream; param1=\"\\\"\"",
+			want:  "application/octet-stream; param1=\"\\\"\"",
+		},
 		{
 			input: "application/rtf; charset=iso-8859-1; name=b\"V047411.rtf\".rtf",
 			want:  "application/rtf; charset=iso-8859-1; name=\"b\\\"V047411.rtf\\\".rtf\"",
diff --git a/internal/coding/charsets_test.go b/internal/coding/charsets_test.go
index 57a4ad2..9a732cb 100644
--- a/internal/coding/charsets_test.go
+++ b/internal/coding/charsets_test.go
@@ -60,6 +60,7 @@ func TestFindCharsetInHTML(t *testing.T) {
 		{`<meta charset=big5 other=value>`, "big5"},
 		{`<meta charset=us-ascii>`, "us-ascii"},
 		{`<meta charset=windows-1250/>`, "windows-1250"},
+		{`<meta>`, ""},
 	}
 
 	for _, tt := range ttable {
diff --git a/internal/coding/idheader_test.go b/internal/coding/idheader_test.go
index 51d9d1b..a099028 100644
--- a/internal/coding/idheader_test.go
+++ b/internal/coding/idheader_test.go
@@ -12,6 +12,7 @@ func TestFromIDHeader(t *testing.T) {
 	}{
 		{"", ""},
 		{"<>", ""},
+		{"<%🤯>", "%🤯"},
 		{"<foo@inbucket.org>", "foo@inbucket.org"},
 		{"<foo%25bar>", "foo%bar"},
 		{"foo+bar", "foo bar"},