csrf: sanitize token after reading from cookie #6969
Conversation
There was a problem hiding this comment.
Thanks for the PR!
After a closer look, I found a better way of handling it, see fc15ac4
After sanitization:
Let me know what you think!
It is true that the general sanitization isn't as clean as the regex one in terms of stripping, but since what characters are used to generate a CSRF token isn't in the control of the application ("Gogs"), I think it would be more future-proof as long as no XSS is possible.
Codecov Report
@@ Coverage Diff @@
## main #6969 +/- ##
=====================================
Coverage 8.89% 8.89%
=====================================
Files 99 99
Lines 13262 13262
=====================================
Hits 1179 1179
Misses 11917 11917
Partials 166 166 |
|
Looking at your screen shot, it seems the markup.Sanitize is not stripping all brackets (or sanitizing them) ...
should not be part of the variable name, no angle brackets should break through. In addition, it looks like you would permit other markup that should not be allowed for the _csrf variable looking at NewSanitizer() in internal/markup/sanitizer.go. I'd have to vote no on this proposal for compliance. |
|
Looking at the source for go-macaron csrf, only alphanumeric characters are allowed for the token, if you can create a new Sanitizer that strips all but a-z, A-Z, 0-9 that would be better. |
|
OK, makes sense! Pushed another version in bc85458 |
|
Looks good! |
Co-authored-by: Joe Chen <jc@unknwon.io>
Co-authored-by: Joe Chen <jc@unknwon.io>
Prevent XSS from _csrf cookie by sanitizing user cookie
To prevent a Cross-Site Scripting (XSS) vulnerability, we should sanitize the user-supplied cookie header. Strip all but a-z, A-Z and 0-9 from the _csrf cookie.
Link to the issue
Checklist