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.

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

Cookies: Don't override cookies with outdated values #7831

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

TheOneRing
Copy link
Member

This code was actually not breaking most cookie handling by accident.
As the raw cookies where not split properly we added cookies with values like
"key: val; key2 = val2; key3 = val3"
When the code was corrected we overwrote the newer values in the jar with
the old ones from a request.

This code was actually not breaking most cookie handling by accident.
As the raw cookies where not split properly we added cookies with values like
"key: val; key2 = val2; key3 = val3"
When the code was corrected we overwrote the newer values in the jar with
the old ones from a request.
@ogoffart
Copy link
Contributor

The code must have been like this for a reason... But i cannot remember why (maybe @guruz knows)

@TheOneRing
Copy link
Member Author

The code was introduced in b037e63

And it can never have worked when more than one cookie was set.
And with that code fixed, it was possible that we received a new cookie in a reply, and overwrote it with the old one.

As we don't support cookie based authentication anymore we can provide cookies here.
This fixes issues with loadbalancers access policy managers.
@TheOneRing TheOneRing requested a review from guruz April 1, 2020 08:47
Copy link
Contributor

@guruz guruz left a comment

Choose a reason for hiding this comment

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

I don't remember why, but if we later can confirm that the client still works in setups where there is some cookie-based sticky load balancing then i'm fine with it CC @HanaGemela

@TheOneRing TheOneRing merged commit 189c1d3 into owncloud:2.6 Apr 1, 2020
@TheOneRing TheOneRing deleted the cookies branch April 1, 2020 08:50
@HanaGemela HanaGemela mentioned this pull request Apr 29, 2020
@HanaGemela
Copy link
Contributor

@TheOneRing @guruz Please provide steps to test

@TheOneRing
Copy link
Member Author

We don't really have a test case for this besides running the client with any server.
As this seems to work and the patch fixed the client behaviour with a more complex cookie setup (f5) I expect it to work.

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.

4 participants