Confirmed Use statechange listener in PWA app

[digitalpoint]

Have an event listener that fires when the app is loaded so you can refresh CSRF token and/or fetch counters (for conversations and alerts).

 

[Chris D]

Actually going to move this to bugs because I believe given enough time being inactive from the app, the CSRF token will expire and won’t refresh making alerts menu and others fail as it still has the old token.

 

[digitalpoint]

It's definitely not the most ideal thing to do, but I have a guest page caching option in my Cloudflare addon (caches guest pages at the network edge), and I had to work around what was fundamentally the same issue (fetching a CSRF token when you don't have a valid CSRF token to start with). It's a little ugly, but effectively disabled the need for a pre-existing valid CSRF token if you are trying to get a CSRF token.

class Login extends XFCP_Login
{
	public function assertValidCsrfToken($token = null, $validityPeriod = null)
	{
		if (\XF::options()->cfPageCachingSeconds && \XF::app()->router()->routeToController(\XF::app()->request()->getRoutePath())->getAction() === 'keep-alive')
		{
			return;
		}
		parent::assertValidCsrfToken($token, $validityPeriod);
	}
}

 

[digitalpoint]

If you didn't want to make the CSRF token universally able to be updated without an existing one, you could also look at the XF.config.csrf property. I believe that has the time the csrf token is good from, so you could infer when it expires. If it's within the validity period, do a simple JavaScript call to get a valid CSRF token to use. And as a bonus, that same call automatically updates the counters:

XF.KeepAlive.refresh();

If it's outside the valid time period (I think it's after 24 hours), you could force the page to reload (although that might end up with some weirdness if the page they are on was a result of a POST:

location.reload();

Personally, I think the solution in the previous post is going to be the cleanest/most universal. The login/keep-alive route is POST only, and never writes data, it's only used to fetch a CSRF token via the POST request. So simply removing the requirement to have a CSRF token in order to get a CSRF token doesn't seem terrible to me. Then you just add an event listener for statechange and fire XF.KeepAlive.refresh(); when the state is changed to active. Then you aren't doing different things depending on when the csrf token was generated and dealing with trying to reload a page that was the result of a POST.

 

[Chris D]

Interestingly, XF.KeepAlive.refresh() does not update the counters.

It does the cross tab communication stuff, which keeps the counters and other info in-sync across multiple browser tabs, but it doesn't update the counters.

Part of the reason for that is because we're using the native jQuery $.ajax() call rather than XF.ajax(). The latter of which fires some additional events.

That might be intentional.

So maybe we should leave that as it is.

But I'm not sure there's a valid reason we can't just:

if (data.visitor)
{
    XF.updateVisitorCounts(data.visitor, true);
}

Which does update the alert/inbox badges, and the app badge update.

So, then, could we just do:

document.addEventListener("visibilitychange", () => {
  if (document.visibilityState === "visible") {
    XF.keepAlive.refresh();
  }
});

The above probably only needs to happen specifically for the PWA (which we gate behind a display-mode: standalone check).

It seems pretty simple, unless I'm overlooking something.

The aforementioned CSRF issue I haven't actually experienced since my phone stopped being whacky. Doesn't XF.KeepAlive.refresh() handle an expired token already?

 

[digitalpoint]

The aforementioned CSRF issue I haven't actually experienced since my phone stopped being whacky. Doesn't XF.KeepAlive.refresh() handle an expired token already?

I don't think so... when I was dealing with it in my Cloudflare addon for guest page caching, it really just makes a request to whatever is set in XF.config.url.keepAlive, which is login/keep-alive. There is a bypass within the checkCsrfIfNeeded method, but if you go digging, the actionKeepAlive method calls the assertValidCsrfToken method if there is an existing csrf cookie (like an old or expired one maybe).

TL;DR
There is a bypass at the controller dispatch level, however the problem is that when actionKeepAlive is called, it's calling assertValidCsrfToken if there's a csrf cookie in the request, which makes it all fail if the user had an old csrf token.

Maybe just makes sense to remove the check for a valid csrf token for keep alive? Because it works fine if there's NO csrf token... so why do we care if there's an invalid/expired one? Could see it maybe being a security issue if it was a GET request, but actionKeepAlive has to be a POST. So not sure why it would care about expired tokens when it doesn't care about missing tokens.

 

[digitalpoint]

Another thing to consider would be to adopt usage of Sec-Fetch-Site HTTP header as an alternative to CSRF. Could still fallback to CSRF token if the Sec-Fetch-Site header doesn't exist (old browsers). Sec-Fetch-Site wasn't practical before because Safari didn't support it, but Safari added support in 16.4 (at this point the only browser that doesn't support it is IE... which isn't even a browser anymore). Might not be a bad time to explore using Sec-Fetch-Site within the CSRF checking code (with the existing cookie/CSRF token system as the fallback if it's not there).

That's a fairly simple change to shim that into XenForo core, and a byproduct is that you don't have to screw around with XF.KeepAlive.refresh() for PWA activation (probably would still want to do it just to get counters), but there wouldn't be an issue with making underlying POST/AJAX requests with stale CSRF tokens. This would also solve problems in other areas of XenForo (for example XenForo's native guest page caching, as well as issues with updating CSRF tokens embedded in GET requests). That would also allow CSRFed GET requests to still work (like the Logout link). See:

Logout bug when Guest caching is enabled

The issue described here https://xenforo.com/community/threads/guest-page-caching.164816/ has been existing for a long time with no resolution .. I tried doing some troubleshooting to see the reason behind it because it was really annoying , I found the bug is very simple at method...

xenforo.com

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

Ran into this for something else, but it's also the same reason for this bug report: https://xenforo.com/community/threads/logout-bug-when-guest-caching-is-enabled.200977/ XF.KeepAlive.refresh() updates XF.config.csrf and hidden input fields containing csrf, but it does not update URLs with...

xenforo.com

Compatibility for CSRF protection & Cloudflare full HTML page caching

I'm curious if there's any better way for CSRF protection that would work with Cloudflare or other CDN's guest full HTML page caching which uses cookies to differentiate between logged in/logged out guest users? The issue that arises with Xenforo 2.x in CSRF and full page HTML caching is...

xenforo.com

Duplicate - Add an option to disable CSRF checks for modern browsers

XenForo uses a double submit cookie approach to prevent CSRF attacks which does work quite well (except for cases where it breaks). As CSRF tokens are included in the HTML this does complicate things in some cases, for example when caching HTML for guests. It would be nice if the requirement...

xenforo.com

headers HTTP header: Sec-Fetch-Site | Can I use... Support tables for HTML5, CSS3, etc

caniuse.com

The underlying need for CSRF is on the way out anyway...

 

[Moshe1010]

Is this issue related to the fact that when the app is open, and there is a new notification coming in, when clicking on i, it opens the app but nothing happens (it doesn’t redirect to the notified message/PM/whatever) since it doesn’t refresh the page?

 

[digitalpoint]

More or less, yes. It's basically as if your browser window sat idle for whatever amount of time you didn't use the site... eventually the CSRF token goes stale.

 

[Chris D]

Is this issue related to the fact that when the app is open, and there is a new notification coming in, when clicking on i, it opens the app but nothing happens (it doesn’t redirect to the notified message/PM/whatever) since it doesn’t refresh the page?

Actually I don’t know if this is a separate issue.

This I think is just iOS failing to do what it’s supposed to. The code in the service_worker that sets the click handler for the notification is pretty basic. Not a lot to go wrong. So it feels like iOS is going wrong here.

Whether or not other changes we might make might make a difference I don’t know at this point.

 

[Moshe1010]

Actually I don’t know if this is a separate issue.

Is there a way to trigger a page refresh automatically every time the app is loaded? There is a page refresh anyway in between pages, when navigating the app, so maybe triggering a "hard" page refresh every time somebody is clicking on a notification/loading the app could solve this for now?

 

[digitalpoint]

I’ve been running into an issue on this site where following a push notification link to the PWA app and you end up with an invalid CSRF token even if the page was reloaded (for example a reply to a thread… I know the page loaded because I see the reply in the app and it’s from 2 minutes ago). But AJAX doesn’t work (security error happens due to invalid CSRF). In the case I just ran across, I got the security error when replying, not thinking, I reloaded the page and my reply was lost because without a valid CSRF it also couldn’t save it as a draft. Not sure what’s going on internally to cause this (maybe the PWA app is losing the csrf cookie and it takes 2 reloads to get a valid one for some reason)?

Another reason to consider using CSRF as a fallback for old browsers, and move to Sec-Fetch-Site (see a few posts back in this thread).