From 0d36609aa32886db0a827883431a5600b274fc10 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 17 May 2026 11:12:36 +0200 Subject: [PATCH 1/4] fix(middleware): cap filename length on untyped formData uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The typed (codegen) parameter-binding path goes through runtime.BindForm with BindFormFile declarations and inherits the DefaultMaxUploadFilenameLength (1 KiB) cap. The untyped path calls request.FormFile directly without declaring a binder, so the cap never fires — leaving the untyped pipeline open to multi-MB FileHeader.Filename submissions that bloat parser memory. Extracts the existing inline check from bindFormFile into a new exported helper runtime.ValidateFilenameLength so both paths enforce the same rule. The untyped binder calls it after request.FormFile, before assigning runtime.File to the target. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Frederic BIDON --- form.go | 31 ++++++++++++++++++++++++------- form_test.go | 37 +++++++++++++++++++++++++++++++++++++ middleware/parameter.go | 8 ++++++++ middleware/request_test.go | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 7 deletions(-) diff --git a/form.go b/form.go index 63f57b73..fdf59428 100644 --- a/form.go +++ b/form.go @@ -32,6 +32,28 @@ const DefaultMaxUploadBodySize = int64(32) << 20 // rejects a too-long filename. const filenamePreviewLen = 32 +// ValidateFilenameLength enforces the FileHeader.Filename length cap +// that [BindForm] applies via [BindFormFile] declarations. Untyped +// binder paths that fetch the file via [http.Request.FormFile] +// directly (rather than declaring the file through BindFormFile) call +// this to opt into the same protection. +// +// Returns nil if filename length is within maxLen or maxLen <= 0. +// Otherwise returns an *errors.ParseError suitable for direct return +// from a parameter binder. The error embeds a truncated preview of +// the offending filename to keep the error message bounded. +func ValidateFilenameLength(paramName, paramIn, filename string, maxLen int) error { + if maxLen <= 0 || len(filename) <= maxLen { + return nil + } + preview := filename + if len(preview) > filenamePreviewLen { + preview = preview[:filenamePreviewLen] + } + return errors.NewParseError(paramName, paramIn, preview, + fmt.Errorf("filename length %d exceeds limit %d", len(filename), maxLen)) +} + // FileBinder is the per-file callback invoked by [BindForm] when a // declared file field is present. // @@ -276,13 +298,8 @@ func bindFormFile(r *http.Request, spec formFileSpec, maxFilenameLen int) error return errors.NewParseError(spec.name, "formData", "", err) } - if maxFilenameLen > 0 && len(header.Filename) > maxFilenameLen { - preview := header.Filename - if len(preview) > filenamePreviewLen { - preview = preview[:filenamePreviewLen] - } - return errors.NewParseError(spec.name, "formData", preview, - fmt.Errorf("filename length %d exceeds limit %d", len(header.Filename), maxFilenameLen)) + if err := ValidateFilenameLength(spec.name, "formData", header.Filename, maxFilenameLen); err != nil { + return err } if spec.bind == nil { diff --git a/form_test.go b/form_test.go index d11d123b..1f9cfb7b 100644 --- a/form_test.go +++ b/form_test.go @@ -412,3 +412,40 @@ func TestBindForm_idempotent(t *testing.T) { assert.FalseT(t, fatal2) assert.EqualT(t, "x", r.Form.Get(testFieldDesc)) } + +// TestValidateFilenameLength covers the exported helper used by both +// BindForm's BindFormFile path and the untyped middleware/parameter.go +// formData path. Security scrub Lens 3 / L3.1. +func TestValidateFilenameLength(t *testing.T) { + t.Run("within cap returns nil", func(t *testing.T) { + require.NoError(t, ValidateFilenameLength("avatar", "formData", "ok.txt", 1024)) + }) + t.Run("at cap returns nil", func(t *testing.T) { + name := strings.Repeat("x", 10) + require.NoError(t, ValidateFilenameLength("avatar", "formData", name, 10)) + }) + t.Run("over cap returns ParseError", func(t *testing.T) { + name := strings.Repeat("x", 50) + err := ValidateFilenameLength("avatar", "formData", name, 10) + require.Error(t, err) + var pe *errors.ParseError + require.True(t, stderrors.As(err, &pe)) + assert.EqualT(t, "avatar", pe.Name) + assert.EqualT(t, "formData", pe.In) + }) + t.Run("preview is truncated", func(t *testing.T) { + name := strings.Repeat("y", 200) + err := ValidateFilenameLength("avatar", "formData", name, 10) + require.Error(t, err) + var pe *errors.ParseError + require.True(t, stderrors.As(err, &pe)) + // preview must fit filenamePreviewLen (32 bytes). + assert.LessOrEqual(t, len(pe.Value), filenamePreviewLen) + }) + t.Run("maxLen<=0 disables the cap", func(t *testing.T) { + name := strings.Repeat("z", 10000) + require.NoError(t, ValidateFilenameLength("avatar", "formData", name, 0)) + require.NoError(t, ValidateFilenameLength("avatar", "formData", name, -1)) + }) +} + diff --git a/middleware/parameter.go b/middleware/parameter.go index d8b8e3ba..3c96b21c 100644 --- a/middleware/parameter.go +++ b/middleware/parameter.go @@ -140,6 +140,14 @@ func (p *untypedParamBinder) bindFormData(request *http.Request, _ RouteParams, return nil } + // Mirror the FileHeader.Filename length cap that BindForm + // applies to typed (codegen) paths through BindFormFile, so + // untyped formData bindings get the same protection. + if err := runtime.ValidateFilenameLength(p.Name, p.parameter.In, header.Filename, + runtime.DefaultMaxUploadFilenameLength); err != nil { + return err + } + target.Set(reflect.ValueOf(runtime.File{Data: file, Header: header})) return nil } diff --git a/middleware/request_test.go b/middleware/request_test.go index a0323724..74cebe93 100644 --- a/middleware/request_test.go +++ b/middleware/request_test.go @@ -547,3 +547,37 @@ func TestBindingOptionalFileUpload(t *testing.T) { require.NoError(t, err) assert.Equal(t, []byte("the file contents"), bb) } + +// TestBindingFileUpload_RejectsOversizedFilename exercises the +// filename-length cap on the untyped formData path: a multipart +// body with a multi-MB filename must be rejected with a ParseError +// before the file is bound. +// +// Mirrors the BindFormFile-path coverage in +// runtime.TestBindForm_maxFilenameLen_exceeded. Security scrub +// Lens 3 / L3.1. +func TestBindingFileUpload_RejectsOversizedFilename(t *testing.T) { + binder := paramsForFileUpload() + + body := bytes.NewBuffer(nil) + writer := multipart.NewWriter(body) + longName := strings.Repeat("x", runtime.DefaultMaxUploadFilenameLength+1) + ".txt" + part, err := writer.CreateFormFile("file", longName) + require.NoError(t, err) + _, err = part.Write([]byte("contents")) + require.NoError(t, err) + require.NoError(t, writer.WriteField(paramKeyName, "the-name")) + require.NoError(t, writer.Close()) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, testURL, body) + require.NoError(t, err) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + data := fileRequest{} + bindErr := binder.Bind(req, nil, runtime.JSONConsumer(), &data) + require.Error(t, bindErr) + assert.Contains(t, bindErr.Error(), "exceeds limit") + // File must NOT have been bound past the cap. + assert.Nil(t, data.File.Data) + assert.Nil(t, data.File.Header) +} From 5cabd7013f842d80d85a547ea761cf136699deb4 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 17 May 2026 11:12:46 +0200 Subject: [PATCH 2/4] fix(client): strip CR/LF from multipart filename and field name escapeQuotes was extending only backslash and double-quote when building the Content-Disposition header for an outbound multipart part. A caller passing attacker-influenced bytes (typically a filename) containing CR or LF could split the header line and inject a forged header or part, mirroring the known stdlib gap golang/go#19038. Replaces CR and LF with '_' so the resulting header value remains on a single physical line regardless of the source string. Threat model: the developer feeds user input into a NamedReadCloser's Name() (or a field name) without sanitising upstream; without this fix, the input lands on the wire verbatim. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Frederic BIDON --- client/internal/request/escape_test.go | 72 ++++++++++++++++++++++++++ client/internal/request/request.go | 16 +++++- 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 client/internal/request/escape_test.go diff --git a/client/internal/request/escape_test.go b/client/internal/request/escape_test.go new file mode 100644 index 00000000..a59edec0 --- /dev/null +++ b/client/internal/request/escape_test.go @@ -0,0 +1,72 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package request + +import ( + "strings" + "testing" + + "github.com/go-openapi/testify/v2/assert" +) + +// TestEscapeQuotes_StripsCRLF verifies that escapeQuotes neutralises +// CR / LF in Content-Disposition parameter values, preventing +// header-injection through attacker-influenced field names or +// filenames. Mirrors the known stdlib gap golang/go#19038. +// +// Security scrub Lens 3 / L3.2. +func TestEscapeQuotes_StripsCRLF(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + { + name: "embedded CR", + in: "file\rname.txt", + want: "file_name.txt", + }, + { + name: "embedded LF", + in: "file\nname.txt", + want: "file_name.txt", + }, + { + name: "embedded CRLF", + in: "file\r\nname.txt", + want: "file__name.txt", + }, + { + name: "CRLF + injected header", + in: "evil.txt\r\nContent-Type: forged", + want: "evil.txt__Content-Type: forged", + }, + { + name: "no control chars", + in: "regular.txt", + want: "regular.txt", + }, + { + name: "still escapes quote", + in: `with"quote.txt`, + want: `with\"quote.txt`, + }, + { + name: "still escapes backslash", + in: `with\backslash.txt`, + want: `with\\backslash.txt`, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := escapeQuotes(c.in) + assert.Equal(t, c.want, got) + // Belt-and-braces: result must contain no literal CR/LF + // regardless of how the input was assembled. + assert.False(t, strings.ContainsAny(got, "\r\n"), + "output retained a CR or LF: %q", got) + }) + } +} diff --git a/client/internal/request/request.go b/client/internal/request/request.go index d5e42438..f0bd23bc 100644 --- a/client/internal/request/request.go +++ b/client/internal/request/request.go @@ -825,8 +825,22 @@ func (r *Request) writeNonStreamPayload(mediaType string, producers map[string]r return r.buf, nil } +// escapeQuotes escapes backslash and double-quote for embedding in a +// quoted-string Content-Disposition parameter value, and rewrites +// CR / LF to '_' to prevent header-injection through attacker-influenced +// field names or filenames. +// +// RFC 7578 §4.2 limits parameter values to printable characters; this +// is the conservative subset relevant to security (control characters +// that would split the header line into a forged header or part). +// Mirrors the known stdlib gap golang/go#19038. func escapeQuotes(s string) string { - return strings.NewReplacer("\\", "\\\\", `"`, "\\\"").Replace(s) + return strings.NewReplacer( + "\\", "\\\\", + `"`, "\\\"", + "\r", "_", + "\n", "_", + ).Replace(s) } // setStreamContentType resolves and writes the wire Content-Type for a From fa336825535097d6b36eef67830c87ebe2970273 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 17 May 2026 11:12:56 +0200 Subject: [PATCH 3/4] test(security): fuzz targets for BindForm parse + filename cap Two new fuzz targets exercising the multipart form-binding surface of the security scrub (lens 3): - FuzzBindForm: feeds arbitrary bytes as a multipart body and asserts the contract that fatal=true implies err!=nil and that the parser never panics or hangs. - FuzzBindFormFilename: builds a well-formed multipart wrapper around an arbitrary filename string and asserts that any filename that survives binding fits within DefaultMaxUploadFilenameLength. CI auto-discovers FuzzXxx via the shared go-test-monorepo workflow. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Frederic BIDON --- form_fuzz_test.go | 133 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 form_fuzz_test.go diff --git a/form_fuzz_test.go b/form_fuzz_test.go new file mode 100644 index 00000000..52e03454 --- /dev/null +++ b/form_fuzz_test.go @@ -0,0 +1,133 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package runtime + +import ( + "bytes" + "fmt" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// FuzzBindForm exercises the full BindForm parse path with arbitrary +// multipart bodies. Invariants: must not panic, hang, or return +// fatal=true alongside err=nil. +// +// Security scrub Lens 3 / L3.8 — fuzz coverage for the form-binding +// surface. +func FuzzBindForm(f *testing.F) { + const boundary = "FUZZBOUND" + ct := "multipart/form-data; boundary=" + boundary + + seeds := [][]byte{ + // Well-formed single text part. + []byte("--" + boundary + "\r\n" + + `Content-Disposition: form-data; name="x"` + "\r\n\r\n" + + "v\r\n" + + "--" + boundary + "--\r\n"), + // Well-formed single file part. + []byte("--" + boundary + "\r\n" + + `Content-Disposition: form-data; name="f"; filename="t.txt"` + "\r\n" + + "Content-Type: text/plain\r\n\r\n" + + "data\r\n" + + "--" + boundary + "--\r\n"), + // Empty body. + nil, + // Only the closing boundary. + []byte("--" + boundary + "--\r\n"), + // Truncated body (no closing boundary). + []byte("--" + boundary + "\r\n" + + `Content-Disposition: form-data; name="x"` + "\r\n\r\n"), + // Adversarial filename (long). + []byte("--" + boundary + "\r\n" + + `Content-Disposition: form-data; name="f"; filename="` + + strings.Repeat("a", 4096) + `"` + "\r\n\r\n" + + "x\r\n" + + "--" + boundary + "--\r\n"), + // Garbage that doesn't start with a boundary. + []byte("not-a-multipart-body"), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, body []byte) { + r := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", bytes.NewReader(body)) + r.Header.Set("Content-Type", ct) + + fatal, err := BindForm(r, + BindFormFile("f", false, func(_ multipart.File, _ *multipart.FileHeader) error { return nil }), + ) + + if fatal && err == nil { + t.Fatalf("BindForm returned fatal=true with err=nil for body %q", body) + } + }) +} + +// FuzzBindFormFilename targets the filename-cap path specifically. +// It feeds an arbitrary filename through a synthetic well-formed +// multipart body and asserts the bound *FileHeader.Filename length +// never exceeds DefaultMaxUploadFilenameLength. +// +// Security scrub Lens 3 / L3.1 + L3.8. +func FuzzBindFormFilename(f *testing.F) { + seeds := []string{ + "normal.txt", + "", + strings.Repeat("a", DefaultMaxUploadFilenameLength), // exactly at cap + strings.Repeat("a", DefaultMaxUploadFilenameLength+1), // one over + strings.Repeat("a", DefaultMaxUploadFilenameLength*100), // way over + "a/b/c.txt", + "../etc/passwd", + "\x00", + "file\r\nContent-Type: forged", + string([]byte{0xff, 0xfe}), + } + for _, s := range seeds { + f.Add(s) + } + + const boundary = "FUZZBOUND" + + f.Fuzz(func(t *testing.T, filename string) { + // Build a well-formed multipart wrapper around the fuzzed + // filename. %q quote-escapes so the wire stays parseable; + // the bytes BindForm sees as Filename are the same fuzz + // input after the stdlib parser decodes the quoted-string. + body := fmt.Sprintf( + "--%s\r\n"+ + `Content-Disposition: form-data; name="f"; filename=%q`+"\r\n"+ + "Content-Type: application/octet-stream\r\n"+ + "\r\n"+ + "data\r\n"+ + "--%s--\r\n", + boundary, filename, boundary, + ) + r := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", + strings.NewReader(body)) + r.Header.Set("Content-Type", "multipart/form-data; boundary="+boundary) + + var bound *multipart.FileHeader + fatal, err := BindForm(r, + BindFormFile("f", false, func(_ multipart.File, h *multipart.FileHeader) error { + bound = h + return nil + }), + ) + + if fatal && err == nil { + t.Fatalf("fatal=true with err=nil for filename %q", filename) + } + if err == nil && bound != nil { + if len(bound.Filename) > DefaultMaxUploadFilenameLength { + t.Fatalf("BindForm bound a file with filename length %d > cap %d (filename=%q)", + len(bound.Filename), DefaultMaxUploadFilenameLength, bound.Filename) + } + } + }) +} From 8bf148cb896bf0e7f0c22bbd6a3501d82b900b4b Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 17 May 2026 11:28:14 +0200 Subject: [PATCH 4/4] chore: cleanup linter config, reformat, optimized strings replacer Signed-off-by: Frederic BIDON --- .golangci.yml | 9 ++------- client/internal/request/request.go | 14 ++++++++------ form.go | 5 +---- form_test.go | 1 - 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 038c18c8..ef2ff12b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,13 +2,9 @@ version: "2" linters: default: all disable: - #- cyclop - depguard - err113 # disabled temporarily: there are just too many issues to address - #- errchkjson - #- errorlint - exhaustruct - #- forcetypeassert - funlen - gochecknoglobals - gochecknoinits @@ -16,12 +12,12 @@ linters: - godot - godox - gomoddirectives # moved to mono-repo, multi-modules, so replace directives are needed + - gomodguard + - gomodguard_v2 - gosmopolitan - inamedparam - ireturn # this repo adopted a pattern where there are quite many returned interfaces. To be challenged with v2 - #- lll - musttag - #- nestif - nilerr # nilerr crashes on this repo - nlreturn - noinlineerr @@ -31,7 +27,6 @@ linters: - testpackage - thelper - tparallel - #- unparam - varnamelen - whitespace - wrapcheck diff --git a/client/internal/request/request.go b/client/internal/request/request.go index f0bd23bc..eaaaaffb 100644 --- a/client/internal/request/request.go +++ b/client/internal/request/request.go @@ -825,6 +825,13 @@ func (r *Request) writeNonStreamPayload(mediaType string, producers map[string]r return r.buf, nil } +var quoter = strings.NewReplacer( + "\\", "\\\\", + `"`, "\\\"", + "\r", "_", + "\n", "_", +) + // escapeQuotes escapes backslash and double-quote for embedding in a // quoted-string Content-Disposition parameter value, and rewrites // CR / LF to '_' to prevent header-injection through attacker-influenced @@ -835,12 +842,7 @@ func (r *Request) writeNonStreamPayload(mediaType string, producers map[string]r // that would split the header line into a forged header or part). // Mirrors the known stdlib gap golang/go#19038. func escapeQuotes(s string) string { - return strings.NewReplacer( - "\\", "\\\\", - `"`, "\\\"", - "\r", "_", - "\n", "_", - ).Replace(s) + return quoter.Replace(s) } // setStreamContentType resolves and writes the wire Content-Type for a diff --git a/form.go b/form.go index fdf59428..7e6c971a 100644 --- a/form.go +++ b/form.go @@ -46,10 +46,7 @@ func ValidateFilenameLength(paramName, paramIn, filename string, maxLen int) err if maxLen <= 0 || len(filename) <= maxLen { return nil } - preview := filename - if len(preview) > filenamePreviewLen { - preview = preview[:filenamePreviewLen] - } + preview := filename[:min(len(filename), filenamePreviewLen)] return errors.NewParseError(paramName, paramIn, preview, fmt.Errorf("filename length %d exceeds limit %d", len(filename), maxLen)) } diff --git a/form_test.go b/form_test.go index 1f9cfb7b..d08eedff 100644 --- a/form_test.go +++ b/form_test.go @@ -448,4 +448,3 @@ func TestValidateFilenameLength(t *testing.T) { require.NoError(t, ValidateFilenameLength("avatar", "formData", name, -1)) }) } -