From 2d41191ca5ca993306aac01dfa5a3260bb9a2838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Gro=C3=9Fe?= Date: Sat, 2 Dec 2023 08:14:46 +0000 Subject: [PATCH 1/3] Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" (#2742)" This reverts commit 28be17f929cfa7d3c27dd292fc3956f2f9882e22. --- docs/api/middleware/requestid.md | 2 +- middleware/requestid/config.go | 2 +- middleware/requestid/requestid_test.go | 7 ++++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/api/middleware/requestid.md b/docs/api/middleware/requestid.md index 112d21d48a..c8ca8d599e 100644 --- a/docs/api/middleware/requestid.md +++ b/docs/api/middleware/requestid.md @@ -45,7 +45,7 @@ app.Use(requestid.New(requestid.Config{ | Next | `func(*fiber.Ctx) bool` | Next defines a function to skip this middleware when returned true. | `nil` | | Header | `string` | Header is the header key where to get/set the unique request ID. | "X-Request-ID" | | Generator | `func() string` | Generator defines a function to generate the unique identifier. | utils.UUID | -| ContextKey | `string` | ContextKey defines the key used when storing the request ID in the locals for a specific request. | "requestid" | +| ContextKey | `interface{}` | ContextKey defines the key used when storing the request ID in the locals for a specific request. | "requestid" | ## Default Config The default config uses a fast UUID generator which will expose the number of diff --git a/middleware/requestid/config.go b/middleware/requestid/config.go index b3b605e590..cc69907822 100644 --- a/middleware/requestid/config.go +++ b/middleware/requestid/config.go @@ -26,7 +26,7 @@ type Config struct { // the locals for a specific request. // // Optional. Default: requestid - ContextKey string + ContextKey interface{} } // ConfigDefault is the default config diff --git a/middleware/requestid/requestid_test.go b/middleware/requestid/requestid_test.go index 451e96b4b2..ee3d33d629 100644 --- a/middleware/requestid/requestid_test.go +++ b/middleware/requestid/requestid_test.go @@ -55,20 +55,21 @@ func Test_RequestID_Next(t *testing.T) { func Test_RequestID_Locals(t *testing.T) { t.Parallel() reqID := "ThisIsARequestId" - ctxKey := "ThisIsAContextKey" + type ContextKey int + const requestContextKey ContextKey = iota app := fiber.New() app.Use(New(Config{ Generator: func() string { return reqID }, - ContextKey: ctxKey, + ContextKey: requestContextKey, })) var ctxVal string app.Use(func(c *fiber.Ctx) error { - ctxVal = c.Locals(ctxKey).(string) //nolint:forcetypeassert,errcheck // We always store a string in here + ctxVal = c.Locals(requestContextKey).(string) //nolint:forcetypeassert,errcheck // We always store a string in here return c.Next() }) From 12c311c9248876b41a68f830a6b05c2a8f149f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Gro=C3=9Fe?= Date: Sat, 2 Dec 2023 08:23:55 +0000 Subject: [PATCH 2/3] fix: request ContextKey default value condition Should check for `nil` since it is `any`. --- middleware/requestid/config.go | 6 ++++-- middleware/requestid/requestid_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/middleware/requestid/config.go b/middleware/requestid/config.go index cc69907822..b535ec9033 100644 --- a/middleware/requestid/config.go +++ b/middleware/requestid/config.go @@ -24,8 +24,10 @@ type Config struct { // ContextKey defines the key used when storing the request ID in // the locals for a specific request. + // Should be a private type instead of string, but too many apps probably + // rely on this exact value. // - // Optional. Default: requestid + // Optional. Default: "requestid" ContextKey interface{} } @@ -57,7 +59,7 @@ func configDefault(config ...Config) Config { if cfg.Generator == nil { cfg.Generator = ConfigDefault.Generator } - if cfg.ContextKey == "" { + if cfg.ContextKey == nil { cfg.ContextKey = ConfigDefault.ContextKey } return cfg diff --git a/middleware/requestid/requestid_test.go b/middleware/requestid/requestid_test.go index ee3d33d629..b2dc2ac7bf 100644 --- a/middleware/requestid/requestid_test.go +++ b/middleware/requestid/requestid_test.go @@ -77,3 +77,27 @@ func Test_RequestID_Locals(t *testing.T) { utils.AssertEqual(t, nil, err) utils.AssertEqual(t, reqID, ctxVal) } + +// go test -run Test_RequestID_DefaultKey +func Test_RequestID_DefaultKey(t *testing.T) { + t.Parallel() + reqID := "ThisIsARequestId" + + app := fiber.New() + app.Use(New(Config{ + Generator: func() string { + return reqID + }, + })) + + var ctxVal string + + app.Use(func(c *fiber.Ctx) error { + ctxVal = c.Locals("requestid").(string) //nolint:forcetypeassert,errcheck // We always store a string in here + return c.Next() + }) + + _, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) + utils.AssertEqual(t, nil, err) + utils.AssertEqual(t, reqID, ctxVal) +} From c7359e77d1bf45a2e27298ed3f217668f1c7b8eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Gro=C3=9Fe?= Date: Sat, 2 Dec 2023 17:38:51 +0000 Subject: [PATCH 3/3] fix: don't constrain middlewares' context-keys to strings `context` recommends using "unexported type" as context keys to avoid collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals. The official go blog also recommends this https://go.dev/blog/context. `fiber.Ctx.Locals(key any, value any)` correctly allows consumers to use unexported types or e.g. strings. But some fiber middlewares constrain their context-keys to `string` in their "default config structs", making it impossible to use unexported types. This PR removes the `string` _constraint_ from all middlewares, allowing to now use unexported types as per the official guidelines. However the default value is still a string, so it's not a breaking change, and anyone still using strings as context keys is not affected. --- docs/api/middleware/basicauth.md | 4 ++-- docs/api/middleware/csrf.md | 4 ++-- docs/api/middleware/keyauth.md | 2 +- middleware/adaptor/adaptor_test.go | 5 +++-- middleware/basicauth/config.go | 8 ++++---- middleware/csrf/config.go | 6 +++--- middleware/csrf/csrf.go | 2 +- middleware/idempotency/idempotency.go | 6 ++++-- middleware/keyauth/config.go | 4 ++-- 9 files changed, 22 insertions(+), 19 deletions(-) diff --git a/docs/api/middleware/basicauth.md b/docs/api/middleware/basicauth.md index 0e90eafed0..d0f3609bde 100644 --- a/docs/api/middleware/basicauth.md +++ b/docs/api/middleware/basicauth.md @@ -67,8 +67,8 @@ app.Use(basicauth.New(basicauth.Config{ | Realm | `string` | Realm is a string to define the realm attribute of BasicAuth. The realm identifies the system to authenticate against and can be used by clients to save credentials. | `"Restricted"` | | Authorizer | `func(string, string) bool` | Authorizer defines a function to check the credentials. It will be called with a username and password and is expected to return true or false to indicate approval. | `nil` | | Unauthorized | `fiber.Handler` | Unauthorized defines the response body for unauthorized responses. | `nil` | -| ContextUsername | `string` | ContextUsername is the key to store the username in Locals. | `"username"` | -| ContextPassword | `string` | ContextPassword is the key to store the password in Locals. | `"password"` | +| ContextUsername | `interface{}` | ContextUsername is the key to store the username in Locals. | `"username"` | +| ContextPassword | `interface{}` | ContextPassword is the key to store the password in Locals. | `"password"` | ## Default Config diff --git a/docs/api/middleware/csrf.md b/docs/api/middleware/csrf.md index 2c30ed5cbe..536e8a4b2d 100644 --- a/docs/api/middleware/csrf.md +++ b/docs/api/middleware/csrf.md @@ -152,14 +152,14 @@ app.Use(csrf.New(csrf.Config{ | Storage | `fiber.Storage` | Store is used to store the state of the middleware. | `nil` | | Session | `*session.Store` | Session is used to store the state of the middleware. Overrides Storage if set. | `nil` | | SessionKey | `string` | SessionKey is the key used to store the token within the session. | "fiber.csrf.token" | -| ContextKey | `string` | Context key to store the generated CSRF token into the context. If left empty, the token will not be stored within the context. | "" | +| ContextKey | `inteface{}` | Context key to store the generated CSRF token into the context. If left empty, the token will not be stored within the context. | "" | | KeyGenerator | `func() string` | KeyGenerator creates a new CSRF token. | utils.UUID | | CookieExpires | `time.Duration` (Deprecated) | Deprecated: Please use Expiration. | 0 | | Cookie | `*fiber.Cookie` (Deprecated) | Deprecated: Please use Cookie* related fields. | `nil` | | TokenLookup | `string` (Deprecated) | Deprecated: Please use KeyLookup. | "" | | ErrorHandler | `fiber.ErrorHandler` | ErrorHandler is executed when an error is returned from fiber.Handler. | DefaultErrorHandler | | Extractor | `func(*fiber.Ctx) (string, error)` | Extractor returns the CSRF token. If set, this will be used in place of an Extractor based on KeyLookup. | Extractor based on KeyLookup | -| HandlerContextKey | `string` | HandlerContextKey is used to store the CSRF Handler into context. | "fiber.csrf.handler" | +| HandlerContextKey | `interface{}` | HandlerContextKey is used to store the CSRF Handler into context. | "fiber.csrf.handler" | ### Default Config diff --git a/docs/api/middleware/keyauth.md b/docs/api/middleware/keyauth.md index ecabe122e7..1a719c134d 100644 --- a/docs/api/middleware/keyauth.md +++ b/docs/api/middleware/keyauth.md @@ -221,7 +221,7 @@ curl --header "Authorization: Bearer my-super-secret-key" http://localhost:3000 | KeyLookup | `string` | KeyLookup is a string in the form of "`:`" that is used to extract key from the request. | "header:Authorization" | | AuthScheme | `string` | AuthScheme to be used in the Authorization header. | "Bearer" | | Validator | `func(*fiber.Ctx, string) (bool, error)` | Validator is a function to validate the key. | A function for key validation | -| ContextKey | `string` | Context key to store the bearer token from the token into context. | "token" | +| ContextKey | `interface{}` | Context key to store the bearer token from the token into context. | "token" | ## Default Config diff --git a/middleware/adaptor/adaptor_test.go b/middleware/adaptor/adaptor_test.go index 340c05c04b..6e03b05a2d 100644 --- a/middleware/adaptor/adaptor_test.go +++ b/middleware/adaptor/adaptor_test.go @@ -36,7 +36,8 @@ func Test_HTTPHandler(t *testing.T) { expectedURL, err := url.ParseRequestURI(expectedRequestURI) utils.AssertEqual(t, nil, err) - expectedContextKey := "contextKey" + type contextKeyType string + expectedContextKey := contextKeyType("contextKey") expectedContextValue := "contextValue" callsCount := 0 @@ -293,7 +294,7 @@ func testFiberToHandlerFunc(t *testing.T, checkDefaultPort bool, app ...*fiber.A utils.AssertEqual(t, expectedResponseBody, string(w.body), "Body") } -func setFiberContextValueMiddleware(next fiber.Handler, key string, value interface{}) fiber.Handler { +func setFiberContextValueMiddleware(next fiber.Handler, key, value interface{}) fiber.Handler { return func(c *fiber.Ctx) error { c.Locals(key, value) return next(c) diff --git a/middleware/basicauth/config.go b/middleware/basicauth/config.go index 3845e91538..d69f48be67 100644 --- a/middleware/basicauth/config.go +++ b/middleware/basicauth/config.go @@ -44,12 +44,12 @@ type Config struct { // ContextUser is the key to store the username in Locals // // Optional. Default: "username" - ContextUsername string + ContextUsername interface{} // ContextPass is the key to store the password in Locals // // Optional. Default: "password" - ContextPassword string + ContextPassword interface{} } // ConfigDefault is the default config @@ -95,10 +95,10 @@ func configDefault(config ...Config) Config { return c.SendStatus(fiber.StatusUnauthorized) } } - if cfg.ContextUsername == "" { + if cfg.ContextUsername == nil { cfg.ContextUsername = ConfigDefault.ContextUsername } - if cfg.ContextPassword == "" { + if cfg.ContextPassword == nil { cfg.ContextPassword = ConfigDefault.ContextPassword } return cfg diff --git a/middleware/csrf/config.go b/middleware/csrf/config.go index 539e84967c..9ab6cab063 100644 --- a/middleware/csrf/config.go +++ b/middleware/csrf/config.go @@ -93,7 +93,7 @@ type Config struct { // If left empty, token will not be stored in context. // // Optional. Default: "" - ContextKey string + ContextKey interface{} // KeyGenerator creates a new CSRF token // @@ -124,7 +124,7 @@ type Config struct { // HandlerContextKey is used to store the CSRF Handler into context // // Default: "fiber.csrf.handler" - HandlerContextKey string + HandlerContextKey interface{} } const HeaderName = "X-Csrf-Token" @@ -204,7 +204,7 @@ func configDefault(config ...Config) Config { if cfg.SessionKey == "" { cfg.SessionKey = ConfigDefault.SessionKey } - if cfg.HandlerContextKey == "" { + if cfg.HandlerContextKey == nil { cfg.HandlerContextKey = ConfigDefault.HandlerContextKey } diff --git a/middleware/csrf/csrf.go b/middleware/csrf/csrf.go index f4dee74c8e..939daaea79 100644 --- a/middleware/csrf/csrf.go +++ b/middleware/csrf/csrf.go @@ -129,7 +129,7 @@ func New(config ...Config) fiber.Handler { c.Vary(fiber.HeaderCookie) // Store the token in the context if a context key is specified - if cfg.ContextKey != "" { + if cfg.ContextKey != nil { c.Locals(cfg.ContextKey, token) } diff --git a/middleware/idempotency/idempotency.go b/middleware/idempotency/idempotency.go index 604f867c76..5affc59620 100644 --- a/middleware/idempotency/idempotency.go +++ b/middleware/idempotency/idempotency.go @@ -12,9 +12,11 @@ import ( // Inspired by https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-idempotency-key-header-02 // and https://github.com/penguin-statistics/backend-next/blob/f2f7d5ba54fc8a58f168d153baa17b2ad4a14e45/internal/pkg/middlewares/idempotency.go +type localsKeys string + const ( - localsKeyIsFromCache = "idempotency_isfromcache" - localsKeyWasPutToCache = "idempotency_wasputtocache" + localsKeyIsFromCache localsKeys = "idempotency_isfromcache" + localsKeyWasPutToCache localsKeys = "idempotency_wasputtocache" ) func IsFromCache(c *fiber.Ctx) bool { diff --git a/middleware/keyauth/config.go b/middleware/keyauth/config.go index 39d71b6305..c762d72ce6 100644 --- a/middleware/keyauth/config.go +++ b/middleware/keyauth/config.go @@ -41,7 +41,7 @@ type Config struct { // Context key to store the bearertoken from the token into context. // Optional. Default: "token". - ContextKey string + ContextKey interface{} } // ConfigDefault is the default config @@ -87,7 +87,7 @@ func configDefault(config ...Config) Config { if cfg.Validator == nil { panic("fiber: keyauth middleware requires a validator function") } - if cfg.ContextKey == "" { + if cfg.ContextKey == nil { cfg.ContextKey = ConfigDefault.ContextKey }