Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 tasks done
Max-Cheng opened this issue Apr 24, 2024 · 10 comments

Comments

@Max-Cheng
Copy link

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:

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

welcome bot commented Apr 24, 2024

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
Copy link
Member

gaby commented Apr 24, 2024

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

@Max-Cheng
Copy link
Author

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

@gaby
Copy link
Member

gaby commented Apr 24, 2024

Ping @sixcolors

@Max-Cheng
Copy link
Author

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

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

@Max-Cheng
Copy link
Author

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
Copy link
Member

sixcolors commented Apr 25, 2024

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
Copy link
Member

sixcolors commented Apr 25, 2024

@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
Copy link
Member

sixcolors commented Apr 25, 2024

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants