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

Add unsafe-no-cors mode #1533

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bvandersloot-mozilla
Copy link

@bvandersloot-mozilla bvandersloot-mozilla commented Nov 7, 2022

We identified a potential need for a more sustainable no-cors mode in discussion surrounding FedCM. The purpose is to create a browser-process priveleged mode that will not fail the Access-Control-Allow-Origin CORS checks while otherwise behaving like a normal CORS request.

Here are the deviations I have made from cors mode to make unsafe-no-cors are:

  • do not perform the "CORS check" (ACAO/ACAC)
  • allow the request to set a new omit origin flag that forces omission of the Origin header
  • require a request to have a policy container specified (via the client is allowed)
  • require the service worker mode to not be all

Because this is such an unsafe mode I added an explanation inline with the other definitions of request modes and a warning about concerns and hand-waves about the client's agent cluster.

Happy to get feedback on this draft!

  • At least two implementers are interested (and none opposed):
    • Mozilla
  • Tests are written and can be reviewed and commented upon at:
    *
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (not for CORS changes): …
  • MDN issue is filed:
    *

Preview | Diff

We identified a potential need for a more sustainable no-cors mode in discussion
surrounding FedCM. The purpose is to create a browser-process priveleged mode
that will not fail the Access-Control-Allow-Origin CORS checks while otherwise
behaving like a normal CORS request.

Here are the deviations I have made from cors mode to make unsafe-no-cors are:

- do not perform the "CORS check" (ACAO/ACAC)
- allow the request to set a new omit origin flag that forces omission of the Origin header
- require a request to have a policy container specified (via the client is allowed)
- require the service worker mode to not be all

Because this is such an unsafe mode I added an explanation inline with the other
definitions of request modes and a warning about concerns and hand-waves about the
client's agent cluster.

Happy to get feedback on this draft!
@annevk
Copy link
Member

annevk commented Nov 11, 2022

Reading your draft I think you and I might have understood the outcome of the meeting differently. Here's what I think:

  • Requests should not be more powerful than "cors" without preflight or "no-cors". You cannot use a DELETE method with this, for instance, or include custom headers.
  • The main benefit of this is not bypassing the CORS check, but bypassing "no-cors"-related checks, such as the upcoming opaque-response blocking and the existing Cross-Origin-Resource-Policy header. (As those are why "no-cors" is not an option and how we plan to make "no-cors" largely safe.)
  • It's also not clear to me that policy container is a drop-in replacement for an environment, although it might fit for a number of cases. That needs a bit more checking I think.

@bvandersloot-mozilla
Copy link
Author

Reading your draft I think you and I might have understood w3c-fedid/FedCM#320 (comment) differently.

I think we understood the same thing at the end of the meeting, by my understanding diverged with the days between the meeting and when I sat down to draft and my own poor note-taking. I will take another pass to more closely reflect the version you describe in your first two bullets.

It's also not clear to me that policy container is a drop-in replacement for an environment.

I don't think that's quite what I'm doing. I made two distinct changes wrt policy containers:

  • Make the cross-origin resource policy check use the request's policy container, rather than ignoring it and assuming a value of "client" and non-null client. This looked like a bug that needed fixing.
  • Assert that unsafe-no-cors requests do not fallback to a default policy container because none was provided. This is removing a foot-gun of using this version of unsafe-no-cors.

I was thinking that the caller using unsafe-no-cors would construct or have available a global or environment that is the "main process" context and be able to use that. That felt out of the scope of this spec, aside from the warning I added, since the request's client is an argument.

@annevk
Copy link
Member

annevk commented Nov 14, 2022

Make the cross-origin resource policy check use the request's policy container

Right, but you are changing the type of argument here. So all callers would need to start passing in a different argument as well and I guess we're hoping/knowing that we need nothing that isn't available on a policy container?

@bvandersloot-mozilla
Copy link
Author

Make the cross-origin resource policy check use the request's policy container

Right, but you are changing the type of argument here. So all callers would need to start passing in a different argument as well and I guess we're hoping/knowing that we need nothing that isn't available on a policy container?

Ah, I missed that the algorithm definition is export.

"stable no-cors" vision
@bvandersloot-mozilla
Copy link
Author

Alright, I'm about to upload a new version that is more in line with a "stable no-cors" vision.

The diff looks small because some of the algorithms actually just work the way we want when a new enum value is added without any change needed. For example, in no-cors specific checks like "Cross-Origin-Embedder-Policy allows credentials" we are exempt because we are not "no-cors".

I still need to consider the ECMAScript and HTML integration and sort out the neatest way to get caller-control over the presence of the Origin header in unsafe-no-cors. However, I think this benefits from share-early-share-often, if only to keep visibility into the thought process.

fetch.bs Outdated
@@ -1796,13 +1798,27 @@ to not have to set <a for=/>request</a>'s <a for=request>referrer</a>.
<dt>"<code>navigate</code>"
<dd>This is a special mode used only when <a>navigating</a> between documents.

<dt>"<code>unsafe-no-cors</code>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider naming this something like "host-no-cors" or "ua-no-cors" or something else that indicates what purpose it serves. Just "unsafe" is likely to scare off people who should use it, and not dissuade overconfident people who think they know what they're doing. Since this is only for use in web standards, that'll get caught eventually in spec review, but that could be much later than we want, and a better name could move the right design earlier.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsafe-no-cors was @annevk's name for it, so I'll defer to him for a renaming decision.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unsafe is exactly right given that it exposes the contents of the response in the process it was invoked in. It's a name that is supposed to make you ask your peers for advice. I don't think host or ua has that kind of effect, especially since we don't have clearly delineated "host/ua agent clusters".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the point of this mode is that it must only be used from the browser process or an equivalently-trusted process, so the response was already exposed there. If it's invoked by that sort of caller, are there any situations where it would still be unsafe?

I have a general rule to avoid "unsafe" and "safe" in naming, since they usually refer to just one kind of safety that was salient to the original author (here, exposure of resource bytes, I think), and future users tend to assume a different kind of safety and so get into trouble.

We could be more specific about what's unsafe instead of trying to name according to the intended caller. E.g. "no-cors-override-exposure-policies". But I suspect that naming according to the intended caller will make it easier to figure out how to maintain this mode's behavior in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I follow the policy that we use for introducing "unsafe" in various places in the web platform. Namely that it requires additional attention. Is easy to lint, etc.

It also very much depends on what happens with the response bytes, how the request is setup, etc.

One point against "unsafe" here might be that it's not useful to web developers as this value is also exposed to them. From that perspective "user-agent-no-cors" is prolly better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point against "unsafe" here might be that it's not useful to web developers as this value is also exposed to them. From that perspective "user-agent-no-cors" is prolly better.

Just to double check: do I understand it correctly that this is also going to be exposed to web developers (as Sec-Fetch-Mode=unsafe-no-cors), in addition to browser engineers?

If so, the name shouldn't trigger suspicion by the web developer (who should trust that the spec was written in a careful and thoughtful way, rather than something that the developer has to re-check again), right?

From that perspective "user-agent-no-cors" is prolly better.

Right. user-agent-no-cors would make it do and I think would also address @jyasskin 's point (and @annevk 's, to trigger spec reviewers to look carefully).

FWIW, mediated-no-cors could work too for me, just as a suggestion (user-agent-no-cors works too I think).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think in this case since web developers are not making the unsafe decision it's indeed probably best if they don't see the word unsafe. I think there are other cases though where web developers seeing the word unsafe is good, e.g., unsafe-eval.

I like user-agent-no-cors best still.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated
@@ -1796,13 +1798,27 @@ to not have to set <a for=/>request</a>'s <a for=request>referrer</a>.
<dt>"<code>navigate</code>"
<dd>This is a special mode used only when <a>navigating</a> between documents.

<dt>"<code>unsafe-no-cors</code>"
<dd>This is a special mode for the <a>host environment</a> to use internally to wittingly make
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "host environment" the right term? It's odd to borrow a Javascript term for something in Fetch that's not integrated in any other way with Javascript infrastructure. I might just use [=user agent=].

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reached for "host environment" because I was talking about agent clusters in the warning and also mentioned the host environment there. I'll change this reference to [=user agent=], but do you think I should leave the reference in the warning alone?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably change both to "user agent" or say something about the required memory isolation in the warning, but I don't feel strongly about it. I think we really want something about how trusted the other code in that agent cluster's process is, but I don't think we have the right terms defined for that, and I don't think this change needs to block on thinking through and defining those.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'm changing the warning to be "user agent" as well and mention memory isolation explicitly, rather than directly call in Javascript concepts.

fetch.bs Outdated Show resolved Hide resolved
@domfarolino
Copy link
Member

In the documentation for the new mode, we should probably also mention that the request is made from a context that is in some way "detached" from the document/client environment settings object.

Regarding the rest of the spec, this entails at least two things:

  1. Changing how the response process algorithms are scheduled. Right now, for example, the "process response" algorithm is called by queueing a fetch task on either a global object or a parallel queue, which seems to kind of break in the unsafe-no-cors model. We have no notion of a "browser process" in the spec, meaning callers of unsafe-no-cors requests couldn't pass in browser-process-specific scheduling primitives; I think in specs in general, we've been treating the "browser process" as if it were just a bunch of per-caller parallel queues, as we do for much of the navigation and session history infra in HTML. Given that, should we somewhere assert (and also document under the introduction of the new mode) that the associated task destination for unsafe-no-cors fetches must be a parallel queue?
  2. Handling request's client. As mentioned on our last VC, the spec has many assumptions around "client", especially with its interaction with other members. One specific example: a requests' origin defaults to the string "client", and assumes that the request's actual client is non-null in this case, which crashes fetch step 10 if you make an unsafe-no-cors fetch without manually setting request's origin. But that's an awkward requirement, since I think you mentioned your PR has logic that automatically ensures the Origin header isn't sent anyways. This is emblematic of the larger problem: Do you have a plan for handling the many uses of a request's client with this "new" type of request? Should we audit them all, or take a snapshot of the Document's ESO at some point? (As Anne mentioned, request's "client" should be something like a "snapshot" anyways, although I don't know if that's explicit anywhere. Also (aside) it seems weird that a request's "window" is pretending to have access to the actual Window object when we're not fetching on the Document's main thread)

@bvandersloot-mozilla
Copy link
Author

bvandersloot-mozilla commented Nov 30, 2022

Given that, should we somewhere assert (and also document under the introduction of the new mode) that the associated task destination for unsafe-no-cors fetches must be a parallel queue?

I think so. I'm adding the conditioned assertion into fetch.

RE: the Origin discussion:

This is a shortcoming of this PR and I need to add a new member to request to permit forced Origin header elision.
Generally speaking, I was operating under the assumption that the client would either be a cloned ESO or null, with the necessary other arguments specified, which is already in the spec as a possible argument.

Do you have a plan for handling the many uses of a request's client with this "new" type of request? Should we audit them all, or take a snapshot of the Document's ESO at some point?

I am open to either solution here. I think if there is an expectation that the "client" is a snapshot of some variety, that should be clear and able to be relied upon, so I lean that direction. However, a null client should be applicable here.

Looking through the spec there are only a couple of instances where a null client raises my attention and I am curious how they are handled currently:

Also (aside) it seems weird that a request's "window" is pretending to have access to the actual Window object when we're not fetching on the Document's main thread)

I would expect most uses to have a "window" of "no-window" but do not see a reason to exclude other values.

@bvandersloot-mozilla
Copy link
Author

Converted to user-agent-no-cors

@domfarolino
Copy link
Member

I was operating under the assumption that the client would either be a cloned ESO or null, with the necessary other arguments specified, which is already in the spec as a possible argument.

When you say "which is already in the spec as a possible argument," do you mean that we expect all unsafe-no-cors consumers to explicitly specify request.policy container and request.referrer policy etc., and so on, for all of the request members that would otherwise be derived implicitly from client (which I guess is null for these kinds of requests)?

@bvandersloot-mozilla
Copy link
Author

I was operating under the assumption that the client would either be a cloned ESO or null, with the necessary other arguments specified, which is already in the spec as a possible argument.

When you say "which is already in the spec as a possible argument," do you mean that we expect all unsafe-no-cors consumers to explicitly specify request.policy container and request.referrer policy etc., and so on, for all of the request members that would otherwise be derived implicitly from client (which I guess is null for these kinds of requests)?

I meant that this spec already supports a null client, so I assumed this wasn't breaking new ground. If request.client is null, I expect that consumers would have to explicitly specify all arguments that default to "client".

@domfarolino
Copy link
Member

I guess we need to make a decision regarding what to do with client/window in general. I think we don't have the concept of a "snapshotted" ESO at the moment that a user of unsafe-no-cors would pass in, but I guess for discussion we could pretend that this exists, and decide whether that would be a suitable input for requests like this, alongside the normal null.

Snapshotting client

If we decide that snapshotting ESO is a good idea, then I think this means every user of these kinds of requests would be responsible for snapshotting some ESO themselves as the request's client (or maybe the Fetch entrypoint could try and implicitly do it somehow?). Some concerns:

  1. This means the snapshot could of course get out-of-sync with the true ESO on the main thread right when the fetching is started. That seems unavoidable, but probably we should quantify how many checks/steps this impacts: I suppose it would be all of the checks/steps that Fetch does right before going in parallel (ones that are run on the main thread in the usual case)?
  2. There are probably places in Fetch where we use the client not just as a bag of state, but as some interactive thing with an event loop and thing that can handle tasks etc. We should look for such cases, to see if any would cause problems if they suddently start operating on a state snapshot of ESO, instead of the real thing. Perhaps the 3 client usages you provided above might be on this list.

In any case, this approach is simple in for the null client case as you mentioned. We don't have to do anything special that Fetch doesn't already do today, because passing in null as your client would be an explicit decision and I guess you better know what you're doing RE other state. A typical unsafe-no-cors user would probably not pass in null I think, but would use the snapshot.

Always null client

If we decide against the snapshot ESO idea and instead require client and/or window to always be null, then we have to make decisions about all of client-derived state. While null is indeed a valid client value today, its implications are likely not what the typical unsafe-no-cors would want (no referrer policy, CSP bypass, ...) i.e., a lot of that is centralized in policy container, which gets wiped anew in https://fetch.spec.whatwg.org/#ref-for-concept-request-policy-container%E2%91%A0 for null clients, and seems bad for every request like this to endure (see FedCM fetches, one of the motivations for unsafe-no-cors in the first place, which tries to hack together its own CSP check elsewhere).

So if we went this route we'd probably want to assert that an explicit policy container + any other interesting state gets explicitly passed in on the request. That state would likely just be a clone/snapshot of the Document's policy container and additional state, which isn't any better than the snapshotting ESO idea above.

I think if there is an expectation that the "client" is a snapshot of some variety, that should be clear and able to be relied upon, so I lean that direction.

Given all of this, I too lean in the direction of assuming that unless explicitly passed in, the request's client is a snapshot of the associated Document's ESO. But we should gather more feedback, perhaps @annevk has opinions?


Also, it would be great to more explicitly lay-out the integration that this has with #1442 and whatever response-blocking technology already exists (I'm told CORB no longer lives in Fetch, so maybe there is none?), to get a feel for what major behavior change this would entail especially for implementers. Is it simply the fact that unsafe-no-cors requests have "basic" response tainting that exempts them from all of the checks that make no-cors an unsuitable alternative?

@@ -4039,7 +4071,9 @@ the request.
<a for="environment settings object">global object</a> is a {{Window}} object; otherwise
"<code>no-window</code>".

<li><p>If <var>request</var>'s <a for=request>origin</a> is "<code>client</code>", then set
<li><p>If <var>request</var>'s <a for=request>origin</a> is "<code>client</code>" and either
<var>request</var>'s <a for=request>mode</a> is not "<code>user-agent-no-cors</code>" or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an "or and" in this sentence (and I cannot decide whether it should be "or" or "and")

@bvandersloot-mozilla
Copy link
Author

Based on conversations at TPAC, cookie layering work looks like it will solve this eventually. Closing this for completeness.

@domfarolino
Copy link
Member

I'm not sure. The motivation for this was some discussion around FedCM fetch requests, of which there are two interesting ones:

  1. The accounts list fetch
  2. The ID assertion fetch

We agreed that the ID assertion fetch can use CORS and can piggyback off of the cookie layering work to send SameSite=None cookies. But the accounts list fetch I think is still somewhat unsolved. That either has to:

  1. Use something like unsafe-no-cors, which will get around ORB blocking of a cross-origin no-cors JSON request/response
  2. Or hack around this by manually changing the initiator of the request to be same-origin with the resource, so ORB won't block it. Maybe this can be justified because the request is "browser-mediated", but maybe not. I think we probably need to discuss this more.

I'll re-open until we discuss the accounts fetch solution more, if that's alright.

@domfarolino domfarolino reopened this Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants