diff --git a/docs/cli.md b/docs/cli.md index f860572e..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 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. 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/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 a9d4aeff..1121edf0 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,6 +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. 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`, @@ -145,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, @@ -163,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 || base64.RawURLEncoding.EncodeToString(digest) != key { + 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 40fb3913..4aa4a25c 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -5,15 +5,18 @@ import ( "encoding/json" "errors" "fmt" + stdhtml "html" "net/http" "net/http/httptest" "os" "path/filepath" "strings" "sync" + "sync/atomic" "testing" "github.com/basecamp/hey-cli/internal/apierr" + "github.com/basecamp/hey-cli/internal/htmlutil" ) type attachmentServerState struct { @@ -146,6 +149,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-*") @@ -195,6 +214,165 @@ func TestAttachmentsListsFilesFromKnownThread(t *testing.T) { } } +func TestAttachmentsListsAndSavesNamedFilesInRenderedOrder(t *testing.T) { + 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") + 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")) + 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) + } + })) + 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, 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: response.Data[1].ID, filename: "conference-agenda.pdf", content: "conference agenda"}, + {id: "101:01", filename: "venue-map.pdf", content: "venue map"}, + } { + 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) + } + } +} + +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) { @@ -581,12 +759,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 597c1adb..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. @@ -64,6 +66,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. @@ -73,7 +77,7 @@ func ExtractAttachments(s string) []Attachment { return nil } var attachments []Attachment - findAttachments(doc, &attachments) + findAttachments(doc, &attachments, 0) return attachments } @@ -342,38 +346,59 @@ 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": - 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 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 { + findAttachments(doc, attachments, depth+1) + } } case "figure": - if trix := parseTrixAttachment(n); trix != nil && trix.URL != "" && trix.Filename != "" { + trix := parseTrixAttachment(n) + switch { + case trix == nil: + case attachmentfiles.IsHEYBlobURL(trix.URL) && trix.Filename != "": *attachments = append(*attachments, Attachment{ URL: trix.URL, Filename: trix.Filename, 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 + // 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) } } +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 28e1d3f5..7ee42300 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") { @@ -194,9 +211,54 @@ 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/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 := `` + 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/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 := `` + 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) + } + + 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) + } +} + func TestExtractAttachments(t *testing.T) { - h := ` -
` + h := ` +
` attachments := ExtractAttachments(h) if len(attachments) != 2 { t.Fatalf("ExtractAttachments got %d attachments, want 2", len(attachments)) @@ -204,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)) @@ -226,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/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/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) 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