summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Vatamaniuc <vatamane@apache.org>2020-09-17 12:00:24 -0400
committerNick Vatamaniuc <nickva@users.noreply.github.com>2020-09-17 12:23:37 -0400
commit41e75e63d533e9e74e7ca7e00fafe3139980a68e (patch)
tree7b079daf2a39ef3d3c4e8121c4bb0bb7bfb22fa5
parentad93d1378048ab1049dcd5d6066e947afb614e67 (diff)
downloadcouchdb-41e75e63d533e9e74e7ca7e00fafe3139980a68e.tar.gz
Add url validation in replicator creds stripping logic
Previously, in 3.x we re-parsed the endpoint URLs with `ibrowse_lib:parse_url/1` when stripping credentials, which threw an error if the URL was invalid. So we try to preserve that same logic. Backport some tests from 3.x to make sure URL stripping works when URL is valid and, also use the nicer ?TDEF and ?TDEF_FE test helpers.
-rw-r--r--src/couch_replicator/src/couch_replicator.erl104
1 files changed, 58 insertions, 46 deletions
diff --git a/src/couch_replicator/src/couch_replicator.erl b/src/couch_replicator/src/couch_replicator.erl
index a53aa1045..8ab36e587 100644
--- a/src/couch_replicator/src/couch_replicator.erl
+++ b/src/couch_replicator/src/couch_replicator.erl
@@ -35,6 +35,7 @@
]).
+-include_lib("ibrowse/include/ibrowse.hrl").
-include_lib("couch/include/couch_db.hrl").
-include("couch_replicator.hrl").
@@ -483,6 +484,10 @@ ejson_url(null) ->
-spec strip_url_creds(binary()) -> binary() | null.
strip_url_creds(Url) ->
try
+ case ibrowse_lib:parse_url(binary_to_list(Url)) of
+ #url{} -> ok;
+ {error, Error} -> error(Error)
+ end,
iolist_to_binary(couch_util:url_strip_password(Url))
catch
error:_ ->
@@ -510,6 +515,8 @@ check_authorization(JobId, #user_ctx{} = Ctx) when is_binary(JobId) ->
-ifdef(TEST).
-include_lib("eunit/include/eunit.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+
authorization_test_() ->
{
@@ -517,40 +524,34 @@ authorization_test_() ->
fun () -> ok end,
fun (_) -> meck:unload() end,
[
- t_admin_is_always_authorized(),
- t_username_must_match(),
- t_replication_not_found()
+ ?TDEF_FE(t_admin_is_always_authorized),
+ ?TDEF_FE(t_username_must_match),
+ ?TDEF_FE(t_replication_not_found)
]
}.
-t_admin_is_always_authorized() ->
- ?_test(begin
- expect_job_data({ok, #{?REP => #{?REP_USER => <<"someuser">>}}}),
- UserCtx = #user_ctx{name = <<"adm">>, roles = [<<"_admin">>]},
- ?assertEqual(ok, check_authorization(<<"RepId">>, UserCtx))
- end).
+t_admin_is_always_authorized(_) ->
+ expect_job_data({ok, #{?REP => #{?REP_USER => <<"someuser">>}}}),
+ UserCtx = #user_ctx{name = <<"adm">>, roles = [<<"_admin">>]},
+ ?assertEqual(ok, check_authorization(<<"RepId">>, UserCtx)).
-t_username_must_match() ->
- ?_test(begin
- expect_job_data({ok, #{?REP => #{?REP_USER => <<"user1">>}}}),
- UserCtx1 = #user_ctx{name = <<"user1">>, roles = [<<"somerole">>]},
- ?assertEqual(ok, check_authorization(<<"RepId">>, UserCtx1)),
- UserCtx2 = #user_ctx{name = <<"other">>, roles = [<<"somerole">>]},
- ?assertThrow({unauthorized, _}, check_authorization(<<"RepId">>,
- UserCtx2))
- end).
+t_username_must_match(_) ->
+ expect_job_data({ok, #{?REP => #{?REP_USER => <<"user1">>}}}),
+ UserCtx1 = #user_ctx{name = <<"user1">>, roles = [<<"somerole">>]},
+ ?assertEqual(ok, check_authorization(<<"RepId">>, UserCtx1)),
+ UserCtx2 = #user_ctx{name = <<"other">>, roles = [<<"somerole">>]},
+ ?assertThrow({unauthorized, _}, check_authorization(<<"RepId">>,
+ UserCtx2)).
-t_replication_not_found() ->
- ?_test(begin
- expect_job_data({error, not_found}),
- UserCtx1 = #user_ctx{name = <<"user">>, roles = [<<"somerole">>]},
- ?assertEqual(not_found, check_authorization(<<"RepId">>, UserCtx1)),
- UserCtx2 = #user_ctx{name = <<"adm">>, roles = [<<"_admin">>]},
- ?assertEqual(not_found, check_authorization(<<"RepId">>, UserCtx2))
- end).
+t_replication_not_found(_) ->
+ expect_job_data({error, not_found}),
+ UserCtx1 = #user_ctx{name = <<"user">>, roles = [<<"somerole">>]},
+ ?assertEqual(not_found, check_authorization(<<"RepId">>, UserCtx1)),
+ UserCtx2 = #user_ctx{name = <<"adm">>, roles = [<<"_admin">>]},
+ ?assertEqual(not_found, check_authorization(<<"RepId">>, UserCtx2)).
expect_job_data(JobDataRes) ->
@@ -558,7 +559,7 @@ expect_job_data(JobDataRes) ->
strip_url_creds_test_() ->
- {
+ {
setup,
fun() ->
meck:expect(config, get, fun(_, _, Default) -> Default end)
@@ -566,28 +567,39 @@ strip_url_creds_test_() ->
fun(_) ->
meck:unload()
end,
- [
- t_strip_url_creds_errors()
- ]
+ with([
+ ?TDEF(t_strip_http_basic_creds),
+ ?TDEF(t_strip_url_creds_errors)
+ ])
}.
-t_strip_url_creds_errors() ->
- ?_test(begin
- Bad1 = <<"http://adm:pass/bad">>,
- ?assertEqual(null, strip_url_creds(Bad1)),
- Bad2 = <<"more garbage">>,
- ?assertEqual(null, strip_url_creds(Bad2)),
- Bad3 = <<"http://a:b:c">>,
- ?assertEqual(null, strip_url_creds(Bad3)),
- Bad4 = <<"http://adm:pass:pass/bad">>,
- ?assertEqual(null, strip_url_creds(Bad4)),
- ?assertEqual(null, strip_url_creds(null)),
- ?assertEqual(null, strip_url_creds(42)),
- ?assertEqual(null, strip_url_creds([<<"a">>, <<"b">>])),
- Bad5 = <<"http://adm:pass/bad">>,
- ?assertEqual(null, strip_url_creds(Bad5))
- end).
+t_strip_http_basic_creds(_) ->
+ Url1 = <<"http://adm:pass@host/db/">>,
+ ?assertEqual(<<"http://adm:*****@host/db/">>, strip_url_creds(Url1)),
+ Url2 = <<"https://adm:pass@host/db/">>,
+ ?assertEqual(<<"https://adm:*****@host/db/">>, strip_url_creds(Url2)),
+ Url3 = <<"http://adm:pass@host:80/db/">>,
+ ?assertEqual(<<"http://adm:*****@host:80/db/">>, strip_url_creds(Url3)),
+ Url4 = <<"http://adm:pass@host/db?a=b&c=d">>,
+ ?assertEqual(<<"http://adm:*****@host/db?a=b&c=d">>,
+ strip_url_creds(Url4)).
+
+
+t_strip_url_creds_errors(_) ->
+ Bad1 = <<"http://adm:pass/bad">>,
+ ?assertEqual(null, strip_url_creds(Bad1)),
+ Bad2 = <<"more garbage">>,
+ ?assertEqual(null, strip_url_creds(Bad2)),
+ Bad3 = <<"http://a:b:c">>,
+ ?assertEqual(null, strip_url_creds(Bad3)),
+ Bad4 = <<"http://adm:pass:pass/bad">>,
+ ?assertEqual(null, strip_url_creds(Bad4)),
+ ?assertEqual(null, strip_url_creds(null)),
+ ?assertEqual(null, strip_url_creds(42)),
+ ?assertEqual(null, strip_url_creds([<<"a">>, <<"b">>])),
+ Bad5 = <<"http://adm:pass/bad">>,
+ ?assertEqual(null, strip_url_creds(Bad5)).
-endif.