CSRF token not always updated with XF.KeepAlive.refresh()

digitalpoint

Well-known member
Affected version
2.2.12
Ran into this for something else, but it's also the same reason for this bug report:


XF.KeepAlive.refresh() updates XF.config.csrf and hidden input fields containing csrf, but it does not update URLs with t={csrf_token}. Things like Logout button, the advanced cookie consent buttons, language selector, style selector and a few other things.

XF.KeepAlive is an anonymous function so there wasn't a real way to extend it to fix it for now. However I did come up with an extremely hacky way to do it (using JavaScript Proxy() to monitor for XF.config.csrf changes).

Either way, if XenForo is supposed to update csrf with XF.KeepAlive.refresh(), it should do it in all the places, not just some.
 
Hmm .. updating tokens in links could be problematic, it would have to take care to not update "wrong" links (like links that did not have a valid token initially which seems complicated as the client cannot validate old tokens) - otherwise this could create a vulnerability and / or break links.


IMHO GET requests should not change state and thus should not require tokens to begin with.

So instead of updating tokens on links a different approach might be better:
  • Get rid of the t parameter
  • Add a click handler like data-xf-click="submit-as-post" that turns those links into POST when clicked
  • Add a fallback confirmation if the link is accessed as GET like is is already being done for changing style:
    April Style
 
Last edited:
Similar to one of @Kirby's ideas, I've been thinking the simplest way to handle it (without fundamental changes) is to have a click handler that simply adds the token to the URL when the URL is clicked. Something like, data-xf-click="csrf", which then simply adds t={XF.config.csrf}. Even better would be to use the same parameter that is used for AJAX requests, _xfToken={XF.config.csrf}, for sake of consistency.

So instead of hard-coding the t parameter in HTML source, give those links a click handler instead that handles it. Afterall, XF.config.csrf is available via JavaScript (and then you also don't need to actually do anything else to handle changing CSRF tokens via XF.KeepAlive).
 
I've been thinking the simplest way to handle it (without fundamental changes) is to have a click handler that simply adds the token to the URL when the URL is clicked. Something like, data-xf-click="csrf", which then simply adds t={XF.config.csrf}
Hmm, not sure if I like that approach

  • I does not degrade nicely if JavaScript is not available
    While it is somewhat debatable if requiring JavaScript for "basic" things is acceptable or not, I think if it isn't too difficult to provide a fallback it should be done
  • It does not work if the user chooses to open the link in a new tab or window
    This does work with standard XenForo
    IMHO this is a more serious limitation than requiring JavaScript
  • It still seems conceptionally wrong to me - GET should not modify state
 
Hmm, not sure if I like that approach

  • I does not degrade nicely if JavaScript is not available
    While it is somewhat debatable if requiring JavaScript for "basic" things is acceptable or not, I think if it isn't too difficult to provide a fallback it should be done
Yep, although this guy made a good point.

Like it or not, expecting JavaScript to be enabled is reasonable in 2020.

  • It does not work if the user chooses to open the link in a new tab or window
    This does work with standard XenForo
    IMHO this is a more serious limitation than requiring JavaScript
Yep, agreed.

  • It still seems conceptionally wrong to me - GET should not modify state
Also agree, just trying to think about working around without fundamental changes, that's all.
 
The best solution imo is simply to convert the few places that use CSRF in URLs/GET requests to POST requests. No need for JavaScript trickery, no need for one-off handling of tokens passing in as t variable in some controllers and it also fixes the underlying issue of this bug report.
 
Just ran into this again for something else. Why exactly are we mutating state with GET requests? These really should be POST requests, not GET requests, imo:
  • Logout
  • Language chooser
  • Style chooser
  • Advanced cookie consent
  • Profile post moderation
If we stop using GET requests and instead use POST requests, the underlying issue of the CSRF token not being updated when XF.KeepAlive.refresh() is called fixes itself (no longer need to do kludgey CSRF token insertion into URLs).
 
Back
Top Bottom