Skip to content

Commit

Permalink
feat!(middleware/session): re-write session middleware with handler (#…
Browse files Browse the repository at this point in the history 
…3016)

* feat!(middleware/session): re-write session middleware with handler

* test(middleware/session): refactor to IdleTimeout

* fix: lint errors

* test: Save session after setting or deleting raw data in CSRF middleware

* Update middleware/session/middleware.go

Co-authored-by: Renan Bastos <[email protected]>

* fix: mutex and globals order

* feat: Re-Add read lock to session Get method

* feat: Migrate New() to return middleware

* chore: Refactor session middleware to improve session handling

* chore: Private get on store

* chore: Update session middleware to use saveSession instead of save

* chore: Update session middleware to use getSession instead of get

* chore: Remove unused error handler in session middleware config

* chore: Update session middleware to use NewWithStore in CSRF tests

* test: add test

* fix: destroyed session and GHSA-98j2-3j3p-fw2v

* chore: Refactor session_test.go to use newStore() instead of New()

* feat: Improve session middleware test coverage and error handling

This commit improves the session middleware test coverage by adding assertions for the presence of the Set-Cookie header and the token value. It also enhances error handling by checking for the expected number of parts in the Set-Cookie header.

* chore: fix lint issues

* chore: Fix session middleware locking issue and improve error handling

* test: improve middleware test coverage and error handling

* test: Add idle timeout test case to session middleware test

* feat: add GetSession(id string) (*Session, error)

* chore: lint

* docs: Update session middleware docs

* docs: Security Note to examples

* docs: Add recommendation for CSRF protection in session middleware

* chore: markdown lint

* docs: Update session middleware docs

* docs: makrdown lint

* test(middleware/session): Add unit tests for session config.go

* test(middleware/session): Add unit tests for store.go

* test(middleware/session): Add data.go unit tests

* refactor(middleware/session): session tests and add session release test

- Refactor session tests to improve readability and maintainability.
- Add a new test case to ensure proper session release functionality.
- Update session.md

* refactor: session data locking in middleware/session/data.go

* refactor(middleware/session): Add unit test for session middleware store

* test: fix session_test.go and store_test.go unit tests

* refactor(docs): Update session.md with v3 changes to Expiration

* refactor(middleware/session): Improve data pool handling and locking

* chore(middleware/session): TODO for Expiration field in session config

* refactor(middleware/session): Improve session data pool handling and locking

* refactor(middleware/session): Improve session data pool handling and locking

* test(middleware/csrf): add session middleware coverage

* chroe(middleware/session): TODO for unregistered session middleware

* refactor(middleware/session): Update session middleware for v3 changes

* refactor(middleware/session): Update session middleware for v3 changes

* refactor(middleware/session): Update session middleware idle timeout

- Update the default idle timeout for session middleware from 24 hours to 30 minutes.
- Add a note in the session middleware documentation about the importance of the middleware order.

* docws(middleware/session): Add note about IdleTimeout requiring save using legacy approach

* refactor(middleware/session): Update session middleware idle timeout

Update the idle timeout for the session middleware to 30 minutes. This ensures that the session expires after a period of inactivity. The previous value was 24 hours, which is too long for most use cases. This change improves the security and efficiency of the session management.

* docs(middleware/session): Update session middleware idle timeout and configuration

* test(middleware/session): Fix tests for updated panics

* refactor(middleware/session): Update session middleware initialization and saving

* refactor(middleware/session): Remove unnecessary comment about negative IdleTimeout value

* refactor(middleware/session): Update session middleware make NewStore public

* refactor(middleware/session): Update session middleware Set, Get, and Delete methods

Refactor the Set, Get, and Delete methods in the session middleware to use more descriptive parameter names. Instead of using "middlewareContextKey", the methods now use "key" to represent the key of the session value. This improves the readability and clarity of the code.

* feat(middleware/session): AbsoluteTimeout and key any

* fix(middleware/session): locking issues and lint errors

* chore(middleware/session): Regenerate code in data_msgp.go

* refactor(middleware/session): rename GetSessionByID to GetByID

This commit also includes changes to the session_test.go and store_test.go files to add test cases for the new GetByID method.

* docs(middleware/session): AbsoluteTimeout

* refactor(middleware/csrf): Rename Expiration to IdleTimeout

* docs(whats-new): CSRF Rename Expiration to IdleTimeout and remove SessionKey field

* refactor(middleware/session): Rename expirationKeyType to absExpirationKeyType and update related functions

* refactor(middleware/session): rename Test_Session_Save_Absolute to Test_Session_Save_AbsoluteTimeout

* chore(middleware/session): update as per PR comments

* docs(middlware/session): fix indent lint

* fix(middleware/session): Address EfeCtn Comments

* refactor(middleware/session): Move bytesBuffer to it's own pool

* test(middleware/session): add decodeSessionData error coverage

* refactor(middleware/session): Update absolute timeout handling

- Update absolute timeout handling in getSession function
- Set absolute expiration time in getSession function
- Delete expired session in GetByID function

* refactor(session/middleware): fix *Session nil ctx when using Store.GetByID

* refactor(middleware/session): Remove unnecessary line in session_test.go

* fix(middleware/session): *Session lifecycle issues

* docs(middleware/session): Update GetByID method documentation

* docs(middleware/session): Update GetByID method documentation

* docs(middleware/session): markdown lint

* refactor(middleware/session): Simplify error handling in DefaultErrorHandler

* fix( middleware/session/config.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* add ctx releases for the test cases

---------

Co-authored-by: Renan Bastos <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Juan Calderon-Perez <[email protected]>
Co-authored-by: René <[email protected]>
  • Loading branch information
@sixcolors @renanbastos93
@coderabbitai @gaby @ReneWerner87
5 people authored Oct 25, 2024
1 parent 298975a commit e3232c1
Show file tree
Hide file tree
Showing 18 changed files with 2,797 additions and 425 deletions.
15 changes: 6 additions & 9 deletions docs/middleware/csrf.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ app.Use(csrf.New(csrf.Config{
KeyLookup: "header:X-Csrf-Token",
CookieName: "csrf_",
CookieSameSite: "Lax",
Expiration: 1 * time.Hour,
IdleTimeout: 30 * time.Minute,
KeyGenerator: utils.UUIDv4,
Extractor: func(c fiber.Ctx) (string, error) { ... },
}))
Expand Down Expand Up @@ -106,15 +106,14 @@ func (h *Handler) DeleteToken(c fiber.Ctx) error
| CookieSecure | `bool` | Indicates if the CSRF cookie is secure. | false |
| CookieHTTPOnly | `bool` | Indicates if the CSRF cookie is HTTP-only. | false |
| CookieSameSite | `string` | Value of SameSite cookie. | "Lax" |
| CookieSessionOnly | `bool` | Decides whether the cookie should last for only the browser session. Ignores Expiration if set to true. | false |
| Expiration | `time.Duration` | Expiration is the duration before the CSRF token will expire. | 1 * time.Hour |
| CookieSessionOnly | `bool` | Decides whether the cookie should last for only the browser session. (cookie expires on close). | false |
| IdleTimeout | `time.Duration` | IdleTimeout is the duration of inactivity before the CSRF token will expire. | 30 * time.Minute |
| KeyGenerator | `func() string` | KeyGenerator creates a new CSRF token. | utils.UUID |
| 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 |
| SingleUseToken | `bool` | SingleUseToken indicates if the CSRF token be destroyed and a new one generated on each use. (See TokenLifecycle) | false |
| 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 in the session. | "csrfToken" |
| TrustedOrigins | `[]string` | TrustedOrigins is a list of trusted origins for unsafe requests. This supports subdomain matching, so you can use a value like "https://*.example.com" to allow any subdomain of example.com to submit requests. | `[]` |

### Default Config
Expand All @@ -124,11 +123,10 @@ var ConfigDefault = Config{
KeyLookup: "header:" + HeaderName,
CookieName: "csrf_",
CookieSameSite: "Lax",
Expiration: 1 * time.Hour,
IdleTimeout: 30 * time.Minute,
KeyGenerator: utils.UUIDv4,
ErrorHandler: defaultErrorHandler,
Extractor: FromHeader(HeaderName),
SessionKey: "csrfToken",
}
```

Expand All @@ -144,12 +142,11 @@ var ConfigDefault = Config{
CookieSecure: true,
CookieSessionOnly: true,
CookieHTTPOnly: true,
Expiration: 1 * time.Hour,
IdleTimeout: 30 * time.Minute,
KeyGenerator: utils.UUIDv4,
ErrorHandler: defaultErrorHandler,
Extractor: FromHeader(HeaderName),
Session: session.Store,
SessionKey: "csrfToken",
}
```

Expand Down Expand Up @@ -304,7 +301,7 @@ The Referer header is automatically included in requests by all modern browsers,

## Token Lifecycle

Tokens are valid until they expire or until they are deleted. By default, tokens are valid for 1 hour, and each subsequent request extends the expiration by 1 hour. The token only expires if the user doesn't make a request for the duration of the expiration time.
Tokens are valid until they expire or until they are deleted. By default, tokens are valid for 30 minutes, and each subsequent request extends the expiration by the idle timeout. The token only expires if the user doesn't make a request for the duration of the idle timeout.

### Token Reuse

Expand Down
Loading

1 comment on commit e3232c1

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: e3232c1 Previous: 8c84b0f Ratio
Benchmark_Ctx_Send 7.142 ns/op 0 B/op 0 allocs/op 4.335 ns/op 0 B/op 0 allocs/op 1.65
Benchmark_Ctx_Send - ns/op 7.142 ns/op 4.335 ns/op 1.65
Benchmark_Utils_GetOffer/1_parameter 203.7 ns/op 0 B/op 0 allocs/op 131 ns/op 0 B/op 0 allocs/op 1.55
Benchmark_Utils_GetOffer/1_parameter - ns/op 203.7 ns/op 131 ns/op 1.55
Benchmark_Middleware_BasicAuth - B/op 80 B/op 48 B/op 1.67
Benchmark_Middleware_BasicAuth - allocs/op 5 allocs/op 3 allocs/op 1.67
Benchmark_Middleware_BasicAuth_Upper - B/op 80 B/op 48 B/op 1.67
Benchmark_Middleware_BasicAuth_Upper - allocs/op 5 allocs/op 3 allocs/op 1.67
Benchmark_CORS_NewHandler - B/op 16 B/op 0 B/op +∞
Benchmark_CORS_NewHandler - allocs/op 1 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerSingleOrigin - B/op 16 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerSingleOrigin - allocs/op 1 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflight - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflight - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflightSingleOrigin - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflightSingleOrigin - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflightWildcard - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflightWildcard - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_Middleware_CSRF_GenerateToken - B/op 520 B/op 327 B/op 1.59
Benchmark_Middleware_CSRF_GenerateToken - allocs/op 10 allocs/op 6 allocs/op 1.67

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.