From 078ef8145c1514ffbeec1bcd952f9b73295974d0 Mon Sep 17 00:00:00 2001 From: Wang-Yeah623 Date: Fri, 22 May 2026 15:40:10 +0800 Subject: [PATCH 1/3] fix(common): escape special chars in multipart form filenames MultipartWriter.CreateFormFile concatenated the fieldname and filename into the Content-Disposition header without escaping, so a filename containing a double-quote, backslash, CR, or LF produced a malformed header. For example, uploading `report "draft" v2.pdf` via `task +upload-attachment` made the server see `filename="report "` (truncated at the first internal quote) and drop the rest. Drop the custom override and let CreateFormFile be promoted from the embedded *multipart.Writer, which applies the stdlib's quoteEscaper (backslash and double-quote get a backslash prefix; CR and LF get percent-encoded). The Content-Type ("application/octet-stream") and the wrapper API are unchanged, so the existing `task +upload-attachment` call site is unaffected -- filenames with special characters just now round-trip correctly. Add helpers_test.go covering plain, quoted, backslashed, mixed, and unicode filenames. The test asserts both the on-wire encoding and a round-trip through mime.ParseMediaType (bypassing Part.FileName, whose filepath.Base is platform-dependent for backslash on Windows). --- shortcuts/common/helpers.go | 15 ++--- shortcuts/common/helpers_test.go | 104 +++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 shortcuts/common/helpers_test.go diff --git a/shortcuts/common/helpers.go b/shortcuts/common/helpers.go index 1a15da436..dfef27940 100644 --- a/shortcuts/common/helpers.go +++ b/shortcuts/common/helpers.go @@ -7,10 +7,13 @@ import ( "encoding/json" "io" "mime/multipart" - "net/textproto" ) -// MultipartWriter wraps multipart.Writer for file uploads. +// MultipartWriter wraps multipart.Writer for file uploads. CreateFormFile is +// promoted from the embedded *multipart.Writer so that special characters in +// the filename (`"`, `\`, CR, LF) are properly escaped per the stdlib's +// quoteEscaper — otherwise filenames like `report "draft".pdf` would produce +// a malformed Content-Disposition header. type MultipartWriter struct { *multipart.Writer } @@ -20,14 +23,6 @@ func NewMultipartWriter(w io.Writer) *MultipartWriter { return &MultipartWriter{multipart.NewWriter(w)} } -// CreateFormFile creates a form file with the given field name and file name. -func (mw *MultipartWriter) CreateFormFile(fieldname, filename string) (io.Writer, error) { - h := make(textproto.MIMEHeader) - h.Set("Content-Disposition", `form-data; name="`+fieldname+`"; filename="`+filename+`"`) - h.Set("Content-Type", "application/octet-stream") - return mw.Writer.CreatePart(h) -} - // ParseJSON unmarshals JSON data into v. func ParseJSON(data []byte, v interface{}) error { return json.Unmarshal(data, v) diff --git a/shortcuts/common/helpers_test.go b/shortcuts/common/helpers_test.go new file mode 100644 index 000000000..ba5b87ace --- /dev/null +++ b/shortcuts/common/helpers_test.go @@ -0,0 +1,104 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package common + +import ( + "bytes" + "io" + "mime" + "mime/multipart" + "strings" + "testing" +) + +// TestMultipartWriter_CreateFormFile_EscapesFilename verifies that filenames +// containing characters that require escaping in a Content-Disposition header +// round-trip correctly through the multipart body. +// +// Regression test: an earlier custom CreateFormFile concatenated raw strings +// without escaping, so a filename like `report "draft".pdf` produced a +// malformed header that servers parsed as `filename="report "` (truncated at +// the first internal quote). +// +// Filename parameters are read via mime.ParseMediaType on the raw +// Content-Disposition header — Part.FileName runs the result through +// filepath.Base which is platform-dependent for backslash on Windows. +func TestMultipartWriter_CreateFormFile_EscapesFilename(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + filename string + wantEncoded string // expected escaped form embedded in the header + }{ + {"plain", "report.pdf", "report.pdf"}, + {"with double quote", `report "draft" v2.pdf`, `report \"draft\" v2.pdf`}, + {"with backslash", `report\draft.pdf`, `report\\draft.pdf`}, + {"with both", `path\to "weird" file.bin`, `path\\to \"weird\" file.bin`}, + {"unicode", "报告 v2.pdf", "报告 v2.pdf"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + mw := NewMultipartWriter(&buf) + w, err := mw.CreateFormFile("file", tc.filename) + if err != nil { + t.Fatalf("CreateFormFile error: %v", err) + } + if _, err := io.WriteString(w, "body-bytes"); err != nil { + t.Fatalf("write body: %v", err) + } + if err := mw.Close(); err != nil { + t.Fatalf("close writer: %v", err) + } + + body := buf.String() + wantHeader := `filename="` + tc.wantEncoded + `"` + if !strings.Contains(body, wantHeader) { + t.Errorf("Content-Disposition does not contain %q\nbody:\n%s", wantHeader, body) + } + + r := multipart.NewReader(strings.NewReader(body), mw.Boundary()) + part, err := r.NextPart() + if err != nil { + t.Fatalf("read part: %v", err) + } + _, params, err := mime.ParseMediaType(part.Header.Get("Content-Disposition")) + if err != nil { + t.Fatalf("ParseMediaType on Content-Disposition: %v", err) + } + if got := params["filename"]; got != tc.filename { + t.Errorf("filename round-trip: got %q, want %q", got, tc.filename) + } + if got := params["name"]; got != "file" { + t.Errorf("name: got %q, want %q", got, "file") + } + }) + } +} + +// TestMultipartWriter_CreateFormFile_ContentType verifies that the file part +// carries the expected Content-Type for binary uploads. +func TestMultipartWriter_CreateFormFile_ContentType(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + mw := NewMultipartWriter(&buf) + if _, err := mw.CreateFormFile("file", "x.bin"); err != nil { + t.Fatalf("CreateFormFile: %v", err) + } + if err := mw.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + r := multipart.NewReader(&buf, mw.Boundary()) + part, err := r.NextPart() + if err != nil { + t.Fatalf("read part: %v", err) + } + if got := part.Header.Get("Content-Type"); got != "application/octet-stream" { + t.Errorf("Content-Type: got %q, want application/octet-stream", got) + } +} From 46974bfbda06a1dc84018664fd2532091052d5ee Mon Sep 17 00:00:00 2001 From: Wang-Yeah623 Date: Fri, 22 May 2026 15:57:38 +0800 Subject: [PATCH 2/3] test(common): cover CR/LF/CRLF in multipart filename escaping Per code-review feedback, extend the helpers_test.go cases table with CR, LF, and CRLF filenames so the test exercises both legs of the stdlib's quoteEscaper: - backslash and double-quote use backslash escaping (quoted-pair); these round-trip exactly through mime.ParseMediaType. - CR and LF use percent encoding to prevent header injection; the MIME parser does not decode percent escapes, so the read-side filename param contains literal "%0D"/"%0A". The cases table grows a wantParsed column so each case can declare its expected post-parse value (same as filename for backslash-escaped chars, percent-encoded for CR/LF). --- shortcuts/common/helpers_test.go | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/shortcuts/common/helpers_test.go b/shortcuts/common/helpers_test.go index ba5b87ace..154a5463d 100644 --- a/shortcuts/common/helpers_test.go +++ b/shortcuts/common/helpers_test.go @@ -14,13 +14,21 @@ import ( // TestMultipartWriter_CreateFormFile_EscapesFilename verifies that filenames // containing characters that require escaping in a Content-Disposition header -// round-trip correctly through the multipart body. +// are properly encoded on the wire. // // Regression test: an earlier custom CreateFormFile concatenated raw strings // without escaping, so a filename like `report "draft".pdf` produced a // malformed header that servers parsed as `filename="report "` (truncated at // the first internal quote). // +// The stdlib's quoteEscaper applies two different schemes: +// - backslash and double-quote use backslash escaping (quoted-pair), which +// mime.ParseMediaType reverses on read, so they round-trip exactly. +// - CR and LF use percent encoding (to prevent header injection, since a +// literal CRLF would break the header). The MIME parser does NOT decode +// percent escapes, so on read the filename param contains the literal +// "%0D"/"%0A" — server-side code is expected to URL-decode it. +// // Filename parameters are read via mime.ParseMediaType on the raw // Content-Disposition header — Part.FileName runs the result through // filepath.Base which is platform-dependent for backslash on Windows. @@ -31,12 +39,16 @@ func TestMultipartWriter_CreateFormFile_EscapesFilename(t *testing.T) { name string filename string wantEncoded string // expected escaped form embedded in the header + wantParsed string // what mime.ParseMediaType returns; differs from filename only when percent-encoded }{ - {"plain", "report.pdf", "report.pdf"}, - {"with double quote", `report "draft" v2.pdf`, `report \"draft\" v2.pdf`}, - {"with backslash", `report\draft.pdf`, `report\\draft.pdf`}, - {"with both", `path\to "weird" file.bin`, `path\\to \"weird\" file.bin`}, - {"unicode", "报告 v2.pdf", "报告 v2.pdf"}, + {"plain", "report.pdf", "report.pdf", "report.pdf"}, + {"with double quote", `report "draft" v2.pdf`, `report \"draft\" v2.pdf`, `report "draft" v2.pdf`}, + {"with backslash", `report\draft.pdf`, `report\\draft.pdf`, `report\draft.pdf`}, + {"with both", `path\to "weird" file.bin`, `path\\to \"weird\" file.bin`, `path\to "weird" file.bin`}, + {"unicode", "报告 v2.pdf", "报告 v2.pdf", "报告 v2.pdf"}, + {"with CR", "file\rname.pdf", "file%0Dname.pdf", "file%0Dname.pdf"}, + {"with LF", "file\nname.pdf", "file%0Aname.pdf", "file%0Aname.pdf"}, + {"with CRLF", "file\r\nname.pdf", "file%0D%0Aname.pdf", "file%0D%0Aname.pdf"}, } for _, tc := range cases { @@ -69,8 +81,8 @@ func TestMultipartWriter_CreateFormFile_EscapesFilename(t *testing.T) { if err != nil { t.Fatalf("ParseMediaType on Content-Disposition: %v", err) } - if got := params["filename"]; got != tc.filename { - t.Errorf("filename round-trip: got %q, want %q", got, tc.filename) + if got := params["filename"]; got != tc.wantParsed { + t.Errorf("filename round-trip: got %q, want %q", got, tc.wantParsed) } if got := params["name"]; got != "file" { t.Errorf("name: got %q, want %q", got, "file") From 528022b722e99885a08a8712e69dddfaabb656b4 Mon Sep 17 00:00:00 2001 From: Wang-Yeah623 Date: Sat, 23 May 2026 13:03:38 +0800 Subject: [PATCH 3/3] refactor(common): polish doc comments and regroup test cases Two follow-up tweaks suggested by a re-read of the PR: - helpers.go: stop naming the stdlib's internal `quoteEscaper` in the doc comment. Describe the observable behaviour ("escapes special characters") instead, so the comment stays valid if the stdlib ever renames or reimplements its escaping. - helpers_test.go: rename the vague `with both` case to `backslash and quote`; split the table-driven cases into three visually-separated groups (happy path / backslash escaping / percent encoding) so it is obvious why two cases have a different wantParsed than filename. No behaviour change; tests still pass 8/8. --- shortcuts/common/helpers.go | 8 ++++---- shortcuts/common/helpers_test.go | 25 +++++++++++++++---------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/shortcuts/common/helpers.go b/shortcuts/common/helpers.go index dfef27940..2aff5a4d5 100644 --- a/shortcuts/common/helpers.go +++ b/shortcuts/common/helpers.go @@ -10,10 +10,10 @@ import ( ) // MultipartWriter wraps multipart.Writer for file uploads. CreateFormFile is -// promoted from the embedded *multipart.Writer so that special characters in -// the filename (`"`, `\`, CR, LF) are properly escaped per the stdlib's -// quoteEscaper — otherwise filenames like `report "draft".pdf` would produce -// a malformed Content-Disposition header. +// promoted from the embedded *multipart.Writer, which escapes special +// characters in the field name and filename — a filename like +// `report "draft".pdf` therefore round-trips through the Content-Disposition +// header instead of being truncated at the first unescaped quote. type MultipartWriter struct { *multipart.Writer } diff --git a/shortcuts/common/helpers_test.go b/shortcuts/common/helpers_test.go index 154a5463d..51f9eff34 100644 --- a/shortcuts/common/helpers_test.go +++ b/shortcuts/common/helpers_test.go @@ -21,11 +21,11 @@ import ( // malformed header that servers parsed as `filename="report "` (truncated at // the first internal quote). // -// The stdlib's quoteEscaper applies two different schemes: +// The stdlib applies two different schemes when serializing a filename: // - backslash and double-quote use backslash escaping (quoted-pair), which // mime.ParseMediaType reverses on read, so they round-trip exactly. -// - CR and LF use percent encoding (to prevent header injection, since a -// literal CRLF would break the header). The MIME parser does NOT decode +// - CR and LF use percent encoding to prevent header injection (a literal +// CRLF would break the header). mime.ParseMediaType does NOT decode // percent escapes, so on read the filename param contains the literal // "%0D"/"%0A" — server-side code is expected to URL-decode it. // @@ -41,14 +41,19 @@ func TestMultipartWriter_CreateFormFile_EscapesFilename(t *testing.T) { wantEncoded string // expected escaped form embedded in the header wantParsed string // what mime.ParseMediaType returns; differs from filename only when percent-encoded }{ - {"plain", "report.pdf", "report.pdf", "report.pdf"}, - {"with double quote", `report "draft" v2.pdf`, `report \"draft\" v2.pdf`, `report "draft" v2.pdf`}, - {"with backslash", `report\draft.pdf`, `report\\draft.pdf`, `report\draft.pdf`}, - {"with both", `path\to "weird" file.bin`, `path\\to \"weird\" file.bin`, `path\to "weird" file.bin`}, + // happy path: no characters need escaping + {"plain ASCII", "report.pdf", "report.pdf", "report.pdf"}, {"unicode", "报告 v2.pdf", "报告 v2.pdf", "报告 v2.pdf"}, - {"with CR", "file\rname.pdf", "file%0Dname.pdf", "file%0Dname.pdf"}, - {"with LF", "file\nname.pdf", "file%0Aname.pdf", "file%0Aname.pdf"}, - {"with CRLF", "file\r\nname.pdf", "file%0D%0Aname.pdf", "file%0D%0Aname.pdf"}, + + // backslash escaping: round-trips exactly through mime.ParseMediaType + {"double quote", `report "draft" v2.pdf`, `report \"draft\" v2.pdf`, `report "draft" v2.pdf`}, + {"backslash", `report\draft.pdf`, `report\\draft.pdf`, `report\draft.pdf`}, + {"backslash and quote", `path\to "weird" file.bin`, `path\\to \"weird\" file.bin`, `path\to "weird" file.bin`}, + + // percent encoding: on-wire %0D/%0A is not decoded by mime.ParseMediaType + {"carriage return", "file\rname.pdf", "file%0Dname.pdf", "file%0Dname.pdf"}, + {"line feed", "file\nname.pdf", "file%0Aname.pdf", "file%0Aname.pdf"}, + {"CRLF", "file\r\nname.pdf", "file%0D%0Aname.pdf", "file%0D%0Aname.pdf"}, } for _, tc := range cases {