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

OPTIONS does not actually handle CORS pre-flight requests #3027

Open
laurenceisla opened this issue Oct 27, 2023 · 4 comments · May be fixed by #3052
Open

OPTIONS does not actually handle CORS pre-flight requests #3027

laurenceisla opened this issue Oct 27, 2023 · 4 comments · May be fixed by #3052
Assignees
Labels
bug http http compliance

Comments

@laurenceisla
Copy link
Member

laurenceisla commented Oct 27, 2023

Problem

While adding Origin validation for CORS requests, we found out that our OPTIONS implementation does not actually handle pre-flight requests, at least not completely #2986 (comment).

This is how it works right now:

Successful request

$ curl -X OPTIONS "http://localhost:3000/projects" \
        -H "Access-Control-Request-Method: POST" \
        -H "Access-Control-Request-Headers: Content-Type" \
        -H "Origin: http://localhost" -i
HTTP/1.1 200 OK
Transfer-Encoding: chunked
Date: Fri, 27 Oct 2023 19:05:19 GMT
Server: postgrest/11.2.0 (8c9213d)
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, PATCH, PUT, OPTIONS, HEAD, POST
Access-Control-Allow-Headers: Authorization, Content-Type, Accept, Accept-Language, Content-Language
Access-Control-Max-Age: 86400

The actual response is given by the wai-cors library. We can notice because our implementation does not return pre-flight headers, only Access-Control-Allow-Origin. In summary, our OPTIONS does nothing here, the library does the response.

Failing request

$ curl -X OPTIONS "http://localhost:3000/projects" \
        -H "Access-Control-Request-Method: TRACE" \
        -H "Access-Control-Request-Headers: Content-Type" \
        -H "Origin: http://localhost" -i
HTTP/1.1 200 OK
Transfer-Encoding: chunked
Date: Fri, 27 Oct 2023 19:16:15 GMT
Server: postgrest/11.2.0 (8c9213d)
Access-Control-Allow-Origin: *
Allow: OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE

Only when it fails, our OPTIONS implementation goes through and returns this info. This happens because we set the library to not respond with an error and let the request continue:

, Wai.corsIgnoreFailures = True

This still fails the pre-flight (in Firefox and Chrome at least) because Access-Control-Allow-Methods is not set (different from Allow which has no meaning in CORS pre-flight).

Solution

We have two options here:

  • Let the library return the error which is a 400 with a predefined message indicating the problem.
  • Implement our own, which would mean to verify if the OPTIONS CORS request is pre-flight (with the required headers) and respond with a custom error indicating the problem.

Any of these would liberate the simple OPTIONS CORS request (not preflight) to work as we want. For instance, it could bring back the schema definition mentioned in #790.

@laurenceisla laurenceisla changed the title OPTIONS does not actually handle CORS pre-flight requests completely OPTIONS does not actually handle CORS pre-flight requests Oct 27, 2023
@taimoorzaeem taimoorzaeem added the http http compliance label Oct 29, 2023
@wolfgangwalther
Copy link
Member

Is there any downside to letting the library handle this? Is there any upside to rolling our own?

@laurenceisla
Copy link
Member Author

laurenceisla commented Nov 10, 2023

The pros are that the library returns the errors nicely, with a message like "Origin header is missing" or
"Unsupported origin..." using:

corsFailure msg = WAI.responseLBS HTTP.status400 [("Content-Type", "text/html; charset-utf-8")] (LB8.fromStrict msg)

The con is that it does not follow our "code, message, details, hint" structure for errors with application/json. An exception could be made since this is relevant mostly (only?) for browsers... or, if possible, we could intercept the library response and adapt it to our structure.

@wolfgangwalther
Copy link
Member

I think it would be fine to let the library handle this entirely, since we don't really expect this error to be seen by any api user ever.

@laurenceisla laurenceisla self-assigned this Nov 16, 2023
@laurenceisla laurenceisla linked a pull request Nov 16, 2023 that will close this issue
@laurenceisla
Copy link
Member Author

Found a problem with the library: it expects that any OPTIONS request that has an Origin header should be considered a pre-flight request and it complains when it doesn't have an Access-Control-Request-Method header (IMO, it shouldn't be that way: that's a valid CORS request not a pre-flight).

Solution is to add an Origin validation just for that case. I'll continue in PR #3052.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug http http compliance
Development

Successfully merging a pull request may close this issue.

3 participants