search

gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
33.67k stars 1.65k forks source link

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

Closed rere61 closed 1 year ago

rere61 commented 1 year ago

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:

  1. Go to '....'
  2. Click on '....'
  3. Do '....'
  4. 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:

welcome[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

ReneWerner87 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

rere61 commented 1 year ago

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 commented 1 year ago

@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>

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

  1. 1st teacher's Family name
  2. 1st teacher's Given name
  3. 2nd teacher's Family name
  4. 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 commented 2 weeks ago

@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 commented 1 week ago

@ngocchien

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