Repository: jhillyerd/enmime
Pull request #144 (2020-01-03 17:21:58)

Handle very empty MIME part
When one MIME boundary immediately follows another, treat it as an empty part.

This comes from an issue I saw in a production environment where there was an email where a MIME part delimiter was immediately followed by a terminator. Like this:

```txt
Content-type: multipart/mixed; boundary="the-boundary"

--the-boundary
Content-type: text/plain

Some content.

--the-boundary
--the-boundary--
```

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

Coverage decreased (-0.06%) to 95.463% when pulling **0dd3affc0bd06e67b89bc8f46c77b77cfe385682 on dcormier:dc/veryEmptyPart** into **7bc1c63531f87d742f365198938c884c023d3c17 on jhillyerd:develop**.

requaos: ```
		if b.atPartStart && n > 0 {
			b.atPartStart = false
		}
```
can be simplified to 
```
		b.atPartStart = b.atPartStart && n == 0
```
dcormier: Why not only set the variable only if it needs to be set?
requaos: The block doesn't ever evaluate to true at `boundary.go:77`, which makes `boundary.go:78` unreachable.
For this one block, if we change it to an assignment statement instead of an if-block, we get to keep the business logic without giving up the ~0.06% in code-coverage or readability.
dcormier: Yes, that would increase the covered code in the coverage report, but it would also hide the fact that there's a condition not exercised by the current test suite.

From what I've been able to tell, I don't _think_ there's a condition where it could get into that block. But I'm not 100% sure (and I have tried to write a test for it, just to be sure), and maybe something will change in the future that would cause that block to be exercised.

If you still want me to make the change to hide the untested condition, I'll go ahead and do that. I just wanted to make sure it was clear.
requaos: @dcormier Those are very good points. I have adjusted one of the boundaryReader tests to hit that block of code instead and opened a PR to your branch: [Test Cover](https://github.com/dcormier/enmime/pull/1)

Patch:
diff --git a/boundary.go b/boundary.go
index 19da4d9..13a6240 100644
--- a/boundary.go
+++ b/boundary.go
@@ -21,6 +21,7 @@ var errNoBoundaryTerminator = stderrors.New("expected boundary not present")
 type boundaryReader struct {
 	finished         bool          // No parts remain when finished
 	partsRead        int           // Number of parts read thus far
+	atPartStart      bool          // Whether the current part is at its beginning
 	r                *bufio.Reader // Source reader
 	nlPrefix         []byte        // NL + MIME boundary prefix
 	prefix           []byte        // MIME boundary prefix
@@ -72,10 +73,15 @@ func newBoundaryReader(reader *bufio.Reader, boundary string) *boundaryReader {
 func (b *boundaryReader) Read(dest []byte) (n int, err error) {
 	if b.buffer.Len() >= len(dest) {
 		// This read request can be satisfied entirely by the buffer.
-		return b.buffer.Read(dest)
+		n, err = b.buffer.Read(dest)
+		if b.atPartStart && n > 0 {
+			b.atPartStart = false
+		}
+
+		return n, err
 	}
 
-	for i := 0; i < cap(dest); i++ {
+	for i := 0; i < len(dest); i++ {
 		cs, err := b.r.Peek(1)
 		if err != nil && err != io.EOF {
 			return 0, errors.WithStack(err)
@@ -93,6 +99,14 @@ func (b *boundaryReader) Read(dest []byte) (n int, err error) {
 			// Check for line feed as potential LF boundary prefix.
 			case '\n':
 				check = true
+
+			default:
+				if b.atPartStart {
+					// If we're at the very beginning of the part (even before the headers),
+					// check to see if there's a delimiter that immediately follows.
+					padding = 0
+					check = true
+				}
 			}
 
 			if check {
@@ -113,6 +127,9 @@ func (b *boundaryReader) Read(dest []byte) (n int, err error) {
 						n, err = b.buffer.Read(dest)
 						switch err {
 						case nil, io.EOF:
+							if b.atPartStart && n > 0 {
+								b.atPartStart = false
+							}
 							return n, io.EOF
 						default:
 							return 0, errors.WithStack(err)
@@ -141,6 +158,9 @@ func (b *boundaryReader) Read(dest []byte) (n int, err error) {
 
 	// Read the contents of the buffer into the destination slice.
 	n, err = b.buffer.Read(dest)
+	if b.atPartStart && n > 0 {
+		b.atPartStart = false
+	}
 	return n, err
 }
 
@@ -169,6 +189,7 @@ func (b *boundaryReader) Next() (bool, error) {
 		if err != io.EOF && b.isDelimiter(line) {
 			// Start of a new part.
 			b.partsRead++
+			b.atPartStart = true
 			return true, nil
 		}
 		if err == io.EOF {


Test patch:
diff --git a/boundary_test.go b/boundary_test.go
index 2146836..54260ce 100644
--- a/boundary_test.go
+++ b/boundary_test.go
@@ -186,6 +186,21 @@ func TestBoundaryReaderParts(t *testing.T) {
 			boundary: "STOP",
 			parts:    []string{"part1", "part2"},
 		},
+		{
+			input:    "--STOP\npart1\n--STOP\n--STOP--\n",
+			boundary: "STOP",
+			parts:    []string{"part1", ""},
+		},
+		{
+			input:    "--STOP\n--STOP\npart2\n--STOP--\n",
+			boundary: "STOP",
+			parts:    []string{"", "part2"},
+		},
+		{
+			input:    "--STOP\n--STOP\n--STOP--\n",
+			boundary: "STOP",
+			parts:    []string{"", ""},
+		},
 	}
 
 	for _, tt := range ttable {
@@ -427,7 +442,8 @@ func TestBoundaryReaderReadErrors(t *testing.T) {
 	// Destination byte slice is shorter than buffer length
 	dest := make([]byte, 1)
 	br := &boundaryReader{
-		buffer: bytes.NewBuffer([]byte{'1', '2', '3'}),
+		buffer:      bytes.NewBuffer([]byte{'1', '2', '3'}),
+		atPartStart: true,
 	}
 	n, err := br.Read(dest)
 	if n != 1 {
@@ -436,6 +452,9 @@ func TestBoundaryReaderReadErrors(t *testing.T) {
 	if err != nil {
 		t.Fatal("Read() should not have returned an error, failed")
 	}
+	if br.atPartStart {
+		t.Fatal("Read() of non-zero length should have unset atStartPart boolean")
+	}
 
 	// Using bufio.Reader with a 0 length buffer will cause
 	// Peek method to return a non io.EOF error.
@@ -457,3 +476,47 @@ func TestBoundaryReaderReadErrors(t *testing.T) {
 		t.Fatal("Read() should have returned bufio.ErrBufferFull error, failed")
 	}
 }
+
+// TestReadLenNotCap checks that the `boundaryReader` `io.Reader` implementation fills the provided
+// slice based on its length (as per the `io.Reader` documentation), and not its capacity.
+func TestReadLenNotCap(t *testing.T) {
+	t.Parallel()
+
+	input := "--STOP\nabcdefghijklm\n--STOP\nnopqrstuvwxyz\n--STOP--\n"
+	boundary := "STOP"
+	parts := []string{"abcdefghijklm", "nopqrstuvwxyz"}
+
+	ir := bufio.NewReader(strings.NewReader(input))
+	br := newBoundaryReader(ir, boundary)
+
+	for i, want := range parts {
+		next, err := br.Next()
+		if err != nil {
+			t.Fatalf("Error %q on part %v, input %q", err, i, input)
+		}
+		if !next {
+			t.Fatal("Next() = false, want: true")
+		}
+
+		var out []byte
+		b := make([]byte, 6, 20) // Ensure the capacity is greater than the length.
+		max := len(b)
+		var c int
+		for err == nil {
+			c, err = br.Read(b)
+			if c > max {
+				t.Errorf("Per the docuemtation for io.Reader, should not have read more than %d bytes, but read %d", max, c)
+			}
+
+			out = append(out, b[0:c]...)
+		}
+
+		if err != io.EOF {
+			t.Errorf("Expected %v, but got: %+v", io.EOF, err)
+		}
+
+		if want != string(out) {
+			t.Errorf("Expected part to be read as %q, but got %q", want, out)
+		}
+	}
+}