Skip to content

Commit 5e4cd61

Browse files
chrischallclaude
chrischall
and
claude
committed
fix(gmail): stable plain TSV for --text and time-bounded PDF extraction
Address review on the --inline/--text attachment modes: - --plain output stays a stable one-record-per-line TSV: values containing tabs/newlines (extracted text) are emitted as a single Go-quoted field, with a regression test that round-trips multiline text via strconv.Unquote. - PDF parsing of attacker-controlled attachments is now bounded: a 10s timeout (on top of the existing 3 MiB input cap and panic recovery) so a pathological PDF cannot hang the command or a long-lived MCP server. - Adversarial fixture test: corrupt PDF input surfaces a clear reason instead of text, without panicking. Co-Authored-By: Claude Fable 5 <[email protected]>
1 parent e0dd518 commit 5e4cd61

4 files changed

Lines changed: 134 additions & 2 deletions

File tree

‎internal/cmd/execute_gmail_attachment_text_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package cmd
33
import (
44
"bytes"
55
"fmt"
6+
"strconv"
67
"strings"
78
"testing"
89
)
@@ -190,3 +191,57 @@ func TestExecute_GmailAttachment_Text_NoMetadata_SniffsPlainText(t *testing.T) {
190191
t.Fatalf("text=%#v (reason=%v)", parsed["text"], parsed["reason"])
191192
}
192193
}
194+
195+
// --plain output is documented as stable TSV; multiline/tabbed extracted text
196+
// must be escaped into a single field, not printed verbatim.
197+
func TestExecute_GmailAttachment_Text_PlainOutput_EscapesMultiline(t *testing.T) {
198+
content := "line one\nline two\twith tab"
199+
svc := newGmailAttachmentTestService(t, []byte(content), "notes.txt", "text/plain")
200+
201+
result := executeWithGmailTestService(t, []string{
202+
"--plain", "--account", "[email protected]",
203+
"gmail", "attachment", "m1", "a1",
204+
"--out", tempFilePath(t, "notes.txt"), "--text",
205+
}, svc)
206+
if result.err != nil {
207+
t.Fatalf("Execute: %v\nstderr=%q", result.err, result.stderr)
208+
}
209+
210+
lines := strings.Split(strings.TrimRight(result.stdout, "\n"), "\n")
211+
if len(lines) != 6 { // path, cached, bytes, filename, mimeType, text
212+
t.Fatalf("expected 6 TSV rows, got %d: %q", len(lines), result.stdout)
213+
}
214+
var textLine string
215+
for _, line := range lines {
216+
key, value, ok := strings.Cut(line, "\t")
217+
if !ok || strings.ContainsAny(value, "\t\n") {
218+
t.Fatalf("row is not a stable 2-field TSV record: %q", line)
219+
}
220+
if key == "text" {
221+
textLine = value
222+
}
223+
}
224+
unquoted, err := strconv.Unquote(textLine)
225+
if err != nil || unquoted != content {
226+
t.Fatalf("text field must round-trip via quoting: value=%q err=%v", textLine, err)
227+
}
228+
}
229+
230+
func TestExecute_GmailAttachment_Text_CorruptPDF_ReturnsReason(t *testing.T) {
231+
data := []byte("%PDF-1.4\nthis is not a parseable pdf body")
232+
svc := newGmailAttachmentTestService(t, data, "broken.pdf", "application/pdf")
233+
234+
parsed := executeGmailAttachmentJSON(t, svc,
235+
"--json", "--account", "[email protected]",
236+
"gmail", "attachment", "m1", "a1",
237+
"--out", tempFilePath(t, "broken.pdf"), "--text",
238+
)
239+
240+
if _, ok := parsed["text"]; ok {
241+
t.Fatalf("corrupt PDF must not yield text: %#v", parsed)
242+
}
243+
reason, _ := parsed["reason"].(string)
244+
if !strings.Contains(reason, "pdf text extraction failed") {
245+
t.Fatalf("reason=%q", reason)
246+
}
247+
}

‎internal/cmd/gmail_attachment.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"fmt"
88
"os"
99
"path/filepath"
10+
"strconv"
1011
"strings"
1112

1213
"google.golang.org/api/gmail/v1"
@@ -40,12 +41,23 @@ func printAttachmentDownloadResult(ctx context.Context, u *ui.UI, payload map[st
4041
u.Out().Linef("bytes\t%d", payload["bytes"])
4142
for _, key := range []string{"filename", "mimeType", "contentBase64", "text", "note", "reason"} {
4243
if v, ok := payload[key]; ok {
43-
u.Out().Linef("%s\t%v", key, v)
44+
u.Out().Linef("%s\t%s", key, tsvSafeValue(v))
4445
}
4546
}
4647
return nil
4748
}
4849

50+
// tsvSafeValue keeps plain output a stable one-record-per-line TSV: values
51+
// containing tabs or newlines (e.g. extracted attachment text) are emitted as
52+
// a single Go-quoted field.
53+
func tsvSafeValue(v any) string {
54+
s := fmt.Sprintf("%v", v)
55+
if strings.ContainsAny(s, "\t\n\r") {
56+
return strconv.Quote(s)
57+
}
58+
return s
59+
}
60+
4961
func (c *GmailAttachmentCmd) Run(ctx context.Context, flags *RootFlags) error {
5062
u := ui.FromContext(ctx)
5163
messageID := normalizeGmailMessageID(c.MessageID)

‎internal/cmd/gmail_attachment_text.go‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"os"
99
"path/filepath"
1010
"strings"
11+
"time"
1112

1213
"github.com/ledongthuc/pdf"
1314

@@ -36,7 +37,9 @@ func addTextContent(payload map[string]any, path string, size int64, filename, m
3637

3738
switch {
3839
case isPDFAttachment(data, filename, mimeType):
39-
text, err := extractPDFText(data)
40+
text, err := runPDFExtractionWithTimeout(func() (string, error) {
41+
return extractPDFText(data)
42+
}, pdfExtractTimeout)
4043
if err != nil {
4144
payload["reason"] = fmt.Sprintf("pdf text extraction failed: %v; use --inline or --out for the raw bytes", err)
4245
return
@@ -85,6 +88,31 @@ func isTextAttachment(mimeType string) bool {
8588
return false
8689
}
8790

91+
// pdfExtractTimeout bounds how long parsing an (attacker-controlled) PDF may
92+
// run; the input size is already capped by maxInlineAttachmentBytes.
93+
const pdfExtractTimeout = 10 * time.Second
94+
95+
// runPDFExtractionWithTimeout runs fn and gives up after the timeout, so a
96+
// pathological PDF cannot hang the command (or a long-lived MCP server)
97+
// indefinitely. On timeout the extraction goroutine is abandoned.
98+
func runPDFExtractionWithTimeout(fn func() (string, error), timeout time.Duration) (string, error) {
99+
type result struct {
100+
text string
101+
err error
102+
}
103+
ch := make(chan result, 1)
104+
go func() {
105+
text, err := fn()
106+
ch <- result{text: text, err: err}
107+
}()
108+
select {
109+
case r := <-ch:
110+
return r.text, r.err
111+
case <-time.After(timeout):
112+
return "", fmt.Errorf("timed out after %s", timeout)
113+
}
114+
}
115+
88116
// extractPDFText pulls the plain text out of a PDF. The pdf library panics on
89117
// some malformed inputs, so recover and surface those as errors.
90118
func extractPDFText(data []byte) (text string, err error) {

‎internal/cmd/gmail_attachment_text_unit_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
package cmd
2+
3+
import (
4+
"errors"
5+
"strings"
6+
"testing"
7+
"time"
8+
)
9+
10+
func TestRunPDFExtractionWithTimeout_TimesOut(t *testing.T) {
11+
_, err := runPDFExtractionWithTimeout(func() (string, error) {
12+
time.Sleep(500 * time.Millisecond)
13+
return "too late", nil
14+
}, 10*time.Millisecond)
15+
if err == nil || !strings.Contains(err.Error(), "timed out") {
16+
t.Fatalf("err=%v", err)
17+
}
18+
}
19+
20+
func TestRunPDFExtractionWithTimeout_PassesThroughResult(t *testing.T) {
21+
text, err := runPDFExtractionWithTimeout(func() (string, error) {
22+
return "ok", nil
23+
}, time.Second)
24+
if err != nil || text != "ok" {
25+
t.Fatalf("text=%q err=%v", text, err)
26+
}
27+
}
28+
29+
func TestRunPDFExtractionWithTimeout_PassesThroughError(t *testing.T) {
30+
wantErr := errors.New("boom")
31+
_, err := runPDFExtractionWithTimeout(func() (string, error) {
32+
return "", wantErr
33+
}, time.Second)
34+
if !errors.Is(err, wantErr) {
35+
t.Fatalf("err=%v", err)
36+
}
37+
}

0 commit comments

Comments
 (0)