gofiber / fiber

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

🤗 [Question]: Enable DisableHeaderNormalizing config will occur wrong cors middleware behaviour #2985

Open Max-Cheng opened 6 months ago

Max-Cheng commented 6 months ago

Question Description

About fiber.Config{DisableHeaderNormalizing:true} and using cors middleware will occur client send origin: hostxxx will not return CORS header.

Code Snippet (optional)

package main

import (
    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/log"
    "github.com/gofiber/fiber/v2/middleware/cors"
)

func main() {
    app := fiber.New()
    app.Use(cors.New(cors.Config{
        AllowOrigins: "*",
        AllowMethods: "*",
        AllowHeaders: "*",
    }))
    // #### Request 1 ####
    // GET http://localhost:3000
    // Origin: http://localhost:3000
    // User-Agent: IntelliJ HTTP Client/GoLand 2024.1
    // Accept-Encoding: br, deflate, gzip, x-gzip
    // Accept: */*
    // content-length: 0
    //
    // HTTP/1.1 404 Not Found
    // Date: Wed, 24 Apr 2024 13:54:10 GMT
    // Content-Type: text/plain; charset=utf-8
    // Content-Length: 12
    // Access-Control-Allow-Origin: *
    // #### Request 1 END ####
    //
    // #### Request 2 ####
    // GET http://localhost:3000
    // origin: http://localhost:3000
    // User-Agent: IntelliJ HTTP Client/GoLand 2024.1
    // Accept-Encoding: br, deflate, gzip, x-gzip
    // Accept: */*
    // content-length: 0
    //
    // HTTP/1.1 404 Not Found
    // Date: Wed, 24 Apr 2024 13:55:52 GMT
    // Content-Type: text/plain; charset=utf-8
    // Content-Length: 12
    // Access-Control-Allow-Origin: *
    //
    // Cannot GET /
    // #### Request 2 END ####

    app.Get("/", func(c *fiber.Ctx) error {
        return c.SendString("Hello, World!")
    })
    log.Fatal(app.Listen(":3000"))
}

package main

import (
    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/log"
    "github.com/gofiber/fiber/v2/middleware/cors"
)

func main() {
    app := fiber.New(fiber.Config{
        DisableHeaderNormalizing: true,
    })
    app.Use(cors.New(cors.Config{
        AllowOrigins: "*",
        AllowMethods: "*",
        AllowHeaders: "*",
    }))
    // #### Request 3 ####
    // GET http://localhost:3000
    // origin: http://localhost:3000
    // User-Agent: IntelliJ HTTP Client/GoLand 2024.1
    // Accept-Encoding: br, deflate, gzip, x-gzip
    // Accept: */*
    // content-length: 0
    //
    // HTTP/1.1 200 OK
    // Date: Wed, 24 Apr 2024 13:58:15 GMT
    // Content-Type: text/plain; charset=utf-8
    // Content-Length: 13
    //
    // Hello, World!
    //
    // #### Request 3 End ####

    // #### Request 4 ####
    // GET http://localhost:3000
    // Origin: http://localhost:3000
    //
    // HTTP/1.1 200 OK
    // Date: Wed, 24 Apr 2024 13:59:37 GMT
    // Content-Type: text/plain; charset=utf-8
    // Content-Length: 13
    // Access-Control-Allow-Origin: *
    // #### Request 4 End ####

    app.Get("/", func(c *fiber.Ctx) error {
        return c.SendString("Hello, World!")
    })

    log.Fatal(app.Listen(":3000"))
}

Checklist:

welcome[bot] commented 6 months 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

gaby commented 6 months ago

@Max-Cheng Which version of v2 are you running?

Max-Cheng commented 6 months ago

The main issue is whether Enable DisableHeaderNormalizing should affect the behavior of the CORS middleware or not.

gaby commented 6 months ago

Ping @sixcolors

Max-Cheng commented 6 months ago

@Max-Cheng Which version of v2 are you running?

github.com/gofiber/fiber/v2 v2.52.4

Max-Cheng commented 6 months ago

In my situation, I'm using a global middleware to enforce the Origin header to be in uppercase and process the correct logical path.

package main

import (
    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/log"
    "github.com/gofiber/fiber/v2/middleware/cors"
)

func main() {
    app := fiber.New(fiber.Config{
        DisableHeaderNormalizing: true,
    })
    app.Use(func(c *fiber.Ctx) error {
        if c.Get("origin") != "" {
            c.Request().Header.Set(fiber.HeaderOrigin, c.Get("origin"))
            return c.Next()
        }
        return c.Next()
    })
    app.Use(cors.New(cors.Config{
        AllowOrigins: "*",
        AllowMethods: "*",
        AllowHeaders: "*",
    }))
    app.Get("/", func(c *fiber.Ctx) error {
        return c.SendString("Hello, World!")
    })

    log.Fatal(app.Listen(":3000"))
}
sixcolors commented 6 months ago

This should not be the case. CORS middleware calls originHeader := strings.ToLower(c.Get(fiber.HeaderOrigin)), and c.Get is case insensitive, https://docs.gofiber.io/api/ctx/#get.

I will test and get back to you.

sixcolors commented 6 months ago

@Max-Cheng I understand what you mean now. If you set app := fiber.New(fiber.Config{ DisableHeaderNormalizing: true, }), it will impact headers in any fiber middleware.

Since the Cross-Origin Resource Sharing (CORS) and other middleware included with fiber use c.Get for headers, the middleware behaviour will follow this pattern. According to https://datatracker.ietf.org/doc/html/rfc2616#section-4.2, "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive." Therefore, the default fiber behaviour is correct. However, the DisableHeaderNormalizing option allows users to disable this, which would cause the behaviour you noted.

However, the comment for DisableHeaderNormalizing does not seem to adequately capture this:

    // When set to true, disables header normalization.
    // By default all header names are normalized: conteNT-tYPE -> Content-Type.
    //
    // Default: false

Because https://docs.gofiber.io/api/ctx/#get notes that: "The match is case-insensitive." and the DisableHeaderNormalizing does not specify that it has other effects, I think we can address that in the documentation.

sixcolors commented 6 months ago

It appears that the only effect that DisableHeaderNormalizing has is to set app.server.DisableHeaderNamesNormalizing = app.config.DisableHeaderNormalizing.

The fasthttp documentation for this setting is a bit more detailed on its effects:

    // Header names are passed as-is without normalization
    // if this option is set.
    //
    // Disabled header names' normalization may be useful only for proxying
    // incoming requests to other servers expecting case-sensitive
    // header names. See https://github.com/valyala/fasthttp/issues/57
    // for details.
    //
    // By default request and response header names are normalized, i.e.
    // The first letter and the first letters following dashes
    // are uppercased, while all the other letters are lowercased.
    // Examples:
    //
    //     * HOST -> Host
    //     * content-type -> Content-Type
    //     * cONTENT-lenGTH -> Content-Length
    DisableHeaderNamesNormalizing bool
Max-Cheng commented 6 months ago

Yes. I don't think we should change the behaviour of the CORS middleware

Max-Cheng commented 5 months ago

Another mistake in this case. If DisableHeaderNormalizing is enabled and the client is using HTTP/2 protocol to access the backend. CORS middleware will judge that this request is not a CORS request.

https://github.com/gofiber/fiber/blob/v2/middleware/cors/cors.go#L177