Merge pull request #315 from abhinavxd/fix/quoted-inline-dedup

Fix quoted reply inline images returning no signed URL

cmd/messages.go

@@ -71,7 +71,8 @@ func handleGetMessages(r *fastglue.Request) error {
 			att := messages[i].Attachments[j]
 			messages[i].Attachments[j].URL = app.media.GetURL(att.UUID, att.ContentType, att.Name)
 		}
-		resolveContentCIDs(&messages[i], rootURL)
+		resolveQuotedCIDs(app, &messages[i])
+		resolveAttachmentCIDs(&messages[i], rootURL)
 	}
 
 	// Process CSAT status for all messages (will only affect CSAT messages)
@@ -134,7 +135,8 @@ func handleGetMessage(r *fastglue.Request) error {
 		att := message.Attachments[j]
 		message.Attachments[j].URL = app.media.GetURL(att.UUID, att.ContentType, att.Name)
 	}
-	resolveContentCIDs(&message, rootURL)
+	resolveQuotedCIDs(app, &message)
+	resolveAttachmentCIDs(&message, rootURL)
 
 	return r.SendEnvelope(message)
 }
@@ -269,9 +271,9 @@ func handleSendMessage(r *fastglue.Request) error {
 	return r.SendEnvelope(message)
 }
 
-// resolveContentCIDs replaces inline image cid: references in email message content
+// resolveAttachmentCIDs replaces inline image cid: references in email message content
 // with actual attachment URLs and resolves relative /uploads/ paths to absolute URLs.
-func resolveContentCIDs(msg *cmodels.Message, rootURL string) {
+func resolveAttachmentCIDs(msg *cmodels.Message, rootURL string) {
 	for _, att := range msg.Attachments {
 		if att.ContentID != "" && att.URL != "" {
 			msg.Content = strings.ReplaceAll(msg.Content, "cid:"+att.ContentID, att.URL)
@@ -282,3 +284,16 @@ func resolveContentCIDs(msg *cmodels.Message, rootURL string) {
 		msg.Content = strings.ReplaceAll(msg.Content, `src='/uploads/`, `src='`+rootURL+`/uploads/`)
 	}
 }
+
+// resolveQuotedCIDs replaces cid: refs to media on other messages with signed URLs.
+func resolveQuotedCIDs(app *App, msg *cmodels.Message) {
+	refs, err := app.conversation.GetInlineMediaRefs(msg)
+	if err != nil {
+		app.lo.Error("error fetching inline media refs", "conversation_uuid", msg.ConversationUUID, "error", err)
+		return
+	}
+	for _, ref := range refs {
+		url := app.media.GetURL(ref.UUID, ref.ContentType, ref.Filename)
+		msg.Content = strings.ReplaceAll(msg.Content, "cid:"+ref.ContentID, url)
+	}
+}

cmd/messages_test.go

@@ -7,7 +7,7 @@ import (
 	cmodels "github.com/abhinavxd/libredesk/internal/conversation/models"
 )
 
-func TestResolveContentCIDs(t *testing.T) {
+func TestResolveAttachmentCIDs(t *testing.T) {
 	tests := []struct {
 		name    string
 		msg     cmodels.Message
@@ -270,9 +270,9 @@ func TestResolveContentCIDs(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			resolveContentCIDs(&tt.msg, tt.rootURL)
+			resolveAttachmentCIDs(&tt.msg, tt.rootURL)
 			if tt.msg.Content != tt.want {
-				t.Errorf("resolveContentCIDs()\n got  = %s\n want = %s", tt.msg.Content, tt.want)
+				t.Errorf("resolveAttachmentCIDs()\n got  = %s\n want = %s", tt.msg.Content, tt.want)
 			}
 		})
 	}

internal/conversation/conversation.go

@@ -140,6 +140,7 @@ type mediaStore interface {
 	Attach(id int, model string, modelID int) error
 	SetContentID(id int, contentID string) error
 	GetByModel(id int, model string) ([]mmodels.Media, error)
+	GetByContentIDs(contentIDs []string, conversationUUID string) ([]mmodels.Media, error)
 	ContentIDExists(contentID string) (bool, string, error)
 	Upload(fileName, contentType string, content io.ReadSeeker) (string, string, error)
 	UploadAndInsert(fileName, contentType, contentID string, modelType null.String, modelID null.Int, content io.ReadSeeker, fileSize int, disposition null.String, meta []byte) (mmodels.Media, error)

internal/conversation/message.go

@@ -1005,6 +1005,26 @@ func extractInlineImageUUIDs(content string) []string {
 	return out
 }
 
+// extractInlineContentIDs returns unique content_ids referenced via <img src="cid:..."> in the body.
+func extractInlineContentIDs(content string) []string {
+	matches := imgSrcPattern.FindAllStringSubmatch(content, -1)
+	seen := make(map[string]bool, len(matches))
+	out := make([]string, 0, len(matches))
+	for _, m := range matches {
+		url := m[1]
+		if !strings.HasPrefix(url, "cid:") {
+			continue
+		}
+		cid := strings.TrimPrefix(url, "cid:")
+		if cid == "" || seen[cid] {
+			continue
+		}
+		seen[cid] = true
+		out = append(out, cid)
+	}
+	return out
+}
+
 // rewriteInlineImagesToCID rewrites every <img src="...<uuid>..."> to <img src="cid:ldsk-<uuid>">. Already-cid form is left alone.
 func rewriteInlineImagesToCID(content string) string {
 	return imgSrcPattern.ReplaceAllStringFunc(content, func(match string) string {
@@ -1061,35 +1081,26 @@ func (m *Manager) uploadMessageAttachments(message *models.Message) error {
 	}
 
 	for _, attachment := range message.Attachments {
-		// Check if this attachment already exists by the content ID, as inline images can be repeated across conversations.
 		contentID := attachment.ContentID
 		if contentID != "" {
-			// Make content ID MORE unique by prefixing it with the conversation UUID, as content id is not globally unique practically,
-			// different messages can have the same content ID, I do not have the message ID at this point, so I am using sticking with the conversation UUID
-			// to make it more unique.
-			contentID = message.ConversationUUID + "_" + contentID
+			storedCID, exists, mediaUUID := m.findExistingMedia(contentID, message.ConversationUUID)
 
-			exists, uuid, err := m.mediaStore.ContentIDExists(contentID)
-			if err != nil {
-				m.lo.Error("error checking media existence by content ID", "content_id", contentID, "error", err)
+			// Make body's cid match the stored content_id so the read path can find it.
+			if storedCID != contentID {
+				message.Content = strings.ReplaceAll(message.Content, fmt.Sprintf("cid:%s", contentID), fmt.Sprintf("cid:%s", storedCID))
 			}
 
-			// This attachment already exists, replace the cid:content_id with the media relative url, not using absolute path as the root path can change.
 			if exists {
-				m.lo.Debug("attachment with content ID already exists replacing content ID with media relative URL", "content_id", contentID, "media_uuid", uuid)
-				message.Content = strings.ReplaceAll(message.Content, fmt.Sprintf("cid:%s", attachment.ContentID), "/uploads/"+uuid)
+				m.lo.Debug("inline attachment exists, reusing", "content_id", storedCID, "media_uuid", mediaUUID)
 				continue
 			}
-
-			// Attachment does not exist, replace the content ID with the new more unique content ID.
-			message.Content = strings.ReplaceAll(message.Content, fmt.Sprintf("cid:%s", attachment.ContentID), fmt.Sprintf("cid:%s", contentID))
+			contentID = storedCID
 		}
 
 		// Sanitize filename.
 		attachment.Name = stringutil.SanitizeFilename(attachment.Name)
 
-		m.lo.Debug("uploading message attachment", "name", attachment.Name, "content_id", contentID, "size", attachment.Size, "content_type", attachment.ContentType,
-			"content_id", contentID, "disposition", attachment.Disposition)
+		m.lo.Debug("uploading message attachment", "name", attachment.Name, "content_id", contentID, "size", attachment.Size, "content_type", attachment.ContentType, "disposition", attachment.Disposition)
 
 		// Upload and insert entry in media table.
 		attachReader := bytes.NewReader(attachment.Content)
@@ -1187,6 +1198,30 @@ func (m *Manager) messageExistsBySourceID(messageSourceIDs []string) (int, error
 	return conversationID, nil
 }
 
+// GetInlineMediaRefs returns media referenced via cid: in the body but linked to other messages (quoted history).
+func (m *Manager) GetInlineMediaRefs(message *models.Message) ([]mmodels.Media, error) {
+	cids := extractInlineContentIDs(message.Content)
+	if len(cids) == 0 {
+		return nil, nil
+	}
+	existing := make(map[string]bool, len(message.Attachments))
+	for _, a := range message.Attachments {
+		if a.ContentID != "" {
+			existing[a.ContentID] = true
+		}
+	}
+	missing := make([]string, 0, len(cids))
+	for _, cid := range cids {
+		if !existing[cid] {
+			missing = append(missing, cid)
+		}
+	}
+	if len(missing) == 0 {
+		return nil, nil
+	}
+	return m.mediaStore.GetByContentIDs(missing, message.ConversationUUID)
+}
+
 // fetchMessageAttachments fetches attachments (also inline images) for a single message ID.
 func (m *Manager) fetchMessageAttachments(messageID int) (attachment.Attachments, error) {
 	var attachments attachment.Attachments
@@ -1392,3 +1427,16 @@ func (m *Manager) getMediaPreview(media mmodels.Media) string {
 func inlineContentID(uuid string) string {
 	return "ldsk-" + uuid
 }
+
+// findExistingMedia resolves an inbound cid to its stored form: ldsk-* is left as-is, others are namespaced by conversation to avoid cross-conversation collisions.
+func (m *Manager) findExistingMedia(rawContentID, conversationUUID string) (string, bool, string) {
+	storedCID := rawContentID
+	if !strings.HasPrefix(rawContentID, "ldsk-") {
+		storedCID = conversationUUID + "_" + rawContentID
+	}
+	exists, mediaUUID, err := m.mediaStore.ContentIDExists(storedCID)
+	if err != nil {
+		m.lo.Error("error checking media existence by content ID", "content_id", storedCID, "error", err)
+	}
+	return storedCID, exists, mediaUUID
+}

internal/conversation/message_test.go

@@ -397,6 +397,74 @@ func TestExtractInlineImageUUIDs(t *testing.T) {
 	}
 }
 
+func TestExtractInlineContentIDs(t *testing.T) {
+	tests := []struct {
+		name string
+		body string
+		want []string
+	}{
+		{
+			name: "empty_body",
+			body: "",
+			want: []string{},
+		},
+		{
+			name: "ignores_non_cid_src",
+			body: `<img src="/uploads/` + testUUID + `">`,
+			want: []string{},
+		},
+		{
+			name: "single_cid_extracted",
+			body: `<img src="cid:ldsk-` + testUUID + `">`,
+			want: []string{"ldsk-" + testUUID},
+		},
+		{
+			name: "mixed_cid_and_url_returns_only_cids",
+			body: `<img src="/uploads/` + testUUID + `"><img src="cid:ldsk-` + testUUID2 + `">`,
+			want: []string{"ldsk-" + testUUID2},
+		},
+		{
+			name: "single_quotes_around_src",
+			body: `<img src='cid:ldsk-` + testUUID + `'>`,
+			want: []string{"ldsk-" + testUUID},
+		},
+		{
+			name: "empty_cid_skipped",
+			body: `<img src="cid:">`,
+			want: []string{},
+		},
+		{
+			name: "multi_with_dedup_and_order",
+			body: `<img src="cid:ldsk-` + testUUID2 + `"><img src="cid:ldsk-` + testUUID + `"><img src="cid:ldsk-` + testUUID2 + `">`,
+			want: []string{"ldsk-" + testUUID2, "ldsk-" + testUUID},
+		},
+		{
+			name: "src_after_other_attributes",
+			body: `<img class="inline" alt="x" src="cid:ldsk-` + testUUID + `">`,
+			want: []string{"ldsk-" + testUUID},
+		},
+		{
+			name: "uppercase_cid_prefix_not_matched",
+			body: `<img src="CID:ldsk-` + testUUID + `">`,
+			want: []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := extractInlineContentIDs(tt.body)
+			if len(got) != len(tt.want) {
+				t.Fatalf("got %v, want %v", got, tt.want)
+			}
+			for i := range got {
+				if got[i] != tt.want[i] {
+					t.Errorf("index %d: got %q, want %q", i, got[i], tt.want[i])
+				}
+			}
+		})
+	}
+}
+
 func TestRewriteInlineImagesToCID(t *testing.T) {
 	tests := []struct {
 		name string

internal/media/media.go

@@ -21,6 +21,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/jmoiron/sqlx"
 	"github.com/knadh/go-i18n"
+	"github.com/lib/pq"
 	"github.com/volatiletech/null/v9"
 	"github.com/zerodha/logf"
 )
@@ -88,6 +89,7 @@ type queries struct {
 	GetByModel              *sqlx.Stmt `query:"get-model-media"`
 	GetUnlinkedMessageMedia *sqlx.Stmt `query:"get-unlinked-message-media"`
 	ContentIDExists         *sqlx.Stmt `query:"content-id-exists"`
+	GetByContentIDs         *sqlx.Stmt `query:"get-media-by-content-ids"`
 	SetContentID            *sqlx.Stmt `query:"set-media-content-id"`
 }
 
@@ -191,6 +193,19 @@ func (m *Manager) ContentIDExists(contentID string) (bool, string, error) {
 	return true, uuid, nil
 }
 
+// GetByContentIDs returns media rows matching any of the given content_ids, scoped to the given conversation to prevent cross-conversation lookup.
+func (m *Manager) GetByContentIDs(contentIDs []string, conversationUUID string) ([]models.Media, error) {
+	out := []models.Media{}
+	if len(contentIDs) == 0 || conversationUUID == "" {
+		return out, nil
+	}
+	if err := m.queries.GetByContentIDs.Select(&out, pq.Array(contentIDs), conversationUUID); err != nil {
+		m.lo.Error("error fetching media by content_ids", "error", err)
+		return nil, fmt.Errorf("fetching media by content_ids: %w", err)
+	}
+	return out, nil
+}
+
 // GetBlob retrieves the raw binary content of a media file by its name.
 func (m *Manager) GetBlob(name string) ([]byte, error) {
 	return m.store.GetBlob(name)

internal/media/queries.sql

@@ -53,6 +53,14 @@ WHERE model_type = 'messages'
 -- name: content-id-exists
 SELECT uuid FROM media WHERE content_id = $1;
 
+-- name: get-media-by-content-ids
+SELECT m.id, m.created_at, m.updated_at, m."uuid", m.store, m.filename, m.content_type, m.content_id, m.model_id, m.model_type, m.disposition, m."size", m.meta
+FROM media m
+INNER JOIN conversation_messages cm ON cm.id = m.model_id
+WHERE m.model_type = 'messages'
+  AND m.content_id = ANY($1)
+  AND cm.conversation_id = (SELECT id FROM conversations WHERE uuid = $2::uuid LIMIT 1);
+
 -- name: set-media-content-id
 UPDATE media
 SET content_id = $2