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

Conversation

Projects
None yet
2 participants
@turt2live
Member

turt2live commented Jul 3, 2018

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

turt2live added some commits Jul 3, 2018

@uhoreg

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.

This comment has been minimized.

@uhoreg

uhoreg Jul 4, 2018

Member

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

This comment has been minimized.

@uhoreg

uhoreg Jul 4, 2018

Member

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

This comment has been minimized.

@turt2live

turt2live Jul 4, 2018

Member

"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

This comment has been minimized.

@turt2live

turt2live Jul 4, 2018

Member

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)

This comment has been minimized.

@uhoreg

uhoreg Jul 9, 2018

Member

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

This comment has been minimized.

Member

turt2live commented Jul 9, 2018

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

@uhoreg

uhoreg approved these changes Jul 10, 2018

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

This comment has been minimized.

@uhoreg

uhoreg Jul 10, 2018

Member

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turt2live turt2live deleted the turt2live:travis/cors branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment