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

CORS #3

Closed
annevk opened this issue Jul 26, 2016 · 17 comments
Closed

CORS #3

annevk opened this issue Jul 26, 2016 · 17 comments

Comments

@annevk
Copy link

annevk commented Jul 26, 2016

Consistently naming it "cors-preflight" seems good. This should only affect requests, not responses.

Allowing credentials while not listing origins should not happen in valid examples, only invalid examples. (Or is Chrome security okay with allowing wildcarded access to credentialed content?)

The syntax should probably be such so that you can make separate declarations for requests without credentials (maybe there origins is *) and those with.

There should be a section on crossdomain.xml and how this is somewhat better since a) we still check on each request if the URL actually knows about the policy (although this doesn't help with cors-preflight, the justification for that can be that we require a specific list of origins if you want to allow credentials), and b) CORS is still required for responses.

@annevk
Copy link
Author

annevk commented Jul 26, 2016

(Also, rename this repository to "origin-policy" perhaps? "Origin Policy" or "Origin Manifest" seems reasonable.)

This is the manifest the web needs.

@mikewest
Copy link
Member

Yeah. The CORS bits are little more than a strawman. I have some vague ideas about the behaviors I'd like to enable (namely "Hey, I know what CORS is. I'm going to examine the incoming headers the way I'm supposed to. Quit it with these preflights."), but I'm totally open to suggestions about how to make them work.

Can you spell out the syntax you'd like to see?

@mikewest
Copy link
Member

Also: renamed. :)

@annevk
Copy link
Author

annevk commented Jul 26, 2016

Name the overall field "cors-preflight". Then we have "origins", "methods", "headers" as subfields and we'd have these subfields prefixed with "unsafe-" (for the credentials case) as well. When prefixed with "unsafe-" these fields cannot contain "*". Both prefixed and unprefixed fields can be used together (covering the different request scenarios).

@mikewest
Copy link
Member

Hrm. Is that much complexity valuable?

As another strawman, could we just have something like "require-cors-preflight": false? The preflight request itself is intended as a verification that the server understand the CORS protocol and is going to do the right kinds of checks on the incoming request before sending a response. As long as we're not screwing with the response in a dynamic way, this doesn't seem to open any new holes.

A server that expects to service cross-origin requests could do something like:

{
  "require-cors-preflight": false,
  "headers": {
    "fallback": [
      {
        "name": "Access-Control-Allow-Origin",
        "value": "*"
      }
    ] 
  }
}

That would allow anonymous access to everything without a preflight, and allow the server to override the allowed origins for specific requests by sending a custom Access-Control-Allow-Origin header with the response.

The boolean switch wouldn't give much granularity in terms of allowing/denying preflights on a case-by-case basis. If folks are using preflights as a firewall of sorts, then this certainly wouldn't be enough. Perhaps @mnot, et al. could point at the kinds of usage that Akamai's customers care about? I think @igrigorik and @jakearchibald have opinions about this as well.

@annevk
Copy link
Author

annevk commented Jul 26, 2016

The complexity matches the complexity of CORS preflights today. And actually offers some additional functionality if we allow multiple origins.

The reason for the complexity is to avoid falling in the same pitfall as crossdomain.xml. We need folks to understand that they put themselves at the risk of XSRF, especially in the credentials case.

It's a rather weird argument that now that we have an origin-wide manifest, we can drastically simplify the security handshake and granularity. That will not help folks think through the problem space.

(I especially like the usage of "unsafe-" for requests that contain credentials.)

Note that I still think that we should very much require CORS on all responses. That's why I think we should call it "cors-preflight". We shouldn't do any defaulting for CORS on responses and keep that as a per-URL thing.

@mikewest
Copy link
Member

Ok, before I argue with you ( :) ), let me try to parrot back the suggestion you've made:

The policy would contain something like:

"cors-preflight": {
  "origins": [ "*" ]
  "methods": [ "*" ]
  "headers": [ "*" ],
},
"unsafe-cors-preflight-with-credentials": {
  "origins": [ "https://example.com/" ]
  "methods": [ "PATCH" ]
  "headers": [ "X-Amazing-Header" ]
}

This policy would bypass all CORS preflight requests whose credentials mode is omit, and bypass CORS preflight requests for requests coming from https://example.com, including those using the PATCH method and/or including an X-Amazing-Header header?

Is that more or less what you have in mind?

It seems like a reasonable first step, and I generally agree with your assessment of the risk of blindly removing the preflight for credentialed requests, so I won't argue too vehemently in favor of anything else, however, this is The Internet, so:

Note that I still think that we should very much require CORS on all responses. That's why I think we should call it "cors-preflight". We shouldn't do any defaulting for CORS on responses and keep that as a per-URL thing.

As long as the headers that a policy manifest can set are static, it seems that we'd be limited to either whitelisting a specific origin as being same-origin with us, or whitelisting * (and therefore limiting ourselves to non-credentialed requests (because we couldn't echo the origin of the request without server-side involvement)). Whether I put those defaults into my Apache config or into a policy manifest my server delivers seems somewhat immaterial, doesn't it?

@annevk
Copy link
Author

annevk commented Jul 26, 2016

Yeah, and I suppose you are right that we could have response defaulting too. Should be a separate field so they do get confused (with preflights).

@tyoshino
Copy link

Don't we need "maxage"?

Do we want to make "origins", "methods" and "headers" field required for "cors-preflight" section? Maybe that's for the complexity we want for preventing misconfiguration even for non-credentialed ones. As we now allow wildcards for all of them, I also have a feeling that we can make them optional for non-credentialed ones (just for allowing for setting defaults). Explicit inclusion of the Access-Control-Allow-Methods header and Access-Control-Allow-Headers header in a preflight response (in addition to Access-Control-Allow-Origin which is also included for non-preflight CORS response) is still required in the Fetch Standard maybe because it's good indication that the server correctly understands the CORS preflight algorithm. But in the origin-policy, it's clear that it's indicating the signal which was carried by the CORS preflight protocol, i.e. the server admin understands what it does correctly.

"unsafe-cors-preflight-with-credentials" syntax looks good.

@mikewest
Copy link
Member

Don't we need "maxage"?

I think this is implicit with the cache lifetime of the manifest. That is, as long as we have an origin policy manifest for an origin, we'll apply these settings to the preflight requests we require.

Do we want to make "origins", "methods" and "headers" field required for "cors-preflight" section? Maybe that's for the complexity we want for preventing misconfiguration even for non-credentialed ones. As we now allow wildcards for all of them, I also have a feeling that we can make them optional for non-credentialed ones (just for allowing for setting defaults).

I could get behind that default for non-credentialed preflights, though I also don't think it much matters as long as it's a configuration we allow. Typing three lines into a manifest is not a difficult task, so keeping a strict default isn't unreasonable.

@tyoshino
Copy link

I think this is implicit with the cache lifetime of the manifest.

I see. There could be demand for fine-grained lifetime configuration, but sgtm for the first version. So, the Fetch algorithm would consult the origin policy cache every time to check need for preflight by treating the result as max-age=0.

@annevk
Copy link
Author

annevk commented Jul 27, 2016

Yeah, checking Origin Policy has to be integrated with Fetch anyway so no need to duplicate that with the CORS-preflight cache. And since it effectively replaces the CORS-preflight cache when in use I don't think we need max-age. (If you don't want an origin-wide policy for CORS-preflights, simply don't use this.)

@annevk
Copy link
Author

annevk commented Jul 27, 2016

I would prefer requiring all three if you want to set them all to "*" for non-credentialed requests. Otherwise we need to support some other value to set them to their original default and folks will get confused.

mikewest added a commit that referenced this issue Jul 27, 2016
mikewest added a commit that referenced this issue Jul 27, 2016
@mikewest
Copy link
Member

I've taken a stab at this in https://mikewest.github.io/origin-policy/#cors-preflight-member (with some examples), and https://mikewest.github.io/origin-policy/#fetch-bypass-preflight (which ought to be called somewhere in the viscinity of step 5.1 of https://fetch.spec.whatwg.org/#http-fetch).

WDYT?

@annevk
Copy link
Author

annevk commented Jul 27, 2016

Looks reasonable. Where you describe the value space you probably want to mention the restrictions for the unsafe variant.

@mikewest
Copy link
Member

Ok. Added a requirement there. I think this is good enough for a first pass; closing this out.

Thanks for the feedback!

@tyoshino
Copy link

Filed #11 but lgtm except for that. Thanks

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

3 participants