Test for opaque redirect handling

[annevk]

As per whatwg/fetch#633 (comment) except using global state instead of the cache API. (Improvements welcome!)

[annevk]

@jakearchibald @wanderview I'd appreciate your help with this. This is the first service worker code/"test" I've written...

I've run it in Firefox and it doesn't do what I discussed in whatwg/fetch#633 as the problematic scenario.

It resolves the Location header when it gets the response the second time, leading it to a different URL than the server intended.

I don't think you can violate the same-origin policy with this and we only ever handle these type of responses for navigation requests, which will be same-origin with the service worker, but it still seems a little sketchy and might allow a clever XSS to exfiltrate redirect data?

[wanderview]

@annevk You probably need to wait for the service worker to activate with waitForState(). You should also add cleanup handlers to unregister the service worker and remove the frame.

BTW, now that we can use async functions some of this can be cleaner to write.

[jakearchibald]

+1 to async functions & wait_for_state. I do cleanup stuff as part of setup since we don't have async cleanup handlers yet (@wanderview or has that changed?).

https://github.com/w3c/web-platform-tests/pull/10348/files#diff-2098c79116c31c91424e3965506554a3R41 - here's the pattern I use.

[jakearchibald]

I also tend to put files that aren't defining tests into a resources directory. That seems to be the convention elsewhere. Also, it means the tests sit outside the service worker scope. I don't know if it's a problem if that isn't the case, but it feels safer.

[jakearchibald]

LGTM aside from the nits mentions, and wait_for_state.

[jakearchibald]

I think the above 4 lines could be:

evilGlobalState = fetch(e.request);
e.respondWith(new Response("stage 1 completed\n"));

[annevk]

Wouldn't that increase the chance evilGlobalState is null when the second fetch comes in?

[jakearchibald]

Yeah, but if that's your intent then use waitUntil

event.waitUntil(new Promise(r => self.globalStateCollected = r));

And call globalStateCollected() once evilGlobalState is no longer needed.

[wanderview]

I do cleanup stuff as part of setup since we don't have async cleanup handlers yet (@wanderview or has that changed?).

That's true, but if you use a unique scope it doesn't matter in practice. (Async cleanup would be great, though.)

[annevk]

Should be (mostly) good now! Thanks again for the help. As I noted on IRC, Safari passes this. I'm not sure how to run this in Edge via BrowserStack yet.

[jakearchibald]

I think the service worker should go in resources too right?

[jakearchibald]

Other than the above, LGTM.

[annevk]

I fixed that issue. What remains is that we need to decide how we want to handle redirects more generally as per the linked Fetch issue and then also add tests for those other scenarios.

[annevk]

Hmm, should opaque-redirect.window.js have .https in the filename or is that implicit for the service-workers directory?