Invalid security token issue - Service worker September 2019 update to spec #4805

summercms · 2019-12-05T03:04:02Z

I previously wrote some code to uninstall a service worker when you try to login to the backend of the cms. It was important to turn off the service worker as it created a caching issue and produced "Invalid security token" messages.

However, there was always a problem with the service worker spec, the old spec does this: If you unregister a service worker, the registration is removed from the list of registrations, but it continues to control existing pages and this continued to cause issues with October using a service worker and trying to login to the backend.

The new update to the spec now fixes this issue! If you unregister a service worker registration it's removed from the list of registrations and stops controlling the existing pages! This means when you now open the backend auth.htm file the service worker will be completely removed and the cached properly cleared now!

Horray! 👍

To admin's if you don't understand this please ask me. This is a really important fix.

bennothommo · 2019-12-05T03:56:38Z

@ayumi-cloud What is the browser support for this feature?

summercms · 2019-12-05T05:12:10Z

@bennothommo

Chrome, Firefox, Safari, Opera, Edge and Samsung Internet right now.

Already been rolled out 4 months ago:
https://bugs.webkit.org/show_bug.cgi?id=201584
https://bugzilla.mozilla.org/show_bug.cgi?id=1557244
https://bugs.chromium.org/p/chromium/issues/detail?id=971571

bennothommo · 2019-12-05T06:06:51Z

@ayumi-cloud Thanks for that. What about the immediate flag you have added? None of the linked bug pages mention that?

daftspunk · 2019-12-05T09:12:17Z

Seems like this overall solution is lacking exception handling for older browsers. Perhaps I'm missing something, what happens if navigator.serviceWorker.getRegistrations() fails because the browser doesn't know what navigator or navigator.serviceWorker is?

summercms · 2019-12-05T13:15:07Z

@bennothommo

What about the immediate flag you have added? None of the linked bug pages mention that?

Yes, the browsers are not mentioning immediate: true directly and instead are using different wordings such as: Clear-Site-Data or CSD - Please read on to fully understand the reason behind this.

@daftspunk

Seems like this overall solution is lacking exception handling for older browsers

Let me give three main examples:

Example 1 - IE11

With IE11 it doesn't support or understand the Service Worker spec. Because of this IE11 won't install any service workers so there is nothing to uninstall. IE11 will still work and won't give a warning message, instead it just ignores the code snippet.

Example 2 - UC Browser

UC Browser for Android does support Service Workers, however it doesn't support the Clear-Site-Data spec. The immediate: true uses the Clear-Site-Data spec to clear the Cache and Service Worker. UC Browser will still work and won't give a warning message, instead it will read the following code line:

registration.unregister({ immediate: true });

as this:

registration.unregister();

The end result will be this:

The registration is removed from the list of registrations, but it continues to control existing pages and this continued to cause issues with October using a service worker and trying to login to the backend.

This is Service Worker version 1 spec.

Example 3 - Chrome, Firefox, Safari, Opera, Edge and Samsung Internet

Let's take Chrome as the example, Chrome fully supports Service Workers and it also fully supports the Site-Clear-Data API. Chrome will read the immediate: true and use the Clear-Site-Data API to remove the Service Worker and clear the cache straight away.

It's like a kill switch and sets the backend login web page as a fresh page ready for a user to add in their credentials.

The end result will be this:

The service worker registration is removed from the list of registrations and stops controlling the existing page(s).

This is Service Worker version 2 spec.

Links

https://www.caniuse.com/#feat=serviceworkers
https://www.caniuse.com/#feat=mdn-http_headers_clear-site-data

p.s. Hope that all makes sense, sorry I wasn't clear before and gave this full explanation.

bennothommo · 2019-12-06T01:11:50Z

@ayumi-cloud

IE11 will still work and won't give a warning message, instead it just ignores the code snippet.

This is incorrect - IE 11 would fail at the point it tried your two code snippets in _auth.htm and _head.htm because navigator.serviceWorker would be undefined. You would need to check for the existence of navigator.serviceWorker first before attempting to use any of its methods.

Regarding the other browsers, the W3C discussion on the kill-switch for service workers was the only reference I've been able to find on this immediate flag, and unless I'm mistaken, they all agreed to use the Clear-Site-Data header as opposed to this flag, so I'm not seeing the need for it?

summercms · 2019-12-06T15:28:19Z

@bennothommo

This is incorrect - IE 11 would fail at the point it tried your two code snippets in _auth.htm and _head.htm because navigator.serviceWorker would be undefined. You would need to check for the existence of navigator.serviceWorker first before attempting to use any of its methods.

Strange as before I wrote my reply I actually asked some people from Google, Firefox and Edge working on Service Workers and adding it to their browsers (so I tend to believe them).

Also when I run a test to confirm their comments - I get the exact results they have described, see below:

Regarding the other browsers, the W3C discussion on the kill-switch for service workers was the only reference I've been able to find on this immediate flag, and unless I'm mistaken, they all agreed to use the Clear-Site-Data header as opposed to this flag, so I'm not seeing the need for it?

That's old and not current, after that github issue, there was a W3C TPAC which clearly says to use immediate flag! The Browser vendors at the W3C TPAC meeting were: Mozilla, Microsoft, Google and WebKit (which is stated in section one).

so I'm not seeing the need for it?

You are totally wrong!

summercms · 2019-12-06T15:50:02Z

I want to raise a side note here:

It's highly frustrating and time wasting, having spent a lot of time researching and asking browser vendor people what is the correct path and then being told to ignore them by the admins here.

Please note Daftspunk said we could create a proposal for October v2 and our company has already started coding it several weeks ago, we are currently re-coding everything and have started on updating various security things. After that we are going to start performance and the next module and so on. If this continues to be highly frustrating and time wasting, we will just not submit our work and hard fork this project to bypass the admins! We don't want to do this because we respect Daftspunk hard work, but there is a real issue here! Hence I thought I mention it.

LukeTowers · 2019-12-06T16:09:26Z

@ayumi-cloud calm down please. No one is ignoring you, we're just trying to get more information about this.

Please keep in mind that even though these issues may seem obvious to you, that's because you already have spent a significant amount of time digging through the details and references to figure out the problem and the solution. We greatly appreciate that effort that you've put in, but for us to be able to merge anything we also have to understand those changes. We just need you to put in some more time passing on the knowledge that you've gained so that we don't have to go around "asking browser vendor people" in order to gain the same understanding as you.

Note that I have also gone through the links you provided and can't find any reference to the "immediate" flag. If the links you've provided are outdated, that's fine; we just need to see the current ones that actually mention that flag being added. Is there a MDN doc page for it?

so I'm not seeing the need for it?

You are totally wrong!

That's fine if we're wrong, but we're saying that because with the current information we don't see a need for it. Instead of just telling us we're wrong, show us how we're wrong; we're more than happy to constantly evolve our opinions as new information is provided.

In summary, you may find this incredibly obvious, but please keep in mind this is because you have already spent a significant amount of time looking into this. You need to be a bit more patient and help us get on the same page with you by providing us with where you're getting the information from and explaining it to us.

bennothommo · 2019-12-06T16:11:17Z

@ayumi-cloud For the sake of continued collaboration, I am going to simply withdraw from interacting with you further. You may deal with @LukeTowers and @daftspunk in getting stuff included.

I hope you will at some point appreciate that my requests for more information are not intended to be hostile, and it simply me asking for more information so I can understand the problem and proposed solution better. While I may come in with incorrect information sometimes, I will admit that I am wrong if suitable evidence is presented to me, like with your IE 11 point above.

I believe @LukeTowers has sufficiently communicated the rest of my points I was intending to make.

w20k · 2019-12-06T18:10:49Z

@ayumi-cloud please refrain from using Edge emulator to test the IE11 compatibility, you should know that Edge doesn't emulate old versions of IE, the way that you could in old versions of IE (like IE11). That is because all of the old rendering engines have been removed. What you are changing just changes the user-agent string sent to servers, and nothing else. Unless you are serving different content based on the user-agents server-side, there should be no difference.

@bennothommo & @daftspunk is 100% correct about IE11 throwing an undefined error.

summercms · 2019-12-06T18:28:51Z

@w20k I used browserling which uses a virtual machine of IE11, see screenshot:

Same thing, please share a screenshot.

p.s. That screenshot had this code running:

october/config/cms.php

Lines 431 to 451 in c1fe12f

    
               /* 
        
               |-------------------------------------------------------------------------- 
        
               | Backend Service Worker 
        
               |-------------------------------------------------------------------------- 
        
               | 
        
               | Allow plugins to run Service Workers in the backend. 
        
               | 
        
               | WARNING: This should always be disabled for security reasons as Service 
        
               | Workers can be hijacked and used to run XSS into the backend. Turning 
        
               | this feature on can create a conflict if you have a frontend Service 
        
               | Worker running. The 'scope' needs to be correctly set and not have a 
        
               | duplicate subfolder structure on the frontend, otherwise it will run 
        
               | on both the frontend and backend of your website. 
        
               | 
        
               | true  - allow service workers to run in the backend 
        
               | 
        
               | false - disallow service workers to run in the backend 
        
               | 
        
               */ 
        
               'enableBackendServiceWorkers' => false,

summercms · 2019-12-06T18:37:01Z

@LukeTowers

Is there a MDN doc page for it?

No, it's not in MDN I have checked. According to the W3C TPAC they implemented the feature and in the processing of writing up the doc's to the w3c spec. When that's done the firefox team will add it to mdn.

Note: This is an over kill because the service worker would not be registered in the first place, so would never be able to get unistalled.

summercms · 2019-12-06T19:09:50Z

Why my last commit is an overkill

If tomorrow I made a new api and added it to the ayumi browser, then I added the api code to a website and loaded it in google chrome browser. Google Chrome would not crash and give errors or stop working! There are safe guards in all browsers to prevent this type of error from happening!

I know this because I asked this question years ago to the google chrome team and that's what they explained to me.

Hence IE11 would ignore the navigator.serviceWorker.getRegistrations() code, because navigator / serviceWorker is not listed in its api list.

Therefore: navigator.serviceWorker.* - is all ignored code.

summercms · 2019-12-06T20:30:20Z

Current status of the browsers adding the api immediate: true is seen below in the screenshot:

As per conversation with Luke I will contact them to get some written information on this api as proof. Also asked Luke to add the "blocked" label to put this pull request on hold until can get some written documentation proof of this new feature.

Info request found here: w3c/ServiceWorker#1489

LukeTowers · 2019-12-06T21:11:30Z

Blocked pending official addition to the spec

modules/backend/layouts/_head.htm

w20k · 2019-12-09T10:30:41Z

modules/backend/layouts/auth.htm

@@ -43,12 +43,12 @@
                /* Only run on HTTPS connections
                * Block off Front-end Service Worker from running in the Backend allowing security injections, see GitHub #4384
                */
-                if (location.protocol === 'https:') {
+                if ((location.protocol === 'https:') && ('serviceWorker' in navigator)) {
                    // Unregister all service workers before signing in to prevent cache issues, see github issue: #3707
                    navigator.serviceWorker.getRegistrations().then(
                        function(registrations) {
                            for (let registration of registrations) {


@ayumi-cloud could you switch to ES5 style. IE11 doesn't support ES6 for ... of loop and let (credits to @tobias-kuendig for pointing out).

w20k · 2019-12-09T10:30:54Z

modules/backend/layouts/_head.htm

@@ -56,12 +56,12 @@
        /* Only run on HTTPS connections
         * Block off Front-end Service Worker from running in the Backend allowing security injections, see GitHub #4384
         */
-        if (location.protocol === 'https:') {
+        if ((location.protocol === 'https:') && ('serviceWorker' in navigator)) {
            // Unregister all service workers before signing in to prevent cache issues, see github issue: #3707
            navigator.serviceWorker.getRegistrations().then(
                function(registrations) {
                    for (let registration of registrations) {


@ayumi-cloud also here.

summercms · 2019-12-16T05:04:53Z

@w20k how about this instead:

<script>
    if(/MSIE \d|Trident.*rv:/.test(navigator.userAgent))
        document.write('<script crossorigin="anonymous" src="https://polyfill.io/v3/polyfill.min.js?features=default%2Ces5%2Ces6"><\/script>');
</script>

Add above the service worker code.

Loads default, es5 and es6 modules - subject to admins choosing which files they want to add.

tobias-kuendig · 2019-12-16T06:06:25Z

I would not use user agent sniffing to detect feature support and introducing a polyfill for this problem seems overkill to me. Why not just change the code to ES5 and support a wider variety of browers? It's really not that big of a change:

if ((location.protocol === 'https:') && ('serviceWorker' in navigator)) {
  navigator.serviceWorker.getRegistrations().then(function(registrations) {
    for (var key in registrations) {
      var registration = registrations[key]
      registration.unregister()
    }
  })
}

w20k · 2019-12-16T09:41:49Z

@ayumi-cloud I totally agree with @tobias-kuendig and example that he provided! Simple and clean.

summercms · 2019-12-16T11:29:44Z

@tobias-kuendig @w20k

Yah ignore my last, I was burning the candle working until 5am when I wrote that comment. Thanks for the code example agree nice and clean.

I was thinking of a different issue last night, I suddenly realised that IE11 is running until 2025 and was thinking in my head, 6 more years of modern browsers bringing out so many more new features and api's and 6 years of IE11 not adding anything and standing still. Just imagine how many more errors and bugs that will create in the years to come! That's why I was thinking maybe at some point it would be best to polyfill IE11 and bridge the gap. But that's another issue and not this one!

Anyway, thanks for your comment and I will add it to a commit in a bit after getting some coffee in 👍

summercms · 2019-12-16T11:45:33Z

Not sure what happened; but my branch is deleted so created a new pr and will close this one ?

New one at: #4831

invalid security token issue

f0848e3

bennothommo added Status: Review Needed Type: Maintenance labels Dec 5, 2019

bennothommo added Status: Response Needed and removed Status: Review Needed labels Dec 5, 2019

test if browser supports service worker or not

52abd1b

Note: This is an over kill because the service worker would not be registered in the first place, so would never be able to get unistalled.

LukeTowers added Status: Blocked and removed Status: Response Needed labels Dec 6, 2019

w20k reviewed Dec 8, 2019

View reviewed changes

modules/backend/layouts/_head.htm Show resolved Hide resolved

w20k reviewed Dec 9, 2019

View reviewed changes

summercms mentioned this pull request Dec 16, 2019

Invalid security token issue - Service worker September 2019 update to spec #4831

Closed

summercms closed this Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalid security token issue - Service worker September 2019 update to spec #4805

Invalid security token issue - Service worker September 2019 update to spec #4805

summercms commented Dec 5, 2019 •

edited

Loading

bennothommo commented Dec 5, 2019

summercms commented Dec 5, 2019

bennothommo commented Dec 5, 2019

daftspunk commented Dec 5, 2019

summercms commented Dec 5, 2019 •

edited

Loading

bennothommo commented Dec 6, 2019

summercms commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 6, 2019

LukeTowers commented Dec 6, 2019

bennothommo commented Dec 6, 2019

w20k commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 6, 2019

summercms commented Dec 6, 2019

summercms commented Dec 6, 2019 •

edited

Loading

LukeTowers commented Dec 6, 2019

w20k Dec 9, 2019

w20k Dec 9, 2019

summercms commented Dec 16, 2019 •

edited

Loading

tobias-kuendig commented Dec 16, 2019

w20k commented Dec 16, 2019

summercms commented Dec 16, 2019

summercms commented Dec 16, 2019

Invalid security token issue - Service worker September 2019 update to spec #4805

Invalid security token issue - Service worker September 2019 update to spec #4805

Conversation

summercms commented Dec 5, 2019 • edited Loading

bennothommo commented Dec 5, 2019

summercms commented Dec 5, 2019

bennothommo commented Dec 5, 2019

daftspunk commented Dec 5, 2019

summercms commented Dec 5, 2019 • edited Loading

Example 1 - IE11

Example 2 - UC Browser

Example 3 - Chrome, Firefox, Safari, Opera, Edge and Samsung Internet

Links

bennothommo commented Dec 6, 2019

summercms commented Dec 6, 2019 • edited Loading

summercms commented Dec 6, 2019

LukeTowers commented Dec 6, 2019

bennothommo commented Dec 6, 2019

w20k commented Dec 6, 2019 • edited Loading

summercms commented Dec 6, 2019 • edited Loading

summercms commented Dec 6, 2019

summercms commented Dec 6, 2019

Why my last commit is an overkill

summercms commented Dec 6, 2019 • edited Loading

LukeTowers commented Dec 6, 2019

w20k Dec 9, 2019

Choose a reason for hiding this comment

w20k Dec 9, 2019

Choose a reason for hiding this comment

summercms commented Dec 16, 2019 • edited Loading

tobias-kuendig commented Dec 16, 2019

w20k commented Dec 16, 2019

summercms commented Dec 16, 2019

summercms commented Dec 16, 2019

summercms commented Dec 5, 2019 •

edited

Loading

summercms commented Dec 5, 2019 •

edited

Loading

summercms commented Dec 6, 2019 •

edited

Loading

w20k commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 6, 2019 •

edited

Loading

summercms commented Dec 16, 2019 •

edited

Loading