ErrNoMultipartForm error and adjacent comments do not match code

[mark-pictor-csec]

See v1.48.0, starting at http.go:937 and continuing to line 955.

The error and the comments for the error and the following function indicate the error is returned for a wrong content type. However, this error is actually returned when the multipart form boundary length is zero (line 954-955). I don't see any content-type checks in the function, and the boundary length check is the only one returning this error.

Suggested fix:
Return a better error on line 955 (e.g. zero-length multipart form boundary), and either add a content type check from which to return ErrNoMultipartForm or remove the error and update the function's comment (lines 943-944) to match.

Found while writing a unit test; was very confused how the content-type header could get lost. Finally looked at the code and realized what was going on.

[mark-pictor-csec]

Ugh... turns out I didn't fully understand what was going on. I'd forgotten the content-type header needed to include the boundary. Since the boundary is read from the content-type, in most situations a zero-len boundary does imply a missing content-type header.

I'd recommend changing the wording of the error a bit, perhaps

-var ErrNoMultipartForm = errors.New("request has no multipart/form-data Content-Type")
+var ErrNoMultipartForm = errors.New("request Content-Type has bad boundary or is not multipart/form-data")

Reproducer (could be cleaned up to remove fiber, but I assume it's a simple enough issue to not matter)

import (
	"bytes"
	"mime/multipart"
	"net/http"
	"testing"

	"github.com/gofiber/fiber/v2"
)

func multipartForm(t *testing.T, addr string) *http.Request {
	buf := &bytes.Buffer{}
	w := multipart.NewWriter(buf)
	if err := w.WriteField("field", "value"); err != nil {
		t.Fatal(err)
	}
	file, err := w.CreateFormFile("field2", "file1")
	if err != nil {
		t.Fatal(err)
	}
	if _, err := file.Write([]byte("file content")); err != nil {
		t.Fatal(err)
	}
	if err := w.Close(); err != nil {
		t.Fatal(err)
	}
	req, err := http.NewRequest("POST", "http://"+addr+"/a/b/c?query=blah", buf)
	if err != nil {
		t.Fatal(err)
	}
	req.Header.Set("Content-Type", "multipart/form-data")   // causes error
	// req.Header.Set("Content-Type", w.FormDataContentType()) // no error
	return req
}

func TestMultipart(t *testing.T) {

	app := fiber.New(fiber.Config{
		DisableStartupMessage: true,
	})
	t.Cleanup(func() { _ = app.Shutdown() })
	app.All("/:param/:optional?/*", func(ctx *fiber.Ctx) error {
		form, err := ctx.MultipartForm() // calls fasthttp function of the same name
		if err != nil {
			t.Error(err)
			return nil
		}
		if len(form.File) == 0 {
			t.Error("setup error - no files")
			return nil
		}
		if len(form.Value) == 0 {
			t.Error("setup error - no values")
			return nil
		}
		return nil
	})
	ln := newLocalListener(t, "tcp")
	go func() {
		if err := app.Listener(ln); err != nil {
			t.Error(err)
		}
	}()
	addr := ln.Addr().String()

	req := multipartForm(t, addr)
	_, err := http.DefaultClient.Do(req)
	if err != nil {
		t.Fatal(err)
	}
}

Toggle commenting on the two req.Header.Set(...) lines for a functional version.

[erikdubbelboer]

Thanks! I have copied your suggestion.