Skip to content

Commit

Permalink
fix: don't constrain middlewares' context-keys to strings 🐛 (#2751)
Browse files Browse the repository at this point in the history
* Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" (#2742)"

This reverts commit 28be17f.

* fix: request ContextKey default value condition

Should check for `nil` since it is `any`.

* 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.
  • Loading branch information
benjajaja committed Dec 12, 2023
1 parent c441bdf commit b185083
Show file tree
Hide file tree
Showing 12 changed files with 56 additions and 26 deletions.
4 changes: 2 additions & 2 deletions docs/api/middleware/basicauth.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions docs/api/middleware/csrf.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion docs/api/middleware/keyauth.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 "`<source>:<name>`" 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

Expand Down
2 changes: 1 addition & 1 deletion docs/api/middleware/requestid.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions middleware/adaptor/adaptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions middleware/basicauth/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions middleware/csrf/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion middleware/csrf/csrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
6 changes: 4 additions & 2 deletions middleware/idempotency/idempotency.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions middleware/keyauth/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
8 changes: 5 additions & 3 deletions middleware/requestid/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ 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
ContextKey string
// Optional. Default: "requestid"
ContextKey interface{}
}

// ConfigDefault is the default config
Expand Down Expand Up @@ -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
Expand Down
31 changes: 28 additions & 3 deletions middleware/requestid/requestid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,45 @@ 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()
})

_, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
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()
})

Expand Down

0 comments on commit b185083

Please sign in to comment.