Foreign fetch

[mkruisselbrink]

Sorry for the delay, here is my foreign fetch pull request for issue #684 again, with some of the changes @jungkees made to it already applied on top.
This updates the spec to add support for foreign fetch, and adds a separate foreign fetch explainer document.

[mkruisselbrink]

Any opinions on this? Since we require foreign fetch scopes to be "inside" the scope of the service worker it superficially seems to maybe make sense to parse relative urls relative to the scope. But parsing relative urls relative to the location of the sw script also seems like it would make sense, and might be less surprising.

[jungkees]

+1 to using settings object's API base URL. Having a consistency with other APIs seems better for devs so they don't have to look up a document for this usage.

[bsittler]

I think SW-relative is much more sensible and makes it easier to ensure registrations match for foreign and same-origin interceptions for the same URL in cases where the logic will in fact be identical.

[wanderview]

FYI, I intend to review this before TPAC, but likely won't get to it for a couple weeks.

[mkruisselbrink]

I updated the spec to add a (poorly spec-ed) 'withCredentials' option to foreign fetch (sort of as briefly discussed at TPAC). I also added a separate foreign fetch explainer document, since that seemed potentially helpful.

[mkruisselbrink]

@wanderview or @annevk (or anybody else): any comments on this? Things that I should change/improve etc? The currently suggested spec is definitely not perfect, but I'm not entirely sure how to better spec the credentials part (and if what it is trying to say is even the correct behavior, or if there is more we could/should do minimize the risks of badly written service workers exposing data they didn't intend to expose).

[annevk]

1. I think it's a very bad idea to reuse the fetch event. I don't think we should do that.
2. We shouldn't use withCredentials since that is XMLHttpRequest terminology.
3. If the request is without credentials, this also needs to affect the used service worker in some way. Otherwise there's still ambient authority leakage.

[annevk]

@ehsan you probably want to look at the links in #751 (comment) to review foreign fetch.

[mkruisselbrink]

I think it's a very bad idea to reuse the fetch event. I don't think we should do that.

Okay, changed it to use a separate foreignfetch event (although it was my (apparently mistaken) understanding from our conversation at TPAC that using the same event would probable be okay). I don't have a strong opinion on same vs. different event, and most people seem to have at least the initial reaction that this should be a separate event, so making it a separate event is probably the way to go.

We shouldn't use withCredentials since that is XMLHttpRequest terminology.

Agreed; not sure what the best name is. Things like "dontOmitCredentials" would be even worse (with the double negation etc), but I guess naming this also somewhat depends on what exactly it is going to do.

If the request is without credentials, this also needs to affect the used service worker in some way. Otherwise there's still ambient authority leakage.

Agreed here as well, although unfortunately I'm not quite familiar enough with all the details/intricacies around the fetch spec, cors and authority leakage to really come up with what exactly the desired effect here is going to be. Hopefully something we can discuss at the SW F2F.

[annevk]

So:

1. The service worker itself is always fetched with credentials included.
2. Any foreign fetch from A to B may or may not have credentials included.
   1. Whether credentials are included is exposed on the Request class, though in a somewhat confusing way in the context of foreign fetch. E.g., what does "same-origin" mean?
   2. Although A makes the foreign fetch, only B can turn this into a network fetch. This means that to B's server, it looks like a same-origin fetch.

Perhaps the answer is that B cannot give basic responses to A. Only responses that are synthetic (generated by a service worker that knows this was a foreign fetch due to the new event), CORS responses that approve of A in some manner, or responses that are turned into CORS responses through some new kind of API. This should protect B's service worker and server quite a bit.

[mkruisselbrink]

I updated the explainer with what I think were the resolutions from the F2F. Also updated the spec to get rid of the withCredentials flag, and instead have a list of origins when registering for foreign fetch.
I didn't specify any kind of makeVisible method yet, but did change it to now turn all responses in opaque responses.

[mkruisselbrink]

Question: Should the valid values for the origin parameter be '*', ['*'], 'https://some.origin/', ['https://some.origin', 'https://other.origin'] or should ['*'] not be in that list? And more in general I'm not sure how to best deal with a string-or-sequence-of-strings paremeter in spec algorithms. I guess adding something like "if origins is a string, set origins to a list containing origins` might do it, but is probably not the best/clearest phrasing.

[annevk]

1. Make registerForeignFetchScopes() accept a dictionary. That'll be better going forward.
2. Both subScopes and origins should just take a sequence I think. No need to get fancy. Wrapping the string in [ and ] is not hard.

[mkruisselbrink]

Make registerForeignFetchScopes() accept a dictionary. That'll be better going forward.

Not sure I agree with this. Sure, every method in every API could just have a single dictionary argument with possibly required fields in the dictionary, but that is not how most APIs are written. Many APIs do have an optional dictionary parameter with optional extra arguments, where I fully agree that a dictionary is the way to go, I just don't think that here with an API where the arguments are in no way interchangeable (no subscope should be a valid origin, and no origin would be a valid subscope), and are always required would really be better if it used a dictionary.

Both subScopes and origins should just take a sequence I think. No need to get fancy. Wrapping the string in [ and ] is not hard.

Okay, made that change.

[annevk]

When you already have two arguments and will likely add more in the future it seems better to just have a dictionary from the start since the eventual code will become more readable.

[mkruisselbrink]

When you already have two arguments and will likely add more in the future it seems better to just have a dictionary from the start since the eventual code will become more readable.

Okay, changed it to a dictionary.

[jungkees]

• Handle Foreign Fetch step 11.1 (in html file):

s/<var>originMatches to true.</var>/<var>originMatches</var> to true./

• It seems better to place Handle Foreign Fetch right after Handle Fetch and Match Service Worker for Foreign Fetch right after Match Service Worker Registration.

[mkruisselbrink]

Handle Foreign Fetch step 11.1 (in html file):
s/originMatches to true./originMatches to true./

Done

It seems better to place Handle Foreign Fetch right after Handle Fetch and Match Service Worker for Foreign Fetch right after Match Service Worker Registration.

Done

[mkruisselbrink]

Unless somebody has strong objections against it, I think I'd like to go ahead and merge this change (and iterate/fix things as they come up afterwards). So, any objections?

[jungkees]

merge this change (and iterate/fix things

+1

[annevk]

So, makeVisible is not defined yet, there are no issues filed against Fetch for it, what's the story there? How do we want to approach that and who is going to maintain the security boundary?

[mkruisselbrink]

So, makeVisible is not defined yet, there are no issues filed against Fetch for it, what's the story there? How do we want to approach that and who is going to maintain the security boundary?

Yes, that's correct. Currently foreign fetch is specced to always return responses as opaque filtered responses, which of course isn't very useful, so this is definitely something that still needs to be addressed.

I think there are still some open questions/concerns around the exact shape of some kind of makeVisible API, so my plan was to file a separate (service worker) issue for that after landing this change (it seemed odd to file issues for features not technically part of the spec yet), to at least make sure it's well documented how/why we ended up with whatever makeVisible style API we'll end up with.

[annevk]

I recommend filing all deferred issues now and have them link here before landing. That way it's much more clear what is going on (and you could have avoided my question).

[mkruisselbrink]

Okay, filed #841 for the makeVisible part and whatwg/fetch#223 to add a step to fetch to call handle foreign fetch.