Skip to content
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

Remove opaqueredirect response type in fetch #8351

Closed
lucacasonato opened this issue Nov 11, 2020 · 7 comments · Fixed by #8353
Closed

Remove opaqueredirect response type in fetch #8351

lucacasonato opened this issue Nov 11, 2020 · 7 comments · Fixed by #8353
Labels
ext/fetch related to the ext/fetch

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Nov 11, 2020

Currently fetching with the option redirect: "manual" results in a opaqueredirect response when a redirect is encountered. This means that in Deno (a server side runtime), it is not possible to manually follow a HTTP redirect... 🤯

We should fundamentally rethink how we are implementing the fetch API. The reason opaqueredirect exists, is to prevent leaking information that is accessible because of the origin cookie jar. We do not have an origin cookie jar, so we do not need to protect against leaks it causes. We thus do not need opaqueredirect. We should do what Cloudflare Workers does for redirect: "manual", which is to just respond with the actual response (headers, status code, and body present) - so a type: "default" response.

Same goes for all other response types. There is no reason for us to have cors, basic, or opaque. These all exist to prevent against attacks which can not happen in Deno, because we do not have an origin cookie jar.

Sources:
https://fetch.spec.whatwg.org/#atomic-http-redirect-handling

@lucacasonato
Copy link
Member Author

lucacasonato commented Nov 11, 2020

Another thing:

You can currently just manually invoke the underlying fetch op to get this information... so this is some fake security that is implemented fully in JS, and can be bypassed relatively easily.

@KNnut
Copy link
Contributor

KNnut commented Nov 11, 2020

Related to #4389

@kitsonk
Copy link
Contributor

kitsonk commented Nov 12, 2020

@nayeemrmn and @benjamingr thoughts in breaking from the spec?

@benjamingr
Copy link
Contributor

Mostly "this indeed sucks, we've run into this and the mismatch when talking about fetch in Node.js". See some discussion here.

I feel strongly that:

  • Deno should ship web standard APIs so fetch should be the real spec compliant fetch as much as possible. Node was bitten in the past when it shipped non-standard APIs.
  • Deno should acknowledge the fact it does not have things like the cross-origin policy.

In this case, I agree with @lucacasonato and returning manual makes more sense than opaqueredirect - where does the spec actually require opaqueredirect here?

One thing we have discussed in Node is a createFetch function that takes an agent and returns a configured fetch function that lets you set things (like the cookie store).

You can currently just manually invoke the underlying fetch op to get this information... so this is some fake security that is implemented fully in JS, and can be bypassed relatively easily.

This seems unrelated and can be fixed? Security in JS can work fine? (At least since ±2011 and the Caja stuff / strict mode)

@lucacasonato
Copy link
Member Author

where does the spec actually require opaqueredirect here?

image
https://fetch.spec.whatwg.org/#requests

Deno should ship web standard APIs so fetch should be the real spec compliant fetch as much as possible.

I agree. But I think the way the API works should make sense in the context of a server side runtime. Cross-origin policy does not make sense in Deno, because 1. we don't have an origin, and 2. we don't have a global cookie store.

I guess the issue here is that we have to make calls about how and where we deviate from the spec. I will investigate what other server side fetch implementations do (node-fetch, Cloudflare Workers), so we don't start getting disagreement about where to deviate between implementations. Ideally this would be specified, but I assume there is no interest from the standards side. cc @annevk

This seems unrelated and can be fixed?

Maybe. But Deno considers all JS code to be untrusted right now, so at the moment we don't have a means to fix it. Indeed unrelated though.

@annevk
Copy link

annevk commented Nov 13, 2020

It might be reasonable to have something, not sure. Is there a description of what servers want to do?

@benjamingr
Copy link
Contributor

@annevk I am happy to (try and) work on this (though not this month). Node is also very interested in adopting fetch and we've run into this (and similar limitations) when trying to adopt it.

I think the spec can be amended in such a way that would not require browser implementors to make actual changes while enabling non-browser users of the spec to adopt fetch.

I think you outlined the biggest issues yourself a year and a half ago :]

I think there will be tremendous value in "fetch-everywhere" and for there to be one primitive developers can build on be it in Node, Deno and browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/fetch related to the ext/fetch
Projects
None yet
5 participants