gofiber / fiber

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

🐛 [Bug]: ctx.Accepts("text", "json") with headerAccept, "text/*, application/json" fails on macOS #2383

Closed sixcolors closed 1 year ago

sixcolors commented 1 year ago

Bug Description

ctx.Accepts("text", "json") with headerAccept, "text/*, application/json" is not the same on macOS and linux

On macOS it returns "text"

On linux it returns "json"

How to Reproduce

Run test on macOS and linux

Expected Behavior

consistent return and tests pass

Fiber Version

v2.43.0

Code Snippet (optional)

func Test_Ctx_Accepts(t *testing.T) {
    t.Parallel()
    app := New()
    c := app.AcquireCtx(&fasthttp.RequestCtx{})
    defer app.ReleaseCtx(c)

    c.Request().Header.Set(HeaderAccept, "text/*, application/json")
    utils.AssertEqual(t, "json", c.Accepts("json", "text"))
}

// called with above params DEBUG comments added
func (c *Ctx) Accepts(offers ...string) string {
    if len(offers) == 0 {
        return ""
    }
    header := c.Get(HeaderAccept)
    if header == "" {
        return offers[0]
    }

    spec, commaPos := "", 0
    for len(header) > 0 && commaPos != -1 {
        commaPos = strings.IndexByte(header, ',')
        if commaPos != -1 {
            spec = utils.Trim(header[:commaPos], ' ')
        } else {
            spec = utils.TrimLeft(header, ' ')
        }
        if factorSign := strings.IndexByte(spec, ';'); factorSign != -1 {
            spec = spec[:factorSign]
        }

        var mimetype string
        for _, offer := range offers { // DEBUG first loop "text"
            if len(offer) == 0 {
                continue
                // Accept: */*
            } else if spec == "*/*" {
                return offer
            }

            if strings.IndexByte(offer, '/') != -1 {
                mimetype = offer // MIME type
            } else {
                mimetype = utils.GetMIME(offer) // DEBUG on linux: "application/octet-stream", on macOS: "text/plain; charset=utf-8"
            }

            if spec == mimetype {
                // Accept: <MIME_type>/<MIME_subtype>
                return offer
            }

            s := strings.IndexByte(mimetype, '/')
            // Accept: <MIME_type>/*
            if strings.HasPrefix(spec, mimetype[:s]) && (spec[s:] == "/*" || mimetype[s:] == "/*") { // DEBUG first loop mimeType = "text/*"
                return offer // DEBUG on macOS text spec matches the string before / (text), on linux text does not match string before / (application)
            }
        }
        if commaPos != -1 {
            header = header[commaPos+1:]
        }
    }

    return ""
}

Further context

Caused by: utils.GetMIME("text"), which calls mime.TypeByExtension("." + extension) on macOS returns "text/plain; charset=utf-8" while on linux it returns ""

Checklist:

sixcolors commented 1 year ago

If we add "text" to http.go mime extensions

// - Add "text" as an alias for "text/plain" as recommended per https://datatracker.ietf.org/doc/html/rfc2046#section-4.1var mimeExtensions = map[string]string{
var mimeExtensions = map[string]string{
...
"text":    "text/plain",
"txt":     "text/plain",
...
}

Then behaviour becomes consistent across platforms.

This still leaves the question of if subtypes take priority in the same way that types do in the mime headers. https://docs.gofiber.io/api/ctx#accepts indicates such, as does test assertion in ctx_test.go to line 52.

If so the code does not do that and only by coincidence of mime.TypeByExtension(".text") returning "" does GetMIME default to returning MIMEOctetStream which does not match "text" in as the first spec and returning early from the for a loop.

sixcolors commented 1 year ago

The section of RFC 2046 that defines the "text" media type and its subtypes is Section 4.1. In this section, the RFC states:

The top-level media type "text" has a number of subtype values which indicate human-readable text in one form or another. The primary subtype, "plain", indicates plain (unformatted) text.

...

Other subtypes of "text" usefully include types that represent formatted text containing controlled white space (e.g., "text/tab- separated-values") and text representing markup languages (e.g., "text/html").

...

It is important to note that "text" cannot be assumed to be anything other than a structured byte sequence whose precise structure is not generally known to the recipient.

...

The recommended practice for the Internet is to label human- readable content types as "text/plain" and most machine-readable formats as "application/octet-stream".

This passage implies that "text" is a top-level media type that encompasses various subtypes, including "text/plain". Later in the same section, the RFC notes that "text/plain" is the recommended subtype for human-readable content types, suggesting that "text" can be used as an alias for "text/plain" in these contexts.

ReneWerner87 commented 1 year ago

Its related to https://github.com/gofiber/fiber/pull/2340

sixcolors commented 1 year ago

Its related to #2340

I think adding text to the mime map would be an acceptable solution to normalize the behavior.

But what about the specific scenario: if text is a mime type matching text/ in the inner loop should that return text? Or should it check for type or subtype match and return json?

If so the Accepts code needs to be updated.

Based on the test my thinking is the loops are reversed. We should check the offers first then see if they match one of the headerAccept mime types. Perhaps using a [acceptHeader] type/subtype map.

That also makes me wonder about media type parameters (if we need to match those too).

Thoughts?

sixcolors commented 1 year ago

And what about the q value? https://developer.mozilla.org/en-US/docs/Glossary/Quality_values

ReneWerner87 commented 1 year ago

@derkan can you help here

derkan commented 1 year ago

@sixcolors, what is the go version?

I've tested with go1.19.7 and by adding a new test line to ctx_test.go like this:

...
c.Request().Header.Set(HeaderAccept, "text/*, application/json")
...
utils.AssertEqual(t, "json", c.Accepts("text", "json"))

Unfortunately I got same result: "json" on macOS for both go versions(and fiber master@latest). Am I missing something or this problem is because of go version?

sixcolors commented 1 year ago

@sixcolors, what is the go version?

I've tested with go1.19.7 and by adding a new test line to ctx_test.go like this:

...
c.Request().Header.Set(HeaderAccept, "text/*, application/json")
...
utils.AssertEqual(t, "json", c.Accepts("text", "json"))

Unfortunately I got same result: "json" on macOS for both go versions(and fiber master@latest). Am I missing something or this problem is because of go version?

1.20.2

sixcolors commented 1 year ago

@sixcolors, what is the go version? I've tested with go1.19.7 and by adding a new test line to ctx_test.go like this:

...
c.Request().Header.Set(HeaderAccept, "text/*, application/json")
...
utils.AssertEqual(t, "json", c.Accepts("text", "json"))

Unfortunately I got same result: "json" on macOS for both go versions(and fiber master@latest). Am I missing something or this problem is because of go version?

1.20.2

However it probably has more to do with macOS install than go version looking at https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/mime/type.go;l=102

I’ll check the files when I’m back on a computer (on phone right now)

derkan commented 1 year ago

I've also tested with following line added to ctx_test.go on macOS and go1.20.2, no problem.

c.Request().Header.Set(HeaderAccept, "text/*, application/json")
utils.AssertEqual(t, "json", c.Accepts("text", "json"))
sixcolors commented 1 year ago

I'm seeing .text registered with text edit. So that should be on basically every macOS install...

mdls -name kMDItemContentTypeTree example.text kMDItemContentTypeTree = ( "public.plain-text", "public.text", "public.data", "public.item", "public.content" )

duti -x text TextEdit.app /System/Applications/TextEdit.app com.apple.TextEdit

let me write a sample app.

sixcolors commented 1 year ago

@derkan made a sample and test pass using CI GitHub action but fails locally. Checking a fresh install of macOS to make sure.

https://github.com/sixcolors/textmime/ https://github.com/sixcolors/textmime/actions/runs/4524905041

~/textmime (main) » go test -v ./... sixcolors@Jasons-MacBook-Pro === RUN TestAccepts main_test.go:13: mimetype is text/plain; charset=utf-8, want application/json --- FAIL: TestAccepts (0.00s) FAIL FAIL github.com/sixcolors/textmime 0.596s FAIL

sixcolors commented 1 year ago

@derkan yes it fails on multiple local macOS versions including a fresh install of the latest macOS

Please see the test project.

sixcolors commented 1 year ago

@ReneWerner87 and @derkan

The other part is this:

...
c.Request().Header.Set(HeaderAccept, "text/*, application/json")
...
utils.AssertEqual(t, "json", c.Accepts("json", "txt"))
utils.AssertEqual(t, "json", c.Accepts("json", "text/plain"))

Those should still pass, right? If so then the logic in Accepts is wrong.

ReneWerner87 commented 1 year ago

I think the accepts method works wrong

instead of going through the arguments in the loop and comparing them one by one with the header values

the header values are split and compared with the passed arguments one after the other