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

Standardize supported subset of fetch #2

Open
benjamingr opened this issue Apr 1, 2022 · 6 comments
Open

Standardize supported subset of fetch #2

benjamingr opened this issue Apr 1, 2022 · 6 comments

Comments

@benjamingr
Copy link
Member

We should discuss things like cors behavior - there are questions/suggestions about this in nodejs/undici#1315 (comment)

I think https://deno.land/manual/runtime/web_platform_apis#spec-deviations is a good baseline but I would request the following deviations for what we standardize:

  1. The server user agent does not have a cookie jar. As such, the set-cookie header on a response is not processed, or filtered from the visible response headers.
  2. Servers do not follow the same-origin policy, because the http agent currently does not have the concept of origins, and it does not have a cookie jar. This means servers do not need to protect against leaking authenticated data cross origin. Because of this servers do not implement the following sections of the WHATWG fetch specification:
  • Section 3.1. 'Origin' header.
  • Section 3.2. CORS protocol.
  • Section 3.5. CORB.
  • Section 3.6. 'Cross-Origin-Resource-Policy' header.
  • Atomic HTTP redirect handling.
  • The opaqueredirect response type.
  1. A fetch with a redirect mode of manual will return a basic response rather than an opaqueredirect response.
  2. The request and response header guards are implemented, but unlike browsers do not have any constraints on which header names are allowed.
  3. The referrer, referrerPolicy, mode, credentials, cache, integrity, keepalive, and window properties and their relevant behaviours in RequestInit are not implemented. The relevant fields are not present on the Request object.

Of course, this would need to be bike-shedded and written more formally. Please suggest any more deviations we'd want here.

Note this list omits the handling of file: urls. Node.js does not wish to implement file url support at the moment because of security concerns. People (@mcollina for example) have raised good concerns it would be too easy to get a file url from a user and pass that to fetch. I think it's probably fine for servers/edge to deviate on this?

cc @lucacasonato

@jasnell
Copy link

jasnell commented Apr 15, 2022

With regards to CORS, I can imagine a case where a runtime does implement the notion of an origin (e.g. imagine some future where we allow specifying an origin at Node.js startup or when creating a worker thread). For this, I think we craft this bit to say that if the runtime implements origins then it should fully implement the CORS portion of the fetch specification, otherwise those pieces should be omitted / stubbed out when necessary.

Note that this also raises the question about whether runtimes that choose not to implement any given API should just simply omit those APIs vs. implementing non-functional stubs... Whatever we decide on that should be a rule we apply consistently across everything. This is relevant, for instance in regards to Request object, per the comment:

The relevant fields are not present on the Request object

I think that even if the fields are not supported, the fields should be at least present on the Request object. This is similar to the way we handle unsupported bits on the Event API, for instance.

@benjamingr
Copy link
Member Author

I don't think that's work for Deno @jasnell since they implement the location API and have a globalThis.location which they use to make relative requests in fetch but do not implement any CORS. Right @lucacasonato ?

@benjamingr
Copy link
Member Author

I think that even if the fields are not supported, the fields should be at least present on the Request object. This is similar to the way we handle unsupported bits on the Event API, for instance.

That bit makes sense to me

@jasnell
Copy link

jasnell commented Apr 17, 2022

I don't think that's work for Deno @jasnell since they implement the location API and have a globalThis.location which they use to make relative requests in fetch but do not implement any CORS.

Then let's explicitly make this piece fully optional as opposed to saying that server's don't implement it. It's entirely possible that some non-browser runtime could support this and we don't want to rule it out. What we can say instead is that given that the need to support origins is not absolute across runtimes, it is perfectly fine for implementations of this profile of fetch to not implement any of the origin/CORS-related mechanisms.

@glasser
Copy link

glasser commented May 9, 2022

One thing we've desired to do in our projects (such as Apollo Server) is allow users who care about precisely how our software makes HTTP requests to pass in their own implementation of the fetch function which our software consumes. So they can choose between node-fetch, make-fetch-happen, undici, the new Node 18 undici-based fetch, etc. One challenge we have run into is that these implementations are pretty inconsistent about how they detect whether their arguments contain Request or Headers objects; for example, some of these implementations use instanceof Request checks which mean that Request/Headers objects generated from other libraries are not recognized. It would be useful for a portable fetch standard to more precisely define how these objects are detected in arguments (ideally without using instanceof, similarly to how Promises are detected via their then method rather than their prototype). This of course isn't required in the web context where there's only one built-in implementation of fetch and its associated types.

(The solution we came up with is to refuse to ever pass Request/Headers objects to a fetch invocation and even publish TypeScript types for "fetch minus Request/Headers arguments" but this doesn't feel ideal.)

(Apologies if this should have been a separate issue; not clear what the scope of this one is.)

@Ethan-Arrowood
Copy link
Collaborator

In my opinion (based on working on undici), aspects of a spec that do not apply to a given environment (runtime) should be implemented as a no-op or error path.

For example, in Node.js, since we currently do not have the concept of an origin, the code path in fetch that essentially boils down to if (opts.origin) { ... should log or throw a warning/error along the lines of This Fetch implementation does not support the `origin` option. Please see [doc link] for more information. In the case where it logs a warning, the operation should just continue as normal (essentially a no-op, and maybe these warnings could be disabled somehow?). In the case of an error, it should throw and halt execution like any other error.

The WinterCG Fetch spec could explicitly define these warnings/error paths based on relative aspects of Fetch such as origin, location, cookies, etc.

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

4 participants