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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(🩹): error handling in CSRF token storage retrieval #3021

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions middleware/csrf/csrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"time"

"github.com/gofiber/fiber/v3"
"github.com/gofiber/fiber/v3/log"
)

var (
Expand All @@ -18,8 +19,10 @@
ErrRefererNoMatch = errors.New("referer does not match host and is not a trusted origin")
ErrOriginInvalid = errors.New("origin invalid")
ErrOriginNoMatch = errors.New("origin does not match host and is not a trusted origin")
errOriginNotFound = errors.New("origin not supplied or is null") // internal error, will not be returned to the user
dummyValue = []byte{'+'}
ErrNotGetStorage = errors.New("unable to retrieve data from CSRF storage")
renanbastos93 marked this conversation as resolved.
Show resolved Hide resolved

errOriginNotFound = errors.New("origin not supplied or is null") // internal error, will not be returned to the user
dummyValue = []byte{'+'}
)

// Handler for CSRF middleware
Expand Down Expand Up @@ -102,10 +105,9 @@
switch c.Method() {
case fiber.MethodGet, fiber.MethodHead, fiber.MethodOptions, fiber.MethodTrace:
cookieToken := c.Cookies(cfg.CookieName)

if cookieToken != "" {
raw := getRawFromStorage(c, cookieToken, cfg, sessionManager, storageManager)

// In this case, handling error doesn't make sense because we have validations after the switch.
raw, _ := getRawFromStorage(c, cookieToken, cfg, sessionManager, storageManager) //nolint:errcheck

Check failure on line 110 in middleware/csrf/csrf.go

View workflow job for this annotation

GitHub Actions / lint

directive `//nolint:errcheck` should provide explanation such as `//nolint:errcheck // this is why` (nolintlint)
if raw != nil {
token = cookieToken // Token is valid, safe to set it
}
Expand Down Expand Up @@ -148,9 +150,10 @@
return cfg.ErrorHandler(c, ErrTokenInvalid)
}

raw := getRawFromStorage(c, extractedToken, cfg, sessionManager, storageManager)
raw, err := getRawFromStorage(c, extractedToken, cfg, sessionManager, storageManager)
if err != nil || raw == nil {
renanbastos93 marked this conversation as resolved.
Show resolved Hide resolved
log.Error("Failed to retrieve CSRF token: ", err)

if raw == nil {
// If token is not in storage, expire the cookie
expireCSRFCookie(c, cfg)
// and return an error
Expand Down Expand Up @@ -209,7 +212,7 @@

// getRawFromStorage returns the raw value from the storage for the given token
// returns nil if the token does not exist, is expired or is invalid
func getRawFromStorage(c fiber.Ctx, token string, cfg Config, sessionManager *sessionManager, storageManager *storageManager) []byte {
func getRawFromStorage(c fiber.Ctx, token string, cfg Config, sessionManager *sessionManager, storageManager *storageManager) ([]byte, error) {
if cfg.Session != nil {
return sessionManager.getRaw(c, token, dummyValue)
}
Expand Down
18 changes: 10 additions & 8 deletions middleware/csrf/session_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,22 @@
}

// get token from session
func (m *sessionManager) getRaw(c fiber.Ctx, key string, raw []byte) []byte {
func (m *sessionManager) getRaw(c fiber.Ctx, key string, raw []byte) ([]byte, error) {
sess, err := m.session.Get(c)
if err != nil {
return nil
return nil, err

Check warning on line 32 in middleware/csrf/session_manager.go

View check run for this annotation

Codecov / codecov/patch

middleware/csrf/session_manager.go#L32

Added line #L32 was not covered by tests
}

token, ok := sess.Get(m.key).(Token)
if ok {
if token.Expiration.Before(time.Now()) || key != token.Key || !compareTokens(raw, token.Raw) {
return nil
}
return token.Raw
if !ok {
return nil, ErrTokenInvalid
}

if token.Expiration.Before(time.Now()) || key != token.Key || !compareTokens(raw, token.Raw) {
return nil, ErrTokenInvalid
}

return nil
return token.Raw, nil
}

// set token in session
Expand Down
35 changes: 28 additions & 7 deletions middleware/csrf/storage_manager.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package csrf

import (
"fmt"
"sync"
"time"

"github.com/gofiber/fiber/v3"
"github.com/gofiber/fiber/v3/internal/memory"
"github.com/gofiber/fiber/v3/log"
"github.com/gofiber/utils/v2"
)

Expand Down Expand Up @@ -40,20 +42,35 @@
}

// get raw data from storage or memory
func (m *storageManager) getRaw(key string) []byte {
var raw []byte
func (m *storageManager) getRaw(key string) ([]byte, error) {
var (
raw []byte
err error
)
if m.storage != nil {
raw, _ = m.storage.Get(key) //nolint:errcheck // TODO: Do not ignore error
raw, err = m.storage.Get(key)
if err != nil {
return nil, fmt.Errorf("%w: %s", ErrNotGetStorage, err.Error())

Check warning on line 53 in middleware/csrf/storage_manager.go

View check run for this annotation

Codecov / codecov/patch

middleware/csrf/storage_manager.go#L51-L53

Added lines #L51 - L53 were not covered by tests
}
} else {
raw, _ = m.memory.Get(key).([]byte) //nolint:errcheck // TODO: Do not ignore error
var ok bool
raw, ok = m.memory.Get(key).([]byte)
if !ok {
return nil, ErrNotGetStorage
}
}
return raw

return raw, nil
}

// set data to storage or memory
func (m *storageManager) setRaw(key string, raw []byte, exp time.Duration) {
if m.storage != nil {
_ = m.storage.Set(key, raw, exp) //nolint:errcheck // TODO: Do not ignore error
err := m.storage.Set(key, raw, exp)
if err != nil {
log.Warnf("csrf: failed to save session in storage: %s", err.Error())
return

Check warning on line 72 in middleware/csrf/storage_manager.go

View check run for this annotation

Codecov / codecov/patch

middleware/csrf/storage_manager.go#L69-L72

Added lines #L69 - L72 were not covered by tests
}
} else {
// the key is crucial in crsf and sometimes a reference to another value which can be reused later(pool/unsafe values concept), so a copy is made here
m.memory.Set(utils.CopyString(key), raw, exp)
Expand All @@ -63,7 +80,11 @@
// delete data from storage or memory
func (m *storageManager) delRaw(key string) {
if m.storage != nil {
_ = m.storage.Delete(key) //nolint:errcheck // TODO: Do not ignore error
err := m.storage.Delete(key)
if err != nil {
log.Warnf("csrf: failed to delete session in storage: %s", err.Error())
return

Check warning on line 86 in middleware/csrf/storage_manager.go

View check run for this annotation

Codecov / codecov/patch

middleware/csrf/storage_manager.go#L83-L86

Added lines #L83 - L86 were not covered by tests
}
} else {
m.memory.Delete(key)
}
Expand Down
Loading