Allow custom HTTP headers in BOSH and WebSockets #2907
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| -module(mod_bosh_SUITE). | ||
| -compile([export_all]). | ||
| -include_lib("eunit/include/eunit.hrl"). | ||
| -define(PORT, 5280). | ||
| -define(IP, {127, 0, 0, 1}). | ||
| -define(CUSTOM_HEADERS, | ||
| [{<<"strict-transport-security">>, <<"max-age=31536000; includeSubDomains">>}, | ||
| {<<"access-control-allow-origin">>, <<"*">>}]). | ||
| -define(BOSH_BIND_PATH, "/http-bind"). | ||
| -define(HANDSHAKE_TIMEOUT, 3000). | ||
| -define(eq(E, I), ?assertEqual(E, I)). | ||
| -define(FAST_PING_RATE, 500). | ||
|
There was a problem hiding this comment. Some of these defines can be removed. I suspect it's a direct copy & paste from websockets suite? :) |
||
| -define(NEW_TIMEOUT, 1200). | ||
| %The timeout is long enough to pass all test cases for ping interval settings | ||
| %using NEW_TIMEOUT value. In these tests we wait for at most 2 pings. | ||
| %The 300ms is just an additional overhead | ||
| -define(IDLE_TIMEOUT, ?NEW_TIMEOUT * 2 + 300). | ||
|
|
||
| all() -> | ||
| custom_headers_tests(). | ||
|
|
||
| custom_headers_tests() -> | ||
| [verify_custom_headers]. | ||
|
|
||
| init_per_suite(C) -> | ||
| setup(), | ||
| C. | ||
|
|
||
| end_per_suite(_) -> | ||
| teardown(), | ||
| ok. | ||
|
|
||
| init_per_testcase(_, C) -> | ||
| C. | ||
|
|
||
| end_per_testcase(_, C) -> | ||
| C. | ||
|
|
||
|
|
||
| setup() -> | ||
| meck:unload(), | ||
| application:ensure_all_started(cowboy), | ||
| application:ensure_all_started(jid), | ||
| meck:new(supervisor, [unstick, passthrough, no_link]), | ||
| meck:new(gen_mod,[unstick, passthrough, no_link]), | ||
| %% Set ping rate | ||
| meck:expect(supervisor, start_child, | ||
| fun(ejabberd_listeners, {_, {_, start_link, [_]}, transient, | ||
| infinity, worker, [_]}) -> {ok, self()}; | ||
| (A,B) -> meck:passthrough([A,B]) | ||
| end), | ||
| meck:expect(ejabberd_config, get_local_option, fun(_) -> undefined end), | ||
| %% Start websocket cowboy listening | ||
|
|
||
| Opts = [{num_acceptors, 10}, | ||
| {max_connections, 1024}, | ||
| {modules, [{"_", "/http-bind", mod_bosh, [{custom_headers, ?CUSTOM_HEADERS}]}]}], | ||
| ejabberd_cowboy:start_listener({?PORT, ?IP, tcp}, Opts). | ||
|
|
||
| teardown() -> | ||
| meck:unload(), | ||
| cowboy:stop_listener(ejabberd_cowboy:ref({?PORT, ?IP, tcp})), | ||
| application:stop(cowboy), | ||
| %% Do not stop jid, Erlang 21 does not like to reload nifs | ||
| ok. | ||
|
|
||
| verify_custom_headers(_Config) -> | ||
| application:ensure_all_started(gun), | ||
| {ok, ConnPid} = gun:open("127.0.0.1", ?PORT), | ||
| {ok, http} = gun:await_up(ConnPid), | ||
| StreamRef = gun:get(ConnPid, ?BOSH_BIND_PATH), | ||
| case gun:await(ConnPid, StreamRef) of | ||
| {response, fin, _Status, _Headers} -> | ||
| ct:fail(no_bosh_http_response); | ||
| {response, nofin, 200, Headers} -> | ||
| {ok, _Body} = gun:await_body(ConnPid, StreamRef), | ||
| RcvdHdrsSet = sets:from_list(Headers), | ||
| CustomHdrsSet = sets:from_list(?CUSTOM_HEADERS), | ||
| ?assert(sets:is_subset(CustomHdrsSet, RcvdHdrsSet)) | ||
| end, | ||
| application:stop(gun). | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Needs to be rewritten for TOML
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.
Any thoughts on how you want it written for TOML? The precedent in
mod_revproxyis tuple list of headers. I cannot tell if you're gonna move that mod to TOML or drop it.Since Cowboy natively uses maps HTTP headers, I'd like to map the headers as a map in the TOML. Will that work, even if I stray from the extra headers convention in
mod_revproxy?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.
The whole
mod_revproxyfunctionality is deprecated and will be dropped in the next release unless a new demand appears (which is unlikely). Custom headers would need to be added to TOML as a table (which is parsed as a map bytomerl). However, the whole TOML parser is being rewritten at the moment and a part of this code would need to be rewritten, unless you merge this PR to thetoml-configbranch.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.
I'll wait until #2929 is merged. I don't think it'd make sense to pollute that branch with this change, which is more functionality than config. I also plan on adding some default security headers like:
{ws_https_custom_headers, "[[listen.http.handlers.mod_websockets.custom_headers]] strict-transport-security: "max-age=31536000 ; includeSubDomains" "}.to the HTTPS endpoints as part of the standard as part of the standard
vars-tool.config.in.