Skip to content

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

Merged
nickva merged 1 commit intomainfrom
handle-multiple-set-cookie-headers-in-replicator
May 22, 2024
Merged

Handle multiple Set-Cookie headers in replicator session plugin#5066
nickva merged 1 commit intomainfrom
handle-multiple-set-cookie-headers-in-replicator

Conversation

@nickva
Copy link
Contributor

@nickva nickva commented May 22, 2024

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

Fix: #5064

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 <rnewson@apache.org>

Fix: #5064
@nickva nickva requested review from janl and rnewson May 22, 2024 05:42
-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.

Headers1 = mochiweb_headers:make(Headers),
Headers2 = mochiweb_headers:to_list(Headers1),
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.

@nickva nickva merged commit 8405bad into main May 22, 2024
@nickva nickva deleted the handle-multiple-set-cookie-headers-in-replicator branch May 22, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

replicator session auth cannot handle multiple Set-Cookie headers in /_session request

2 participants