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: Is 404 on OPTIONS request the right thing to do? #2868

Closed
mtharrison opened this issue Oct 22, 2015 · 14 comments
Closed

CORS: Is 404 on OPTIONS request the right thing to do? #2868

mtharrison opened this issue Oct 22, 2015 · 14 comments
Assignees
Labels
Milestone

Comments

@mtharrison
Copy link
Contributor

@mtharrison mtharrison commented Oct 22, 2015

Sorry to dredge this up, I know there's a lot of changes been made on CORS recently.

Say I make an cross-domain XHR request to a route with CORS switched on (origin allowed) and I include a custom header (e.g. x-custom-header) but that header is not whitelisted in config.cors.headers or config.cors.additionalHeaders, the following will happen:

  • This code will be invoked
  • The options request will responded to with a 404

Once the above happens, the browser will show an error like:

Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.

This message is concealing the real issue that the custom header isn't allowed. If this check wasn't there at all:

  • A proper OPTIONS response would be sent with the access-control-allow-headers header missing the custom header

Also a more relevant error would be shown by the browser that would help with debugging:

Request header field x-custom-header is not allowed by Access-Control-Allow-Headers in preflight response.`

Do we need to validate headers here at all? Is that the browser's job?

@bonbonio
Copy link

@bonbonio bonbonio commented Oct 22, 2015

+1

Perhaps a 400 response would be more suited, as I took the 404 as it being a case that I had to write a handler for an OPTIONS request (which is a perfectly logical assumption) and thats why it wasn't found.

@mtharrison
Copy link
Contributor Author

@mtharrison mtharrison commented Oct 22, 2015

Why even a 400? Why not just a 200 with the appropriate headers? There's nothing wrong with the OPTIONS request.

@devinivy
Copy link
Member

@devinivy devinivy commented Oct 22, 2015

If I understand the spec correctly, a non-2xx response on a preflight is treated as though there was a network issue during preflight, which does not involve taking into account the preflight response headers. A 2xx response kicks the browser into validating the original request using the preflight response headers. So perhaps it should be a 200 response.

But yes, I believe the server ought to validate the headers listed in Access-Control-Allow-Headers just like it currently does.

@bonbonio
Copy link

@bonbonio bonbonio commented Oct 22, 2015

To clarify, yes I would prefer a 200 if the request is valid, the 400 was just a preference over a page not found which is totally incorrect as the route is present, I just didnt like the indication the route was missing (and that it might need to be added).

Although if @devinivy is correct then the suggestion is moot!

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 22, 2015

The spec isn't clear on this. It just say "terminate these steps". I think a 404 is much safer option than 200 without the CORS headers. 400 is not appropriate here as there is nothing wrong with the request.

Is there any other system we can look at to see how they handle this? Also, if the request is missing some of the required request headers we can't even look up if the route exists and a 200 is misleading.

@mtharrison
Copy link
Contributor Author

@mtharrison mtharrison commented Oct 22, 2015

@hueniverse which part of the spec are you referencing there so I can go read up on it?

It looks like Express CORS will do a 200 without CORS headers (using this code https://gist.github.com/mtharrison/65ba1e7f6856df2ba8fa). What would the issue be with doing a 200 with the CORS headers and then the browser knows exactly why the request can't happen?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 22, 2015

@devinivy
Copy link
Member

@devinivy devinivy commented Oct 22, 2015

Under http://www.w3.org/TR/cors/#cross-origin-request-with-preflight

If the response has an HTTP status code that is not in the 2xx range

  • Apply the network error steps.

Providing a 4xx will have a similar effect, but the browser isn't given the chance to even use the preflight response headers. So hapi is putting the onus on itself to understand the browser spec for a "failed" preflight.

@mtharrison
Copy link
Contributor Author

@mtharrison mtharrison commented Oct 22, 2015

Yes. That's the crux of it I think. So my original question was supposed to mean should we be doing that? Or should we be letting the browser receive those preflight response headers and then decide what happens?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 22, 2015

To be clear, a 200 response would include no CORS headers. That part is clear from the spec.

Here is my proposal: if the request is missing Origin or Access-Control-Request-Method, return 404 as this means the request is not a CORS request and we don't have an OPTIONS handler for those. If the request includes those two, but it fails the tests, return a 200 without any CORS headers.

@hueniverse hueniverse added bug and removed support labels Oct 22, 2015
@hueniverse hueniverse self-assigned this Oct 22, 2015
@mtharrison
Copy link
Contributor Author

@mtharrison mtharrison commented Oct 24, 2015

To be clear, a 200 response would include no CORS headers. That part is clear from the spec.

I'm still not totally convinced on that. From what I understand it's supposed to be the browser doing the validation of the preflight, not the server. So it seems reasonable that the browser should get the CORS headers back to validate the request.

I've done a test, serving a file from Google Cloud Storage and they indeed send back a 200 with CORS headers in response to a request with a non-whitlisted header. (You can try it out here: http://mattharrison.s3.amazonaws.com/cors/google-cloud-storage.html).

The browser error message in this case is the helpful one indicating exactly what went wrong:

Request header field x-custom-header is not allowed by Access-Control-Allow-Headers in preflight response.

With both the current and suggested implementation in hapi, it seems like it wouldn't be possible to get that error, because in response to an invalid header, hapi won't provide the CORS response headers in the response.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 24, 2015

The spec is pretty clear on that part. The section I linked to before is for the server: http://www.w3.org/TR/cors/#resource-preflight-requests. There is another section for the browser which is completely irrelevant here.

The bottom line is that I don't really care what other servers are doing. My reading of the spec is that when the required headers are missing or not matching, "terminate this set of steps" means don't do anything else form this spec such as including any CORS headers.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 24, 2015

"If any of the header field-names is not a ASCII case-insensitive match for any of the values in list of headers do not set any additional headers and terminate this set of steps."

That's pretty clear.

@mtharrison
Copy link
Contributor Author

@mtharrison mtharrison commented Oct 24, 2015

Yes, that is very clear! Maybe I've been thinking about this in reverse or something. hapi seems pretty close to the spec.

Thanks for your patience, anyway.

@hueniverse hueniverse added this to the 11.0.3 milestone Oct 26, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants