🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma #2557

rere61 · 2023-07-31T05:17:49Z

Bug Description

In an HTML form, if there are multiple input elements with same name and the user enters content containing commas(,), thenBodyParser() function will fail to parse correctly into the slices member of the struct

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo'  type='text' value='a,b'>
          <input name='todo' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

  
  func (c *Test) Test(ctx *fiber.Ctx) error {
	  type MyForm struct {
		  Todo []string `form:"todo"`
	  }
	  
	  var frm MyForm
	  ctx.BodyParser(&frm)
	  
          //incorrect member size 3
          // frm.Todo[0] = "a"      frm.Todo[1]="b"   frm.Todo[2]="c"
        
         // correct members should be:  frm.Todo[0]="a,b"      frm.Todo[1]="c"

	  fmt.Println(len(frm.Todo))
  }

How to Reproduce

Steps to reproduce the behavior:

Go to '....'
Click on '....'
Do '....'
See '....'

Expected Behavior

The number of elements in the slice member parsed into the struct must be the same as the number of input elements in the HTML form, and their content must also match.

Fiber Version

v2.48.0

Code Snippet (optional)

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo'  type='text' value='a,b'>
          <input name='todo' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

package main

import "github.com/gofiber/fiber/v2"
import "log"

func main() {
  app := fiber.New()

  // Steps to reproduce
	app.Post("/",func(ctx *fiber.Ctx) error {
		type MyForm struct {
			Todo []string `form:"todo"`
		}

		var frm MyForm
		ctx.BodyParser(&frm)

	    //incorrect member size 3
          // frm.Todo[0] = "a"      frm.Todo[1]="b"   frm.Todo[2]="c"
        
         // correct members should be:  frm.Todo[0]="a,b"      frm.Todo[1]="c"
		return nil
	})
  log.Fatal(app.Listen(":3000"))
}

Checklist:

I agree to follow Fiber's Code of Conduct.
I have checked for existing issues that describe my problem prior to opening this one.
I understand that improperly formatted bug reports may be closed without explanation.

welcome · 2023-07-31T05:17:50Z

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

ReneWerner87 · 2023-07-31T06:19:45Z

the usage is incorrect, you cannot use the same key for multiple elements in one form

regardless of the backend, the data is already overwritten during the transfer from the frontend and the last value with the same key should win

rere61 · 2023-07-31T06:40:43Z

the usage is incorrect, you cannot use the same key for multiple elements in one form

regardless of the backend, the data is already overwritten during the transfer from the frontend and the last value with the same key should win

Nope.
HTML Form elements with same names are allowed, and don't be overwritten as you think, you can use browser's dev tools to see how browser send(post) form data to backend server.
An example in http requested body will be: todo=a%2Cb&todo=c
that is URL encoded data, its origin data is todo=a,b&todo=c

The root cause of this bug is in the following implementation of BodyParser:

if strings.HasPrefix(ctype, MIMEApplicationForm) {
		data := make(map[string][]string)
		var err error

		c.fasthttp.PostArgs().VisitAll(func(key, val []byte) {
			if err != nil {
				return
			}

			k := c.app.getString(key)
			v := c.app.getString(val)

			if strings.Contains(k, "[") {
				k, err = parseParamSquareBrackets(k)
			}

                       //why do this ?  split values by comma? this is input data from users.
			if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
				values := strings.Split(v, ",")
				for i := 0; i < len(values); i++ {
					data[k] = append(data[k], values[i])
				}
			} else {
				data[k] = append(data[k], v)
			}
		})

		return c.parseToStruct(bodyTag, out, data)
	}

ReneWerner87 · 2023-07-31T06:51:09Z

ok good to know
at that time there were several requests with the requirement to split the values which are list values directly

ReneWerner87 · 2023-07-31T06:53:49Z

why don't you send the structure right away as you expect it?

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo.0'  type='text' value='a,b'>
          <input name='todo.1' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

rere61 · 2023-07-31T06:58:19Z

Dear sir:

Please see the following Go's standard net/http package for references,
this is the correct behavior like other web frameworks.
you cannot split input values ,from users's input , by comma inside BodyParser automatically, that is not the expected behavior.
If there are two inputs with same name, then the backend will get two inputs, not three with incorrect values.
Please see the implementation of Request.Form.

for my example, r.Form["todo"] will get multiple values .

// FormValue returns the first value for the named component of the query.
// POST and PUT body parameters take precedence over URL query string values.
// FormValue calls ParseMultipartForm and ParseForm if necessary and ignores
// any errors returned by these functions.
// If key is not present, FormValue returns the empty string.
// To access multiple values of the same key, call ParseForm and
// then inspect Request.Form directly.
func (r *Request) FormValue(key string) string {
	if r.Form == nil {
		r.ParseMultipartForm(defaultMaxMemory)
	}
	if vs := r.Form[key]; len(vs) > 0 {
		return vs[0]
	}
	return ""
}

	// Form contains the parsed form data, including both the URL
	// field's query parameters and the PATCH, POST, or PUT form data.
	// This field is only available after ParseForm is called.
	// The HTTP client ignores Form and uses Body instead.
	Form url.Values

	// PostForm contains the parsed form data from PATCH, POST
	// or PUT body parameters.
	//
	// This field is only available after ParseForm is called.
	// The HTTP client ignores PostForm and uses Body instead.
	PostForm url.Values

html sample:

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo'  type='text' value='a,b'>
          <input name='todo' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

backend code sample with net/http:

package main

import (
	"fmt"
	"log"
	"net/http"
)

func hello(w http.ResponseWriter, r *http.Request) {
	if r.Method == http.MethodPost {
		r.ParseForm()
		fmt.Println(r.Form["todo"])
		fmt.Println(len(r.Form["todo"]))  // slice size 2
	}
}

func main() {
	http.HandleFunc("/", hello)

	if err := http.ListenAndServe(":8080", nil); err != nil {
		log.Fatal(err)
	}
}

output:

[a,b c]

but the usecases above in GoFiber will have the wrong result because the data contains comma: [a,b,c], and slice size is 3, that is GoFibier's different behavior compared to other framework or Go built-in net/http package.
If the element number with same name in HTML form are 2, then the backend need to report them accurately like net/http, whatever user's input contains comma or not.

rere61 · 2023-07-31T23:29:36Z

why don't you send the structure right away as you expect it?

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo.0'  type='text' value='a,b'>
          <input name='todo.1' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

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

@ReneWerner87
It does not help, the slice size is still 3 not 2, and the first element of slice is "a" not "a,b".
In about example the slice size should be 2 whatever the input value contains comma(",") or not.
If the first input value does not contain comma, the result is right(size 2),
So please don't split values by comma in framework side.

Sorry for my bad English, I hope you understand what I mentioned.

Could you please look at the implementation BodyParser() function.

And please look at my explanation about how standard net/http respond this.

In addition to standard net/http, I also try other web framework like gin, and the result is expected as size 2.

<html>
<head>
</head>
<body>
<form method="post" action="http://localhost:8080/">
<input name='todo'  type='text' value='a,b'>
<input name='todo' type='text' value='c'>
<input type='submit' value='submit'>
</form>
</body>
</html>

package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
)

type Form struct {
	Todo []string `form:"todo"`
}

func main() {
	router := gin.Default()

	router.POST("/", func(c *gin.Context) {
		var frm Form

		c.Bind(&frm)
		fmt.Println(len(frm.Todo))
		fmt.Println(frm.Todo)
	})

	router.Run(":8080")

}

standard net/http:

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='todo'  type='text' value='a,b'>
          <input name='todo' type='text' value='c'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

package main

import (
	"fmt"
	"log"
	"net/http"
)

func hello(w http.ResponseWriter, r *http.Request) {
	if r.Method == http.MethodPost {
		r.ParseForm()
		fmt.Println(r.Form["todo"])
		fmt.Println(len(r.Form["todo"]))  // slice size 2
	}
}

func main() {
	http.HandleFunc("/", hello)

	if err := http.ListenAndServe(":8080", nil); err != nil {
		log.Fatal(err)
	}
}

So the problem is the following code in GoFiber as I mentioned:
why do this ? split values by comma? this is input data from users, and comma could happen in different scenarios.

			if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
				values := strings.Split(v, ",")
				for i := 0; i < len(values); i++ {
					data[k] = append(data[k], values[i])
				}
			} else {
				data[k] = append(data[k], v)
			}

ReneWerner87 · 2023-08-01T06:58:04Z

Release https://github.com/gofiber/fiber/releases/tag/v2.0.3
it was requested in #782
and implemented in #817
for the QueryParser and we have adopted the logic for the other parsers as well

rere61 · 2023-08-01T07:14:47Z

Release https://github.com/gofiber/fiber/releases/tag/v2.0.3 it was requested in #782 and implemented in #817 for the QueryParser and we have adopted the logic for the other parsers as well

@renanbastos93
@ReneWerner87
@Fenny
@kiyonlin
@koddr
Please try to understand how standard Go's net/http and other web framworks(even in other programming languages) do,
I think I already provide enough codes and explanations to help you understand that.
I think this behavior you just mentioned is total wrong.
Sometimes, users' requirements are not correct.

rere61 · 2023-08-02T14:30:14Z

@ReneWerner87
Supposed there are a form for users to input their teachers in school, like the following html,
and this user enter 2 teachers(teachers' name is family name, given name) in the form.

please guest what will happen in GoFiber、net/http、Gin or other frameworks ?
what is the correct result in you expectation?

<html>
      <head>
      </head>
      <body>
        <form method="post" action="/">
          <input name='teacher'  type='text' value='Hemingway, Ernest Miller'>
          <input name='teacher' type='text' value='Olivier, Laurence Kerr'>
<--- user can dynamically add input elements by javascript --->
    <input name='teacher'  type='text' value='.....'>
          <input type='submit' value='submit'>
      </form>
      </body>
  </html>

in Go's standard net/http, there are 2 teachers ["Hemingway, Ernest Miller","Olivier, Laurence Kerr"]
in Gin , there are 2 teachers["Hemingway, Ernest Miller","Olivier, Laurence Kerr"]
in GoFiber, there are 4 teachers["Hemingway", "Ernest Miller","Olivier", "Laurence Kerr"]

Oh My, GoFiber split two teachers name by '," in the format "Family name, Given name" to a slice containing 4 elements,

1st teacher's Family name
1st teacher's Given name
2nd teacher's Family name
2nd teacher's Given name

and tell backend code: " User just enter 4 teachers "
then some day, this user found the bug, and go to tell the programmer,
User: I only enter 2 teachers, your program has bug.
Programmer: after some investigation Oh, No. There is not what I expected in others framework, even in Go standard net/http.

ngocchien · 2024-10-08T07:19:50Z

@rere61,
Has your problem been resolved? Could you share your solution to resolve this problem?
I am facing a problem the same way you are.

Thank you so much.

rere61 · 2024-10-08T08:04:53Z

@ngocchien

You can update gofiber to the latest version, and your problem should be solved.

rere61 added the ☢️ Bug label Jul 31, 2023

rere61 changed the title ~~🐛 [Bug]:~~ 🐛 [Bug]: BodyParser will fail to parse correctly into the slices member of the struct if input data contains comma Jul 31, 2023

rere61 changed the title ~~🐛 [Bug]: BodyParser will fail to parse correctly into the slices member of the struct if input data contains comma~~ 🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma Jul 31, 2023

ReneWerner87 added 💭 Under Consideration 🔍 Under Investigation labels Aug 2, 2023

efectn mentioned this issue Aug 6, 2023

🔥 add config to enable splitting by comma in parsers #2560

Merged

9 tasks

ReneWerner87 closed this as completed in #2560 Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma #2557

🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma #2557

rere61 commented Jul 31, 2023 •

edited

Loading

welcome bot commented Jul 31, 2023

ReneWerner87 commented Jul 31, 2023

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

ReneWerner87 commented Jul 31, 2023

ReneWerner87 commented Jul 31, 2023

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

ReneWerner87 commented Aug 1, 2023 •

edited

Loading

rere61 commented Aug 1, 2023 •

edited

Loading

rere61 commented Aug 2, 2023 •

edited

Loading

ngocchien commented Oct 8, 2024

rere61 commented Oct 8, 2024

🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma #2557

🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma #2557

Comments

rere61 commented Jul 31, 2023 • edited Loading

Bug Description

How to Reproduce

Expected Behavior

Fiber Version

Code Snippet (optional)

Checklist:

welcome bot commented Jul 31, 2023

ReneWerner87 commented Jul 31, 2023

rere61 commented Jul 31, 2023 • edited by ReneWerner87 Loading

ReneWerner87 commented Jul 31, 2023

ReneWerner87 commented Jul 31, 2023

rere61 commented Jul 31, 2023 • edited by ReneWerner87 Loading

rere61 commented Jul 31, 2023 • edited by ReneWerner87 Loading

ReneWerner87 commented Aug 1, 2023 • edited Loading

rere61 commented Aug 1, 2023 • edited Loading

rere61 commented Aug 2, 2023 • edited Loading

ngocchien commented Oct 8, 2024

rere61 commented Oct 8, 2024

rere61 commented Jul 31, 2023 •

edited

Loading

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

rere61 commented Jul 31, 2023 •

edited by ReneWerner87

Loading

ReneWerner87 commented Aug 1, 2023 •

edited

Loading

rere61 commented Aug 1, 2023 •

edited

Loading

rere61 commented Aug 2, 2023 •

edited

Loading