From f9ed326d7246d5609b841b67f6ace74f56baeb08 Mon Sep 17 00:00:00 2001 From: Abhinav Raut Date: Wed, 20 Aug 2025 03:29:16 +0530 Subject: [PATCH 1/3] fix: handle message empty message ids in imap --- internal/inbox/channel/email/imap.go | 64 ++++++--- internal/inbox/channel/email/imap_test.go | 155 ++++++++++++++++++++++ 2 files changed, 203 insertions(+), 16 deletions(-) create mode 100644 internal/inbox/channel/email/imap_test.go diff --git a/internal/inbox/channel/email/imap.go b/internal/inbox/channel/email/imap.go index 0b3c7659..d3d2eda8 100644 --- a/internal/inbox/channel/email/imap.go +++ b/internal/inbox/channel/email/imap.go @@ -140,6 +140,7 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. headerAutoSubmitted, headerAutoreply, headerLibredeskLoopPrevention, + headerMessageID, }, }, }, @@ -147,10 +148,11 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. // Collect messages to process later. type msgData struct { - env *imap.Envelope - seqNum uint32 - autoReply bool - isLoop bool + env *imap.Envelope + seqNum uint32 + autoReply bool + isLoop bool + extractedMessageID string } var messages []msgData @@ -182,9 +184,10 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. } var ( - env *imap.Envelope - autoReply bool - isLoop bool + env *imap.Envelope + autoReply bool + isLoop bool + extractedMessageID string ) // Process all fetch items for the current message. for { @@ -215,6 +218,9 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. if isLoopMessage(envelope, inboxEmail) { isLoop = true } + + // Extract Message-Id from raw headers as fallback for problematic Message IDs + extractedMessageID = extractMessageIDFromHeaders(envelope) } // Envelope. @@ -223,12 +229,13 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. } } - // Skip if we couldn't get headers or envelope. + // Skip if we couldn't get the envelope. if env == nil { + e.lo.Warn("skipping message without envelope", "seq_num", msg.SeqNum, "inbox_id", e.Identifier()) continue } - messages = append(messages, msgData{env: env, seqNum: msg.SeqNum, autoReply: autoReply, isLoop: isLoop}) + messages = append(messages, msgData{env: env, seqNum: msg.SeqNum, autoReply: autoReply, isLoop: isLoop, extractedMessageID: extractedMessageID}) } // Now process each collected message. @@ -253,7 +260,7 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. } // Process the envelope. - if err := e.processEnvelope(ctx, client, msgData.env, msgData.seqNum, inboxID); err != nil && err != context.Canceled { + if err := e.processEnvelope(ctx, client, msgData.env, msgData.seqNum, inboxID, msgData.extractedMessageID); err != nil && err != context.Canceled { e.lo.Error("error processing envelope", "error", err) } } @@ -262,17 +269,32 @@ func (e *Email) fetchAndProcessMessages(ctx context.Context, client *imapclient. } // processEnvelope processes a single email envelope. -func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, env *imap.Envelope, seqNum uint32, inboxID int) error { +func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, env *imap.Envelope, seqNum uint32, inboxID int, extractedMessageID string) error { if len(env.From) == 0 { e.lo.Warn("no sender received for email", "message_id", env.MessageID) return nil } var fromAddress = strings.ToLower(env.From[0].Addr()) + // Determine final Message ID - prefer extracted from raw headers, fallback to IMAP-parsed + messageID := extractedMessageID + if messageID == "" { + messageID = env.MessageID + if messageID != "" { + e.lo.Debug("using IMAP-parsed Message-ID as fallback", "message_id", messageID, "subject", env.Subject, "from", fromAddress) + } + } + + // Drop message if we still don't have a valid Message ID + if messageID == "" { + e.lo.Error("dropping message: no valid Message-ID found in raw headers or IMAP parsing", "subject", env.Subject, "from", fromAddress) + return nil + } + // Check if the message already exists in the database; if it does, ignore it. - exists, err := e.messageStore.MessageExists(env.MessageID) + exists, err := e.messageStore.MessageExists(messageID) if err != nil { - e.lo.Error("error checking if message exists", "message_id", env.MessageID) + e.lo.Error("error checking if message exists", "message_id", messageID) return fmt.Errorf("checking if message exists in DB: %w", err) } if exists { @@ -291,7 +313,7 @@ func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, return nil } - e.lo.Debug("processing new incoming message", "message_id", env.MessageID, "subject", env.Subject, "from", fromAddress, "inbox_id", inboxID) + e.lo.Debug("processing new incoming message", "message_id", messageID, "subject", env.Subject, "from", fromAddress, "inbox_id", inboxID) // Make contact. firstName, lastName := getContactName(env.From[0]) @@ -350,7 +372,7 @@ func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, InboxID: inboxID, Status: models.MessageStatusReceived, Subject: env.Subject, - SourceID: null.StringFrom(env.MessageID), + SourceID: null.StringFrom(messageID), Meta: meta, }, Contact: contact, @@ -385,7 +407,7 @@ func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, } if fullItem, ok := fullFetchItem.(imapclient.FetchItemDataBodySection); ok { - e.lo.Debug("fetching full message body", "message_id", env.MessageID) + e.lo.Debug("fetching full message body", "message_id", messageID) return e.processFullMessage(fullItem, incomingMsg) } } @@ -534,3 +556,13 @@ func extractAllHTMLParts(part *enmime.Part) []string { return htmlParts } + +// extractMessageIDFromHeaders extracts and cleans the Message-ID from email headers. +// This function handles problematic Message IDs by extracting them from raw headers +// and cleaning them of angle brackets and whitespace. +func extractMessageIDFromHeaders(envelope *enmime.Envelope) string { + if rawMessageID := envelope.GetHeader(headerMessageID); rawMessageID != "" { + return strings.TrimSpace(strings.Trim(rawMessageID, "<>")) + } + return "" +} diff --git a/internal/inbox/channel/email/imap_test.go b/internal/inbox/channel/email/imap_test.go new file mode 100644 index 00000000..bce74242 --- /dev/null +++ b/internal/inbox/channel/email/imap_test.go @@ -0,0 +1,155 @@ +package email + +import ( + "bytes" + "os" + "testing" + + "github.com/jhillyerd/enmime" +) + +func TestExtractMessageIDFromHeaders(t *testing.T) { + tests := []struct { + name string + emlFile string + expectedResult string + description string + }{ + { + name: "Normal Message ID", + emlFile: "testdata/normal_message_id.eml", + expectedResult: "normal123@example.com", + description: "Standard Message-ID format should be extracted correctly", + }, + { + name: "Double @ Message ID", + emlFile: "testdata/double_at_message_id.eml", + expectedResult: "message123@username@example.org", + description: "Message-ID with multiple @ symbols should be extracted correctly", + }, + { + name: "Triple @ Message ID", + emlFile: "testdata/triple_at_message_id.eml", + expectedResult: "id@user@company@domain.com", + description: "Message-ID with three @ symbols should be extracted correctly", + }, + { + name: "Missing Message ID", + emlFile: "testdata/missing_message_id.eml", + expectedResult: "", + description: "Email without Message-ID header should return empty string", + }, + { + name: "Whitespace Message ID", + emlFile: "testdata/whitespace_message_id.eml", + expectedResult: "whitespace123@username@example.org", + description: "Message-ID with whitespace and multiple @ symbols should be cleaned and extracted", + }, + { + name: "No Brackets Message ID", + emlFile: "testdata/no_brackets_message_id.eml", + expectedResult: "nobrackets123@username@example.org", + description: "Message-ID without angle brackets should be extracted correctly", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Read the email file + emlData, err := os.ReadFile(tt.emlFile) + if err != nil { + t.Fatalf("Failed to read email file %s: %v", tt.emlFile, err) + } + + // Parse the email with enmime + envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) + if err != nil { + t.Fatalf("Failed to parse email: %v", err) + } + + // Test the actual function + result := extractMessageIDFromHeaders(envelope) + + // Verify the result + if result != tt.expectedResult { + t.Errorf("extractMessageIDFromHeaders() = %q, want %q\nDescription: %s", + result, tt.expectedResult, tt.description) + } + + // Log the raw header for debugging + rawHeader := envelope.GetHeader(headerMessageID) + t.Logf("Raw Message-ID header: %q", rawHeader) + t.Logf("Extracted Message-ID: %q", result) + }) + } +} + +func TestProblematicMessageIDScenarios(t *testing.T) { + // Test Message-ID with multiple @ symbols that cause go-imap parsing to fail + t.Run("Multiple @ Symbols in Message-ID", func(t *testing.T) { + emlData, err := os.ReadFile("testdata/double_at_message_id.eml") + if err != nil { + t.Fatalf("Failed to read email file: %v", err) + } + + envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) + if err != nil { + t.Fatalf("Failed to parse email: %v", err) + } + + result := extractMessageIDFromHeaders(envelope) + expected := "message123@username@example.org" + + if result != expected { + t.Errorf("Failed to extract Message-ID with multiple @ symbols. Got %q, want %q", result, expected) + } + + // Verify it's not empty (which would cause conversation corruption) + if result == "" { + t.Error("Extracted Message-ID is empty, this would cause conversation corruption") + } + }) + + // Test various Message-ID formatting edge cases + t.Run("Message-ID Formatting Edge Cases", func(t *testing.T) { + testCases := []struct { + file string + description string + }{ + {"testdata/triple_at_message_id.eml", "Message-ID with three @ symbols"}, + {"testdata/whitespace_message_id.eml", "Message-ID with extra whitespace"}, + {"testdata/no_brackets_message_id.eml", "Message-ID without angle brackets"}, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + emlData, err := os.ReadFile(tc.file) + if err != nil { + t.Fatalf("Failed to read email file %s: %v", tc.file, err) + } + + envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) + if err != nil { + t.Fatalf("Failed to parse email %s: %v", tc.file, err) + } + + result := extractMessageIDFromHeaders(envelope) + + // Ensure Message-ID extraction succeeds + if result == "" { + t.Errorf("Message-ID extraction failed for %s, got empty string", tc.file) + } + + // Ensure proper cleaning - no angle brackets or whitespace + if result != "" { + if result[0] == '<' || result[len(result)-1] == '>' { + t.Errorf("Message-ID still contains angle brackets: %q", result) + } + if result[0] == ' ' || result[len(result)-1] == ' ' { + t.Errorf("Message-ID still contains whitespace: %q", result) + } + } + }) + } + }) +} From 6bbfbe8cf634030fb63e53073d289cda1be3ab9e Mon Sep 17 00:00:00 2001 From: Abhinav Raut Date: Wed, 20 Aug 2025 04:18:37 +0530 Subject: [PATCH 2/3] add test cases for imap msg id parsing --- internal/inbox/channel/email/imap_test.go | 222 +++++++++------------- 1 file changed, 95 insertions(+), 127 deletions(-) diff --git a/internal/inbox/channel/email/imap_test.go b/internal/inbox/channel/email/imap_test.go index bce74242..ce49be9d 100644 --- a/internal/inbox/channel/email/imap_test.go +++ b/internal/inbox/channel/email/imap_test.go @@ -1,155 +1,123 @@ package email import ( - "bytes" - "os" + "strings" "testing" + "github.com/emersion/go-message/mail" "github.com/jhillyerd/enmime" ) -func TestExtractMessageIDFromHeaders(t *testing.T) { + +// TestGoIMAPMessageIDParsing shows how go-imap fails to parse malformed Message-IDs +// and demonstrates the fallback solution. +// go-imap uses mail.Header.MessageID() which strictly follows RFC 5322 and returns +// empty strings for Message-IDs with multiple @ symbols. +// +// This caused emails to be dropped since we require Message-IDs for deduplication. +// References: +// - https://community.mailcow.email/d/701-multiple-at-in-message-id/5 +// - https://github.com/emersion/go-message/issues/154#issuecomment-1425634946 +func TestGoIMAPMessageIDParsing(t *testing.T) { + testCases := []struct { + input string + expectedIMAP string + expectedFallback string + name string + }{ + {"", "normal@example.com", "normal@example.com", "normal message ID"}, + {"", "", "malformed@@example.com", "double @ - IMAP fails, fallback works"}, + {"<001c01d710db$a8137a50$f83a6ef0$@jones.smith@example.com>", "", "001c01d710db$a8137a50$f83a6ef0$@jones.smith@example.com", "mailcow-style - IMAP fails, fallback works"}, + {"", "", "test@@@domain.com", "triple @ - IMAP fails, fallback works"}, + {" ", "abc123@example.com", "abc123@example.com", "with whitespace - both handle correctly"}, + {"abc123@example.com", "", "abc123@example.com", "no angle brackets - IMAP fails, fallback works"}, + {"", "", "", "empty input"}, + {"<>", "", "", "empty brackets"}, + {"", "", "CAFnQjQFhY8z@mail.example.com@gateway.company.com", "gateway-style - IMAP fails, fallback works"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Test go-imap parsing behavior + var h mail.Header + h.Set("Message-Id", tc.input) + imapResult, _ := h.MessageID() + + if imapResult != tc.expectedIMAP { + t.Errorf("IMAP parsing of %q: expected %q, got %q", tc.input, tc.expectedIMAP, imapResult) + } + + // Test fallback solution + if tc.input != "" { + rawEmail := "From: test@example.com\nMessage-ID: " + tc.input + "\n\nBody" + envelope, err := enmime.ReadEnvelope(strings.NewReader(rawEmail)) + if err != nil { + t.Fatal(err) + } + + fallbackResult := extractMessageIDFromHeaders(envelope) + if fallbackResult != tc.expectedFallback { + t.Errorf("Fallback extraction of %q: expected %q, got %q", tc.input, tc.expectedFallback, fallbackResult) + } + + // Critical check: ensure fallback works when IMAP fails + if imapResult == "" && tc.expectedFallback != "" && fallbackResult == "" { + t.Errorf("CRITICAL: Both IMAP and fallback failed for %q - would drop email!", tc.input) + } + } + }) + } +} + + +// TestEdgeCasesMessageID tests additional edge cases for Message-ID extraction. +func TestEdgeCasesMessageID(t *testing.T) { tests := []struct { - name string - emlFile string - expectedResult string - description string + name string + email string + expected string }{ { - name: "Normal Message ID", - emlFile: "testdata/normal_message_id.eml", - expectedResult: "normal123@example.com", - description: "Standard Message-ID format should be extracted correctly", + name: "no Message-ID header", + email: `From: test@example.com +To: inbox@test.com +Subject: Test + +Body`, + expected: "", }, { - name: "Double @ Message ID", - emlFile: "testdata/double_at_message_id.eml", - expectedResult: "message123@username@example.org", - description: "Message-ID with multiple @ symbols should be extracted correctly", + name: "malformed header syntax", + email: `From: test@example.com +Message-ID: malformed-no-brackets@@domain.com +To: inbox@test.com + +Body`, + expected: "malformed-no-brackets@@domain.com", }, { - name: "Triple @ Message ID", - emlFile: "testdata/triple_at_message_id.eml", - expectedResult: "id@user@company@domain.com", - description: "Message-ID with three @ symbols should be extracted correctly", - }, - { - name: "Missing Message ID", - emlFile: "testdata/missing_message_id.eml", - expectedResult: "", - description: "Email without Message-ID header should return empty string", - }, - { - name: "Whitespace Message ID", - emlFile: "testdata/whitespace_message_id.eml", - expectedResult: "whitespace123@username@example.org", - description: "Message-ID with whitespace and multiple @ symbols should be cleaned and extracted", - }, - { - name: "No Brackets Message ID", - emlFile: "testdata/no_brackets_message_id.eml", - expectedResult: "nobrackets123@username@example.org", - description: "Message-ID without angle brackets should be extracted correctly", + name: "multiple Message-ID headers (first wins)", + email: `From: test@example.com +Message-ID: +Message-ID: +To: inbox@test.com + +Body`, + expected: "first@example.com", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Read the email file - emlData, err := os.ReadFile(tt.emlFile) + envelope, err := enmime.ReadEnvelope(strings.NewReader(tt.email)) if err != nil { - t.Fatalf("Failed to read email file %s: %v", tt.emlFile, err) + t.Fatal(err) } - // Parse the email with enmime - envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) - if err != nil { - t.Fatalf("Failed to parse email: %v", err) - } - - // Test the actual function result := extractMessageIDFromHeaders(envelope) - - // Verify the result - if result != tt.expectedResult { - t.Errorf("extractMessageIDFromHeaders() = %q, want %q\nDescription: %s", - result, tt.expectedResult, tt.description) + if result != tt.expected { + t.Errorf("Expected %q, got %q", tt.expected, result) } - - // Log the raw header for debugging - rawHeader := envelope.GetHeader(headerMessageID) - t.Logf("Raw Message-ID header: %q", rawHeader) - t.Logf("Extracted Message-ID: %q", result) }) } } - -func TestProblematicMessageIDScenarios(t *testing.T) { - // Test Message-ID with multiple @ symbols that cause go-imap parsing to fail - t.Run("Multiple @ Symbols in Message-ID", func(t *testing.T) { - emlData, err := os.ReadFile("testdata/double_at_message_id.eml") - if err != nil { - t.Fatalf("Failed to read email file: %v", err) - } - - envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) - if err != nil { - t.Fatalf("Failed to parse email: %v", err) - } - - result := extractMessageIDFromHeaders(envelope) - expected := "message123@username@example.org" - - if result != expected { - t.Errorf("Failed to extract Message-ID with multiple @ symbols. Got %q, want %q", result, expected) - } - - // Verify it's not empty (which would cause conversation corruption) - if result == "" { - t.Error("Extracted Message-ID is empty, this would cause conversation corruption") - } - }) - - // Test various Message-ID formatting edge cases - t.Run("Message-ID Formatting Edge Cases", func(t *testing.T) { - testCases := []struct { - file string - description string - }{ - {"testdata/triple_at_message_id.eml", "Message-ID with three @ symbols"}, - {"testdata/whitespace_message_id.eml", "Message-ID with extra whitespace"}, - {"testdata/no_brackets_message_id.eml", "Message-ID without angle brackets"}, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - emlData, err := os.ReadFile(tc.file) - if err != nil { - t.Fatalf("Failed to read email file %s: %v", tc.file, err) - } - - envelope, err := enmime.ReadEnvelope(bytes.NewReader(emlData)) - if err != nil { - t.Fatalf("Failed to parse email %s: %v", tc.file, err) - } - - result := extractMessageIDFromHeaders(envelope) - - // Ensure Message-ID extraction succeeds - if result == "" { - t.Errorf("Message-ID extraction failed for %s, got empty string", tc.file) - } - - // Ensure proper cleaning - no angle brackets or whitespace - if result != "" { - if result[0] == '<' || result[len(result)-1] == '>' { - t.Errorf("Message-ID still contains angle brackets: %q", result) - } - if result[0] == ' ' || result[len(result)-1] == ' ' { - t.Errorf("Message-ID still contains whitespace: %q", result) - } - } - }) - } - }) -} From 550a3fa801d1a0faed4d00fcfcc937f310f153bf Mon Sep 17 00:00:00 2001 From: Abhinav Raut Date: Wed, 20 Aug 2025 04:26:23 +0530 Subject: [PATCH 3/3] fix: update Message-ID determination logic to prefer IMAP-parsed IDs over raw headers --- internal/inbox/channel/email/imap.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/inbox/channel/email/imap.go b/internal/inbox/channel/email/imap.go index d3d2eda8..1920326b 100644 --- a/internal/inbox/channel/email/imap.go +++ b/internal/inbox/channel/email/imap.go @@ -276,18 +276,18 @@ func (e *Email) processEnvelope(ctx context.Context, client *imapclient.Client, } var fromAddress = strings.ToLower(env.From[0].Addr()) - // Determine final Message ID - prefer extracted from raw headers, fallback to IMAP-parsed - messageID := extractedMessageID + // Determine final Message ID - prefer IMAP-parsed, fallback to raw header extraction + messageID := env.MessageID if messageID == "" { - messageID = env.MessageID + messageID = extractedMessageID if messageID != "" { - e.lo.Debug("using IMAP-parsed Message-ID as fallback", "message_id", messageID, "subject", env.Subject, "from", fromAddress) + e.lo.Debug("using raw header Message-ID as fallback for malformed ID", "message_id", messageID, "subject", env.Subject, "from", fromAddress) } } // Drop message if we still don't have a valid Message ID if messageID == "" { - e.lo.Error("dropping message: no valid Message-ID found in raw headers or IMAP parsing", "subject", env.Subject, "from", fromAddress) + e.lo.Error("dropping message: no valid Message-ID found in IMAP parsing or raw headers", "subject", env.Subject, "from", fromAddress) return nil }