Make Cowboy CORS friendly #947
Comments
|
I've just made a pull request for this feature. I've also changed type descriptions a bit: -type cors_allowed_origins() :: [binary()] | binary().
-type cors_allowed_methods() :: [binary()].
-type cors_allowed_headers() :: [binary()].
-type cors_max_age() :: non_neg_integer() | max.
-type cors_header() :: {origins, cors_allowed_origins()}
| {exposed_headers, cors_allowed_headers()}
| {credentials, boolean()}.
-type cors_preflight_header() :: {origins, cors_allowed_origins()}
| {methods, cors_allowed_methods()}
| {headers, cors_allowed_headers()}
| {credentials, boolean()}
| {max_age, max | non_neg_integer()}. |
|
From the server-side perspective, we need to parse just "Origin", "Access-Control-Request-Method" and "Access-Control-Expose-Headers" headers. Other CORS headers are produced by the server. So, it looks like we should create functions: access_control_allow_credentials/1
access_control_allow_headers/1
access_control_allow_methods/1
access_control_allow_origin/1
access_control_expose_headers/1
access_control_max_age/1in the module % @todo -export([parse_access_control_allow_credentials/1]). CORS
% @todo -export([parse_access_control_allow_headers/1]). CORS
% @todo -export([parse_access_control_allow_methods/1]). CORS
% @todo -export([parse_access_control_allow_origin/1]). CORS
% @todo -export([parse_access_control_expose_headers/1]). CORS
% @todo -export([parse_access_control_max_age/1]). CORSIn this way, we would have all the parsing/producing headers code within the |
|
Cowlib is both for clients and servers. I understand your concerns are only for the server, but if you'd like to also contribute client parsing code, be my guest. :-) Though AFAIK this wouldn't usually be useful to plain HTTP clients so no worries there. Otherwise, the names sound correct, if they take Erlang representation and return an iodata() for the header value. |
|
My notes on CORS so far:
I will add more notes as I go through your PRs this week-end. |
|
I added a note in the Cowboy PR. I'm not convinced by the interface though. The functions are a little too magical to my taste. The way I see it, only the Origin matching is tricky, so we definitely need a function for that, otherwise cowboy_req:header/3 and cowboy_req:set_resp_header/3 are more than enough. I don't see why the match methods/headers are doing for example, this concern is strictly client-side, as far as the server is concerned it should just send what it allows and be done with it. And the rest of the code is just that, setting headers. So considering cowlib will be fully documented in 2.0, I don't think we need much more than Origin matching and REST support. |
|
As for the whole thing. Yes, those functions are a little magical to my taste too, and I like the idea of the deeper integration of CORS to the REST handler, but we also have the middleware layer, plain and loop handlers. And there we can reuse some build-in functions or copy-pasting some template code each time. I prefer the first. In any case we need something build-in. Having just |
|
I don't mean set_cors_origin_header, I mean none of it should be in cowboy_req. All we need is a way to go from Origin header + list of allowed origins to CORS/not CORS and with what origin. The rest can be done with set_resp_header. I think good documentation > magic functions. CORS enables many different cases, we can't think of them all, so the interface should keep it simple. We do have more than REST, but REST is the only place where we actually care about semantics, the other handlers/middlewares are more of a "write your own" so set_resp_header is fine. The spec says the list is without bounds but in practice people are going to whitelist a handful of headers at most (github has about 10 there for example) so it makes little sense to spend processing power on that, especially considering the browser can then cache for all headers in one go. |
|
We also need to validate that case with allowed credentials and Origin. So, the way is a little bit further: Origin header + list of allowed origins, validation of Credentials and Origin headers and the rest. |
|
We don't really need to validate that case. We only need to if we return "" from that function. I think we shouldn't. Either the service uses "" and doesn't need to call that function, or it doesn't use "*" and will call the function and get a specific Origin (or disable CORS). |
|
What shall we return to a user when we use "*" on the server and have got a value of origin header equals to "null" or it includes unknown to the server URI scheme? |
|
|
|
The
So, The value which a user will get back on the response is a different story. We could get it from request's header for the "*". I think, It's expected behaviour for most of user agents. The question is which one from the request's origin list we should use? The last one? |
|
I am talking about what we send in response. Not sure what most of your comment refers to. Basically we get a request in, and want to match the Origin header against the origins we allow. Two things can happen: either there is a match, or there isn't. The list of origins we allow is either a detailed list of origins The case of references is special and I'm not sure we really care about it? From what I understand it's basically for file:// or data:// or similar. We can either accept those or we don't. It's a special case though, so I have no problem leaving it aside and adding a separate match function with arity+1 that would accept those should it ever be needed. But until we have a use case, it's not worth spending too much time on it. |
|
My thoughts according that matching function.
It seems ok if we send "null" back to the user agent (for Chrome it works actually).
In this case we have an origins list from the request's origin header, depending on request type (preflight or simple), we can perform our matching against an allowed origins list differently.
So, in our matching function, we should match request's origin header and request's method against an allowed origins lists. It could be a good idea to pass a request object to it
I encounter of some strange behaviour of Firefox in a case when a server returns a redirect to another origin on the cross-origin request (I can reproduce it). Resource is accepted by the user agent only when the server responds with "*" as a value of "access-control-allow-origin" headers. Seems like a bug in Firefox. So, probably, we shouldn't care about it.
|
|
1- OK let me ask you a different question: can you trigger the sending of "null" by a browser in a reasonably common manner? If not, we should just drop "null" entirely. Otherwise it should be configurable. 2- Same question, can you get a browser to send two origins? I can't even find an example of this happening, and even the Mozilla doc says it's a single URI: https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Origin 1- 2- Specs are nice and all but all that matters is implementations. :-) 3- We should care and document it. But it shouldn't impact the matching algorithm. 4- First we need a match function, then see what users do with it and take it from there. One step at a time. |
|
2 - I've never seen it 1 - Yes, I made it yesterday For that I set up the following:
Next, I send an ajax request from the web page on "http://localhost:8000" to the server handler "http://localhost:8002/redirect". Chrome replaced the origin header to "null" before performing the redirect. I logged requests on those servers, so you can see what happened: fetch("http://localhost:8082/redirect", {method: 'GET', mode: 'cors'})
.then(resp => resp.text())
.then(data => console.log(data))
.catch(error => console.log(error));%% localhost:8082
=INFO REPORT==== 3-Apr-2016::20:05:24 ===
#{bindings => [],
body_length => 0,
has_body => false,
headers => #{<<"accept">> => <<"*/*">>,
<<"accept-encoding">> => <<"gzip, deflate, sdch">>,
<<"accept-language">> => <<"en-US,en;q=0.8">>,
<<"connection">> => <<"keep-alive">>,
<<"host">> => <<"localhost:8082">>,
<<"origin">> => <<"http://localhost:8000">>,
<<"referer">> => <<"http://localhost:8000/">>,
<<"user-agent">> => <<"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2698.0 Safari/537.36">>},
host => <<"localhost">>,
host_info => undefined,
method => <<"GET">>,
path => <<"/redirect">>,
path_info => undefined,
pid => <0.156.0>,
port => 8082,
qs => <<>>,
ref => http_listner,
resp_headers => #{<<"access-control-allow-origin">> => <<"http://localhost:8000">>},
scheme => <<"http">>,
streamid => 1,
version => 'HTTP/1.1'}
%% localhost:8083
=INFO REPORT==== 3-Apr-2016::20:05:24 ===
#{bindings => [],
body_length => 0,
has_body => false,
headers => #{<<"accept">> => <<"*/*">>,
<<"accept-encoding">> => <<"gzip, deflate, sdch">>,
<<"accept-language">> => <<"en-US,en;q=0.8">>,
<<"access-control-request-headers">> => <<"x-devtools-emulate-network-conditions-client-id">>,
<<"access-control-request-method">> => <<"GET">>,
<<"connection">> => <<"keep-alive">>,
<<"host">> => <<"localhost:8083">>,
<<"origin">> => <<"null">>,
<<"referer">> => <<"http://localhost:8000/">>,
<<"user-agent">> => <<"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2698.0 Safari/537.36">>},
host => <<"localhost">>,
host_info => undefined,
method => <<"OPTIONS">>,
path => <<"/ping">>,
path_info => undefined,
pid => <0.156.0>,
port => 8083,
qs => <<>>,
ref => http_listner,
resp_headers => #{<<"access-control-allow-headers">> => <<"X-DevTools-Emulate-Network-Conditions-Client-Id">>,
<<"access-control-allow-methods">> => <<"GET">>,
<<"access-control-allow-origin">> => <<"null">>,
<<"access-control-max-age">> => <<"0">>},
scheme => <<"http">>,
streamid => 1,
version => 'HTTP/1.1'}
=INFO REPORT==== 3-Apr-2016::20:05:24 ===
#{bindings => [],
body_length => 0,
has_body => false,
headers => #{<<"accept">> => <<"*/*">>,
<<"accept-encoding">> => <<"gzip, deflate, sdch">>,
<<"accept-language">> => <<"en-US,en;q=0.8">>,
<<"connection">> => <<"keep-alive">>,
<<"host">> => <<"localhost:8083">>,
<<"origin">> => <<"null">>,
<<"referer">> => <<"http://localhost:8000/">>,
<<"user-agent">> => <<"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2698.0 Safari/537.36">>},
host => <<"localhost">>,
host_info => undefined,
method => <<"GET">>,
path => <<"/ping">>,
path_info => undefined,
pid => <0.156.0>,
port => 8083,
qs => <<>>,
ref => http_listner,
resp_headers => #{<<"access-control-allow-origin">> => <<"null">>},
scheme => <<"http">>,
streamid => 2,
version => 'HTTP/1.1'} |
|
2- Then let's drop it and come back to it only when it's observed in the wild. 1- Interesting. One thing for sure: it is entirely dependent on the service. Basically do you accept other services to redirect requests to yours. And I think the default should be to reject that behavior. But it should be configurable. Maybe we can just use "null" for this indeed, but that means the matching must be a little special ("null" must match references), whereas the other matching is exact matches. I'm OK with something like this. |
|
An insightful tidbit of information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Origin |
|
Could be our function: -spec match_origin([{binary(), binary(), 0..65535} | reference()] | '*', cowboy_req:req()) -> {ok, binary()} | error.
match_origin(AllowedOrigins, #{headers := #{<<"origin">> := HeaderVal}}) ->
%% NOTE: we assume we always deal with single origin
[Val] = cow_http_hd:parse_origin(HeaderVal),
case {Val, AllowedOrigins} of
{Val, '*'} when is_reference(Val) -> {ok, HeaderVal};
{Val, '*'} -> {ok, HeaderVal};
{Val, Val} -> {ok, HeaderVal};
{Val, L} when is_list(L) -> case lists:member(Val, L) of true -> {ok, HeaderVal}; _ -> error end;
_ -> error
end;
match_origin(_, _) ->
error.How we can use it in a middleware: -spec execute(Req, Env) -> {ok | stop, Req, Env} when Req :: cowboy_req:req(), Env :: any().
execute(Req, Env) ->
%%AllowedOrigins = '*',
AllowedOrigins =
[ {<<"http">>, <<"localhost">>, 9000},
{<<"http">>, <<"example.org">>, 80} ],
case match_origin(AllowedOrigins, Req) of
{ok, Origin} -> {ok, handle_cors_request(Origin, Req), Env};
error -> {ok, Req, Env}
end.
-spec handle_cors_request(binary(), cowboy_req:req()) -> cowboy_req:req().
handle_cors_request(Origin, #{method := Method} = Req) ->
Req2 = cowboy_req:set_resp_header(<<"access-control-allow-origin">>, Origin, Req),
Req3 = cowboy_req:set_resp_header(<<"vary">>, <<"Origin">>, Req2),
case Method of
<<"OPTIONS">> ->
Req4 = cowboy_req:set_resp_header(<<"access-control-allow-methods">>, <<"GET,PUT">>, Req3),
Req5 = cowboy_req:set_resp_header(<<"access-control-allow-headers">>, <<"X-DevTools-Emulate-Network-Conditions-Client-Id">>, Req4),
Req6 = cowboy_req:set_resp_header(<<"access-control-max-age">>, <<"0">>, Req5),
Req6;
_ ->
Req3
end. |
|
Or we could move the matching logic to the -spec match_origin(Origin, [Origin] | '*') -> boolean() when Origin :: {binary(), binary(), 0..65535} | reference().
match_origin(Val, '*') when is_reference(Val) -> true;
match_origin(_, '*') -> true;
match_origin(Val, Val) -> true;
match_origin(Val, L) when is_list(L) -> lists:member(Val, L);
match_origin(_, _) -> false.Middleware: -spec execute(Req, Env) -> {ok | stop, Req, Env} when Req :: cowboy_req:req(), Env :: any().
execute(#{headers := #{<<"origin">> := HeaderVal}} = Req, Env) ->
%%AllowedOrigins = '*',
AllowedOrigins =
[ {<<"http">>, <<"localhost">>, 9000},
{<<"http">>, <<"example.org">>, 80} ],
%% NOTE: we assume we always deal with single origin
[Val] = cow_http_hd:parse_origin(HeaderVal),
case cow_http_hd:match_origin(Val, AllowedOrigins) of
true -> {ok, handle_cors_request(HeaderVal, Req), Env};
_ -> {ok, Req, Env}
end;
execute(Req, Env) ->
{ok, Req, Env}. |
|
@essen, what do you think about merging CORS related parsing/producing functions for cowlib? |
|
Yep will do. Almost done fixing a breaking bug, so probably after that. |
|
@essen, what do you also think about putting the |
|
In theory I have nothing against it but right now I've been away from CORS too long so I don't want to make too hasty decisions. I think I will want to have an rfc6454 test suite in Cowboy before deciding on where things should go. And that is largely dependent on adding CORS support to cowboy_rest. |
Maybe we could make Cowboy a bit more CORS friendly?
By now, to enable CORS we should work with raw headers, copy-pasting code like this from one header to another:
In this way we do not actually care about allowed header's value types. For instance, "access-control-allow-methods" and "access-control-allow-headers" should only accept a list of binaries, while only a numeric value makes sense for the "access-control-max-age" header, "*" origin cannot be used for a resource that supports credentials. Ignoring this information leads to misspelling errors, breaking CORS responses and isn't flexible for working with.
My proposal is to add two new functions (
set_cors_headers/2andset_cors_preflight_headers/2) to thecowboy_reqmodule extending functionality ofset_resp_header/3.In my projects, I'm using this simple syntax sugar in REST handlers:
It's also possible to use those functions in a middleware:
I believe those functions could make Cowboy yet more friendly and useful tool and would love to make a pull request, if it's ok with Cowboy's philosophy.
The text was updated successfully, but these errors were encountered: