-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add X-Frame-Options #582
Add X-Frame-Options #582
Changes from 6 commits
2f85a09
ab56891
a64060c
b4807fe
3d1755e
976bcd3
310289e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,7 @@ allow_jsonp = false | |
;socket_options = [{recbuf, 262144}, {sndbuf, 262144}, {nodelay, true}] | ||
socket_options = [{recbuf, 262144}, {sndbuf, 262144}] | ||
enable_cors = false | ||
enable_xframe_options = true | ||
; CouchDB can optionally enforce a maximum uri length; | ||
; max_uri_length = 8000 | ||
; changes_timeout = 60000 | ||
|
@@ -187,6 +188,14 @@ credentials = false | |
; List of accepted methods | ||
; methods = | ||
|
||
[x_frame_options] | ||
; Settings same-origin will return X-Frame-Options: SAMEORIGIN. | ||
; If same origin is set, it will ignore the hosts setting | ||
; same_origin = true | ||
; Settings hosts will return X-Frame-Options: ALLOW-FROM https://example.com/ | ||
; List of hosts separated by a comma. * means accept all | ||
; hosts = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the consequences of having typos here besides the embedder not being able to put the site in frames and complaining to us? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its the same as for cors or anything else. The typo's will give incorrect results and mean that they might have a situation where a page can be or cannot be embedded |
||
|
||
[couch_httpd_oauth] | ||
; If set to 'true', oauth token and consumer secrets will be looked up | ||
; in the authentication database (_users). These secrets are stored in | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
% Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
% use this file except in compliance with the License. You may obtain a copy of | ||
% the License at | ||
% | ||
% http://www.apache.org/licenses/LICENSE-2.0 | ||
% | ||
% Unless required by applicable law or agreed to in writing, software | ||
% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
% License for the specific language governing permissions and limitations under | ||
% the License. | ||
|
||
-module(chttpd_xframe_options). | ||
|
||
-export([ | ||
header/2, | ||
header/3 | ||
]). | ||
|
||
-define(DENY, "DENY"). | ||
|
||
|
||
-include_lib("couch/include/couch_db.hrl"). | ||
|
||
% X-Frame-Options protects against clickjacking by limiting whether a response can be used in a | ||
% <frame>, <iframe> or <object>. | ||
|
||
header(Req, Headers) -> | ||
header(Req, Headers, get_xframe_config(Req)). | ||
|
||
|
||
|
||
header(Req, Headers, Config) -> | ||
case lists:keyfind(enabled, 1, Config) of | ||
{enabled, true} -> | ||
generate_xframe_header(Req, Headers, Config); | ||
_ -> | ||
Headers | ||
end. | ||
|
||
|
||
|
||
generate_xframe_header(Req, Headers, Config) -> | ||
XframeOption = case lists:keyfind(same_origin, 1, Config) of | ||
{same_origin, true} -> | ||
"SAMEORIGIN"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any particular reason you have a macro for "DENY" and not using one here for "SAMEORIGIN"? |
||
_ -> | ||
check_host(Req, Config) | ||
end, | ||
[{"X-Frame-Options", XframeOption } | Headers]. | ||
|
||
|
||
|
||
check_host(#httpd{mochi_req = MochiReq} = Req, Config) -> | ||
Host = couch_httpd_vhost:host(MochiReq), | ||
case Host of | ||
[] -> | ||
?DENY; | ||
Host -> | ||
FullHost = chttpd:absolute_uri(Req, ""), | ||
AcceptedHosts = get_accepted_hosts(Config), | ||
AcceptAll = ["*"] =:= AcceptedHosts, | ||
case AcceptAll orelse lists:member(FullHost, AcceptedHosts) of | ||
true -> "ALLOW-FROM " ++ FullHost; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any particular reason you have a macro for "DENY" and not using one here for "ALLOW-FROM"? |
||
false -> ?DENY | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. three carriage returns here need to be removed |
||
|
||
end. | ||
|
||
|
||
|
||
get_xframe_config(#httpd{xframe_config = undefined}) -> | ||
EnableXFrame = config:get("httpd", "enable_xframe_options", "false") =:= "true", | ||
SameOrigin = config:get("x_frame_options", "same_origin", "false") =:= "true", | ||
AcceptedHosts = case config:get("x_frame_options", "hosts", undefined) of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think just doing |
||
undefined -> []; | ||
Hosts -> split_list(Hosts) | ||
end, | ||
[ | ||
{enabled, EnableXFrame}, | ||
{same_origin, SameOrigin}, | ||
{hosts, AcceptedHosts} | ||
]; | ||
get_xframe_config(#httpd{xframe_config = Config}) -> | ||
Config. | ||
|
||
|
||
|
||
get_accepted_hosts(Config) -> | ||
case lists:keyfind(hosts, 1, Config) of | ||
false -> []; | ||
{hosts, AcceptedHosts} -> AcceptedHosts | ||
end. | ||
|
||
|
||
|
||
split_list(S) -> | ||
re:split(S, "\\s*,\\s*", [trim, {return, list}]). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no new line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that this function should be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. U fixed this, basically there was no new line at the end of the file |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
-module(chttpd_xframe_test). | ||
|
||
|
||
-include_lib("couch/include/couch_db.hrl"). | ||
-include_lib("eunit/include/eunit.hrl"). | ||
|
||
setup() -> | ||
ok = meck:new(config), | ||
ok = meck:expect(config, get, fun(_, _, _) -> "X-Forwarded-Host" end), | ||
ok. | ||
|
||
teardown(_) -> | ||
meck:unload(config). | ||
|
||
mock_request() -> | ||
Headers = mochiweb_headers:make([{"Host", "examples.com"}]), | ||
MochiReq = mochiweb_request:new(nil, 'GET', '/', {1, 1}, Headers), | ||
#httpd{mochi_req = MochiReq}. | ||
|
||
config_disabled() -> | ||
[ | ||
{enabled, false} | ||
]. | ||
|
||
config_sameorigin() -> | ||
[ | ||
{enabled, true}, | ||
{same_origin, true} | ||
]. | ||
|
||
config_wildcard() -> | ||
[ | ||
{enabled, true}, | ||
{same_origin, false}, | ||
{hosts, ["*"]} | ||
]. | ||
|
||
config_specific_hosts() -> | ||
[ | ||
{enabled, true}, | ||
{same_origin, false}, | ||
{hosts, ["http://couchdb.org", "http://examples.com"]} | ||
]. | ||
|
||
config_diffent_specific_hosts() -> | ||
[ | ||
{enabled, true}, | ||
{same_origin, false}, | ||
{hosts, ["http://couchdb.org"]} | ||
]. | ||
|
||
no_header_if_xframe_disabled_test() -> | ||
Headers = chttpd_xframe_options:header(mock_request(), [], config_disabled()), | ||
?assertEqual(Headers, []). | ||
|
||
enabled_with_same_origin_test() -> | ||
Headers = chttpd_xframe_options:header(mock_request(), [], config_sameorigin()), | ||
?assertEqual(Headers, [{"X-Frame-Options", "SAMEORIGIN"}]). | ||
|
||
|
||
xframe_host_test_() -> | ||
{ | ||
"xframe host tests", | ||
{ | ||
foreach, fun setup/0, fun teardown/1, | ||
[ | ||
fun allow_with_wildcard_host/1, | ||
fun allow_with_specific_host/1, | ||
fun deny_with_different_host/1 | ||
] | ||
} | ||
}. | ||
|
||
allow_with_wildcard_host(_) -> | ||
Headers = chttpd_xframe_options:header(mock_request(), [], config_wildcard()), | ||
?_assertEqual([{"X-Frame-Options", "ALLOW-FROM http://examples.com"}], Headers). | ||
|
||
allow_with_specific_host(_) -> | ||
Headers = chttpd_xframe_options:header(mock_request(), [], config_specific_hosts()), | ||
?_assertEqual([{"X-Frame-Options", "ALLOW-FROM http://examples.com"}], Headers). | ||
|
||
deny_with_different_host(_) -> | ||
Headers = chttpd_xframe_options:header(mock_request(), [], config_diffent_specific_hosts()), | ||
?_assertEqual([{"X-Frame-Options", "DENY"}], Headers). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. end of file new no new line |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,6 +101,7 @@ | |
original_method, | ||
nonce, | ||
cors_config, | ||
xframe_config, | ||
qs | ||
}). | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the pros and cons are for setting this to true for default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a mistake. It should be false by default.