Optionally check origin for WebSockets connection

[buybackoff]

This is a very small addition which is convenient to do via config. A relevant question on SO.

[buybackoff]

Also, running the CORS example from Chrome at http://localhost:8083/hello with

let corsConfig = { defaultCORSConfig with allowedUris = InclusiveOption.Some [ "http://localhost:8085" ] }

let app =
    choose [ 
        GET >=> path "/hello" >=> cors corsConfig >=> OK "CORS request accepted."
    ] >=> log logger logFormat

doesn't fail in any way and returns "CORS request accepted.". Is this normal and localhost has special rules?

[ademar]

Hi,

Sorry, I do not understand the logic in this patch ...

If you insert the CORS combinator, origin will be checked regardless of the connection being websocket or not .. So I think it already does what you want without any changes ..

Furthermore, regarding your second question:

Is this normal and localhost has special rules?

In your example corsConfig is listing localhost in allowedUris so the request will be accepted.

[buybackoff]

@ademar just simply try to apply CORS to WebSockets now as is... It doesn't work and the linked SO question tells why. As for the second thing - the ports are different. Currently in Suave source the origin is compared with the white list as entered (even the trailing / matters). Sorry, but I tested the behavior and didn't write the patch just out of the blue.

[haf]

Need a lens to go with it

[buybackoff]

What is a lens and where is an example how to add it?

[haf]

Just below, all the static methods

[haf]

doesn't fail in any way and returns "CORS request accepted.". Is this normal and localhost has special rules?

As far as I understand it's up to the client to fail the request in that case, not the server.

[haf]

It doesn't work and the linked SO question tells why.

The question says that the server+browser needs to validate input (if it wants). Nothing broken, just nothing done with regards to that. You want to add input validation, right?

Are you sure it's a concern of CORS to do that input validation? Perhaps validating websockets connection details should be part of the websocket module?

Could you please write a unit test targeting this case? That helps to document your intention.

[buybackoff]

With CORS module, it is just one-liner config, and we are applying the same logic here as CORS. It will be nice to keep WebSocket logic clear and separated, instead of doing validation each time manually. I think this additional config option is not intrusive and zero cost when not used. Otherwise, handShake would require an additional parameter or echo or other business logic would be polluted with stuff that is a part of security and not business logic.

[haf]

Perhaps an authority like @mnot could tell us what the interaction between WebSockets and CORS is supposed to be?

[ademar]

I think there is no need for this. If you do not want CORS applied to the websocker connection just don't place the combinator in front of the websocket handler. Am I missing something ?

[buybackoff]

@ademar CORS is never applied to WebSockets by specification. This patch just enables this as if CORS originally worked with WebSockets. If you put CORS in fron of WebSockets now, it will be ignored.

[ademar]

If you put CORS in fron of WebSockets now, it will be ignored.

But just don't put it there. Putting CORS in front of the WebSocket handler would indicate that you DO WANT for origin to be check.

At least that is the way I read it.

There are couple of issues with your proposition. It multiply entities beyond necessity and it is cross-cutting concerns.

I do not like a CORS combinator that gets applied or not depending on some configuration parameter. It would be confusing for the reader.

[buybackoff]

It will be confusing if someone puts CORS in front of WebSockets expecting that it will be honored (and not knowing that by specification it is ignored, as I did initially). Feel free to close this PR. After additional evaluation and performance/allocations profiling I won't be able to use this project - it would require too many changes, which I guess by this PR will never be merged.

[mnot]

See whatwg/fetch#235 and https://fetch.spec.whatwg.org/#main-fetch