From 58023f5e908723963e6f7e8824036c8719d12265 Mon Sep 17 00:00:00 2001 From: Adam Miribyan <321133+adammiribyan@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:34:10 +0200 Subject: [PATCH 1/4] List the files inside an embedded text/html attachment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An HTML email from outside HEY arrives as one
with contentType "text/html" and the original markup in its content string (21751f9). When that email carried files -- an Outlook sender attaching two PDFs, say -- they sit inside that markup as elements, so `hey attachment list` answered "0 attachments" for a thread whose `hey thread read` plainly showed the 📎 lines, and `hey attachment save` had nothing to address. findAttachments now walks into the embedded markup the way findImages already does, bounded by embeddedContentDepthLimit, and lists what it finds there. The wrapper itself is still not listed: an embedded body is not a downloadable file, and the test that says so still holds. --- internal/htmlutil/htmlutil.go | 18 ++++++++++++++---- internal/htmlutil/htmlutil_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/internal/htmlutil/htmlutil.go b/internal/htmlutil/htmlutil.go index 597c1adb..ec7afe50 100644 --- a/internal/htmlutil/htmlutil.go +++ b/internal/htmlutil/htmlutil.go @@ -73,7 +73,7 @@ func ExtractAttachments(s string) []Attachment { return nil } var attachments []Attachment - findAttachments(doc, &attachments) + findAttachments(doc, &attachments, 0) return attachments } @@ -342,7 +342,7 @@ func parseEmbeddedContent(content string, depth int) *html.Node { return doc } -func findAttachments(n *html.Node, attachments *[]Attachment) { +func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { if n.Type == html.ElementNode { switch n.Data { case "action-text-attachment": @@ -358,7 +358,10 @@ func findAttachments(n *html.Node, attachments *[]Attachment) { *attachments = append(*attachments, attachment) } case "figure": - if trix := parseTrixAttachment(n); trix != nil && trix.URL != "" && trix.Filename != "" { + trix := parseTrixAttachment(n) + switch { + case trix == nil: + case trix.URL != "" && trix.Filename != "": *attachments = append(*attachments, Attachment{ URL: trix.URL, Filename: trix.Filename, @@ -366,11 +369,18 @@ func findAttachments(n *html.Node, attachments *[]Attachment) { ByteSize: nonnegativeAttachmentByteSize(trix.Filesize), SGID: trix.SGID, }) + case trix.Content != "": + // An inbound email's files are inside the embedded markup, not + // on the figure that wraps it. The wrapper itself is not listed: + // an embedded body is not a downloadable file. + if doc := parseEmbeddedContent(trix.Content, depth); doc != nil { + findAttachments(doc, attachments, depth+1) + } } } } for child := n.FirstChild; child != nil; child = child.NextSibling { - findAttachments(child, attachments) + findAttachments(child, attachments, depth) } } diff --git a/internal/htmlutil/htmlutil_test.go b/internal/htmlutil/htmlutil_test.go index 28e1d3f5..c7c6f7ee 100644 --- a/internal/htmlutil/htmlutil_test.go +++ b/internal/htmlutil/htmlutil_test.go @@ -194,6 +194,33 @@ func TestExtractAttachmentsSkipsEmbeddedHTMLAttachment(t *testing.T) { } } +func TestExtractAttachmentsInsideEmbeddedHTMLAttachment(t *testing.T) { + // An HTML email from outside HEY arrives as one text/html trix attachment + // whose content string holds the original markup, files included. + content := `
` + + attachments := ExtractAttachments(content) + if len(attachments) != 1 { + t.Fatalf("ExtractAttachments = %+v, want the file inside the embedded body", attachments) + } + got := attachments[0] + if got.Filename != "payslip.pdf" || got.URL != "/rails/blobs/payslip.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-1" || got.ByteSize == nil || *got.ByteSize != 44218 { + t.Errorf("embedded attachment = %+v", got) + } +} + +func TestExtractAttachmentsEmbeddedContentStopsRecursing(t *testing.T) { + nested := `
` + for range embeddedContentDepthLimit + 2 { + nested = `
` + } + + if attachments := ExtractAttachments(nested); len(attachments) != 0 { + t.Errorf("ExtractAttachments = %+v, should stop before the innermost level", attachments) + } +} + func TestExtractAttachments(t *testing.T) { h := `
` From 578c0a367f089f92e08b6f068aeeb36929516e8a Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 8 Sep 2026 09:42:18 -0400 Subject: [PATCH 2/4] Document embedded attachment listing --- docs/cli.md | 2 +- internal/cmd/attachments.go | 1 + internal/cmd/attachments_test.go | 59 ++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/docs/cli.md b/docs/cli.md index f860572e..1d8ccdcf 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -273,7 +273,7 @@ The Screener is where first-time senders wait. `hey screener list` returns clear `hey bulk-reply preview` is read-only and resolves each posting to its latest replyable entry. `hey bulk-reply send` resolves the selection again, skips threads without a replyable entry, keeps HEY's server-provided name tag, and returns the exact reply count, delivery ID, delayed state, undo URL, and undo command. Posting IDs must be positive and unique. The message can come from `-m`, stdin, or `$EDITOR`; `--attach` is repeatable. -`--attach` is repeatable on `hey compose`, `hey reply`, and `hey bulk-reply send`, and attachment-only messages are supported. The CLI validates and uploads every file before sending the email. `hey attachment list ` returns stable message-and-position IDs such as `456:1`; pass an ID to `hey attachment save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. +`--attach` is repeatable on `hey compose`, `hey reply`, and `hey bulk-reply send`, and attachment-only messages are supported. The CLI validates and uploads every file before sending the email. `hey attachment list ` returns every named downloadable file, including named inline images, with message-and-position IDs such as `456:1`. Positions reflect the current rendered message, so pass an ID from the current listing to `hey attachment save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. Organization actions take the `id` values returned by `hey box view --json`, `hey label view --json`, or `hey search --json`. Reading, replying to, and forwarding a thread take its `topic_id` instead, which `hey box view --json`, `hey label view --json`, `hey collection view --json` and `hey search --json` all carry alongside `id`. `hey box view` also returns `next_page` and accepts `--page ` to continue a box listing; it keeps `next_history_url` for the sync clients that read it, and `--page` accepts that URL as readily as the cursor inside it. Label IDs come from `hey label list`; `hey label view` returns `next_page` and `total_count`, accepts `--page ` for continuation, and supports `--all` for complete traversal. HEY creates a label while adding it to at least one thread, so `hey label create` requires thread item IDs. diff --git a/internal/cmd/attachments.go b/internal/cmd/attachments.go index a9d4aeff..4a632e80 100644 --- a/internal/cmd/attachments.go +++ b/internal/cmd/attachments.go @@ -43,6 +43,7 @@ func newAttachmentsCommand() *attachmentsCommand { attachmentsCommand.cmd = &cobra.Command{ Use: "list ", Short: "List a thread's attachments", + Long: "List every named downloadable file in a thread, including named inline images. IDs reflect the current message order; pass an ID from the current listing to attachment save.", Example: ` hey attachment list 12345 hey attachment list 12345 --json hey attachment list 12345 --allow-partial`, diff --git a/internal/cmd/attachments_test.go b/internal/cmd/attachments_test.go index 40fb3913..1da56d20 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -195,6 +195,65 @@ func TestAttachmentsListsFilesFromKnownThread(t *testing.T) { } } +func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { + content := `
+
+` + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/topics/42/entries.json": + _, _ = w.Write([]byte(`[{"id":101,"kind":"message"}]`)) + case "/messages/101.json": + _ = json.NewEncoder(w).Encode(map[string]any{"id": 101, "content": content}) + case "/rails/active_storage/blobs/conference-agenda.pdf": + w.Header().Set("Content-Type", "application/pdf") + _, _ = w.Write([]byte("conference agenda")) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + stdout, err := runAttachmentCommand(t, server, "attachment", "list", "42") + if err != nil { + t.Fatal(err) + } + var response struct { + Data []threadAttachment `json:"data"` + } + if err := json.Unmarshal([]byte(stdout), &response); err != nil { + t.Fatalf("decode response: %v\n%s", err, stdout) + } + if len(response.Data) != 3 { + t.Fatalf("listed attachments = %+v, want three named files", response.Data) + } + for index, want := range []struct { + id string + filename string + }{ + {id: "101:1", filename: "conference-logo.png"}, + {id: "101:2", filename: "conference-agenda.pdf"}, + {id: "101:3", filename: "venue-map.pdf"}, + } { + if response.Data[index].ID != want.id || response.Data[index].Filename != want.filename { + t.Errorf("attachment %d = %+v, want ID %q and filename %q", index, response.Data[index], want.id, want.filename) + } + } + + destination := filepath.Join(t.TempDir(), "conference-agenda.pdf") + if _, err := runAttachmentCommand(t, server, "attachment", "save", "101:2", "--output", destination); err != nil { + t.Fatal(err) + } + if got, err := os.ReadFile(destination); err != nil { + t.Fatal(err) + } else if string(got) != "conference agenda" { + t.Errorf("saved attachment = %q", got) + } +} + // A thread longer than one page is walked by following HEY's cursor, so each attachment // is listed once and the list is not claimed to be truncated. func TestAttachmentsFollowsTheCursorThroughALongThread(t *testing.T) { From a0bbc5d6ff40a4b9f8e1fcceffd9b41710392155 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 8 Sep 2026 10:39:28 -0400 Subject: [PATCH 3/4] Preserve attachment IDs across embedded files --- docs/cli.md | 2 +- internal/cmd/attachments.go | 74 ++++++++++++++++- internal/cmd/attachments_save.go | 20 +++-- internal/cmd/attachments_test.go | 129 ++++++++++++++++++++++++----- internal/htmlutil/htmlutil.go | 19 ++++- internal/htmlutil/htmlutil_test.go | 55 +++++++++--- internal/htmlutil/markdown_test.go | 7 +- skills/hey/SKILL.md | 2 +- 8 files changed, 254 insertions(+), 54 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 1d8ccdcf..ca8aa210 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -273,7 +273,7 @@ The Screener is where first-time senders wait. `hey screener list` returns clear `hey bulk-reply preview` is read-only and resolves each posting to its latest replyable entry. `hey bulk-reply send` resolves the selection again, skips threads without a replyable entry, keeps HEY's server-provided name tag, and returns the exact reply count, delivery ID, delayed state, undo URL, and undo command. Posting IDs must be positive and unique. The message can come from `-m`, stdin, or `$EDITOR`; `--attach` is repeatable. -`--attach` is repeatable on `hey compose`, `hey reply`, and `hey bulk-reply send`, and attachment-only messages are supported. The CLI validates and uploads every file before sending the email. `hey attachment list ` returns every named downloadable file, including named inline images, with message-and-position IDs such as `456:1`. Positions reflect the current rendered message, so pass an ID from the current listing to `hey attachment save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. +`--attach` is repeatable on `hey compose`, `hey reply`, and `hey bulk-reply send`, and attachment-only messages are supported. The CLI validates and uploads every file before sending the email. `hey attachment list ` returns every named downloadable file, including named inline images. Direct files keep stable message-and-position IDs such as `456:1`; files inside embedded HTML receive opaque IDs scoped to their message. Pass either returned ID to `hey attachment save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. Organization actions take the `id` values returned by `hey box view --json`, `hey label view --json`, or `hey search --json`. Reading, replying to, and forwarding a thread take its `topic_id` instead, which `hey box view --json`, `hey label view --json`, `hey collection view --json` and `hey search --json` all carry alongside `id`. `hey box view` also returns `next_page` and accepts `--page ` to continue a box listing; it keeps `next_history_url` for the sync clients that read it, and `--page` accepts that URL as readily as the cursor inside it. Label IDs come from `hey label list`; `hey label view` returns `next_page` and `total_count`, accepts `--page ` for continuation, and supports `--all` for complete traversal. HEY creates a label while adding it to at least one thread, so `hey label create` requires thread item IDs. diff --git a/internal/cmd/attachments.go b/internal/cmd/attachments.go index 4a632e80..d394a948 100644 --- a/internal/cmd/attachments.go +++ b/internal/cmd/attachments.go @@ -2,7 +2,11 @@ package cmd import ( "context" + "crypto/sha256" + "encoding/base64" "fmt" + "strconv" + "strings" "github.com/spf13/cobra" @@ -43,7 +47,7 @@ func newAttachmentsCommand() *attachmentsCommand { attachmentsCommand.cmd = &cobra.Command{ Use: "list ", Short: "List a thread's attachments", - Long: "List every named downloadable file in a thread, including named inline images. IDs reflect the current message order; pass an ID from the current listing to attachment save.", + Long: "List every named downloadable file in a thread, including named inline images. Each returned ID identifies the same file when passed to attachment save.", Example: ` hey attachment list 12345 hey attachment list 12345 --json hey attachment list 12345 --allow-partial`, @@ -146,9 +150,11 @@ func attachmentsInThread(ctx context.Context, threadID int64) ([]threadAttachmen if loaded.Message == nil { continue } - for attachmentIndex, attachment := range htmlutil.ExtractAttachments(loaded.Message.Content) { + messageAttachments := htmlutil.ExtractAttachments(loaded.Message.Content) + ids := attachmentIDs(loaded.Entry.Id, messageAttachments) + for index, attachment := range messageAttachments { attachments = append(attachments, threadAttachment{ - ID: attachmentID(loaded.Entry.Id, attachmentIndex+1), + ID: ids[index], MessageID: loaded.Entry.Id, Filename: attachment.Filename, ContentType: attachment.ContentType, @@ -164,6 +170,68 @@ func attachmentID(messageID int64, position int) string { return fmt.Sprintf("%d:%d", messageID, position) } +func attachmentIDs(messageID int64, attachments []htmlutil.Attachment) []string { + ids := make([]string, len(attachments)) + directPosition := 0 + embeddedOccurrences := make(map[string]int) + for index, attachment := range attachments { + if !attachment.Embedded { + directPosition++ + ids[index] = attachmentID(messageID, directPosition) + continue + } + + key := embeddedAttachmentKey(attachment) + embeddedOccurrences[key]++ + ids[index] = fmt.Sprintf("%d:e-%s", messageID, key) + if embeddedOccurrences[key] > 1 { + ids[index] += fmt.Sprintf(".%d", embeddedOccurrences[key]) + } + } + return ids +} + +func embeddedAttachmentKey(attachment htmlutil.Attachment) string { + identity := "sgid\x00" + attachment.SGID + if attachment.SGID == "" { + byteSize := "" + if attachment.ByteSize != nil { + byteSize = strconv.FormatInt(*attachment.ByteSize, 10) + } + identity = strings.Join([]string{"file", attachment.URL, attachment.Filename, attachment.ContentType, byteSize}, "\x00") + } + digest := sha256.Sum256([]byte(identity)) + return base64.RawURLEncoding.EncodeToString(digest[:]) +} + +func validAttachmentSelector(selector string) bool { + if position, err := strconv.Atoi(selector); err == nil { + return position > 0 + } + if !strings.HasPrefix(selector, "e-") { + return false + } + key, occurrence, hasOccurrence := strings.Cut(strings.TrimPrefix(selector, "e-"), ".") + digest, err := base64.RawURLEncoding.DecodeString(key) + if err != nil || len(digest) != sha256.Size { + return false + } + if !hasOccurrence { + return true + } + position, err := strconv.Atoi(occurrence) + return err == nil && position > 1 && strconv.Itoa(position) == occurrence +} + +func findAttachmentByID(messageID int64, id string, attachments []htmlutil.Attachment) (htmlutil.Attachment, bool) { + for index, candidateID := range attachmentIDs(messageID, attachments) { + if candidateID == id { + return attachments[index], true + } + } + return htmlutil.Attachment{}, false +} + func formatOptionalByteSize(size *int64) string { if size == nil { return "—" diff --git a/internal/cmd/attachments_save.go b/internal/cmd/attachments_save.go index 6289c8a6..aea7e2f7 100644 --- a/internal/cmd/attachments_save.go +++ b/internal/cmd/attachments_save.go @@ -53,7 +53,7 @@ func (c *attachmentsSaveCommand) run(cmd *cobra.Command, args []string) error { return err } - messageID, position, err := parseAttachmentID(args[0]) + messageID, selector, err := parseAttachmentID(args[0]) if err != nil { return err } @@ -65,10 +65,10 @@ func (c *attachmentsSaveCommand) run(cmd *cobra.Command, args []string) error { return apierr.ErrNotFound("message", strconv.FormatInt(messageID, 10)) } attachments := htmlutil.ExtractAttachments(message.Content) - if position > len(attachments) { + attachment, found := findAttachmentByID(messageID, fmt.Sprintf("%d:%s", messageID, selector), attachments) + if !found { return apierr.ErrNotFound("attachment", args[0]) } - attachment := attachments[position-1] destination, err := attachmentDestination(c.output, attachment.Filename) if err != nil { @@ -101,17 +101,19 @@ func savedAttachmentForMarkdown(attachment savedAttachment) savedAttachment { return attachment } -func parseAttachmentID(id string) (int64, int, error) { +func parseAttachmentID(id string) (int64, string, error) { parts := strings.Split(id, ":") if len(parts) != 2 { - return 0, 0, apierr.ErrUsage(fmt.Sprintf("invalid attachment ID: %s", id)) + return 0, "", apierr.ErrUsage(fmt.Sprintf("invalid attachment ID: %s", id)) } messageID, messageErr := strconv.ParseInt(parts[0], 10, 64) - position, positionErr := strconv.Atoi(parts[1]) - if messageErr != nil || positionErr != nil || messageID <= 0 || position <= 0 { - return 0, 0, apierr.ErrUsage(fmt.Sprintf("invalid attachment ID: %s", id)) + if messageErr != nil || messageID <= 0 || !validAttachmentSelector(parts[1]) { + return 0, "", apierr.ErrUsage(fmt.Sprintf("invalid attachment ID: %s", id)) } - return messageID, position, nil + if position, err := strconv.Atoi(parts[1]); err == nil { + return messageID, strconv.Itoa(position), nil + } + return messageID, parts[1], nil } func attachmentDestination(outputPath, filename string) (string, error) { diff --git a/internal/cmd/attachments_test.go b/internal/cmd/attachments_test.go index 1da56d20..0ee0ddec 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + stdhtml "html" "net/http" "net/http/httptest" "os" @@ -14,6 +15,7 @@ import ( "testing" "github.com/basecamp/hey-cli/internal/apierr" + "github.com/basecamp/hey-cli/internal/htmlutil" ) type attachmentServerState struct { @@ -146,6 +148,22 @@ func runAttachmentCommand(t *testing.T, server *httptest.Server, args ...string) return output.String(), err } +func renderedEmbeddedHTMLFigure(t *testing.T, content string) string { + t.Helper() + attributes, err := json.Marshal(struct { + ContentType string `json:"contentType"` + Content string `json:"content"` + }{ContentType: "text/html", Content: content}) + if err != nil { + t.Fatal(err) + } + return `
` +} + +func renderedCanonicalEmbeddedHTML(content string) string { + return `` +} + func runAttachmentCommandWithStdin(t *testing.T, server *httptest.Server, input string, args ...string) (string, error) { t.Helper() stdin, err := os.CreateTemp(t.TempDir(), "stdin-*") @@ -196,9 +214,14 @@ func TestAttachmentsListsFilesFromKnownThread(t *testing.T) { } func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { - content := `
-
-` + embeddedImage := `` + embeddedPDF := renderedCanonicalEmbeddedHTML(``) + directPDF := `` + content := strings.Join([]string{ + renderedEmbeddedHTMLFigure(t, embeddedImage), + renderedEmbeddedHTMLFigure(t, embeddedPDF), + directPDF, + }, "\n") server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -210,6 +233,9 @@ func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { case "/rails/active_storage/blobs/conference-agenda.pdf": w.Header().Set("Content-Type", "application/pdf") _, _ = w.Write([]byte("conference agenda")) + case "/rails/active_storage/blobs/venue-map.pdf": + w.Header().Set("Content-Type", "application/pdf") + _, _ = w.Write([]byte("venue map")) default: t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) http.Error(w, "not found", http.StatusNotFound) @@ -230,27 +256,35 @@ func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { if len(response.Data) != 3 { t.Fatalf("listed attachments = %+v, want three named files", response.Data) } - for index, want := range []struct { + for index, filename := range []string{"conference-logo.png", "conference-agenda.pdf", "venue-map.pdf"} { + if response.Data[index].Filename != filename { + t.Errorf("attachment %d = %+v, want filename %q", index, response.Data[index], filename) + } + } + if !strings.HasPrefix(response.Data[0].ID, "101:e-") || !strings.HasPrefix(response.Data[1].ID, "101:e-") || response.Data[0].ID == response.Data[1].ID { + t.Errorf("embedded attachment IDs = %q, %q, want distinct embedded IDs", response.Data[0].ID, response.Data[1].ID) + } + if response.Data[2].ID != "101:1" { + t.Errorf("direct attachment ID = %q, want its released ID 101:1", response.Data[2].ID) + } + + for _, test := range []struct { id string filename string + content string }{ - {id: "101:1", filename: "conference-logo.png"}, - {id: "101:2", filename: "conference-agenda.pdf"}, - {id: "101:3", filename: "venue-map.pdf"}, + {id: response.Data[1].ID, filename: "conference-agenda.pdf", content: "conference agenda"}, + {id: "101:01", filename: "venue-map.pdf", content: "venue map"}, } { - if response.Data[index].ID != want.id || response.Data[index].Filename != want.filename { - t.Errorf("attachment %d = %+v, want ID %q and filename %q", index, response.Data[index], want.id, want.filename) + destination := filepath.Join(t.TempDir(), test.filename) + if _, err := runAttachmentCommand(t, server, "attachment", "save", test.id, "--output", destination); err != nil { + t.Fatal(err) + } + if got, err := os.ReadFile(destination); err != nil { + t.Fatal(err) + } else if string(got) != test.content { + t.Errorf("saved attachment = %q, want %q", got, test.content) } - } - - destination := filepath.Join(t.TempDir(), "conference-agenda.pdf") - if _, err := runAttachmentCommand(t, server, "attachment", "save", "101:2", "--output", destination); err != nil { - t.Fatal(err) - } - if got, err := os.ReadFile(destination); err != nil { - t.Fatal(err) - } else if string(got) != "conference agenda" { - t.Errorf("saved attachment = %q", got) } } @@ -640,12 +674,61 @@ func TestAppendUploadedAttachmentsSupportsAttachmentOnlyMessages(t *testing.T) { } } +func TestAttachmentIDsPreserveDirectPositionsAsEmbeddedFilesAreAdded(t *testing.T) { + directReport := htmlutil.Attachment{SGID: "sgid-report"} + directMap := htmlutil.Attachment{SGID: "sgid-map"} + embeddedLogo := htmlutil.Attachment{SGID: "sgid-logo", Embedded: true} + embeddedAgenda := htmlutil.Attachment{SGID: "sgid-agenda", Embedded: true} + + legacy := attachmentIDs(101, []htmlutil.Attachment{directReport, directMap}) + expanded := attachmentIDs(101, []htmlutil.Attachment{embeddedLogo, directReport, embeddedAgenda, directMap}) + if legacy[0] != "101:1" || legacy[1] != "101:2" || expanded[1] != legacy[0] || expanded[3] != legacy[1] { + t.Errorf("legacy IDs = %v, expanded IDs = %v", legacy, expanded) + } + if moved := attachmentIDs(101, []htmlutil.Attachment{embeddedAgenda, embeddedLogo}); expanded[0] != moved[1] { + t.Errorf("embedded logo ID changed from %q to %q when its position changed", expanded[0], moved[1]) + } +} + +func TestAttachmentIDsCoverFilesWithoutSGIDsAndDuplicateRepresentations(t *testing.T) { + withoutSGID := htmlutil.Attachment{ + URL: "/rails/blobs/report.pdf", + Filename: "report.pdf", + ContentType: "application/pdf", + Embedded: true, + } + first := attachmentIDs(101, []htmlutil.Attachment{withoutSGID})[0] + moved := attachmentIDs(101, []htmlutil.Attachment{{SGID: "sgid-logo", Embedded: true}, withoutSGID})[1] + if first != moved { + t.Errorf("fallback attachment ID changed from %q to %q when its position changed", first, moved) + } + + duplicateAttachments := []htmlutil.Attachment{withoutSGID, withoutSGID} + duplicates := attachmentIDs(101, duplicateAttachments) + if duplicates[0] == duplicates[1] || !strings.HasSuffix(duplicates[1], ".2") { + t.Errorf("duplicate attachment IDs = %v, want distinct occurrence suffixes", duplicates) + } + for _, id := range duplicates { + if _, _, err := parseAttachmentID(id); err != nil { + t.Errorf("parseAttachmentID(%q): %v", id, err) + } + if _, found := findAttachmentByID(101, id, duplicateAttachments); !found { + t.Errorf("findAttachmentByID(%q) did not resolve a duplicate representation", id) + } + } +} + func TestParseAttachmentID(t *testing.T) { - messageID, position, err := parseAttachmentID("101:2") - if err != nil || messageID != 101 || position != 2 { - t.Fatalf("parseAttachmentID = %d, %d, %v", messageID, position, err) + messageID, selector, err := parseAttachmentID("0101:+2") + if err != nil || messageID != 101 || selector != "2" { + t.Fatalf("parseAttachmentID = %d, %q, %v", messageID, selector, err) + } + embeddedID := attachmentIDs(101, []htmlutil.Attachment{{SGID: "sgid-embedded", Embedded: true}})[0] + messageID, selector, err = parseAttachmentID(embeddedID) + if err != nil || messageID != 101 || !strings.HasPrefix(selector, "e-") { + t.Fatalf("parseAttachmentID(%q) = %d, %q, %v", embeddedID, messageID, selector, err) } - for _, id := range []string{"", "101", "101:0", "x:1", "1:2:3"} { + for _, id := range []string{"", "101", "101:0", "101:e-short", "101:e-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.1", "101:e-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.02", "x:1", "1:2:3"} { if _, _, err := parseAttachmentID(id); err == nil { t.Errorf("parseAttachmentID(%q) succeeded", id) } diff --git a/internal/htmlutil/htmlutil.go b/internal/htmlutil/htmlutil.go index ec7afe50..99123e6c 100644 --- a/internal/htmlutil/htmlutil.go +++ b/internal/htmlutil/htmlutil.go @@ -64,6 +64,8 @@ type Attachment struct { ContentType string ByteSize *int64 SGID string + // Embedded reports whether the file comes from an opaque embedded HTML body. + Embedded bool } // ExtractAttachments returns downloadable files in their document order. @@ -346,16 +348,21 @@ func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { if n.Type == html.ElementNode { switch n.Data { case "action-text-attachment": - byteSize := parseAttachmentByteSize(getAttr(n, "filesize")) attachment := Attachment{ URL: getAttr(n, "url"), Filename: getAttr(n, "filename"), ContentType: getAttr(n, "content-type"), - ByteSize: byteSize, + ByteSize: parseAttachmentByteSize(getAttr(n, "filesize")), SGID: getAttr(n, "sgid"), + Embedded: depth > 0, } - if attachment.URL != "" && attachment.Filename != "" { + switch { + case attachment.URL != "" && attachment.Filename != "": *attachments = append(*attachments, attachment) + case isHTMLContentType(attachment.ContentType) && getAttr(n, "content") != "": + if doc := parseEmbeddedContent(getAttr(n, "content"), depth); doc != nil { + findAttachments(doc, attachments, depth+1) + } } case "figure": trix := parseTrixAttachment(n) @@ -368,6 +375,7 @@ func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { ContentType: trix.ContentType, ByteSize: nonnegativeAttachmentByteSize(trix.Filesize), SGID: trix.SGID, + Embedded: depth > 0, }) case trix.Content != "": // An inbound email's files are inside the embedded markup, not @@ -384,6 +392,11 @@ func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { } } +func isHTMLContentType(contentType string) bool { + contentType = strings.ToLower(strings.TrimSpace(contentType)) + return contentType == "text/html" || strings.HasPrefix(contentType, "text/html;") +} + func isImageContentType(contentType string) bool { contentType = strings.ToLower(strings.TrimSpace(contentType)) return contentType == "image" || strings.HasPrefix(contentType, "image/") diff --git a/internal/htmlutil/htmlutil_test.go b/internal/htmlutil/htmlutil_test.go index c7c6f7ee..14f335c5 100644 --- a/internal/htmlutil/htmlutil_test.go +++ b/internal/htmlutil/htmlutil_test.go @@ -1,10 +1,28 @@ package htmlutil import ( + "encoding/json" + stdhtml "html" "strings" "testing" ) +func embeddedHTMLFigure(t *testing.T, content string) string { + t.Helper() + attributes, err := json.Marshal(struct { + ContentType string `json:"contentType"` + Content string `json:"content"` + }{ContentType: "text/html", Content: content}) + if err != nil { + t.Fatal(err) + } + return `
` +} + +func canonicalEmbeddedHTML(content string) string { + return `` +} + func TestToTextPlain(t *testing.T) { got := ToText("hello world") if got != "hello world" { @@ -169,10 +187,9 @@ func TestToTextTrixFigure(t *testing.T) { } func TestToTextEmbeddedContentStopsRecursing(t *testing.T) { - nested := `
` - for range embeddedContentDepthLimit + 2 { - nested = `
` + nested := "

innermost

" + for range embeddedContentDepthLimit + 1 { + nested = embeddedHTMLFigure(t, nested) } if got := ToText(nested); strings.Contains(got, "innermost") { @@ -209,15 +226,33 @@ func TestExtractAttachmentsInsideEmbeddedHTMLAttachment(t *testing.T) { } } +func TestExtractAttachmentsInsideCanonicalEmbeddedHTMLAttachment(t *testing.T) { + file := `` + content := embeddedHTMLFigure(t, canonicalEmbeddedHTML(file)) + + attachments := ExtractAttachments(content) + if len(attachments) != 1 { + t.Fatalf("ExtractAttachments = %+v, want the file inside the canonical HTML attachment", attachments) + } + got := attachments[0] + if got.Filename != "deep.pdf" || got.URL != "/rails/blobs/deep.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-deep" || got.ByteSize == nil || *got.ByteSize != 128 || !got.Embedded { + t.Errorf("canonical embedded attachment = %+v", got) + } +} + func TestExtractAttachmentsEmbeddedContentStopsRecursing(t *testing.T) { - nested := `
` - for range embeddedContentDepthLimit + 2 { - nested = `
` + file := `` + withinLimit := file + for range embeddedContentDepthLimit { + withinLimit = embeddedHTMLFigure(t, withinLimit) + } + if attachments := ExtractAttachments(withinLimit); len(attachments) != 1 { + t.Fatalf("ExtractAttachments = %+v, want the file at the nesting limit", attachments) } - if attachments := ExtractAttachments(nested); len(attachments) != 0 { - t.Errorf("ExtractAttachments = %+v, should stop before the innermost level", attachments) + beyondLimit := embeddedHTMLFigure(t, withinLimit) + if attachments := ExtractAttachments(beyondLimit); len(attachments) != 0 { + t.Errorf("ExtractAttachments = %+v, should stop before the file beyond the nesting limit", attachments) } } diff --git a/internal/htmlutil/markdown_test.go b/internal/htmlutil/markdown_test.go index 99f02c84..ad5accee 100644 --- a/internal/htmlutil/markdown_test.go +++ b/internal/htmlutil/markdown_test.go @@ -450,10 +450,9 @@ func TestToMarkdownEmbeddedHTMLAttachment(t *testing.T) { } func TestToMarkdownEmbeddedContentStopsRecursing(t *testing.T) { - nested := `
` - for range embeddedContentDepthLimit + 2 { - nested = `
` + nested := "

innermost

" + for range embeddedContentDepthLimit + 1 { + nested = embeddedHTMLFigure(t, nested) } if got := toMarkdown(nested); strings.Contains(got, "innermost") { diff --git a/skills/hey/SKILL.md b/skills/hey/SKILL.md index e86dad42..747ffe67 100644 --- a/skills/hey/SKILL.md +++ b/skills/hey/SKILL.md @@ -459,7 +459,7 @@ hey attachment save 67890:1 --output ./reports # Save into a directory hey attachment save 67890:1 --output ./report.pdf --force ``` -An attachment ID combines its message ID and position, so `67890:1` identifies the first attachment in message `67890`. Saving uses the original filename unless `--output` names a destination. Existing files are preserved unless `--force` is set. +Direct attachment IDs combine the message ID and position, so `67890:1` identifies the first direct attachment in message `67890`. Named downloadable files inside embedded HTML, including named inline images, use opaque IDs scoped to their message. Pass the ID returned by `hey attachment list` to `hey attachment save`. Saving uses the original filename unless `--output` names a destination. Existing files are preserved unless `--force` is set. ### Email - Reply, Forward & Compose From 53c6f876b028801d33dd1e8195296e4e37edc72c Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 8 Sep 2026 11:31:25 -0400 Subject: [PATCH 4/4] Restrict attachment downloads to HEY blobs --- internal/attachments/save.go | 29 +++++++++- internal/attachments/save_test.go | 52 ++++++++++++++++-- internal/cmd/attachments.go | 2 +- internal/cmd/attachments_test.go | 85 ++++++++++++++++++++++++++++++ internal/htmlutil/htmlutil.go | 6 ++- internal/htmlutil/htmlutil_test.go | 39 ++++++++++---- internal/tui/mail_test.go | 4 +- 7 files changed, 196 insertions(+), 21 deletions(-) diff --git a/internal/attachments/save.go b/internal/attachments/save.go index feab970e..89177224 100644 --- a/internal/attachments/save.go +++ b/internal/attachments/save.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path" "path/filepath" @@ -19,6 +20,25 @@ type Downloader interface { DownloadBlob(context.Context, string, io.Writer) (int64, http.Header, error) } +const heyBlobPathPrefix = "/rails/active_storage/blobs/" + +// IsHEYBlobURL reports whether source is a clean relative path to HEY's blob storage. +func IsHEYBlobURL(source string) bool { + parsed, err := url.Parse(source) + if err != nil || parsed.IsAbs() || parsed.Host != "" || parsed.User != nil || parsed.Opaque != "" || parsed.RawQuery != "" || parsed.Fragment != "" { + return false + } + if parsed.Path == "" || strings.Contains(parsed.Path, `\`) || path.Clean(parsed.Path) != parsed.Path || !strings.HasPrefix(parsed.Path, heyBlobPathPrefix) { + return false + } + for _, character := range parsed.Path { + if unicode.IsControl(character) { + return false + } + } + return len(parsed.Path) > len(heyBlobPathPrefix) +} + func Destination(outputPath, filename string) (string, error) { if outputPath == "" { return PortableFilename(filename) @@ -61,7 +81,7 @@ func PortableFilename(filename string) (string, error) { // SaveBytes safely writes data to destination. Existing paths are preserved unless force is set. func SaveBytes(destination string, data []byte, force bool) (int64, error) { - return Save(context.Background(), byteDownloader(data), destination, "", force) + return save(context.Background(), byteDownloader(data), destination, "", force) } type byteDownloader []byte @@ -72,6 +92,13 @@ func (data byteDownloader) DownloadBlob(_ context.Context, _ string, writer io.W } func Save(ctx context.Context, downloader Downloader, destination, sourceURL string, force bool) (int64, error) { + if !IsHEYBlobURL(sourceURL) { + return 0, apierr.ErrAPI(0, "attachment URL does not identify a HEY blob") + } + return save(ctx, downloader, destination, sourceURL, force) +} + +func save(ctx context.Context, downloader Downloader, destination, sourceURL string, force bool) (int64, error) { if !force { if _, err := os.Lstat(destination); err == nil { return 0, apierr.ErrUsage(fmt.Sprintf("destination already exists: %s (use --force to replace it)", destination)) diff --git a/internal/attachments/save_test.go b/internal/attachments/save_test.go index 35002e58..04b251d0 100644 --- a/internal/attachments/save_test.go +++ b/internal/attachments/save_test.go @@ -72,20 +72,64 @@ func TestSaveBytesWritesContentSafely(t *testing.T) { assertFileContent(t, destination, "Start,End\n09:00,10:00\n") } +func TestIsHEYBlobURL(t *testing.T) { + for _, test := range []struct { + source string + want bool + }{ + {source: "/rails/active_storage/blobs/redirect/signed/report.pdf", want: true}, + {source: "/rails/active_storage/blobs/proxy/signed/report%20copy.pdf", want: true}, + {source: "/identity.json"}, + {source: "https://app.hey.com/rails/active_storage/blobs/redirect/signed/report.pdf"}, + {source: "//app.hey.com/rails/active_storage/blobs/redirect/signed/report.pdf"}, + {source: "/rails/active_storage/blobs/../identity.json"}, + {source: "/rails/active_storage/blobs/%2e%2e/identity.json"}, + {source: `/rails/active_storage/blobs/redirect/signed/..\identity.json`}, + {source: "/rails/active_storage/blobs/redirect/signed/report.pdf?download=1"}, + {source: "/rails/active_storage/blobs/redirect/signed/report.pdf#fragment"}, + {source: "/rails/active_storage/blobs/"}, + } { + if got := IsHEYBlobURL(test.source); got != test.want { + t.Errorf("IsHEYBlobURL(%q) = %t, want %t", test.source, got, test.want) + } + } +} + +func TestSaveRejectsNonBlobURLBeforeDownload(t *testing.T) { + called := false + downloader := downloadFunc(func(_ context.Context, _ string, _ io.Writer) (int64, http.Header, error) { + called = true + return 0, nil, nil + }) + destination := filepath.Join(t.TempDir(), "invoice.pdf") + + _, err := Save(context.Background(), downloader, destination, "/identity.json", true) + var saveErr *apierr.Error + if !errors.As(err, &saveErr) || saveErr.Code != apierr.CodeAPI { + t.Fatalf("Save error = %v", err) + } + if called { + t.Error("Save requested a non-blob URL") + } + if _, err := os.Stat(destination); !os.IsNotExist(err) { + t.Errorf("Save created a destination for a non-blob URL: %v", err) + } +} + func TestSavePreservesExistingFileUnlessForced(t *testing.T) { destination := filepath.Join(t.TempDir(), "quarterly-report.pdf") if err := os.WriteFile(destination, []byte("keep me"), 0o600); err != nil { t.Fatal(err) } - _, err := Save(context.Background(), writeDownload("new report"), destination, "/report.pdf", false) + _, err := Save(context.Background(), writeDownload("new report"), destination, "/rails/active_storage/blobs/redirect/signed/report.pdf", false) var saveErr *apierr.Error if !errors.As(err, &saveErr) || saveErr.Code != "usage" || !strings.Contains(saveErr.Message, "use --force") { t.Fatalf("existing destination error = %v", err) } assertFileContent(t, destination, "keep me") - written, err := Save(context.Background(), writeDownload("new report"), destination, "/report.pdf", true) + written, err := Save(context.Background(), writeDownload("new report"), destination, "/rails/active_storage/blobs/redirect/signed/report.pdf", true) if err != nil { t.Fatal(err) } @@ -109,7 +153,7 @@ func TestSaveDoesNotReplaceFileCreatedDuringDownload(t *testing.T) { return int64(written), nil, nil } - _, err := Save(context.Background(), downloadFunc(downloader), destination, "/report.pdf", false) + _, err := Save(context.Background(), downloadFunc(downloader), destination, "/rails/active_storage/blobs/redirect/signed/report.pdf", false) var saveErr *apierr.Error if !errors.As(err, &saveErr) || saveErr.Code != "usage" { t.Fatalf("concurrent destination error = %v", err) @@ -130,7 +174,7 @@ func TestSaveRemovesPartialFileAndPreservesDownloadError(t *testing.T) { return int64(written), nil, downloadErr } - written, err := Save(context.Background(), downloadFunc(downloader), destination, "/report.pdf", false) + written, err := Save(context.Background(), downloadFunc(downloader), destination, "/rails/active_storage/blobs/redirect/signed/report.pdf", false) if !errors.Is(err, downloadErr) { t.Fatalf("download error = %v, want %v", err, downloadErr) } diff --git a/internal/cmd/attachments.go b/internal/cmd/attachments.go index d394a948..1121edf0 100644 --- a/internal/cmd/attachments.go +++ b/internal/cmd/attachments.go @@ -213,7 +213,7 @@ func validAttachmentSelector(selector string) bool { } key, occurrence, hasOccurrence := strings.Cut(strings.TrimPrefix(selector, "e-"), ".") digest, err := base64.RawURLEncoding.DecodeString(key) - if err != nil || len(digest) != sha256.Size { + if err != nil || len(digest) != sha256.Size || base64.RawURLEncoding.EncodeToString(digest) != key { return false } if !hasOccurrence { diff --git a/internal/cmd/attachments_test.go b/internal/cmd/attachments_test.go index 0ee0ddec..4aa4a25c 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "github.com/basecamp/hey-cli/internal/apierr" @@ -288,6 +289,90 @@ func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { } } +func TestAttachmentsRejectNonBlobURLs(t *testing.T) { + malicious := `` + valid := `` + content := renderedEmbeddedHTMLFigure(t, malicious) + valid + maliciousID := attachmentIDs(101, []htmlutil.Attachment{{ + URL: "/identity.json", + Filename: "invoice.pdf", + SGID: "sgid-identity", + Embedded: true, + }})[0] + var identityRequests atomic.Int64 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/topics/42/entries.json": + _, _ = w.Write([]byte(`[{"id":101,"kind":"message"}]`)) + case "/messages/101.json": + _ = json.NewEncoder(w).Encode(map[string]any{"id": 101, "content": content}) + case "/identity.json": + identityRequests.Add(1) + _, _ = w.Write([]byte(`{"email_address":"private@example.com"}`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + stdout, err := runAttachmentCommand(t, server, "attachment", "list", "42") + if err != nil { + t.Fatal(err) + } + var response struct { + Data []threadAttachment `json:"data"` + } + if err := json.Unmarshal([]byte(stdout), &response); err != nil { + t.Fatalf("decode response: %v\n%s", err, stdout) + } + if len(response.Data) != 1 || response.Data[0].Filename != "report.pdf" || response.Data[0].ID != "101:1" { + t.Errorf("listed attachments = %+v, want only the HEY blob", response.Data) + } + + _, err = runAttachmentCommand(t, server, "attachment", "save", maliciousID, "--output", filepath.Join(t.TempDir(), "invoice.pdf")) + var saveErr *apierr.Error + if !errors.As(err, &saveErr) || saveErr.Code != apierr.CodeNotFound { + t.Fatalf("saving a rejected non-blob attachment error = %v, want not_found", err) + } + if got := identityRequests.Load(); got != 0 { + t.Errorf("identity endpoint received %d requests", got) + } +} + +func TestAttachmentSaveRejectsNonCanonicalOpaqueIDsBeforeRequest(t *testing.T) { + validID := attachmentIDs(101, []htmlutil.Attachment{{SGID: "sgid-report", Embedded: true}})[0] + keyStart := strings.Index(validID, ":e-") + len(":e-") + withNewline := validID[:keyStart+8] + "\r\n" + validID[keyStart+8:] + + const base64URLAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_" + last := strings.IndexByte(base64URLAlphabet, validID[len(validID)-1]) + if last < 0 || last%4 != 0 { + t.Fatalf("opaque ID has unexpected final base64 character: %q", validID) + } + withTrailingBits := validID[:len(validID)-1] + string(base64URLAlphabet[last+1]) + + var requests atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + http.Error(w, "unexpected request", http.StatusInternalServerError) + })) + t.Cleanup(server.Close) + + for _, id := range []string{withNewline, withTrailingBits} { + _, err := runAttachmentCommand(t, server, "attachment", "save", id) + var commandErr *apierr.Error + if !errors.As(err, &commandErr) || commandErr.Code != apierr.CodeUsage { + t.Errorf("attachment save %q error = %v, want usage", id, err) + } + } + if got := requests.Load(); got != 0 { + t.Errorf("malformed opaque IDs caused %d requests", got) + } +} + // A thread longer than one page is walked by following HEY's cursor, so each attachment // is listed once and the list is not claimed to be truncated. func TestAttachmentsFollowsTheCursorThroughALongThread(t *testing.T) { diff --git a/internal/htmlutil/htmlutil.go b/internal/htmlutil/htmlutil.go index 99123e6c..4d69dcc3 100644 --- a/internal/htmlutil/htmlutil.go +++ b/internal/htmlutil/htmlutil.go @@ -7,6 +7,8 @@ import ( "strings" "golang.org/x/net/html" + + attachmentfiles "github.com/basecamp/hey-cli/internal/attachments" ) // ToText converts HTML content to plain text, preserving basic structure. @@ -357,7 +359,7 @@ func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { Embedded: depth > 0, } switch { - case attachment.URL != "" && attachment.Filename != "": + case attachmentfiles.IsHEYBlobURL(attachment.URL) && attachment.Filename != "": *attachments = append(*attachments, attachment) case isHTMLContentType(attachment.ContentType) && getAttr(n, "content") != "": if doc := parseEmbeddedContent(getAttr(n, "content"), depth); doc != nil { @@ -368,7 +370,7 @@ func findAttachments(n *html.Node, attachments *[]Attachment, depth int) { trix := parseTrixAttachment(n) switch { case trix == nil: - case trix.URL != "" && trix.Filename != "": + case attachmentfiles.IsHEYBlobURL(trix.URL) && trix.Filename != "": *attachments = append(*attachments, Attachment{ URL: trix.URL, Filename: trix.Filename, diff --git a/internal/htmlutil/htmlutil_test.go b/internal/htmlutil/htmlutil_test.go index 14f335c5..7ee42300 100644 --- a/internal/htmlutil/htmlutil_test.go +++ b/internal/htmlutil/htmlutil_test.go @@ -214,20 +214,20 @@ func TestExtractAttachmentsSkipsEmbeddedHTMLAttachment(t *testing.T) { func TestExtractAttachmentsInsideEmbeddedHTMLAttachment(t *testing.T) { // An HTML email from outside HEY arrives as one text/html trix attachment // whose content string holds the original markup, files included. - content := `
` + content := `
` attachments := ExtractAttachments(content) if len(attachments) != 1 { t.Fatalf("ExtractAttachments = %+v, want the file inside the embedded body", attachments) } got := attachments[0] - if got.Filename != "payslip.pdf" || got.URL != "/rails/blobs/payslip.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-1" || got.ByteSize == nil || *got.ByteSize != 44218 { + if got.Filename != "payslip.pdf" || got.URL != "/rails/active_storage/blobs/redirect/signed/payslip.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-1" || got.ByteSize == nil || *got.ByteSize != 44218 { t.Errorf("embedded attachment = %+v", got) } } func TestExtractAttachmentsInsideCanonicalEmbeddedHTMLAttachment(t *testing.T) { - file := `` + file := `` content := embeddedHTMLFigure(t, canonicalEmbeddedHTML(file)) attachments := ExtractAttachments(content) @@ -235,13 +235,13 @@ func TestExtractAttachmentsInsideCanonicalEmbeddedHTMLAttachment(t *testing.T) { t.Fatalf("ExtractAttachments = %+v, want the file inside the canonical HTML attachment", attachments) } got := attachments[0] - if got.Filename != "deep.pdf" || got.URL != "/rails/blobs/deep.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-deep" || got.ByteSize == nil || *got.ByteSize != 128 || !got.Embedded { + if got.Filename != "deep.pdf" || got.URL != "/rails/active_storage/blobs/redirect/signed/deep.pdf" || got.ContentType != "application/pdf" || got.SGID != "sgid-deep" || got.ByteSize == nil || *got.ByteSize != 128 || !got.Embedded { t.Errorf("canonical embedded attachment = %+v", got) } } func TestExtractAttachmentsEmbeddedContentStopsRecursing(t *testing.T) { - file := `` + file := `` withinLimit := file for range embeddedContentDepthLimit { withinLimit = embeddedHTMLFigure(t, withinLimit) @@ -257,8 +257,8 @@ func TestExtractAttachmentsEmbeddedContentStopsRecursing(t *testing.T) { } func TestExtractAttachments(t *testing.T) { - h := ` -
` + h := ` +
` attachments := ExtractAttachments(h) if len(attachments) != 2 { t.Fatalf("ExtractAttachments got %d attachments, want 2", len(attachments)) @@ -266,14 +266,14 @@ func TestExtractAttachments(t *testing.T) { if attachments[0].Filename != "quarterly-report.pdf" || attachments[0].ContentType != "application/pdf" || attachments[0].ByteSize == nil || *attachments[0].ByteSize != 128 || attachments[0].SGID != "sgid-1" { t.Errorf("canonical attachment = %+v", attachments[0]) } - if attachments[1].Filename != "photo.png" || attachments[1].URL != "/rails/blobs/photo.png" || attachments[1].ByteSize == nil || *attachments[1].ByteSize != 256 || attachments[1].SGID != "sgid-2" { + if attachments[1].Filename != "photo.png" || attachments[1].URL != "/rails/active_storage/blobs/redirect/signed/photo.png" || attachments[1].ByteSize == nil || *attachments[1].ByteSize != 256 || attachments[1].SGID != "sgid-2" { t.Errorf("Trix attachment = %+v", attachments[1]) } } func TestExtractAttachmentsDistinguishesEmptyFromUnknownSize(t *testing.T) { - h := ` -
` + h := ` +
` attachments := ExtractAttachments(h) if len(attachments) != 2 { t.Fatalf("ExtractAttachments got %d attachments, want 2", len(attachments)) @@ -288,12 +288,29 @@ func TestExtractAttachmentsDistinguishesEmptyFromUnknownSize(t *testing.T) { func TestExtractAttachmentsSkipsIncompleteElements(t *testing.T) { h := ` -
` +
` if attachments := ExtractAttachments(h); len(attachments) != 0 { t.Errorf("ExtractAttachments = %+v, want none", attachments) } } +func TestExtractAttachmentsOnlyReturnsHEYBlobPaths(t *testing.T) { + embedded := embeddedHTMLFigure(t, ` +
+`) + content := embedded + ` +
+
+ + +` + + attachments := ExtractAttachments(content) + if len(attachments) != 2 || attachments[0].Filename != "embedded.pdf" || attachments[1].Filename != "direct.pdf" { + t.Errorf("ExtractAttachments = %+v, want only the embedded and direct HEY blobs", attachments) + } +} + func TestExtractImageURLs(t *testing.T) { h := `

Hello

` urls := ExtractImageURLs(h) diff --git a/internal/tui/mail_test.go b/internal/tui/mail_test.go index 9ff6a482..8da197f8 100644 --- a/internal/tui/mail_test.go +++ b/internal/tui/mail_test.go @@ -2104,8 +2104,8 @@ func TestMailViewDownloadsImageDataOnlyForKittyRenderer(t *testing.T) { _, _ = w.Write([]byte(`[{"id":501,"kind":"message"}]`)) case "/messages/501.json": w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"id":501,"content":""}`)) - case "/rails/blobs/chart.png": + _, _ = w.Write([]byte(`{"id":501,"content":""}`)) + case "/rails/active_storage/blobs/redirect/signed/chart.png": imageRequests.Add(1) w.Header().Set("Content-Type", "image/png") _, _ = w.Write(imageData)