Context Bind: Regression when parsing arrays from multipart/form-data

[benpate]

Issue Description

The code below works in echo v4.12.0, but breaks in v4.13.2. Previously, when reading a request body encoded with multipart/form-data, multiple values for a single variable name would be returned as a slice of strings. Now, in v4.13.2, only the first value is returned.

I believe there was some work around this area with the 4.13.0 release but I haven't dug into the code enough to say what's causing the issue.

This is a problem because it silently broke parts of my application that were expecting a slice but received something else.

Anyway, test case:

Working code to debug

package test

import (
	"bufio"
	"net/http"
	"strings"
	"testing"

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/require"
)

func TestEchoBinder(t *testing.T) {

	// Read the HTTP request
	request, err := getTestRequest()
	require.Nil(t, err)

	// Mock a new Echo context
	ctx := echo.New().NewContext(request, nil)

	// Try to bind the request to a map of any
	data := map[string]any{}
	err = ctx.Bind(&data)
	require.Nil(t, err)

	// Verify that the "ima_slice" value is actually a slice
	// This fails on version 4.13.2 but passes on version 4.12.0
	require.Equal(t, []string{"WEBHOOK", "OTHER"}, data["ima_slice"])
}

// getTestRequest mocks an HTTP request with a multipart form body
func getTestRequest() (*http.Request, error) {

	// Here's the body of the request.  Note two values for "ima_slice"
	body := `POST /6692c69bfe80a9aacf125b0d/edit HTTP/1.1
Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryVfyfBnHAjBwnl9dd
Accept: */*

------WebKitFormBoundaryVfyfBnHAjBwnl9dd
Content-Disposition: form-data; name="ima_slice"

WEBHOOK
------WebKitFormBoundaryVfyfBnHAjBwnl9dd
Content-Disposition: form-data; name="ima_slice"

OTHER
------WebKitFormBoundaryVfyfBnHAjBwnl9dd--
`

	// Create a new HTTP request
	reader := strings.NewReader(body)
	bufferedReader := bufio.NewReader(reader)
	result, err := http.ReadRequest(bufferedReader)
	return result, err
}

Version/commit

[aldas]

could you give me commit hash that you are trying? I tried multiple older versions and this test still fails.

[aldas]

ok, got it working with this

package main

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

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/require"
)

func TestEchoBinder(t *testing.T) {
	bodyBuffer := new(bytes.Buffer)
	mw := multipart.NewWriter(bodyBuffer)
	mw.WriteField("ima_slice", "WEBHOOK")
	mw.WriteField("ima_slice", "OTHER")
	mw.Close()
	body := bodyBuffer.Bytes()

	req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body))
	rec := httptest.NewRecorder()

	contentType := mw.FormDataContentType()
	req.Header.Set(echo.HeaderContentType, contentType)

	ctx := echo.New().NewContext(req, rec)

	// Try to bind the request to a map of any
	data := map[string]interface{}{}
	err := ctx.Bind(&data)
	require.Nil(t, err)

	// Verify that the "ima_slice" value is actually a slice
	// This fails on version 4.13.2 but passes on version 4.12.0
	require.Equal(t, []string{"WEBHOOK", "OTHER"}, data["ima_slice"])
}

and the PR which cause this is #2656

[aldas]

So in v4.11.4 we had different behavior require.Equal(t, string("WEBHOOK"), data["ima_slice"]) would have passed
and with #2554 in v4.12.0 we changed it to pass with require.Equal(t, []string{"WEBHOOK", "OTHER"}, data["ima_slice"])
and in PR #2656 (released with v4.13.0) we reverted it after people reporting as breaking change for them.

[aldas]

@benpate , would changing target type to data := map[string][]string{} work for you? Assuming you expect only string slices and request is always form then I think this would be solution.

[benpate]

Hey @aldas - thanks for looking into this.

Unfortunately, I can't really use a map[string][]string without a large refactor. Right now, everything is getting decoded into map[string]any. I'm exploring other options, right now,

[aldas]

if you are always dealing with multipart forms you could use c.Request().MultipartForm.Value in your specific handler and avoid bind logic, which is dark magick with reflect

	data := map[string]any{}
	ctx.Request().ParseMultipartForm(32 << 20)
	for k, v := range ctx.Request().MultipartForm.Value {
		data[k] = v
	}

[benpate]

Thank you, yes, I think that's where I'm headed. Re-reading my code, I don't necessarily need to put things into a generic map first. So, I'm looking at pulling out ctx.Bind() in several places.