Fix(🩹): error handling in CSRF token storage retrieval #3021
Conversation
Resolved TODO for error handling to ensure code quality in retrieving CSRF token data from storage.
renanbastos93
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes enhance error handling in the CSRF middleware by introducing new error variables and updating function signatures to return errors. This involves modifications in Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Middleware
participant Storage
Client->>Middleware: Request with CSRF token
Middleware->>Storage: getRaw(key)
Storage-->>Middleware: raw, err
alt err is not nil
Middleware-->>Client: Error Response (ErrNotGetStorage)
else
Middleware->>Middleware: Validate Token
alt Token Invalid
Middleware-->>Client: Error Response (ErrTokenInvalid)
else
Middleware-->>Client: Success Response
end
end
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3021 +/- ##
==========================================
- Coverage 83.03% 83.01% -0.02%
==========================================
Files 115 115
Lines 8323 8339 +16
==========================================
+ Hits 6911 6923 +12
- Misses 1079 1087 +8
+ Partials 333 329 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
middleware/csrf/csrf.go (2)
Line range hint
225-225: Ensure to handle the error returned bystorageManager.setRawto prevent potential issues.- storageManager.setRaw(token, dummyValue, cfg.Expiration) + if err := storageManager.setRaw(token, dummyValue, cfg.Expiration); err != nil { + return cfg.ErrorHandler(c, err) + }
Line range hint
233-233: Ensure to handle the error returned bystorageManager.delRawto maintain consistent error handling.- storageManager.delRaw(token) + if err := storageManager.delRaw(token); err != nil { + return cfg.ErrorHandler(c, err) + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- middleware/csrf/csrf.go (4 hunks)
- middleware/csrf/session_manager.go (1 hunks)
- middleware/csrf/storage_manager.go (2 hunks)
Additional context used
GitHub Check: lint
middleware/csrf/session_manager.go
[failure] 29-29:
named return "rawToken" with type "[]byte" found (nonamedreturns)
[failure] 29-29:
named return "err" with type "error" found (nonamedreturns)middleware/csrf/storage_manager.go
[failure] 44-44:
named return "raw" with type "[]byte" found (nonamedreturns)middleware/csrf/csrf.go
[failure] 110-110:
Error return value is not checked (errcheck)
[failure] 213-213:
named return "err" with type "error" found (nonamedreturns)
[failure] 213-213:
named return "raw" with type "[]byte" found (nonamedreturns)
[failure] 225-225:
unhandled-error: Unhandled error in call to function csrf.storageManager.setRaw (revive)
[failure] 225-225:
Error return value ofstorageManager.setRawis not checked (errcheck)
[failure] 233-233:
unhandled-error: Unhandled error in call to function csrf.storageManager.delRaw (revive)
[failure] 233-233:
Error return value ofstorageManager.delRawis not checked (errcheck)
Additional comments not posted (1)
middleware/csrf/storage_manager.go (1)
62-73: The error handling insetRawanddelRawmethods enhances the robustness of the CSRF middleware.Also applies to: 77-87
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
middleware/csrf/csrf.go (3)
24-24: Clarify the comment forerrOriginNotFound.The comment "internal error, will not be returned to the user" could be expanded to explain why this error is internal and under what circumstances it might be used internally.
Line range hint
225-225: Handle the error returned bystorageManager.setRaw.The error returned by
storageManager.setRawis not checked. This could lead to silent failures. Ensure that errors are handled appropriately:- storageManager.setRaw(token, dummyValue, cfg.Expiration) + if err := storageManager.setRaw(token, dummyValue, cfg.Expiration); err != nil { + return cfg.ErrorHandler(c, err) + }
Line range hint
233-233: Check the error returned bystorageManager.delRaw.The error from
storageManager.delRawis not handled, which might result in unreported failures:- storageManager.delRaw(token) + if err := storageManager.delRaw(token); err != nil { + return cfg.ErrorHandler(c, err) + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/csrf/csrf.go (4 hunks)
Additional context used
GitHub Check: lint
middleware/csrf/csrf.go
[failure] 213-213:
named return "raw" with type "[]byte" found (nonamedreturns)
[failure] 213-213:
named return "err" with type "error" found (nonamedreturns)
[failure] 225-225:
unhandled-error: Unhandled error in call to function csrf.storageManager.setRaw (revive)
[failure] 225-225:
Error return value ofstorageManager.setRawis not checked (errcheck)
[failure] 233-233:
unhandled-error: Unhandled error in call to function csrf.storageManager.delRaw (revive)
[failure] 233-233:
Error return value ofstorageManager.delRawis not checked (errcheck)
Additional comments not posted (1)
middleware/csrf/csrf.go (1)
213-213: Remove named return values to enhance code clarity.This issue was previously flagged and remains valid. Consider simplifying the function signature by removing named return values:
- func getRawFromStorage(c fiber.Ctx, token string, cfg Config, sessionManager *sessionManager, storageManager *storageManager) (raw []byte, err error) { + func getRawFromStorage(c fiber.Ctx, token string, cfg Config, sessionManager *sessionManager, storageManager *storageManager) ([]byte, error) {
middleware/csrf/csrf.go
Outdated
| raw, err := getRawFromStorage(c, cookieToken, cfg, sessionManager, storageManager) | ||
| if raw != nil && err == nil { |
There was a problem hiding this comment.
Handle potential errors from getRawFromStorage properly.
The error from getRawFromStorage is checked, but the handling could be more robust. Consider adding specific error handling for different types of errors that could be returned:
raw, err := getRawFromStorage(c, cookieToken, cfg, sessionManager, storageManager)
if err != nil {
+ if errors.Is(err, ErrNotGetStorage) {
+ // Handle specific error
+ }
return cfg.ErrorHandler(c, err)
}Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
@coderabbitai it doesn't make sense, can you explain why I need to do that?
There was a problem hiding this comment.
@renanbastos93 The suggestion to handle specific errors from getRawFromStorage more robustly is aimed at improving the middleware's ability to respond appropriately to different failure scenarios. Currently, the error handling in the switch case for GET, HEAD, OPTIONS, and TRACE methods does not differentiate between the types of errors that could be returned from getRawFromStorage. This function could return errors like ErrNotGetStorage, which specifically indicates issues in retrieving data from storage.
By handling such specific errors distinctly, you can tailor the middleware's response to better suit the nature of the error. For example, if ErrNotGetStorage is encountered, it might be beneficial to log this as a critical error or perform a specific action, because it indicates a failure in the storage backend, which is critical for CSRF protection. This makes the error handling more granular and can help in debugging and maintaining the system more effectively.
Let me know if this clarifies the suggestion or if there's anything else you'd like to discuss!
There was a problem hiding this comment.
@coderabbitai I got it I am waiting for the team comment for that too.
There was a problem hiding this comment.
@renanbastos93, understood! It's great to hear that you're considering the suggestion and discussing it further with your team. If you need any more information or further clarification as you move forward, feel free to reach out. I'm here to help!
|
Why isnt this fix in the rewrite PR #3016 ? It's a 1 line change @sixcolors |
@gaby Since he opened an issue to address error handling, I tried to help him with just that part to save him time so that he could focus on the other changes. |
|
@gaby sorry, Did I make a mistake? |
|
@renanbastos93 Might need to update the tests, the coverage dropped. Existing tests are passing though. |
|
@gaby and @sixcolors of course I will do it. Thanks |
|
@renanbastos93 Any updates on this? |
Hey @gaby , I am so sorry you were waiting for it. I've been sick for the last two weeks and need to finish the unit tests. I will focus on them this week. Do we have any other details to adjust? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: f5933a6 | Previous: 85a5fb8 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels/Zstd_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
BenchmarkAppendMsgstorageManager |
0.6301 ns/op 1586.98 MB/s 0 B/op 0 allocs/op |
0.3186 ns/op 3139.20 MB/s 0 B/op 0 allocs/op |
1.98 |
BenchmarkAppendMsgstorageManager - ns/op |
0.6301 ns/op |
0.3186 ns/op |
1.98 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@renanbastos93 can you refresh the code with the last master |
Of course, I've been in a rush around here and haven't been able to give it the attention it deserves. I'm thinking about closing the PR and opening a new one, what do you think? |
|
ok is also fine for me |
Description
Resolved TODO for error handling to ensure code quality in retrieving CSRF token data from storage.
Fixes #3020
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md