Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle multiple Set-Cookie headers in replicator session plugin #5066

Merged
merged 1 commit into from
May 22, 2024
Merged

Handle multiple Set-Cookie headers in replicator session plugin #5066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Handle multiple Set-Cookie headers in replicator session plugin 
Previously, replicator auth session plugin crashed if additional cookie headers
were added besides the default `AuthSession` one.

Fix replicator session plugin to consider only `Set-Cookie` headers with
'AuthSession' set and ignore others.

Co-Authored-By: Robert Newson <[email protected]>

Fix: #5064
  • Loading branch information
@nickva
nickva committed May 22, 2024
commit a4eacd7306854de7acff68d87d4bdbd62e68eef1
108 changes: 95 additions & 13 deletions src/couch_replicator/src/couch_replicator_auth_session.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,23 +386,50 @@ http_response({error, Error}, #state{session_url = Url, user = User}) ->
{error, {session_request_failed, Url, User, Error}}.

-spec parse_cookie(list()) -> {ok, age(), string()} | {error, term()}.
parse_cookie(Headers0) ->
Headers = mochiweb_headers:make(Headers0),
case mochiweb_headers:get_value("Set-Cookie", Headers) of
undefined ->
parse_cookie(Headers) ->
case get_cookies(Headers) of
[] ->
{error, cookie_not_found};
CookieHeader ->
CookieKVs = mochiweb_cookies:parse_cookie(CookieHeader),
CaseInsKVs = mochiweb_headers:make(CookieKVs),
case mochiweb_headers:get_value("AuthSession", CaseInsKVs) of
undefined ->
{error, cookie_format_invalid};
Cookie ->
MaxAge = parse_max_age(CaseInsKVs),
{ok, MaxAge, Cookie}
[_ | _] = Cookies ->
case get_auth_session_cookies_and_age(Cookies) of
[] -> {error, cookie_format_invalid};
[{Cookie, MaxAge} | _] -> {ok, MaxAge, Cookie}
end
end.

% Return list of cookies from headers, each as a KV list.
% For example:
% [
% [{"AuthSession", "foo"}, {"max-age", "1"}],
% [{"ApiKey", "Secret"}, {"HttpOnly", []}]
% ]
%
-spec get_cookies(list()) -> [list()].
get_cookies(Headers) ->
Headers1 = mochiweb_headers:make(Headers),
Headers2 = mochiweb_headers:to_list(Headers1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need these two steps? Headers is already usable, has been case-folded to lowercase by mochiweb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers is not coming from mochiweb but ibrowse in whatever case they came in. So we do the standard mochiweb "raw" processing, normalization, etc. but the context is all about being on the client side, even though we're using our sever-side mochiweb library.

We could probably do that ourselves but since the headers does some extra stuff like combine headers, trim whitespace it might be safer just to process all headers the same way.

Fun = fun({K, V}) ->
case string:equal(K, "Set-Cookie", true) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise we don't need a case-insensitive check here if the input was already forced to lower.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, because to_list will returns the case format of the first entry for the header it finds. So we can look up the header by "set-cookie" and return the value, but since "looking up" in this case doesn't seem to work, we get the whole list so we have to do some of the case-insensitive match ourselves.

 mochiweb_headers:to_list(mochiweb_headers:make([{"sEt-cooKie", "foo=bar"}, {"SeT-cooKie", "a=b"}, {"set-cookIe", "d=e"}])).
[{"sEt-cooKie","foo=bar"},
 {"sEt-cooKie","a=b"},
 {"sEt-cooKie","d=e"}]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm, that's subtle then. I couldn't get mochiweb to mix things up for me, but I guess I wasn't changing the first header.

true -> {true, mochiweb_cookies:parse_cookie(V)};
false -> false
end
end,
lists:filtermap(Fun, Headers2).

% From a list of cookies, pick out only AuthSession cookies.
% Return a list of {Cookie, MaxAge} tuples
%
-spec get_auth_session_cookies_and_age([list()]) -> [{string(), age()}].
get_auth_session_cookies_and_age(Cookies) ->
Fun = fun(CookieKVs) ->
CaseInsKVs = mochiweb_headers:make(CookieKVs),
case mochiweb_headers:get_value("AuthSession", CaseInsKVs) of
rnewson marked this conversation as resolved.
Show resolved Hide resolved
undefined -> false;
Cookie -> {true, {Cookie, parse_max_age(CaseInsKVs)}}
end
end,
lists:filtermap(Fun, Cookies).

-spec parse_max_age(list()) -> age().
parse_max_age(CaseInsKVs) ->
case mochiweb_headers:get_value("Max-Age", CaseInsKVs) of
Expand Down Expand Up @@ -725,4 +752,59 @@ parse_max_age_test_() ->
]
].

get_cookies_test() ->
?assertEqual([], get_cookies([])),
?assertEqual([], get_cookies([{"abc", ""}])),
?assertEqual([], get_cookies([{"abc", "def"}])),
?assertEqual([], get_cookies([{"xset-cookie", "c=v"}])),
?assertEqual([], get_cookies([{"set-cookiee", "c=v"}])),
?assertEqual([[]], get_cookies([{"set-cookie", ""}])),
?assertEqual([[{"c", "v"}]], get_cookies([{"Set-cOokie", "c=v"}])),
?assertEqual(
[[{"c", "v"}, {"HttpOnly", []}]],
get_cookies([
{"Set-COOkiE", "c=v;HttpOnly"}
])
),
?assertEqual(
[[{"c", "v"}]],
get_cookies([
{"Foo", "Bar"},
{"Set-cOokie", "c=v"}
])
),
?assertEqual(
[
[{"c1", "v1"}, {"x", "y"}],
[{"c2", "v2"}, {"z", ""}]
],
get_cookies([
{"Set-cOokie", "c1=v1;x=y"},
{"Other", "Foo;Bar"},
{"sEt-cookie", "c2=v2;z"}
])
).

get_auth_session_cookies_and_age_test() ->
?assertEqual([], get_auth_session_cookies_and_age([])),
?assertEqual([], get_auth_session_cookies_and_age([[{"c", "v"}]])),
?assertEqual(
[{"x", undefined}],
get_auth_session_cookies_and_age([
[{"c", "v"}], [{"AuthSession", "x"}], [{"z", "w"}]
])
),
?assertEqual(
[
{"x", 10},
{"y", 20},
{"z", undefined}
],
get_auth_session_cookies_and_age([
[{"AuthSession", "x"}, {"Max-Age", "10"}, {"HttpOnly", ""}],
[{"AuthSession", "y"}, {"Max-Age", "20"}],
[{"AuthSession", "z"}, {"Foo", "Bar"}]
])
).

-endif.