Empty string turns into "false" bool during binding

[mariaefi29]

Issue Description

Empty string turns into false bool during binding in requests with content types multipart/form-data and application/x-www-form-urlencoded.

Checklist

• [x] Dependencies installed
• [x] No typos
• [x] Searched existing issues and docs

Expected behaviour

When we bind requests parameters into a struct with a bool field, we expect binding to fail with http.StatusBadRequest if the required bool parameter contains an empty string.

We have this similar behaviour during json.Unmarshal: it cannot unmarshal empty string into a bool variable. It throws an explicit error.

Actual behaviour

If the required bool parameter is an empty string in a request with content types multipart/form-data and application/x-www-form-urlencoded, binding into a struct makes this parameter a bool variable with false value. It doesn't throw any errors.

Steps to reproduce

This unexpected behaviour is due to the function in bind.go file:

func setBoolField(value string, field reflect.Value) error {
	if value == "" {
		value = "false"
	}
	boolVal, err := strconv.ParseBool(value)
	if err == nil {
		field.SetBool(boolVal)
	}
	return err
}

Working code to debug

type request struct {
	Activate bool `form:"activate" `
}

var req UploadRequest
if err := c.Bind(&req); err != nil {
	return err
}

Version/commit

v4.1.15

[mariaefi29]

Dear developers,

Do you have any updates on that issue?

[lammel]

This does not sound wrong actually. A boolean is either true or false and you declare Activate as a bool. You should change it to string if you expected an empty string to be a valid value.

Strings should not change "" to "false" of course, so I hope this does solve your problem.

[mariaefi29]

Dear @lammel,

I don't expect empty string to be a valid value for a bool type :) quite the opposite. That's what I was talking about: empty string is invalid value and it shouldn't turn into a valid one like false.

I would remove this from your function:

if value == "" {
		value = "false"
	}

What do you think? Do you need a PR for that?

[lammel]

I'm only a contributor not the owner of the function.

The same handling to set a valid value for an empty string is done for int, uint, float. Which is probably that way since in Go a string is initialized as an empty string anyway.

This would definitely be a breaking change and can be considered for v5. Some users out there might even rely on that behaviour to retrieve bad values in Params.

@vishr Should we tag issue as "v5" or create a new ticket for strict JSON validation?

[vishr]

Should we tag issue as "v5" or create a new ticket for strict JSON validation?

@lammel Thanks. Will tag as v5.

[mariaefi29]

Thank you!!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mariaefi29]

https://github.com/labstack/echo/pull/1578

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed within a month if no further activity occurs. Thank you for your contributions.

[aldas]

@lammel maybe we should consider changing this now.

We recently had https://github.com/labstack/echo/discussions/2055#discussioncomment-1879293 and I started to think that this "defaulting" makes e.Bind() quite unintuitive or code like that

		if err := (&echo.DefaultBinder{}).BindPathParams(c, u); err != nil {
			return err
		}
		if err := (&echo.DefaultBinder{}).BindQueryParams(c, u); err != nil {
			return err
		}

func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
	if err := b.BindPathParams(c, i); err != nil {
		return err
	}
	if c.Request().Method == http.MethodGet || c.Request().Method == http.MethodDelete {
		if err = b.BindQueryParams(c, i); err != nil {
			return err
		}
	}
	return b.BindBody(c, i)
}

if path params are successfully bound and now we do query params binding - binder will zero/default these values if there are no query values. which is counterintiutive - which makes "priorities" nonexistent. Body binding does not do that as json/xml encoder do not overwrite non-existent fields.