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

Document the CORS/preflight headers #1365

Merged
merged 6 commits into from
Jul 10, 2018
Merged

Conversation

turt2live
Copy link
Member

Fixes #1006

This PR documents existing behaviour and does not attempt to improve or change the situation.

Reference: https://github.com/matrix-org/synapse/blob/53969e196004659c6a9f138f5d8abd86f4957d74/synapse/http/server.py#L455-L462

Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

lgtm other than a couple minor things


It is realistic to expect that some clients will be written to be run within a
web browser or similar environment. In these cases, the homeserver should respond
to pre-flight requests and supply Cross-Origin Resource Sharing (CORS) headers.
Copy link
Member

Choose a reason for hiding this comment

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

maybe "... supply Cross-Origin Resource Sharing (CORS) headers on all requests." (add "on all requests" at the end.)

to pre-flight requests and supply Cross-Origin Resource Sharing (CORS) headers.

When a client approaches the server with a pre-flight (``OPTIONS``) request, the
server should respond with the CORS headers for that route. If the route does not
Copy link
Member

Choose a reason for hiding this comment

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

s/should/must/ ? (and also on the next line)

Copy link
Member Author

Choose a reason for hiding this comment

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

"must" makes sense for the first one. The second one actually seems to have a whole different story to it.

Abbreviated console output:

$ curl -sv -X OPTIONS https://matrix.org/_matrix/client/r0/this/does/not/exist
< HTTP/2 200
< date: Wed, 04 Jul 2018 20:23:25 GMT
< content-type: application/json
< access-control-allow-headers: Origin, X-Requested-With, Content-Type, Accept, Authorization
< access-control-allow-origin: *
< access-control-allow-methods: GET, POST, PUT, DELETE, OPTIONS

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about specing it as 200 OK for OPTIONS? (the alternative being leaving it undefined and putting that part through the proposal process, imo)

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 that we can leave it undefined for now, and, looking at this again, we might also want to leave the exact headers undefined as well, as some servers may want to be more or less strict about some of them. e.g. some servers may want to set Access-Control-Allow-Methods to only return the methods that actually exist on that route. Maybe just suggest that servers should set Access-Control-Allow-Headers to allow for Origin, X-Requested-With, Content-Type, Accept, Authorization.

@turt2live
Copy link
Member Author

@uhoreg This should be updated now. Please take another look.

all requests.

When a client approaches the server with a pre-flight (``OPTIONS``) request, the
server should respond with the CORS headers for that route. The recommended CORS
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally prefer "An example of CORS headers..." rather than "The recommended CORS headers...", but I'm also fine with it just going in like this.

@turt2live turt2live merged commit c79010f into matrix-org:master Jul 10, 2018
@turt2live turt2live deleted the travis/cors branch July 10, 2018 14:35
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

Successfully merging this pull request may close these issues.

2 participants