From 6ee9e78d1dc06879b10259d83c15ec2366ec4e2a Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 12 Jan 2025 11:40:16 +0100 Subject: [PATCH 1/5] Increase multipart writer array size in msgwriter.go Expanded the size of the multiPartWriter array from 3 to 4 to accommodate an additional writer. This adjustment is required for the S/MIME multipart writer to avoid and out of index access. --- msgwriter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msgwriter.go b/msgwriter.go index da784ef4..2cdd154e 100644 --- a/msgwriter.go +++ b/msgwriter.go @@ -57,7 +57,7 @@ type msgWriter struct { depth int8 encoder mime.WordEncoder err error - multiPartWriter [3]*multipart.Writer + multiPartWriter [4]*multipart.Writer partWriter io.Writer writer io.Writer } From 98b5a92834385c84ef113ab05316d09867a0ac18 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 12 Jan 2025 12:55:39 +0100 Subject: [PATCH 2/5] Refactor multipart boundary handling in message rendering Introduce reusable multiPartBoundary logic to ensure consistent boundary management across different MIME types. Replace redundant boundary generation in `msg.go` and centralize it via `getMultipartBoundary` and `startMP` methods in `msgwriter.go`. This improves maintainability and eliminates unnecessary recalculations. --- msg.go | 30 ++++++++++++------------------ msgwriter.go | 46 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/msg.go b/msg.go index e001d7ce..f746476f 100644 --- a/msg.go +++ b/msg.go @@ -120,7 +120,7 @@ type Msg struct { // This count can be helpful to identify where the mail header ends and the mail body starts headerCount uint - // isDelivered indicates wether the Msg has been delivered. + // isDelivered indicates whether the Msg has been delivered. isDelivered bool // middlewares is a slice of Middleware used for modifying or handling messages before they are processed. @@ -131,6 +131,10 @@ type Msg struct { // mimever represents the MIME version used in a Msg. mimever MIMEVersion + // multiPartBoundary holds the rendered boundary strings for consistent boundary rendering + // in case a Msg is rendered several times + multiPartBoundary map[MIMEType]string + // parts is a slice that holds pointers to Part structures, which represent different parts of a Msg. parts []*Part @@ -183,12 +187,13 @@ type MsgOption func(*Msg) // - https://datatracker.ietf.org/doc/html/rfc5321 func NewMsg(opts ...MsgOption) *Msg { msg := &Msg{ - addrHeader: make(map[AddrHeader][]*mail.Address), - charset: CharsetUTF8, - encoding: EncodingQP, - genHeader: make(map[Header][]string), - preformHeader: make(map[Header]string), - mimever: MIME10, + addrHeader: make(map[AddrHeader][]*mail.Address), + charset: CharsetUTF8, + encoding: EncodingQP, + genHeader: make(map[Header][]string), + preformHeader: make(map[Header]string), + multiPartBoundary: make(map[MIMEType]string), + mimever: MIME10, } // Override defaults with optionally provided MsgOption functions. @@ -3064,17 +3069,6 @@ func (m *Msg) signMessage() error { // the S/MIME signature buf := bytes.NewBuffer(nil) mw := &msgWriter{writer: buf, charset: m.charset, encoder: m.encoder} - - // If no boundary is set by the user, we set a fixed random boundary, so that - // the boundary of the parts is consistant during the different rendering - // phases - if m.boundary == "" { - boundary, err := randomBoundary() - if err != nil { - return fmt.Errorf("failed to generate random boundary: %w", err) - } - m.SetBoundary(boundary) - } mw.writeMsg(m) // Since we only want to sign the message body, we need to find the position within diff --git a/msgwriter.go b/msgwriter.go index 2cdd154e..26919d1a 100644 --- a/msgwriter.go +++ b/msgwriter.go @@ -140,20 +140,26 @@ func (mw *msgWriter) writeMsg(msg *Msg) { mw.writeString(DoubleNewLine) } if msg.hasMixed() { - mw.startMP(MIMEMixed, msg.boundary) - if mw.depth == 0 || (msg.hasSMIME() && mw.depth == 1) { + boundary := mw.getMultipartBoundary(msg, MIMEMixed) + boundary = mw.startMP(MIMEMixed, boundary) + msg.multiPartBoundary[MIMEMixed] = boundary + if mw.depth == 1 { mw.writeString(DoubleNewLine) } } if msg.hasRelated() { - mw.startMP(MIMERelated, msg.boundary) - if mw.depth == 0 || (msg.hasSMIME() && mw.depth == 1) { + boundary := mw.getMultipartBoundary(msg, MIMERelated) + boundary = mw.startMP(MIMERelated, boundary) + msg.multiPartBoundary[MIMERelated] = boundary + if mw.depth == 1 { mw.writeString(DoubleNewLine) } } if msg.hasAlt() { - mw.startMP(MIMEAlternative, msg.boundary) - if mw.depth == 0 || (msg.hasSMIME() && mw.depth == 1) { + boundary := mw.getMultipartBoundary(msg, MIMEAlternative) + boundary = mw.startMP(MIMEAlternative, boundary) + msg.multiPartBoundary[MIMEAlternative] = boundary + if mw.depth == 1 { mw.writeString(DoubleNewLine) } } @@ -247,9 +253,12 @@ func (mw *msgWriter) writePreformattedGenHeader(msg *Msg) { // - mimeType: The MIME type of the multipart content (e.g., "mixed", "alternative"). // - boundary: The boundary string separating different parts of the multipart message. // +// Returns: +// - The multipart boundary string +// // References: // - https://datatracker.ietf.org/doc/html/rfc2046 -func (mw *msgWriter) startMP(mimeType MIMEType, boundary string) { +func (mw *msgWriter) startMP(mimeType MIMEType, boundary string) string { multiPartWriter := multipart.NewWriter(mw) if boundary != "" { mw.err = multiPartWriter.SetBoundary(boundary) @@ -266,6 +275,7 @@ func (mw *msgWriter) startMP(mimeType MIMEType, boundary string) { mw.newPart(map[string][]string{"Content-Type": {contentType}}) } mw.depth++ + return multiPartWriter.Boundary() } // stopMP closes the multipart. @@ -279,6 +289,28 @@ func (mw *msgWriter) stopMP() { } } +// getMultipartBoundary returns the appropriate multipart boundary for the given MIME type. +// +// If the Msg has a predefined boundary, it is returned. Otherwise, the function checks +// for a MIME type-specific boundary in the Msg's multiPartBoundary map. If no boundary +// is found, an empty string is returned. +// +// Parameters: +// - msg: A pointer to the Msg containing the boundary and MIME type-specific mappings. +// - mimetype: The MIMEType for which the boundary is being determined. +// +// Returns: +// - A string representing the multipart boundary, or an empty string if none is found. +func (mw *msgWriter) getMultipartBoundary(msg *Msg, mimetype MIMEType) string { + if msg.boundary != "" { + return msg.boundary + } + if msg.multiPartBoundary[mimetype] != "" { + return msg.multiPartBoundary[mimetype] + } + return "" +} + // addFiles adds the attachments/embeds file content to the mail body. // // This function iterates through the list of files, setting necessary headers for each file, From bdf162cabac8a391da7d3e42c3fb724e80f97d37 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 12 Jan 2025 13:06:24 +0100 Subject: [PATCH 3/5] Update MIME boundary handling documentation Clarified guidelines on using predefined MIME boundaries. Added warnings about limitations when using predefined boundaries with multiple multipart parts, emphasizing potential issues with email client readability. --- msg.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/msg.go b/msg.go index f746476f..7a194d2c 100644 --- a/msg.go +++ b/msg.go @@ -278,9 +278,10 @@ func WithMIMEVersion(version MIMEVersion) MsgOption { // WithBoundary sets the boundary of a Msg to the provided string value during its creation or // initialization. // -// Note that by default, random MIME boundaries are created. This option should only be used if -// a specific boundary is required for the email message. Using a predefined boundary can be -// helpful when constructing multipart messages with specific formatting or content separation. +// NOTE: By default, random MIME boundaries are created. This option should only be used if +// a specific boundary is required for the email message. Using a predefined boundary will only +// work with messages that hold a single multipart part. Using a predefined boundary with several +// multipart parts will render the mail unreadable to the mail client. // // Parameters: // - boundary: The string value that specifies the desired boundary for the Msg. @@ -383,10 +384,12 @@ func (m *Msg) SetEncoding(encoding Encoding) { // // This method allows you to specify a custom boundary string for the MIME message. The // boundary is used to separate different parts of the message, especially when dealing -// with multipart messages. By default, the Msg generates random MIME boundaries. This -// function should only be used if you have a specific boundary requirement for the -// message. Ensure that the boundary value does not conflict with any content within the -// message to avoid parsing errors. +// with multipart messages. +// +// NOTE: By default, random MIME boundaries are created. This option should only be used if +// a specific boundary is required for the email message. Using a predefined boundary will only +// work with messages that hold a single multipart part. Using a predefined boundary with several +// multipart parts will render the mail unreadable to the mail client. // // Parameters: // - boundary: The string value representing the boundary to set for the Msg, used in @@ -1540,6 +1543,10 @@ func (m *Msg) GetAttachments() []*File { // particularly in multipart emails. The boundary helps to differentiate between various sections // such as plain text, HTML content, and attachments. // +// NOTE: By default, random MIME boundaries are created. Using a predefined boundary will only +// work with messages that hold a single multipart part. Using a predefined boundary with several +// multipart parts will render the mail unreadable to the mail client. +// // Returns: // - A string representing the boundary of the message. // From 5ebfd4771fa6114870893cad63be7dcbe5503ec5 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 12 Jan 2025 13:06:45 +0100 Subject: [PATCH 4/5] Fix error messages and remove redundant test for rand.Reader Updated error message expectations to reflect accurate details and improved clarity. Removed a redundant test for S/MIME signing with broken rand.Reader to streamline the test suite. --- msg_test.go | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/msg_test.go b/msg_test.go index fdcdd7b8..f14113c1 100644 --- a/msg_test.go +++ b/msg_test.go @@ -5932,9 +5932,9 @@ func TestMsg_WriteTo(t *testing.T) { buffer := bytes.NewBuffer(nil) _, err = message.WriteTo(buffer) if err == nil { - t.Error("expected WritoTo with invalid S/MIME private key to fail") + t.Error("expected WriteTo with invalid S/MIME private key to fail") } - expErr := "failed to generate random boundary: failed to read from rand.Reader: broken reader" + expErr := "failed to read from rand.Reader: broken reader" if !strings.EqualFold(err.Error(), expErr) { t.Errorf("expected S/MIME signing error to be: %s, got: %s", expErr, err) } @@ -7040,29 +7040,6 @@ func TestMsg_signMessage(t *testing.T) { t.Errorf("SMIME signing with invalid header count is expected to fail with %s, but got: %s", expErr, err) } }) - t.Run("signing fails with broken rand.Reader", func(t *testing.T) { - defaultRandReader := rand.Reader - t.Cleanup(func() { rand.Reader = defaultRandReader }) - rand.Reader = &randReader{failon: 1} - - keypair, err := getDummyKeyPairTLS() - if err != nil { - t.Fatalf("failed to load dummy crypto material: %s", err) - } - msg := testMessage(t) - if err = msg.SignWithTLSCertificate(keypair); err != nil { - t.Fatalf("failed to init SMIME configuration: %s", err) - } - msg.headerCount = 1000 - err = msg.signMessage() - if err == nil { - t.Error("SMIME signing with broken rand.Reader is expected to fail") - } - expErr := "failed to generate random boundary: failed to read from rand.Reader: broken reader" - if !strings.EqualFold(err.Error(), expErr) { - t.Errorf("SMIME signing with broken rand.Reader is expected to fail with %s, but got: %s", expErr, err) - } - }) } // uppercaseMiddleware is a middleware type that transforms the subject to uppercase. From ab6fc6076cf8e4d72a2297cc98064741f12c49af Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 12 Jan 2025 13:27:44 +0100 Subject: [PATCH 5/5] Refactor error assertion in S/MIME signing test Updated the test to check if the error message contains the expected substring instead of requiring an exact match. This makes the test more resilient to minor changes in error formatting. --- msg_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/msg_test.go b/msg_test.go index f14113c1..28335190 100644 --- a/msg_test.go +++ b/msg_test.go @@ -5934,9 +5934,9 @@ func TestMsg_WriteTo(t *testing.T) { if err == nil { t.Error("expected WriteTo with invalid S/MIME private key to fail") } - expErr := "failed to read from rand.Reader: broken reader" - if !strings.EqualFold(err.Error(), expErr) { - t.Errorf("expected S/MIME signing error to be: %s, got: %s", expErr, err) + expErr := "broken reader" + if !strings.Contains(err.Error(), expErr) { + t.Errorf("expected S/MIME signing error to contain: %q, got: %s", expErr, err) } }) }