Fix inline image media not linking on S3 presigned URLs

Also simplify the inline-image helpers: one <img src> matcher plus stringutil.ExtractUUID, instead of two combined regexes with offset math.
2026-09-22 10:33:24 +00:00 · 2026-05-14 22:36:11 +05:30
parent 3470347708
commit dbf4d3a0a6
2 changed files with 223 additions and 46 deletions
@@ -35,14 +35,8 @@ const (
 	upgradeWindowTTL = 7 * 24 * time.Hour
 )

-// For <img class="inline-image" src="/uploads/abc-123?sig=xyz">:
-//
-//	group 1 = `<img class="inline-image" src="`
-//	group 2 = `abc-123`                            (media UUID)
-//	group 3 = `"`
-var imgSrcUploadsPattern = regexp.MustCompile(
-	`(?i)(<img\b[^>]*?\bsrc=["'])(?:https?://[^"'<>\s/]+)?/uploads/([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})(?:\?[^"'<>\s]*)?(["'])`,
-)
+// Matches <img ... src="URL"> and captures the URL for downstream parsing.
+var imgSrcPattern = regexp.MustCompile(`(?i)<img\b[^>]*?\bsrc=["']([^"']*)["']`)

 // Run starts a pool of worker goroutines to handle message dispatching via inbox's channel and processes incoming messages. It scans for
 // pending outgoing messages at the specified read interval and pushes them to the outgoing queue to be sent.
@@ -991,34 +985,39 @@ func (c *Manager) generateMessagesQuery(baseQuery string, qArgs []interface{}, p
 	return sqlQuery, pageSize, qArgs, nil
 }

-// extractInlineImageUUIDs returns the unique media UUIDs referenced by
-// <img src=".../uploads/<uuid>"> in the body, in order of first appearance.
+// extractInlineImageUUIDs returns unique media UUIDs from <img src="..."> URLs in order of first appearance, skipping the cid: form.
 func extractInlineImageUUIDs(content string) []string {
-	matches := imgSrcUploadsPattern.FindAllStringSubmatch(content, -1)
+	matches := imgSrcPattern.FindAllStringSubmatch(content, -1)
 	seen := make(map[string]bool, len(matches))
 	out := make([]string, 0, len(matches))
-	for _, sub := range matches {
-		if len(sub) < 3 {
+	for _, m := range matches {
+		url := m[1]
+		if strings.HasPrefix(url, "cid:") {
 			continue
 		}
-		if seen[sub[2]] {
+		u := stringutil.ExtractUUID(url)
+		if u == "" || seen[u] {
 			continue
 		}
-		seen[sub[2]] = true
-		out = append(out, sub[2])
+		seen[u] = true
+		out = append(out, u)
 	}
 	return out
 }

-// rewriteInlineImagesToCID replaces every <img src=".../uploads/<uuid>"> with
-// <img src="cid:ldsk-<uuid>">.
+// rewriteInlineImagesToCID rewrites every <img src="...<uuid>..."> to <img src="cid:ldsk-<uuid>">. Already-cid form is left alone.
 func rewriteInlineImagesToCID(content string) string {
-	return imgSrcUploadsPattern.ReplaceAllStringFunc(content, func(match string) string {
-		sub := imgSrcUploadsPattern.FindStringSubmatch(match)
-		if len(sub) < 4 {
+	return imgSrcPattern.ReplaceAllStringFunc(content, func(match string) string {
+		sub := imgSrcPattern.FindStringSubmatch(match)
+		url := sub[1]
+		if strings.HasPrefix(url, "cid:") {
 			return match
 		}
-		return sub[1] + "cid:" + inlineContentID(sub[2]) + sub[3]
+		u := stringutil.ExtractUUID(url)
+		if u == "" {
+			return match
+		}
+		return strings.Replace(match, url, "cid:"+inlineContentID(u), 1)
 	})
 }

@@ -1031,7 +1030,7 @@ func (m *Manager) linkInlineMediaToMessage(uuids []string, messageID int) {
 		if err != nil {
 			continue
 		}
-		if media.Model.String != mmodels.ModelMessages {
+		if media.Model.Valid && media.Model.String != mmodels.ModelMessages {
 			continue
 		}
 		// Linked to a different message already, leave it.
@@ -5,8 +5,8 @@ import (
 	"testing"
 )

-const testUUID = "abcdef01-2345-6789-abcd-ef0123456789"
-const testUUID2 = "11111111-2222-3333-4444-555555555555"
+const testUUID = "d0355103-455f-4c7d-b9c7-86e9254fe119"
+const testUUID2 = "edb7be78-ef7d-4fe9-888b-22494f0ce076"

 func TestImgSrcUploadsPattern(t *testing.T) {
 	tests := []struct {
@@ -77,8 +77,8 @@ func TestImgSrcUploadsPattern(t *testing.T) {
 			wantUUIDs: []string{testUUID},
 		},
 		{
-			name: "multiline_tag",
-			body: "<img\n  alt=\"x\"\n  src=\"/uploads/" + testUUID + "\"\n>",
+			name:      "multiline_tag",
+			body:      "<img\n  alt=\"x\"\n  src=\"/uploads/" + testUUID + "\"\n>",
 			wantCount: 1,
 			wantUUIDs: []string{testUUID},
 		},
@@ -90,12 +90,11 @@ func TestImgSrcUploadsPattern(t *testing.T) {
 			wantUUIDs: []string{testUUID, testUUID2},
 		},

-		// (?i) makes hex class case-insensitive too.
 		{
-			name:      "quirk_uppercase_hex_uuid_matches",
-			body:      `<img src="/uploads/ABCDEF01-2345-6789-ABCD-EF0123456789">`,
+			name:      "uppercase_hex_uuid_matches",
+			body:      `<img src="/uploads/D0355103-455F-4C7D-B9C7-86E9254FE119">`,
 			wantCount: 1,
-			wantUUIDs: []string{"ABCDEF01-2345-6789-ABCD-EF0123456789"},
+			wantUUIDs: []string{"D0355103-455F-4C7D-B9C7-86E9254FE119"},
 		},
 		// `\b` boundary lets data-src match; harmless, no real src to render.
 		{
@@ -138,34 +137,130 @@ func TestImgSrcUploadsPattern(t *testing.T) {
 			body:      `<img src="/uploads/abcdef01-2345-6789-abcd-ef0123">`,
 			wantCount: 0,
 		},
-		{
-			name:      "trailing_path_segment_no_match",
-			body:      `<img src="/uploads/` + testUUID + `/extra">`,
-			wantCount: 0,
-		},
 		{
 			name:      "empty_src_no_match",
 			body:      `<img src="">`,
 			wantCount: 0,
 		},
+
+		{
+			name:      "trailing_path_segment_still_matches",
+			body:      `<img src="/uploads/` + testUUID + `/extra">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_path_style_public_url",
+			body:      `<img src="https://s3.ap-south-1.amazonaws.com/bucket-name/` + testUUID + `">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_virtual_hosted_url",
+			body:      `<img src="https://bucket-name.s3.ap-south-1.amazonaws.com/` + testUUID + `">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_presigned_url_full",
+			body:      `<img class="inline-image" src="https://s3.ap-south-1.amazonaws.com/bucket-name/` + testUUID + `?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ABC%2F20260514%2Fap-south-1%2Fs3%2Faws4_request&X-Amz-Date=20260514T161618Z&X-Amz-Expires=300&X-Amz-SignedHeaders=host&X-Amz-Signature=deadbeef">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_presigned_url_html_entities",
+			body:      `<img src="https://s3.ap-south-1.amazonaws.com/bucket/` + testUUID + `?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Signature=deadbeef&amp;X-Amz-Expires=300">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_nested_bucket_path",
+			body:      `<img src="https://s3.ap-south-1.amazonaws.com/bucket/childpath1/childpath2/` + testUUID + `">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_nested_bucket_path_presigned",
+			body:      `<img src="https://s3.ap-south-1.amazonaws.com/bucket/2026/05/14/` + testUUID + `?X-Amz-Signature=abc">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "s3_compatible_endpoint",
+			body:      `<img src="https://minio.example.com:9000/bucket/` + testUUID + `?X-Amz-Signature=abc">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name: "multiple_s3_presigned_urls",
+			body: `<img src="https://s3.amazonaws.com/b/` + testUUID + `?X-Amz-Signature=a">` +
+				`<img src="https://s3.amazonaws.com/b/` + testUUID2 + `?X-Amz-Signature=b">`,
+			wantCount: 2,
+			wantUUIDs: []string{testUUID, testUUID2},
+		},
+		{
+			name:      "cdn_proxied_url",
+			body:      `<img src="https://cdn.example.com/media/` + testUUID + `/photo.png">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "uuid_in_query_param",
+			body:      `<img src="https://example.com/render?id=` + testUUID + `">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "cid_form_skipped",
+			body:      `<img src="cid:ldsk-` + testUUID + `">`,
+			wantCount: 0,
+		},
+		{
+			name: "s3_presigned_realistic_long_url",
+			body: `<img class="inline-image" style="max-width: 100%; height: auto;" src="https://s3.ap-south-1.amazonaws.com/example-bucket/` + testUUID +
+				`?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Credential=ASIAEXAMPLE%2F20260514%2Fap-south-1%2Fs3%2Faws4_request` +
+				`&amp;X-Amz-Date=20260514T161618Z&amp;X-Amz-Expires=300` +
+				`&amp;X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJj%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCmFwLXNvdXRoLTEiSDBGAiEA21MRBCy0mE3AzOx9` +
+				`&amp;X-Amz-SignedHeaders=host&amp;response-content-disposition=inline%3B%20filename%3D%22image.png%22` +
+				`&amp;X-Amz-Signature=1ba1c7feb9ba2dd3c7df72e3054dd9bb32aaced52adc48d672f926c4b95e3115">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "fs_store_signed_url",
+			body:      `<img src="https://libredesk.example.com/uploads/` + testUUID + `?sig=deadbeefcafe&exp=1768435200">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "fs_store_signed_url_html_entity",
+			body:      `<img src="https://libredesk.example.com/uploads/` + testUUID + `?sig=deadbeefcafe&amp;exp=1768435200">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
+		{
+			name:      "fs_store_custom_port",
+			body:      `<img src="http://localhost:9000/uploads/` + testUUID + `?sig=abc&exp=1">`,
+			wantCount: 1,
+			wantUUIDs: []string{testUUID},
+		},
 	}

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			matches := imgSrcUploadsPattern.FindAllStringSubmatch(tt.body, -1)
-			if len(matches) != tt.wantCount {
-				t.Fatalf("match count = %d, want %d (matches=%v)", len(matches), tt.wantCount, matches)
+			got := extractInlineImageUUIDs(tt.body)
+			if len(got) != tt.wantCount {
+				t.Fatalf("got %d uuids, want %d (got=%v)", len(got), tt.wantCount, got)
 			}
 			for i, want := range tt.wantUUIDs {
-				if matches[i][2] != want {
-					t.Errorf("match %d uuid = %q, want %q", i, matches[i][2], want)
+				if !strings.EqualFold(got[i], want) {
+					t.Errorf("uuid %d = %q, want %q", i, got[i], want)
 				}
 			}
 		})
 	}
 }

-
 func TestImgSrcUploadsPattern_Adversarial(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -206,10 +301,10 @@ func TestImgSrcUploadsPattern_Adversarial(t *testing.T) {

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			matches := imgSrcUploadsPattern.FindAllStringSubmatch(tt.body, -1)
-			if len(matches) != tt.wantCount {
-				t.Errorf("got %d matches, want %d\nbody: %s\nmatches: %v",
-					len(matches), tt.wantCount, tt.body, matches)
+			got := extractInlineImageUUIDs(tt.body)
+			if len(got) != tt.wantCount {
+				t.Errorf("got %d uuids, want %d\nbody: %s\nuuids: %v",
+					len(got), tt.wantCount, tt.body, got)
 			}
 		})
 	}
@@ -251,6 +346,40 @@ func TestExtractInlineImageUUIDs(t *testing.T) {
 			body: `<img src="cid:ldsk-` + testUUID + `">`,
 			want: []string{},
 		},
+		{
+			name: "mixed_cid_and_s3_extracts_only_s3",
+			body: `<img src="cid:ldsk-` + testUUID + `">` +
+				`<img src="https://s3.amazonaws.com/b/` + testUUID2 + `?X-Amz-Signature=x">`,
+			want: []string{testUUID2},
+		},
+		{
+			name: "s3_presigned_url_extracts_uuid",
+			body: `<img class="inline-image" src="https://s3.ap-south-1.amazonaws.com/example-bucket/` + testUUID + `?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Signature=abc">`,
+			want: []string{testUUID},
+		},
+		{
+			name: "nested_bucket_path_extracts_uuid",
+			body: `<img src="https://s3.amazonaws.com/bucket/2026/05/14/` + testUUID + `">`,
+			want: []string{testUUID},
+		},
+		{
+			name: "dedupes_across_s3_and_relative",
+			body: `<img src="/uploads/` + testUUID + `">` +
+				`<img src="https://s3.amazonaws.com/b/` + testUUID + `?X-Amz-Signature=x">`,
+			want: []string{testUUID},
+		},
+		{
+			name: "footer_image_ignored_inline_extracted",
+			body: `<header><img src="https://static.example.com/brand/logo.png" alt="brand" /></header>` +
+				`<p>Hello</p>` +
+				`<img class="inline-image" src="https://s3.amazonaws.com/example-bucket/` + testUUID + `?X-Amz-Signature=abc">`,
+			want: []string{testUUID},
+		},
+		{
+			name: "fs_store_signed_url_extracts",
+			body: `<img src="https://libredesk.example.com/uploads/` + testUUID + `?sig=deadbeef&amp;exp=1768435200">`,
+			want: []string{testUUID},
+		},
 	}

 	for _, tt := range tests {
@@ -294,6 +423,40 @@ func TestRewriteInlineImagesToCID(t *testing.T) {
 			body: `<img src="https://host.example.com/uploads/` + testUUID + `?sig=abc&exp=1">`,
 			want: `<img src="cid:ldsk-` + testUUID + `">`,
 		},
+		{
+			name: "s3_presigned_url_rewritten_to_cid",
+			body: `<img class="inline-image" src="https://s3.ap-south-1.amazonaws.com/bucket/` + testUUID + `?X-Amz-Signature=abc">`,
+			want: `<img class="inline-image" src="cid:ldsk-` + testUUID + `">`,
+		},
+		{
+			name: "s3_presigned_url_with_html_entities_rewritten",
+			body: `<img src="https://s3.ap-south-1.amazonaws.com/bucket/` + testUUID + `?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Signature=abc&amp;X-Amz-Expires=300">`,
+			want: `<img src="cid:ldsk-` + testUUID + `">`,
+		},
+		{
+			name: "s3_nested_path_rewritten",
+			body: `<img src="https://s3.amazonaws.com/bucket/childpath1/childpath2/` + testUUID + `?X-Amz-Signature=abc">`,
+			want: `<img src="cid:ldsk-` + testUUID + `">`,
+		},
+		{
+			name: "s3_virtual_hosted_rewritten",
+			body: `<img src="https://bucket-name.s3.ap-south-1.amazonaws.com/` + testUUID + `?X-Amz-Signature=abc">`,
+			want: `<img src="cid:ldsk-` + testUUID + `">`,
+		},
+		{
+			name: "multiple_s3_urls_rewritten",
+			body: `<img src="https://s3.amazonaws.com/b/` + testUUID + `?X-Amz-Signature=a">` +
+				`<img src="https://s3.amazonaws.com/b/` + testUUID2 + `?X-Amz-Signature=b">`,
+			want: `<img src="cid:ldsk-` + testUUID + `">` +
+				`<img src="cid:ldsk-` + testUUID2 + `">`,
+		},
+		{
+			name: "mixed_cid_and_s3_leaves_cid_alone",
+			body: `<img src="cid:ldsk-` + testUUID + `">` +
+				`<img src="https://s3.amazonaws.com/b/` + testUUID2 + `?X-Amz-Signature=x">`,
+			want: `<img src="cid:ldsk-` + testUUID + `">` +
+				`<img src="cid:ldsk-` + testUUID2 + `">`,
+		},
 		{
 			name: "preserves_other_attributes",
 			body: `<img class="inline-image" alt="hi" src="/uploads/` + testUUID + `">`,
@@ -348,4 +511,19 @@ func TestRewriteInlineImagesToCID(t *testing.T) {
 			t.Errorf("expected 0 URL-form UUIDs after rewrite, got %v", leftover)
 		}
 	})
+
+	t.Run("round_trip_s3_presigned_to_cid", func(t *testing.T) {
+		body := `<img class="inline-image" src="https://s3.ap-south-1.amazonaws.com/bucket/` + testUUID + `?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Signature=abc&amp;X-Amz-Expires=300">`
+		rewritten := rewriteInlineImagesToCID(body)
+		if strings.Contains(rewritten, "amazonaws.com") {
+			t.Errorf("rewritten body still contains presigned URL: %s", rewritten)
+		}
+		if strings.Contains(rewritten, "X-Amz-Signature") {
+			t.Errorf("rewritten body still contains X-Amz-Signature: %s", rewritten)
+		}
+		leftover := extractInlineImageUUIDs(rewritten)
+		if len(leftover) != 0 {
+			t.Errorf("expected 0 URL-form UUIDs after rewrite, got %v", leftover)
+		}
+	})
 }