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

Use RFC-compliant URL encoding for cookies #25302

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

mziech
Copy link
Contributor

@mziech mziech commented Jan 24, 2021

PHP 7.4.2 changed the way how cookies are decoded, applying RFC-compliant raw URL decoding. This leads to a conflict Nextcloud's own cookie encoding, breaking the remember-me function if the UID contains a space character.

Fixes #24438

Signed-off-by: Marco Ziech [email protected]

PHP 7.4.2 changed the way how cookies are decoded, applying RFC-compliant raw URL decoding. This leads to a conflict Nextcloud's own cookie encoding, breaking the remember-me function if the UID contains a space character.

Fixes nextcloud#24438

Signed-off-by: Marco Ziech <[email protected]>
@kesselb
Copy link
Contributor

kesselb commented Jan 24, 2021

I was observing the same behavior on my instance, essentially all users with a space in the name have to login twice after the session expired.

I tracked down the issue to a non-obvious change in PHP 7.4.2 and 7.4.3, which makes cookie encoding and decoding RFC compliant. See:

Basically PHP < 7.4.2 would encode Hans Peter as Hans+Peter and decode back to Hans Peter. However, PHP > 7.4.3 encode Hans Peter to Hans%20Peter and decode Hans+Peter to Hans+Peter leading to a clash in the expected UID.

php > echo(rawurldecode(urlencode('Hans Peter')));
Hans+Peter
php > echo(urldecode(rawurlencode('Hans Peter')));
Hans Peter
php > echo(urldecode(urlencode('Hans Peter')));
Hans Peter
php > echo(rawurldecode(rawurlencode('Hans Peter')));
Hans Peter

Since NextCloud uses its own cookie encoding function, the PHP 7.4.3 encoding fix won't work. I fixed the issue by changing urlencode to rawurlencode in https://github.com/nextcloud/server/blob/master/lib/private/Http/CookieHelper.php#L46

Originally posted by @mziech in #24438 (comment)

@kesselb kesselb added 3. to review Waiting for reviews bug labels Jan 24, 2021
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

O that is a nasty little change.
Thanks for the detailed explanation.

@rullzer rullzer added this to the Nextcloud 21 milestone Jan 29, 2021
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 so HP can use his Nextcloud again

@ChristophWurst ChristophWurst merged commit 8fcc0e8 into nextcloud:master Jan 29, 2021
@welcome
Copy link

welcome bot commented Jan 29, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@ChristophWurst
Copy link
Member

/backport to stable20

@ChristophWurst
Copy link
Member

/backport to stable19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cookie to receive the login in the browser can not be read -> user is logged out after closing the browser
5 participants