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.