Alternate method to pass CSRF token #4701
Conversation
| // Retrieve CSRF cookie | ||
| var exists = new RegExp("(?:^|;\\s*)" + encodeURIComponent('csrfToken').replace(/[\-\.\+\*]/g, "\\$&") + "\\s*\\=") | ||
| if (exists.test(document.cookie)) { | ||
| csrfToken = decodeURIComponent(document.cookie.replace(new RegExp("(?:(?:^|.*;)\\s*" + encodeURIComponent('csrfToken').replace(/[\-\.\+\*]/g, "\\$&") + "\\s*\\=\\s*([^;]*).*$)|^.*$"), "$1")) || null |
There was a problem hiding this comment.
What's the complex regex doing?
There was a problem hiding this comment.
@LukeTowers that checks and extracts the cookie in pure JS. The code is based on this MDN document: https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie/Simple_document.cookie_framework
There was a problem hiding this comment.
Does jQuery have something built in that's less complicated?
There was a problem hiding this comment.
It does - jquery.cookie - which the Backend uses. However, I went this route as it was possible people are already including that in their themes, and I didn't want to introduce another hard dependency that may conflict with theme assets.
There was a problem hiding this comment.
With regards to jquery.cookie it is very old and out of date and has many XSS issues, you can see their release notes for more details. Here is one such example: js-cookie/js-cookie#400
October uses: jQuery Cookie Plugin v1.4.1
Current version is v3.0.0
I will create a pr update for this now., see here: #4710
|
@nathan-van-der-werf can you provide some more detailed feedback on this PR? |
|
@bennothommo @LukeTowers Thanks for keeping me in the loop. My only feedback after having a brief look at your code is to ask a simple question: Any reason why you don't make all the cookies set to: HttpOnly - ture By default (especially as the cookie you are creating contains sensitive data), code lines: 1608 - 1611. Also to allow people to set the timeout amount in one of the config files code line: 1605. (it's set at 60 in your code right now). |
|
@ayumihamasaki2019 A CSRF token is not considered sensitive data - indeed, a lot of implementations have the token shown in plain text in a meta tag or a hidden input field - because it's generally a "nonce" value, it's used once and then discarded. Because of this, I opted to make the cookie available to JS (not HTTP-only) and not require a secure connection, because the prevention of CSRF is more important. As for other cookies, we can certainly take a look at implementing some of those measures to them in the future. |
Why not use the settings from the config? HttpOnly, Secure and SameSite are all configurable by the user right? |
|
@nathan-van-der-werf the point of this cookie is to be used by the JS framework so at the very least we wouldn't be able to use HttpOnly. What's your feedback to this approach in general though? |
|
@bennothommo Thanks for answer, sorry I was just looking at it like a CSP nonce value. If a hacker could change the X-XSRF-TOKEN Cookie value to whatever they wanted then surely they could bypass the whole thing. With CSP nonces at least we can generate a random string in the php side. I'm probably wrong in my thinking, I just see an Unsecure X-XSRF-TOKEN Cookie easier to hack over X-CSRF-TOKEN being created by php. |
|
@nathan-van-der-werf Only "secure" and "sameSite" are configurable through @ayumihamasaki2019 The token is generated on the PHP side and then added to a cookie which is readable by JavaScript. This whole setup is purely a different mechanism to make the token available to the JavaScript where it needs to be read in order to send it with any AJAX requests - one that doesn't require any manual intervention from the theme developer or site developer. |
Why reinvent the wheel? https://laravel.com/docs/5.5/csrf#csrf-x-csrf-token This solution works (also the solution we chose in our plugin), why make a cookie and add extra JS while it's not needed? |
|
@nathan-van-der-werf because it has to work out of the box when using the js framework, without adding anything in the head. Because it broke for existing users, and most of them would not know they have to add something in the head. |
There was never proper CSRF for AJAX requests... |
|
no there was not, but i am sure that most of the users would get the error of invalid token because of that and they won't know how to deal with it. |
|
@nathan-van-der-werf If so, I propose that cookies are more safe than a meta tag, given that cookies have (at the very least) some methods in place to stop people from simply grabbing cookie data cross-domain, whereas a meta tag in the middle of the source code can easily be read by an AJAX request from anywhere. |
|
@nathan-van-der-werf while the CSRF protection in the frontend never had full coverage until now, implementing the full coverage without a way to provide the CSRF token to the frontend for those that haven't added it to their head already (which is going to be the vast majority of the ~30,000 active sites running OctoberCMS right now) is a massive breaking change that is just not doable. |
|
Even if a cookie was a good option whereas it is NOT. The given pull request doesn't work. The expire time of the cookie isn't the same as the session (and not configurable...) (timezone issue probably). The demo theme AJAX calculator doesn't work anymore out of the box (as far as I've tested). The csrfToken is also passed twice in the request? With a cookie and with an additional header (requests getting larger and larger?)? Also, why would we want to send the cookie along with EVERY REQUEST (another reason this is such a bad idea); The naming of "csrfToken" is not in the same style as the default session name "october_session". Also, if backwards compatibility is an issue, why don't make the use of AJAX CSRF configurable (feature flags)? (also this is something semver could fix cough) |
|
@nathan-van-der-werf You do raise good points about the configuration aspect of this, however, I'm still not convinced that a Same-Site cookie is a bad solution. Do you have any alternatives methods that you may be able to suggest, in the spirit of getting this next stable build released? Preferably, the method should require zero intervention on behalf of the theme developer or the site developer/administrator. |
|
@bennothommo From my point of view i like to see first implemented meta csrf tag support and people can implement when you release update because its not breaking change and maybe add in js core warning that they dont have added meta tag in head when they use data-request? |
|
Closed after discussion with the team. |
|
@bennothommo so what's the final fix? |
|
Try this f542ca8 |
|
Fixed by #4720. @filipac @nathan-van-der-werf @Samuell1 |
@nathan-van-der-werf I agreed with you until I saw this approach that is taken straight from the Laravel playbook. There are complex reasons why it works but I assure you it works |
It might work, but it's the worst approach. You got a link for me to the Laravel playbook? I can only find this link: https://laravel.com/docs/5.8/csrf#csrf-x-csrf-token |
|
@nathan-van-der-werf its under that docs you posted https://laravel.com/docs/5.8/csrf#csrf-x-xsrf-token |
Doesn't have anything to do with being a "good practice" 🧐, they added it for those frameworks... |
|
I don't see how it's not good practice, our front-end framework uses AJAX queries, which necessitates an XSRF token check when CSRF checks are enabled, in order to ensure that forged AJAX queries from outside the system don't come through - same issue that those frameworks would've encountered. For normal CSRF checks, the |
|
@nathan-van-der-werf what specifically do you have against having the cookie as another option to validate AJAX requests? |
My biggest point against it is that you're sending the cookie with every request while not doing anything with the cookie. The token is stored somewhere where it doesn't belong. I'm not against the security part of it, it's just as secure as putting the token in the meta tag, although I kinda think it's ugly having non-httponly cookies and httponly cookies mixed (but that's a personal preference). |
|
If having the cookie exist at all is such a big deal, we could always have a config value (default on) to determine whether to send the cookie or not, but it's not really that big of an issue at least from my perspective. |
+1 for config option, default true is fine. |
…ether or not the XSRF cookie is automatically sent or if CSRF tokens are solely relied on. Related: #4701 (comment) & laravel/framework#24726
|
@nathan-van-der-werf done: 959b85f |
…ether or not the XSRF cookie is automatically sent or if CSRF tokens are solely relied on. Related: octobercms/october#4701 (comment) & laravel/framework#24726
…ether or not the XSRF cookie is automatically sent or if CSRF tokens are solely relied on. Related: octobercms/october#4701 (comment) & laravel/framework#24726
Fixes #4699. Library PR: octobercms/library#435
Adds another workflow for sending CSRF tokens, specifically in the case of pure JS AJAX requests from the CMS pages where the user has CSRF protection enabled but has not sent through a token. This PR adds a cookie-based CSRF token that is read in through the JS framework and added as a header in all AJAX requests.
As per the original CSRF token verification, if a token is provided through a
_tokeninput value, this will take precedence, allowing the CSRF cookie token to be overriden on a per-request basis.