feat(service-worker): bypass on queryparam/header (#21191) #30010

petersalomonsen · 2019-04-21T05:44:51Z

issue #21191

added ngsw-bypass header and queryparameter
for bypassing serviceworker on specific requests

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: 21191

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

petersalomonsen · 2019-04-21T07:40:12Z

@gkalpak I believe everything from #21191 is covered now. Let me know if anything else is needed for this Pull Request.

gkalpak

Thx for taking this on, @petersalomonsen. I have left some minor comments, but generally this LGTM.
Also, can you please update the commit message to follow our guidelines more closely. E.g. something like:

feat(service-worker): support bypassing SW with specific header/query param

Add support for bypassing the ServiceWorker for a request by using the ngsw-bypass header or query
parameter.

Fixes #21191

gkalpak · 2019-04-23T13:21:36Z

aio/content/guide/service-worker-devops.md

@@ -92,6 +92,15 @@ to refresh the resource in the background. This way, broken
 unhashed resources do not remain in the cache beyond their
 configured lifetimes.

+### Bypass the service worker


I would move this to the end of the "Service worker and caching of app resources" section.
I, also, think that "Bypassing" sounds more consistent with the other headings.

This is done.

gkalpak · 2019-04-23T13:46:03Z

packages/service-worker/worker/src/driver.ts

@@ -176,6 +176,11 @@ export class Driver implements Debuggable, UpdateSource {
   */
  private onFetch(event: FetchEvent): void {
    const req = event.request;
+
+    if (req.headers.has('ngsw-bypass') || req.url.indexOf('ngsw-bypass=true') > -1) {


I suggest handling both the header and the query param similarly: Either check for presense only or check for a value of 'true'.
One problem with requiring 'true' is that it implies that adifferent value (e.g. 'false') disables the by-passing, which in turn raises the question of which values are valid, how to handle invalid values (if any), whether the header or the query param has precedence, etc. Therefore, I might lean towards checking for presense only (and making it clear in the docs).

The query param check needs to be stricter. E.g. extend adapter.parseUrl() to also return .search and check something like requestUrlObj.search.slice(1).toLowerCase().split('&').includes('ngsw-bypass' /*or ngsw-bypass=true*/).

Thanks for the review @gkalpak , and good feedback. I see your point, but just want to check with you before proceeding with stricter checks.

If acceptable I'd like to keep it as simple as possible (hopefully lower risk of bugs), and as CPU efficient as possible ( less expensive to search for a string than splitting, converting to lowercase, using regex etc ).

So I've modified the docs, so that it's clearer on the criteria for bypassing the SW. Now it says you can have either the request header with any value, or the query parameter with value true. I think adding the value requirement reduces the risks of collision with ngsw-bypass other places in the URL (like in the path, or other parameters with different postfix). Checking for & means that you can't have the parameter directly after ? so I'd prefer not checking for &.

So my point here is that keeping it simple makes it easier for others to understand, and it's also faster code, while making it more complex also could make it more vulnerable and in that case more difficult to debug.

What do you think?

I'm afraid I will have to disagree 😁

Simpler code is great but only if the resulting requirements are simple. I would aim for simpler (and more intuitive) requirements first. I still stand by my earlier suggestion to:

Have consistency between header and query param values.

Correctly detect query param.

Be case insensitive (which again keeps consistency between header and query param).

To address some of your concerns directly:

I'd like to keep it as simple as possible (hopefully lower risk of bugs) [...].

Keeping it too simple would indeed lower the risk of bugs in our code, but increase the risk of bugs in app code.

I'd like to keep it [...] as CPU efficient as possible (less expensive to search for a string than splitting, converting to lowercase, using regex etc).

I wouldn't be so worried about computational complexity, because this is not a hot path and the other operations involved are much more expensive anyway.

point taken :-) I'll try to implement it as you suggest. hopefully tomorrow.

@gkalpak now only checking for presence of the ngsw-bypass query param, and it's case insensitive. I added the search property to the result of adapter.parseUrl() as you suggested.

Hopefully closer now to what you have in mind now, maybe you want some more test cases? I added some more.

gkalpak · 2019-04-23T13:51:09Z

packages/service-worker/worker/test/happy_spec.ts

+
+      expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
+      server.assertSawRequestFor('/bar.txt');
+    });


Depending on how we decide to handle header/query param values, we need more checks to cover for other cases. E.g.:

Lower/Upper/Mixed case.

Falsy value/No value.

Having more headers/query params.

Having similarly named headers/query params (e.g. ?no-ngsw-bypass=true).

Having both the header and the query param.

Thx for adding the extra tests. They look great.

Can you please add a couple more:

Upper/Mixed case headers.

Falsy value/No value (both headers and query param).

I'd like to see one more test with an empty query param (e.g. ?ngsw-bypass=&foo=ngsw-bypass).

gkalpak

Thx, @petersalomonsen. Looking much better already 😉
I left some more suggestions (but we are close) 😇

gkalpak · 2019-04-24T10:47:03Z

aio/content/guide/service-worker-devops.md

+handling the request. An example is when uploading files, where
+you will not see progress events if the request is going through
+the service worker. To bypass the serviceworker you can set `ngsw-bypass`
+as a request header, or as a query parameter with any value.


"with any value" suggests that there must be a value, which is not true 😁

gkalpak · 2019-04-24T10:47:32Z

packages/service-worker/worker/src/driver.ts

@@ -176,9 +176,16 @@ export class Driver implements Debuggable, UpdateSource {
   */
  private onFetch(event: FetchEvent): void {
    const req = event.request;
+


gkalpak · 2019-04-24T10:50:18Z

packages/service-worker/worker/src/driver.ts

    const scopeUrl = this.scope.registration.scope;
    const requestUrlObj = this.adapter.parseUrl(req.url, scopeUrl);

+    if (req.headers.has('ngsw-bypass') ||
+        requestUrlObj.search.slice(1).toLowerCase().split('&').find(


Since most of the time .search will be empty, you might want to short-circuit the more complex operations if .search is falsy.

Since we don't need the query param (just want to test for presence), .some() is preferrable.

gkalpak · 2019-04-24T11:19:26Z

packages/service-worker/worker/src/driver.ts

    const scopeUrl = this.scope.registration.scope;
    const requestUrlObj = this.adapter.parseUrl(req.url, scopeUrl);

+    if (req.headers.has('ngsw-bypass') ||
+        requestUrlObj.search.slice(1).toLowerCase().split('&').find(
+            param => param.split('=')[0] === 'ngsw-bypass')) {


Since you are concerned about performance, it might be faster to avoid splitting each key-value pair (e.g. use param === 'ngsw-bypass' || param.startsWith('ngsw-bypass=')).

Or even using a RegExp (and avoiding all the slicing/lower-casing/splitting/searching in array): /[?&]ngsw-bypass(?:[=&]|$)/i.test(requestUrlObj.search)

UPDATE:
It turns out, avoiding splitting key-value pairs is twice as fast and using a RegExp is 7x faster: Benchmark
(Again, not that all this should have any noticeable impact on the overall operation 😁)

You are doing all the work here @gkalpak :-) I'm learning a lot, so very happy to be part of the process here! Very nice benchmark ( I suspected regexp was faster, but didn't really benchmark as you did ). I copied your RegExp which seems to do the job.

gkalpak · 2019-04-24T11:25:16Z

packages/service-worker/worker/testing/scope.ts

@@ -184,14 +184,15 @@ export class SwTestHarness implements ServiceWorkerGlobalScope, Adapter, Context
    }, new MockHeaders());
  }

-  parseUrl(url: string, relativeTo?: string): {origin: string, path: string} {
+  parseUrl(url: string, relativeTo?: string): {origin: string, path: string, search: string} {
    const parsedUrl: URL = (typeof URL === 'function') ?
        new URL(url, relativeTo) :
        require('url').parse(require('url').resolve(relativeTo || '', url));


AFAICT, Node's url.parse(...) will set .search to null (instead of an empty string).
So, we either need to normalize null to an empty string or type the return value as search: null | string.

I chose to return an empty string instead of changing the return type, since the use of url.parse(...) only applies to older nodejs versions (todays version has URL type).

gkalpak · 2019-04-24T11:32:26Z

packages/service-worker/worker/test/happy_spec.ts

+
+      expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
+      server.assertSawRequestFor('/bar.txt');
+    });


Thx for adding the extra tests. They look great.

Can you please add a couple more:

Upper/Mixed case headers.

Falsy value/No value (both headers and query param).

petersalomonsen · 2019-04-24T15:44:57Z

Also added tests for no value and mixed case headers. For the mixed case headers test, I had to modify the MockHeaders class to store and look up lower case keys. Testing https://developer.mozilla.org/en-US/docs/Web/API/Headers/has seems to be case insensitive, so MockHeaders should now behave more like that - though an iteration on the MockHeaders keys will only show lowercase keys, which might be an issue? (UPDATE: not an issue, this is how it is in the browser too - iterating through header keys gives lower case results).

gkalpak

A couple of minor comments. LGTM otherwise 👍

gkalpak · 2019-04-24T20:23:33Z

packages/service-worker/worker/testing/scope.ts

    const parsedUrl: URL = (typeof URL === 'function') ?
        new URL(url, relativeTo) :
        require('url').parse(require('url').resolve(relativeTo || '', url));

    return {
      origin: parsedUrl.origin || `${parsedUrl.protocol}//${parsedUrl.host}`,
      path: parsedUrl.pathname,
+      search: parsedUrl.search ? parsedUrl.search : ''


Nit: Could be simplified to parsedUrl.search || ''.

Good point. Did that now.

gkalpak · 2019-04-24T20:28:30Z

packages/service-worker/worker/test/happy_spec.ts

+      await makeRequest(scope, '/bar.txt?ngsw-bypaSS=something');
+      server.assertNoRequestFor('/bar.txt');
+
+      await makeRequest(scope, '/bar.txt?testparam=test&ngsw-bypASS=anything');


gkalpak · 2019-04-24T20:31:10Z

packages/service-worker/worker/test/happy_spec.ts

+      await makeRequest(scope, '/bar?testparam=test&ngsw-bypass&testparam2');
+      server.assertNoRequestFor('/bar');
+
+


Nit: Unnecessary newline.

gkalpak · 2019-04-24T20:32:55Z

packages/service-worker/worker/test/happy_spec.ts

+
+      expect(await makeRequest(scope, '/bar.txt?ngsw-bypass=false')).toBe('this is bar');
+      server.assertSawRequestFor('/bar.txt');
+    });


I'd like to see one more test with an empty query param (e.g. ?ngsw-bypass=&foo=ngsw-bypass).

petersalomonsen · 2019-04-24T21:37:12Z

Also added one more test with empty query param as you asked for.

googlebot · 2019-04-25T13:16:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

gkalpak

Thx, @petersalomonsen 👍
I have added a fixup commit with some suggested fixes. LMK if it looks good to you, so I can go ahead and mark this for merging.

For posterity:

I was pondering whether we should also recognize an x--prefixed header, since that seems to be a common requirement for non-standard headers. But the current recommendation is discouraging the x- prefix.

It is easy to address this (in a non-breaking way) later, so let's re-consider, if this turns out to be a problem. Another approach could be to have the SW remove the header or query param altogether, but that would make the requests differ depending on whether the SW is enabled/disabled. so probably not a good idea.

petersalomonsen · 2019-04-25T14:07:51Z

Looks good @gkalpak . Also tested building the serviceworker package and using the resulting ngsw-worker.js and it works as it should.

Not too worried about using a x- prefixed header, I think ngsw- is probably unique enough. Modifying the request and removing headers / query params is not really needed / wanted as I see it, but I often tend to go for the least comprehensive solutions (which is sometimes good, but not always) :-)

gkalpak · 2019-04-25T14:31:04Z

merge-assistance: No idea what is causing the ci/angular: size check failure or how to fix.

IgorMinar

Lgtm. Thanks

googlebot · 2019-04-25T17:52:05Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

gkalpak · 2019-04-25T18:10:37Z

For posterity:

As @alexeagle's suggested, I have compared the core/hello_world/bundle.br artifacts from master and this PR and there are identical.

… param Add support for bypassing the ServiceWorker for a request by using the ngsw-bypass header or query parameter. Fixes angular#21191

…r/query param

googlebot · 2019-04-25T19:51:47Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-04-25T19:52:07Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

…r/query param

googlebot · 2019-04-25T19:55:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-04-25T20:00:53Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

… param (angular#30010) Add support for bypassing the ServiceWorker for a request by using the ngsw-bypass header or query parameter. Fixes angular#21191 PR Close angular#30010

angular-automatic-lock-bot · 2019-09-15T04:23:11Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

petersalomonsen requested a review from a team as a code owner April 21, 2019 05:44

googlebot added the cla: yes label Apr 21, 2019

petersalomonsen mentioned this pull request Apr 21, 2019

feat(service-worker): Support ignoring specific URLs (e.g. for AJAX progress listener, video streaming, range requests, Firebase uploads) #21191

Closed

petersalomonsen force-pushed the 21191-serviceworker-bypass branch 2 times, most recently from 8e339fd to b7bc822 Compare April 21, 2019 07:21

benlesh added the area: service-worker label Apr 22, 2019

ngbot bot added this to the needsTriage milestone Apr 22, 2019

gkalpak requested changes Apr 23, 2019

View reviewed changes

petersalomonsen force-pushed the 21191-serviceworker-bypass branch 2 times, most recently from c326079 to c7b357a Compare April 24, 2019 07:20

gkalpak requested changes Apr 24, 2019

View reviewed changes

petersalomonsen force-pushed the 21191-serviceworker-bypass branch 2 times, most recently from f802ad9 to a33c301 Compare April 24, 2019 15:27

gkalpak approved these changes Apr 24, 2019

View reviewed changes

gkalpak added action: cleanup target: patch target: major feature and removed target: patch labels Apr 24, 2019

petersalomonsen force-pushed the 21191-serviceworker-bypass branch from a33c301 to da87d4a Compare April 24, 2019 21:33

googlebot added cla: no and removed cla: yes labels Apr 25, 2019

gkalpak approved these changes Apr 25, 2019

View reviewed changes

gkalpak added action: merge merge: caretaker note labels Apr 25, 2019

gkalpak removed the action: cleanup label Apr 25, 2019

IgorMinar approved these changes Apr 25, 2019

View reviewed changes

IgorMinar added cla: yes and removed cla: no labels Apr 25, 2019

petersalomonsen and others added 2 commits April 25, 2019 22:51

fixup! feat(service-worker): support bypassing SW with specific heade…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

9f6307d

…r/query param

gkalpak force-pushed the 21191-serviceworker-bypass branch from ac929e8 to 9f6307d Compare April 25, 2019 19:51

googlebot added cla: no and removed cla: yes labels Apr 25, 2019

AndrewKushnir added cla: yes and removed cla: no labels Apr 25, 2019

fixup! feat(service-worker): support bypassing SW with specific heade…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

14994b4

…r/query param

googlebot added cla: no and removed cla: yes labels Apr 25, 2019

AndrewKushnir added cla: yes and removed cla: no labels Apr 25, 2019

AndrewKushnir closed this in 6200732 Apr 25, 2019

petersalomonsen deleted the 21191-serviceworker-bypass branch April 26, 2019 08:40

This was referenced Aug 18, 2019

S3 File Upload fails in Safari Browser (Zero Bytes Length) Using cognito-identity.amazonaws.com aws/aws-sdk-js#2738

Closed

s3 upload - uploads a file with 0 bytes length aws/aws-sdk-js#1713

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

		await makeRequest(scope, '/bar?testparam=test&ngsw-bypass&testparam2');
		server.assertNoRequestFor('/bar');

feat(service-worker): bypass on queryparam/header (#21191) #30010

feat(service-worker): bypass on queryparam/header (#21191) #30010

Conversation

petersalomonsen commented Apr 21, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petersalomonsen commented Apr 21, 2019

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersalomonsen commented Apr 24, 2019 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersalomonsen commented Apr 24, 2019

googlebot commented Apr 25, 2019

gkalpak left a comment

Choose a reason for hiding this comment

petersalomonsen commented Apr 25, 2019 • edited Loading

gkalpak commented Apr 25, 2019

IgorMinar left a comment

Choose a reason for hiding this comment

googlebot commented Apr 25, 2019

gkalpak commented Apr 25, 2019

googlebot commented Apr 25, 2019

googlebot commented Apr 25, 2019

googlebot commented Apr 25, 2019

googlebot commented Apr 25, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

petersalomonsen commented Apr 21, 2019 •

edited

Loading

gkalpak Apr 24, 2019 •

edited

Loading

petersalomonsen commented Apr 24, 2019 •

edited

Loading

petersalomonsen commented Apr 25, 2019 •

edited

Loading