wintercg / fetch

WinterCG Fetch Standard
https://fetch.spec.wintercg.org/
Other
23 stars 0 forks source link

Standardize supported subset of `fetch` #2

Open benjamingr opened 2 years ago

benjamingr commented 2 years ago

We should discuss things like cors behavior - there are questions/suggestions about this in https://github.com/nodejs/undici/pull/1315#issuecomment-1085012282

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.
  3. A fetch with a redirect mode of manual will return a basic response rather than an opaqueredirect response.
  4. The request and response header guards are implemented, but unlike browsers do not have any constraints on which header names are allowed.
  5. 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.