Fix AJAX login not refreshing CSRF token

[nick-vanpraet]

Q	A
Bug fix? (use the a.b branch)	[ x ]
New feature/enhancement? (use the a.x branch)	[ ]
Deprecations?	[ ]
BC breaks? (use the c.x branch)	[ ]
Automated tests included?	[ x ]
Related user documentation PR URL	mautic/mautic-documentation#...
Related developer documentation PR URL	mautic/developer-documentation#...
Issue(s) addressed	Partly fixes #13527

Description:

The current approach is to set the CSRF token in the HTML, which means that:

1. If you log in using the AJAX embedded login form shown after you are logged out due to inactivity the CSRF token is not updated and you will see an error once you trigger any POSTs
2. Any other tabs also do not receive the updated CSRF token and will see the same error once they trigger a POST.

This PR instead sets the CSRF token in a cookie, and the JS reads the value of that cookie and sets it as a header before making an AJAX POST call. We never read the cookie itself to actually validate any calls server side, as that would defeat the purpose of CSRF protection.

This ensures that the CSRF token available to the JS (in any tab) is always up to date as any request to the server ensures a correct value in the cookie (including a login request).

Important:

• I'm not sure I put the cookie creation in the right spot. Ideally it only sets that cookie for HTML and Ajax requests, not for API calls but I couldn't find a decent spot that met those criteria.
• Tests still need updating.
• I haven't even fully tested every case myself manually.

Haven't found any edge cases so far, but if anyone can think of any please add them. I'm worried about generating a session for, say, leads being tracked. As far as I could find, all those tracking pages are public, but perhaps there is an exception somewhere.

Another option could be to use the INTERACTIVE_LOGIN event, which only fires upon successful login and to check for /s/ there as well which should limit it to only UI logins. I believe it even fires for remember me cookie logins. Unless there is a usecase where the csrf token has been invalidated somehow and needs refreshing but that event doesn't fire, then it would break not until a page refresh but until you log out and in, which seems a worse "worst case" than the current one.

Steps to test this PR:

Several ways to test this.

First, I will explain how to reproduce the error, which can be reproduce on any Mautic instance currently in the wild.

The quickest:

1. Go to any running Mautic
2. Open the site in several tabs
3. Log out in one tab, immediately log back in.
4. Go to one of your other tabs
5. Sort something, or trigger a POST another way, such as saving a form.
6. Receive a csrf error

The real world scenario (unpredictable due to garbage collection chance)

1. Go to any running Mautic
2. Open the site in several tabs
3. Walk away for at least 24 minutes
4. Come back, click a few links until you see an AJAX login form
5. Log in
6. Click around, all works
7. Sort something, or save a form => csrf error
8. Refresh that tab, try again => no csrf error
9. Go to one of your other tabs
10. Try the same, you will still see the csrf error until you also refresh that page.

Forcing garbage collection to force the real world scenario to occur sooner

1. Spin up a Mautic locally (without this PR), install it if needed
2. Add this to the top of index.php:

ini_set('session.gc_maxlifetime', 5); // or however many seconds you want.
session_start();
session_gc(); // forces garbage collection.

3. Open the site in several tabs
4. Wait 5 seconds
5. Click 2 links. You will see the AJAX login form.
6. Log in
7. Click around, all works
8. Sort something, or save a form => csrf error
9. Refresh that tab, try again => no csrf error
10. Go to one of your other tabs
11. Try the same, you will still see the csrf error until you also refresh that page.

To test this PR,open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here) and try any of the scenarios above. They should no longer occur.

[mallezie]

First quick remark. The eventsubcriber should probably better live in https://github.com/mautic/mautic/blob/5.x/app/bundles/CoreBundle/EventListener/RequestSubscriber.php instead of Coresubscriber.

In that place there is already code for isAjaxPost and isSecurePath to leverage to not set for api calls, or add something similar there.

[abhisekmazumdar]

I get the following error over console when trying to edit an email template after applying these changes on my local.

I'm trying to resolve the issue. I'll share the outcome if I'm successful.

[nick-vanpraet]

I pushed a fix, apparently forgot to remove some usages in GrapesJS. Also removed 2 definitions from the GrapesJS Demo folder. Not sure if it relied on those specific values, couldn't find anything at least.

[abhisekmazumdar]

Hi @nick-vanpraet

I followed the issue steps mentioned at #13527 and by following "The quickest" steps I was able to reproduce the issue.

But for the real world steps with Forcing garbage collection enabled in index.php. I never saw the AJAX login form but I see the "csrf error".

Now when I'm testing the PR again, I feel the sorting button is not functioning properly and doesn't work at my end when I click on the header titles.

Before:

After

I'm using ddev at my local for setting up Mautic.

[nick-vanpraet]

@abhisekmazumdar Hmm, did you check the "remember me" when logging in? That may bypass the need to login manually when your session expires, but will still invalidate the csrf token so you'd still see that error without this PR.

[nick-vanpraet]

@abhisekmazumdar as for the seemingly broken JS, any errors in console that you can see or something else? I'll try and reproduce the issue locally as well.

[abhisekmazumdar]

@abhisekmazumdar as for the seemingly broken JS, any errors in console that you can see or something else? I'll try and reproduce the issue locally as well.

Oh, I forgot to mention that. I don't see any console error.

[nick-vanpraet]

gitpod is acting up and doesn't want to start properly, but if I try it locally I don't see any malformed sorted lists. To be fair, it doesn't seem to sort anything at all. even when I just spin up 5.1 without this PR sorting by name or email or anything does not change the order in any way shape or form so it is possible it's broken in some way in 5.1 itself.

Try sorting emails instead, that one still seems to work as it should in 5.1

[nick-vanpraet]

@abhisekmazumdar There is apparently a redesign in 5.x, that isn't in 5.1. I'm assuming your before is on 5.x, this PR is against 5.1 as it is a bugfix. That's why it looks different.

When you sort the list on 5.1, does the sorting work or does nothing happen?

[nick-vanpraet]

@abhisekmazumdar disregard all the things, the 5.1 branch is not an RC it's a leftover from a long time ago. So I definitely made my PR against the wrong branch, I'll rebase and update.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.63%. Comparing base (4883fa0) to head (86c77e7).

Additional details and impacted files

@@            Coverage Diff            @@
##                5.x   #13745   +/-   ##
=========================================
  Coverage     61.62%   61.63%           
- Complexity    34145    34148    +3     
=========================================
  Files          2245     2245           
  Lines        102077   102090   +13     
=========================================
+ Hits          62909    62919   +10     
- Misses        39168    39171    +3     

Files	Coverage Δ
...les/CoreBundle/EventListener/RequestSubscriber.php	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

[abhisekmazumdar]

I can confirm now that I don't see no csrf error and after Log in I was able to save form and do sorting action without any error.

[abhisekmazumdar]

I have conducted functional testing locally and was able to confirm that I do not see any CSRF errors.

I have reviewed the code and do not see any unexpected issues.

[abhisekmazumdar]

Just nitpicking an extra space here.