Skip to content

Define some way for foreign fetch to decide on opaqueness of responses #841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mkruisselbrink opened this issue Feb 24, 2016 · 23 comments
Closed
Milestone

Comments

@mkruisselbrink
Copy link
Collaborator

A service worker with a foreign fetch handler needs some kind of API to decide if responses it returns should be opaque responses or not when received by whoever initiated the fetch (as currently specified all responses will be opaque, which is rather limiting).

Currently proposed is that by default all responses from a foreign fetch handler should be opaque, and we'll have some extra API to explicitly mark a response as non-opaque (assuming the response isn't already opaque to the service worker itself). The proposed API allows you to associate two bits of information with a response:

  1. the origin to which the response should be exposed (similar to Access-Control-Allow-Origin in CORS, but not allowing wildcards), and
  2. (optionally) which headers of the response should be exposed (similar to Access-Control-Expose-Headers in CORS)

Of course that leaves the question why we don't just require the foreign fetch handler to set these CORS headers on their responses, rather then coming up with our own API. I don't quite remember what the arguments for this were (changing headers requires returning a synthetic response, which might behave differently? slightly more complicated code to write to be able to return a synthetic response? semantics are subtly different from CORS so re-using CORS headers isn't ideal? explicit API is just easier to use?)

But assuming we do indeed want some kind of API to mark a response as being non-opaque for the purpose of foreign fetch, what should that API look like?

As proposed:

partial interface Response {
  static Promise<VisibleResponse> makeVisibleTo(promise<Response>,
      USVString origin, optional ResponseVisibilityOptions options);
};

dictionary ResponseVisibilityOptions {
  sequence<USVString> headers;
};

// TODO: define VisibleResponse

combined with changing respondWith to accept a Promise<Response or VisibleResponse> (for a regular fetch event calling makeVisibleTo on the response passed to respondWith wouldn't change any behavior).

The makeVisibleTo method could of course also be a method on the FetchEvent (and there is also the question if foreign fetch should keep using the same FetchEvent interface or have its own ForeignFetchEvent interface that might or might not derive from FetchEvent).

Another option might be to change respondWith to be able to be called asynchronously (as long as waitUntil promises haven't all resolved yet) and just add the new visibility options as an extra argument to respondWith.

/cc @slightlyoff @annevk

@mkruisselbrink mkruisselbrink added this to the Version 2 milestone Feb 24, 2016
@jakearchibald
Copy link
Contributor

semantics are subtly different from CORS so re-using CORS headers isn't ideal?

If we reuse CORS headers, then passing the response back without thinking could reveal more info than intended.

Response.makeVisibleTo(…)

Feels weird to put a method on Response that only really applies to foreign fetch. I prefer foreignFetchEvent.makeVisibleTo().

Another possibility is to make foreignFetchEvent.respondWith receive (a promise for) an object, so the usage would be:

self.addEventListener('foreignfetch', event => {
  event.respondWith(
    fetch(url).then(response => ({
      response,
      visibility: {
        origin: '…',
        headers: []
      }
    }))
  );
});

You lose the opaque default with this method though. Not sure if that's a feature or a bug.

there is also the question if foreign fetch should keep using the same FetchEvent interface or have its own ForeignFetchEvent

I think it must, as we'd want to expose details about the receiving origin. From the explainer, event.request.origin is always going to match location.origin, so using that as the origin you want things to be visible is always going to result in opaque.

Another option might be to change respondWith to be able to be called asynchronously (as long as waitUntil promises haven't all resolved yet)

This gets tricky when it comes to multiple listeners. There'd either be a race to respondWith, or waitUntil would need to change so it prevents the next event listener being called. I don't think either is particularly nice.

@annevk
Copy link
Member

annevk commented Feb 25, 2016

Wait, why would event.request.origin match location.origin? I don't think it should.

We also need to think through if this creates a new kind of filtered response and what returning such a filter response elsewhere would do. E.g., with nested foreign fetch (I tend to think we need to make it such that each layer needs to use an API to reveal it to the next).

Reusing CORS directly doesn't work, since we want CORS responses to become opaque responses by default.

@jakearchibald
Copy link
Contributor

Wait, why would event.request.origin match location.origin? I don't think it should.

You're right, I was mistaking request.origin for the origin of request.url. I guess request.origin is a new thing? Feels like it should sit on the ForeignFetchEvent rather than the request, since it isn't something you'd want to persist in a cache.

@jakearchibald
Copy link
Contributor

I tend to think we need to make it such that each layer needs to use an API to reveal it to the next

Agreed. I don't think this creates a new kind of response. The headers are filtered in the same way they are now via CORS, or the request becomes opaque like it does with no-cors.

The return of foreignFetchEvent.makeVisibleTo() should just be a wrapper around the response. foreignFetchEvent.respondWith() unwraps it unless there's a security error.

@jakearchibald
Copy link
Contributor

Another argument for makeVisibleTo to sit on foreignFetchEvent: it can indicate the visibility of the response after it returns/resolves. Response.makeVisibleTo cannot, as it doesn't know anything about the destination.

Not convinced makeVisibleTo should take a promise either. It could be synchronous right?

@wanderview
Copy link
Member

Bikeshedding: Can we name it reveal() instead of makeVisible()?

@annevk
Copy link
Member

annevk commented Feb 26, 2016

respondWith() cannot simply unwrap. It needs to be a new filtered response of some kind. Otherwise it would reveal all of the response whereas we want to limit exposure to certain headers and such (and we also don't want to expose more headers than were exposed thus far, might be quite tricky to get that right).

@jakearchibald
Copy link
Contributor

I mean the return of foreignFetchEvent.makeVisibleTo() is a wrapper around the new response. I'm trying to avoid creating yet another response type, especially one that's only useful between calling foreignFetchEvent.makeVisibleTo() and foreignFetchEvent.respondWith() checking it.

@annevk
Copy link
Member

annevk commented Feb 26, 2016

@jakearchibald we need a wrapper after respondWith(). I would imagine makeVisibleTo() to simply put some annotations on the response that Fetch can use.

@mkruisselbrink
Copy link
Collaborator Author

Hmm, good point about needing a new filtered response type. If fully opaque/fully visible was all we cared about opaque filtered vs not filtered is all that we would need. But with selectively exposed headers thrown in the mix some new response type is probably needed. It might also be an option to extend the definition of a CORS filtered response to take these annotations into account, but that might be weird.

And if we do add a new filtered response type, should that also include a new ResponseType value, which will be visible in the Response object? Or should this from the point of view of whoever initiated the fetch still be a "cors" response? I don't really mind exposing the fact that a foreign fetch handler was involved, but I think it still would be nice if these foreign fetch exposed responses would still just be regular "cors" responses from the point of view of the fetcher.

Not convinced makeVisibleTo should take a promise either. It could be synchronous right?

I think the only benefit of makeVisibleTo take a promise is that (at least in theory) browser could detect patterns like respondWith(fetch(...)) and respondWith(makeVisible(fetch)) and not have to keep the worker alive while the fetch is in progress. But I'm not even sure if such an optimization really makes sense.

So still trying to decide whether foreignfetch should reuse FetchEvent, or if it should have its own ForeignFetchEvent class. Benefits of using the same event class could be that it makes it easier to use the same event handler for fetch and foreign fetch events (whether that is something that should be encouraged is a separate question of course). Are there different things we'd want to expose to foreign fetch handlers as opposed to regular fetch handlers? The requests's origin has been mentioned, but that's already part of the request (even though it's not exposed to javascript at the moment). As sort of a substitute the request does expose the referrer, which seems like it would provide most of the same information (unless it's set to no-referrer).

What about having makeVisibleTo be a non-static member of a Response? If makeVisibleTo really just sets some annotations on the response (which I think makes a lot of sense) there isn't really any reason for it to consume/return a response. It can just modify the response it is called on and set its "foreign fetch exposed headers" and "foreign fetch exposed origin" (not sure about those names). Also makeVisibleTo should presumably throw if the response it is called on is already an opaque response. (and should these new annotations on the response be part of what is stored in a cache? And I guess we'd need to decide what should happen if makeVisibleTo is called on a response that already has these annotations set).

The entire codepath related to regular fetches would just ignore these extra annotations on the response, but when handling a foreign fetch event, some code (either in Handle Foreign Fetch in SW, or in HTTP Fetch where that algorithm will be called) would:

  • check if any of these annotations are set. If not, return an opaque filtered response (like currently specced)
  • otherwise verify that the origin the response was annotated with matches the request (sort of like CORS check done in the http fetch algorithm today)
  • and then return a "foreign fetch filtered response" (or maybe a different name?) which "is a filtered response whose type is "cors", header list excludes any headers in internal response's header list whose name is not a foreign-fetch-safelisted response-header name, given internal response's foreign fetch exposed headers."

@annevk
Copy link
Member

annevk commented Feb 27, 2016

My new preferred plan is something like this:

  • Instead of makeVisibleTo, respondWith() on ForeignFetchEvent is somewhat different and passed a promise for a dictionary, consisting of {response: ..., origin: ..., headers: ...}.
  • You always need to pass a dictionary, even when you're not passing origin/headers.
  • If you pass headers without origin it'll result in a network error.
  • Fetch uses this metadata to construct a network error, opaque response, or foreign-fetch filtered response.
  • Foreign-fetch filtered responses's will need some kind of header safelist stored on them (or maybe on the internal response).
  • I agree that foreign-fetch filtered responses should look just like CORS would.

Optionally, ForeignFetchEvent also has a validateRepondWith() with method, that takes a dictionary (not a promise for one) and tells you whether it'll result in a network error, opaque response, or foreign-fetch filtered response. We should only add that after we're absolutely sure that's feasible though. So after all the code paths are worked out and we have the complete setup.

@annevk
Copy link
Member

annevk commented Feb 27, 2016

The other thing we need to think through and define is that the response member of the dictionary the promise is resolved with that is passed to respondWith() can contain any kind of response. So when you pass in an opaque response, we cannot make that visible. If you pass in a CORS response, we can only safelist the headers already safelisted by the CORS response. If you pass in a foreign-fetch filtered response, you get the idea.

I don't think we want to make filtered responses multiple layers deep since that would complicate things elsewhere. Though it might be worth trying to think through that angle too.

@delapuente
Copy link

IMHO, visibility rules should be the same as those governing in the fetch from network case, i.e. CORS headers. I really don't get the point of not using CORS.

If someone could clarify what does this means:

Reusing CORS directly doesn't work, since we want CORS responses to become opaque responses by default.

I think responses for foreign fetch should not be different from already existent responses. For the client doing the request, it should not be distinguishable if the response comes from a sw handling a foreign fetch or from the network.

@annevk
Copy link
Member

annevk commented Feb 28, 2016

It won't be distinguishable from the point of view of fetch(), but "using CORS" doesn't work due to the ambient authority of the service worker. We need to know, from the service worker, that this was actually meant to be shared.

@delapuente
Copy link

But you could force CORS responses to be at least as protective as the network response. So if you receive an opaque response you could not make it more visible at all and if you receive a partial opaque response (say, exposing just some headers) you could reduce this set but never extending it.

This way:

self.onforeignfetch = evt => {
    evt.respondWith(self.fetch(evt.request));
};

Acts the same way as if it were no service worker.

@annevk
Copy link
Member

annevk commented Feb 29, 2016

I'm not sure what you're saying.

@delapuente
Copy link

Sorry, then I don't understand what you mean with the 'ambient authority' of the service workers and the need to know that the response was meant to be shared. Anyway, if the fetch() can only see the result as CORS headers, that works for me.

+1 to validateRespondWith() to know in advance if the response will fail.

@annevk
Copy link
Member

annevk commented Feb 29, 2016

The foreign fetch service worker is always fetched with credentials. CORS requests are either with or without credentials. But even for CORS requests without credentials, we'll use the foreign fetch service worker (fetched with credentials). Therefore we want to be absolutely sure that it wants to share a response.

Furthermore, if the foreign fetch service worker itself fetched something cross-origin and wants to share that, this will make that easy where "just use CORS" requires repackaging in a synthetic response.

Having an explicit programmatic security protocol should help with all that. Repackaging responses to include the correct headers is not a great API or the way to go.

@delapuente
Copy link

Got it. Thank you, I think we are aligned here. When talking about CORS should be enough, I was implicitly saying without creating a new response so we require some kind of API to tweak the visibility options.

@mkruisselbrink
Copy link
Collaborator Author

I started trying to write a spec for this (ForeignFetchEvent and the updated Handle Foreign Fetch algorithm). Overall I don't think it's too bad, but getting all the corner cases right and correctly dealing with all the various ways existing filtered responses should be treated is definitely tricky. Also at some parts of this should move to the fetch spec, but for now it seemed easier to just keep it all together in one place.

You always need to pass a dictionary, even when you're not passing origin/headers.

I'm not sure I see the benefit of this? If we allow passing just a Response as respondWith({response: response}), why wouldn't we also allow respondWith(response)? It might not be the common case, but supporting it really doesn't complicate anything.

Foreign-fetch filtered responses's will need some kind of header safelist stored on them (or maybe on the internal response).

Currently I've kind of hand-wavy stored this list on the filtered response, not the internal response. Storing it on the internal response seems like it could be potentially problematic (our foreign fetch handler somehow got hold of a response that already is a foreign fetch filtered response. It then tries to respond with that response and some shorter header list. If we just blindly update the header safelist on the internal response that would also change the headers visible to the javascript code in the foreign fetch handler, unless we clone the internal response first).

I do wonder if having a separate "foreign fetch filtered response" and a "cors filtered response" is really the best approach here though. It might be cleaner to somehow update the definition of "CORS filtered response" to make it usable for this, since after all both filtered response types do nothing but setting the type to "cors", and filtering the header list using some whitelist.

I don't think we want to make filtered responses multiple layers deep since that would complicate things elsewhere. Though it might be worth trying to think through that angle too.

At first I thought that having multiple layers of filtered responses might actually make sense, since it would make sure we never accidentally expose something another filter tried to hide, but I now agree that that indeed wouldn't really simplify anything. Foreign fetch code would still need to be careful to correctly deal with all the various filtered responses the original response can be (a foreign fetch filtered opaque response should just be an opaque response, etc).

@annevk
Copy link
Member

annevk commented Mar 17, 2016

I'm not sure I see the benefit of this?

Distinguishing a Response object from an ordinary object is somewhat ugly and not possible unless you branch on internal slots, which are not available to normal JavaScript. I'm not sure we should do that for the first iteration. Also, it's not the common case, so it's weird to optimize for that.

I think it's okay to update the "cors filtered response" to account for a different kind of safelist. I'm not entirely sure I'm comfortable storing state on filters though. There might be rewrapping happening, especially with multiple levels of nesting, and that would then somehow need to account for that state, which seems very fragile. Agreed that it's problematic with regards to what's observable. I guess we'll need to be clearer on how a response actually moves between environments. As far as I can tell that needs to be some kind of cloning (with potential user agent optimizations under the hood, of course).

@mkruisselbrink
Copy link
Collaborator Author

Distinguishing a Response object from an ordinary object is somewhat ugly and not possible unless you branch on internal slots, which are not available to normal JavaScript. I'm not sure we should do that for the first iteration. Also, it's not the common case, so it's weird to optimize for that.

Ah, okay. That's unfortunate but makes sense. I'll change it that way.

I think it's okay to update the "cors filtered response" to account for a different kind of safelist. I'm not entirely sure I'm comfortable storing state on filters though. There might be rewrapping happening, especially with multiple levels of nesting, and that would then somehow need to account for that state, which seems very fragile. Agreed that it's problematic with regards to what's observable. I guess we'll need to be clearer on how a response actually moves between environments. As far as I can tell that needs to be some kind of cloning (with potential user agent optimizations under the hood, of course).

Agreed that storing state on filters is rather icky. But it seems that the conclusion of #850 might be that we'll be cloning the internal response anyway (at least in spec language), so modifying the internal response to set the list of exposed headers shouldn't be a problem then.

@mkruisselbrink
Copy link
Collaborator Author

This landed as whatwg/fetch@32411c7 and d2fdb14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants