You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2016/02/10 00:34:02 UTC
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-chttpd/pull/101
2945 remove couch http cors
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-chttpd 2945-remove-couch_http_cors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-chttpd/pull/101.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #101
----
commit 2c9a88958801d5213efe20f0fb40f50b74c8b1fb
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2016-02-09T21:22:05Z
Introduce vhosts configuration into CORS
In order to remove code duplication we move vhosts support
from couch_http_cors into chttpd_cors. We also dispatch
chttpd:send_response to couch_http which does call chttpd_cors:headers.
In order to avoid double injection of CORS headers we check for existance of
"Access-Control-Allow-Origin" in response headers.
COUCHDB-2945
commit 3e0255700715e6ce6a9bf5c099f2472c481f2cb7
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2016-02-09T22:41:22Z
Treat value passed in Origin as case sensitive
According to CORS spec here https://www.w3.org/TR/cors
The value of the Origin header is not a case-sensitive
COUCHDB-2945
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-190932361
Can we combine the `send_response/4` and `send_response_no_cors/4` functions, or do we depend on any of the initial headers and/or the ordering in which additional headers are added?
For instance:
```
send_cors_response(Req, Code, Headers0, Body) ->
Headers = chttpd_cors:headers(req, Headers0),
send_response(Req, Code, Headers, Body).
send_response(#httpd{mochi_req=MochiReq} = Req, Code, Headers0, Body) ->
%% send response as normal here without adding CORS headers
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54460434
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,22 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
--- End diff --
Extra newline needed here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53058948
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
Just a note: `couch_httpd_vhost:host/1` returns value of either `X-Forward-Host` (as setup by apache's mod_proxy, for example) or `Host`. Neither include scheme, not when setup by browser, at least, so `HostAsString` is actually plain host:port string. I imagine CORS config setting as noted in default.ini right now just doesn't work, the working config expects [cors:Host] format.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53026567
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
It was taken from [here](https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd_cors.erl#L308) verbatim.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53026146
--- Diff: src/chttpd_cors.erl ---
@@ -360,7 +399,7 @@ get_origin(Req) ->
undefined ->
undefined;
Origin ->
- ?l2b(to_lower(Origin))
+ ?l2b(Origin)
--- End diff --
Hmm not sure then. It contradicts [6.1 point 2](https://www.w3.org/TR/cors/#origin-request-header) where spec says:
> If the value of the Origin header is not a case-sensitive match for any of the values in list of origins, do not set any additional headers and terminate this set of steps.
I'll do some research on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53011410
--- Diff: src/chttpd_cors.erl ---
@@ -360,7 +399,7 @@ get_origin(Req) ->
undefined ->
undefined;
Origin ->
- ?l2b(to_lower(Origin))
+ ?l2b(Origin)
--- End diff --
Actually CORS specs [point](https://www.w3.org/TR/cors/#origin-request-header) to "The Web Origin Concept" RFC and hence require host to be ["converted to lowercase using the ascii-casemap collation"](https://tools.ietf.org/html/rfc6454#section-4) (subsection 5). So I guess we still need to_lower here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53046685
--- Diff: src/chttpd_cors.erl ---
@@ -360,7 +399,7 @@ get_origin(Req) ->
undefined ->
undefined;
Origin ->
- ?l2b(to_lower(Origin))
+ ?l2b(Origin)
--- End diff --
> [If the value of the Origin header is not a case-sensitive match for any of the values in list of origins do not set any additional headers and terminate this set of steps.](https://www.w3.org/TR/cors/#resource-requests)
Ok, I agree here on removing to_lower.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-184918961
@eiri: Could you do a review of one more [extra commit](https://github.com/cloudant/couchdb-chttpd/commit/945ad297b8496ce4de8f9407ad378c887068d89c)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54643770
--- Diff: src/chttpd.erl ---
@@ -689,12 +689,9 @@ send_chunk(Resp, Data) ->
Resp:write_chunk(Data),
{ok, Resp}.
-send_response(#httpd{mochi_req=MochiReq}=Req, Code, Headers0, Body) ->
- couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
- Headers = Headers0 ++ server_header() ++
- [timing(), reqid() | couch_httpd_auth:cookie_auth_header(Req, Headers0)],
- {ok, MochiReq:respond({Code, Headers, Body})}.
-
+send_response(#httpd{} = Req, Code, Headers0, Body) ->
+ Headers1 = [timing(), reqid() | Headers0],
+ couch_httpd:send_response(Req, Code, Headers1, Body).
--- End diff --
@chewbranca: I did rolled back this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53064076
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
The function would work, the config wouldn't. In real life value for header `Host` set to [target's host](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23), not URI, so cors config would be expected to be under `[cors:partner.com]` where partner.com is the name of our vhost and `http://restricted.dev:8000`is the address of the server that hosts javascript that does ajax call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53058362
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
It turned out `Host` could be "" or doesn't contain schema.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53008747
--- Diff: src/chttpd.erl ---
@@ -690,11 +690,8 @@ send_chunk(Resp, Data) ->
{ok, Resp}.
send_response(#httpd{mochi_req=MochiReq}=Req, Code, Headers0, Body) ->
--- End diff --
We now getting unused warning for `MochiReq` here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53045399
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
Then I imagine the following should do the same.
```
{Scheme, HostPort, _, _, _} = mochiweb_util:urlsplit(Host),
lists:flatten(io_lib:format("cors:~s://~s", [Scheme, hd(string:tokens(HostPort, ":"))])).
```
But as I said, I'm fine with moving it here verbatim.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53058131
--- Diff: src/chttpd_cors.erl ---
@@ -272,28 +279,31 @@ allow_credentials(Config, Origin) ->
get_origin_config(Config, Origin, <<"allow_credentials">>,
?CORS_DEFAULT_ALLOW_CREDENTIALS).
-get_cors_config(#httpd{cors_config = undefined}) ->
+get_cors_config(#httpd{cors_config = undefined, mochi_req = MochiReq}) ->
+ Host = couch_httpd_vhost:host(MochiReq),
+
EnableCors = config:get("httpd", "enable_cors", "false") =:= "true",
AllowCredentials = config:get("cors", "credentials", "false") =:= "true",
--- End diff --
This attribute should go under vhost's cors section as well, otherwise working config looks strangly splitted
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54460516
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
+ false ->
+ case get_origin(Req) of
+ undefined ->
+ %% If the Origin header is not present terminate
+ %% this set of steps. The request is outside the scope
+ %% of this specification.
+ %% http://www.w3.org/TR/cors/#resource-processing-model
+ RequestHeaders;
+ Origin ->
+ headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ end;
+ true ->
+ RequestHeaders
end.
--- End diff --
Same thing with newlines here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53031140
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
> Speaking of, vhost's cors config in default.ini mentions that scheme must to be included, are we dropping this req?
We use [string:rchr/2](https://github.com/apache/couchdb-chttpd/pull/101/files#diff-67787f983e20e7ee8910d35365d48a42R329) to find out the position of right most occurrence of `:`. Then we split at that position preserving the schema.
This actually answers your previous question as well:
> I don't get it, why not just string:tokens/2 here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-185262966
@eiri: Thank you for the review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53009242
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
+ false ->
+ case get_origin(Req) of
+ undefined ->
+ %% If the Origin header is not present terminate
+ %% this set of steps. The request is outside the scope
+ %% of this specification.
+ %% http://www.w3.org/TR/cors/#resource-processing-model
+ RequestHeaders;
+ Origin ->
+ headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ end;
+ true ->
+ RequestHeaders
end.
+is_preflight_response(Headers) ->
+ lists:keymember("Access-Control-Allow-Origin", 1, Headers).
--- End diff --
Not sure, but does mochi autocapitalize headers? Otherwise we destine to throw false negative for the clients passing a low case headers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54460411
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,22 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
--- End diff --
This should be two newline spacing like the rest of the module.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54459627
--- Diff: src/chttpd.erl ---
@@ -689,12 +689,9 @@ send_chunk(Resp, Data) ->
Resp:write_chunk(Data),
{ok, Resp}.
-send_response(#httpd{mochi_req=MochiReq}=Req, Code, Headers0, Body) ->
- couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
- Headers = Headers0 ++ server_header() ++
- [timing(), reqid() | couch_httpd_auth:cookie_auth_header(Req, Headers0)],
- {ok, MochiReq:respond({Code, Headers, Body})}.
-
+send_response(#httpd{} = Req, Code, Headers0, Body) ->
+ Headers1 = [timing(), reqid() | Headers0],
+ couch_httpd:send_response(Req, Code, Headers1, Body).
--- End diff --
Switching this to send the response the `couch_httpd:send_response/4` [1] seems a bit odd to me, or at the very least backwards. But either way, switching this changes the underlying semantics in a few ways that we should be clear we want. As you can see in [1] there is a fair bit of things going on, that we may or may not want.
First, in [2] we log the request through couch_httpd, but we're also logging the request in chttpd [3], so it seems like we'll be getting double log items for requests.
Second, we'll be adding in the `http_1_0_keep_alive/2` hack [4]. I would like to hear from @davisp @rnewson (or anyone else familiar with the nuances here) as to whether reintroducing this to chttpd is a good idea.
Third, we're introducing an additional layer of logging that logs the body of requests in `error` for 500s, and `debug` for 400s. Is this a change we explicitly want? IMO it would be handy to have the response bodies logged for 500s, but let's be explicit about deciding to add that in to requests through chttpd.
[1] https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd.erl#L723-L737
[2] https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd.erl#L724
[3] https://github.com/cloudant/couchdb-chttpd/blob/2945-remove-couch_http_cors/src/chttpd.erl#L358-L377
[4] https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd.erl#L726
[5] https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd.erl#L727-L732
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-191479781
@iilyak So I like the approach in d1d2cfe, however while taking another gander at this PR I realized that the heavily used `chttpd:send_json` still calls out to `couch_httpd:send_json` which in turn calls out to `couch_httpd:send_response`, so unfortunately we're still returning and logging different in chttpd vs couch_httpd.
Perhaps the best approach is to do what you originally had and have everything use `couch_httpd:send_response` and then just ensure we're happy with the current implementation there. Although we need to be careful because the delayed chunk logic in `chttpd` has bug fixes not in `couch_httpd`, so we can't always just use `couch_httpd`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-191245049
@chewbranca: We would want to include CORS headers by default and do not include them only for one specific case when we do a call from `chttpd_cors` itself. If we introduce `send_cors_response` we would need to update all of the dependencies to use it. Also we would need to communicate to all developers to stop using `send_response` in new work and use `send_cors_response` instead.
I'm planing to refactor all `send_response*` functions right after this PR is merged in order to implement `before_response` EPI hook. The initial version of which is [here](https://github.com/apache/couchdb-chttpd/pull/99/files).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53065272
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
I agree with you @eiri.
> @iilyak: So in cases when schema is not present it will not find the right section.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-184835507
All right, I'm +1 after allowing `credentials` to be configured on `cors:Host` level.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53029449
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
+ false ->
+ case get_origin(Req) of
+ undefined ->
+ %% If the Origin header is not present terminate
+ %% this set of steps. The request is outside the scope
+ %% of this specification.
+ %% http://www.w3.org/TR/cors/#resource-processing-model
+ RequestHeaders;
+ Origin ->
+ headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ end;
+ true ->
+ RequestHeaders
end.
+is_preflight_response(Headers) ->
+ lists:keymember("Access-Control-Allow-Origin", 1, Headers).
--- End diff --
This check is for our responses. We set `Access-Control-Allow-Origin` [here](https://github.com/cloudant/couchdb-chttpd/blob/2945-remove-couch_http_cors/src/chttpd_cors.erl#L260). This check is needed because we began calling chttpd_cors:headers from couch_htppd:send_response [here](https://github.com/apache/couchdb-couch/pull/138/files#diff-667bf0ed30e8a6b8ef281fa934cb9c06R735). Without this check we would be injecting headers from [here](https://github.com/cloudant/couchdb-chttpd/blob/2945-remove-couch_http_cors/src/chttpd_cors.erl#L41).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54574974
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
--- End diff --
@chewbranca: [see [earlier](https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53029449)]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53062299
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
Existent `split_host_port` function would work even in the absence of schema. [The "`Origins must include the scheme: http://example.com`" comment](https://github.com/cloudant/couchdb/blob/master/rel/overlay/etc/default.ini#L115) comment is related to `origins` key within `[cors:http://example.com]` section. Below is the config I used for testing:
```
[chttpd]
enable_cors = true
[cors]
credentials = true
methods = GET,HEAD,POST,PUT,DELETE,OPTIONS
headers = accept,authorization,content-type
[cors:http://partner.com]
methods = GET,DELETE
headers = Custom_header1,custom_header2,accept,authorization,content-type,x-couchdb-csrf
exposed_headers = x-couchdb-Secret
origins = http://restricted.dev:8000
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54460184
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
--- End diff --
Why do we need the extra level of nesting checking for `is_preflight_response/1`? It's not clear to me that we can reach a scenario where `Access-Control-Allow-Origin` header has been set but the Origin header is absent. Is this addressing a missed edge case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53033306
--- Diff: src/chttpd_cors.erl ---
@@ -360,7 +399,7 @@ get_origin(Req) ->
undefined ->
undefined;
Origin ->
- ?l2b(to_lower(Origin))
+ ?l2b(Origin)
--- End diff --
From [here](https://msdn.microsoft.com/en-us/library/azure/dn535601.aspx)
> AllowedOrigins: The origin domains that are permitted to make a request against the storage service via CORS. The origin domain is the domain from which the request originates. Note that the origin must be an exact case-sensitive match with the origin that the user age sends to the service.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53029016
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
@iilyak that's some groovy stuff... ok, let's keep it here as it was there, to not to increase complexity, and leave the cleaning for a next round.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53015557
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
I don't get it, why not just `string:tokens/2` here? Or after `mochiweb_util:urlsplit/1` if `couch_httpd_vhost:host/1` is returning host with scheme.
Speaking of, vhost's cors config in default.ini mentions that scheme must to be included, are we dropping this req?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54643915
--- Diff: src/chttpd_cors.erl ---
@@ -159,17 +159,24 @@ handle_preflight_request(Req, Config, Origin) ->
headers(Req, RequestHeaders) ->
- case get_origin(Req) of
- undefined ->
- %% If the Origin header is not present terminate
- %% this set of steps. The request is outside the scope
- %% of this specification.
- %% http://www.w3.org/TR/cors/#resource-processing-model
- RequestHeaders;
- Origin ->
- headers(Req, RequestHeaders, Origin, get_cors_config(Req))
+ case is_preflight_response(RequestHeaders) of
--- End diff --
@chewbranca: `is_preflight_response` is removed in favor of explicit `chttpd:send_response_no_cors/4` function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-184921217
@iilyak Much cleaner. Let's remove `split_host_port/1` and `split_host_port/2` as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r54460460
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,22 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(HostValue) ->
+ HostPort = maybe_strip_scheme(HostValue),
+ Host = hd(string:tokens(HostPort, ":")),
+ "cors:" ++ Host.
+
+
+maybe_strip_scheme(Host) ->
+ case string:str(Host, "://") of
+ 0 -> Host;
+ N -> string:substr(Host, N + 3)
+ end.
+
--- End diff --
Extra newline needed here as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-chttpd/pull/101
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#discussion_r53063562
--- Diff: src/chttpd_cors.erl ---
@@ -306,6 +316,35 @@ get_cors_config(#httpd{cors_config = undefined}) ->
get_cors_config(#httpd{cors_config = Config}) ->
Config.
+cors_config(Host, Key, Default) ->
+ config:get(cors_section(Host), Key,
+ config:get("cors", Key, Default)).
+
+cors_section(Host0) ->
+ {Host, _Port} = split_host_port(Host0),
--- End diff --
However I also did set `X-Forwarded-Host:'http://partner.com'` in my tests. So in cases when schema is not present it will not find the right section.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2945 remove couch http cors
Posted by chewbranca <gi...@git.apache.org>.
Github user chewbranca commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/101#issuecomment-191924687
+1
I was concerned about the `http_1_0_keep_alive` logic getting added in unexpectedly, but seeing as we already include that in a large amount of requests I think we're fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---