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

馃悰 Request Header Fields Too Large when running CSRF and encrypt cookie middleware #1631

Closed
sevaho opened this issue Nov 25, 2021 · 7 comments 路 Fixed by #1698
Closed

馃悰 Request Header Fields Too Large when running CSRF and encrypt cookie middleware #1631

sevaho opened this issue Nov 25, 2021 · 7 comments 路 Fixed by #1698

Comments

@sevaho
Copy link

sevaho commented Nov 25, 2021

Fiber version 2

Issue description

When running following script in a browser you will notice that the cookie value is growing by continuous encrypting the value. The browser will respond with Request Header Fields Too Large when cookie is too big.

Code snippet

package main

import "github.com/gofiber/fiber/v2"
import "github.com/gofiber/fiber/v2/middleware/csrf"
import "github.com/gofiber/fiber/v2/middleware/encryptcookie"

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

	app.Use(csrf.New())

    // cookie value generated by running `cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1`
	app.Use(encryptcookie.New(encryptcookie.Config{
		Key: "VWY9XBVap74Zpd0ckbT1reTl0NM6pz7R"}))

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

	app.Listen(":3000")
}
@welcome
Copy link

welcome bot commented Nov 25, 2021

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

@fearofcode
Copy link

fearofcode commented Dec 12, 2021

I can reproduce this on the latest fiber release.

If you run the above code and keep refreshing, the _csrf cookie value eventually becomes too large.

@vinay03
Copy link

vinay03 commented Dec 18, 2021

For time being, changing the order of registering middlewares seems to be avoiding the issue.
Actually, encryptcookie middleware decrypts all cookies from Request Headers and set them again for further use. Then, in the end, it encrypts all cookies from Response Headers and set them again before sending out to Client. This design structure makes it compulsory to register encryptcookie middleware before any other middleware that deals with cookies(For e.g. CSRF).

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/csrf"
	"github.com/gofiber/fiber/v2/middleware/encryptcookie"
)

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

	// cookie value generated by running `cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1`
	app.Use(encryptcookie.New(encryptcookie.Config{
		Key: "VWY9XBVap74Zpd0ckbT1reTl0NM6pz7R"}))

	app.Use(csrf.New())

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

	app.Listen(":3000")
}

@ReneWerner87
Copy link
Member

ReneWerner87 commented Dec 28, 2021

#1343
@amir9480 can you help us with this problem

@silviucm
Copy link

silviucm commented Jan 2, 2022

To what @vinay03 mentioned, I would like to add that technically, the encryptcookie middleware already supports skipping cookies by name via the Except field at config time. So for csrf, one could just skip it, since it has its own generation ops (assuming below that the codebase keeps the default csrf cookie name as csrf_ )

	// cookie value generated by running `cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1`
	app.Use(encryptcookie.New(encryptcookie.Config{
		Key: "VWY9XBVap74Zpd0ckbT1reTl0NM6pz7R", Except: []string{"csrf_"}}))

With that added, you can use csrf independently positioned

@sevaho
Copy link
Author

sevaho commented Jan 9, 2022

Thanks @silviucm I'll close the issue, as it not an issue anymore but a small bug.

@amir9480
Copy link
Contributor

amir9480 commented Jan 9, 2022

@ReneWerner87
Sorry for the late response, I made a PR to fix this issue

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

Successfully merging a pull request may close this issue.

6 participants