Skip to content

Conversation

@Marek-Wojtowicz
Copy link
Contributor

The http headers according to rfc 2616 is iso-8859-1. This patch fixes the behavior when non-ascii characters are present in the header.

@solracsf solracsf changed the title Update Session.php Fix non-ascii characters present in the header Jan 12, 2022
@solracsf solracsf added the 3. to review Waiting for reviews label Jan 12, 2022
@solracsf
Copy link
Member

@Marek-Wojtowicz can you fix the DCO please?

The http headers according to rfc 2616 is iso-8859-1. This patch fixes the behavior when non-ascii characters are present in the header.

Signed-off-by: Marek Wójtowicz <Marek.Wojtowicz@agh.edu.pl>
@Marek-Wojtowicz
Copy link
Contributor Author

Marek-Wojtowicz commented Jan 12, 2022

I think I fixed the DCO 😄

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

This looks sensible but I'm not familiar enough with how the session token works inside nextcloud to say if this might or not break anything

@Marek-Wojtowicz
Copy link
Contributor Author

An SQL query is fired, inserting a token into the oc_authtoken table. Specifically, the userAgent column has a wrong character encoding (MySQL returns: Incorrect string value).

@CarlSchwan CarlSchwan requested review from a team, PVince81, come-nc and juliusknorr and removed request for a team January 13, 2022 16:28
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit f9fb5f5 into nextcloud:master Jan 14, 2022
@welcome
Copy link

welcome bot commented Jan 14, 2022

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

@CarlSchwan
Copy link
Member

Backports?

return false;
}
$name = isset($request->server['HTTP_USER_AGENT']) ? $request->server['HTTP_USER_AGENT'] : 'unknown browser';
$name = isset($request->server['HTTP_USER_AGENT']) ? utf8_encode($request->server['HTTP_USER_AGENT']) : 'unknown browser';
Copy link
Contributor

Choose a reason for hiding this comment

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

utf8_encode is a wrongly named php function, I’d feel better if either we have our own alias named iso-8859-1_to_utf8(), or if we use mb_convert_encoding instead (only possible if we depend upon mbstring).
There are frequent proposal to remove utf8_encode from PHP, all were rejected so far, but it shows this function is confusing.

Copy link
Contributor Author

@Marek-Wojtowicz Marek-Wojtowicz Jan 17, 2022

Choose a reason for hiding this comment

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

I think there will be time to fix the problem as utf8_encode becomes obsolete. Until then, there may be newer mechanisms that can replace utf8_encode.

@Marek-Wojtowicz
Copy link
Contributor Author

/backport to stable23

@Marek-Wojtowicz
Copy link
Contributor Author

/backport to stable22

@Marek-Wojtowicz
Copy link
Contributor Author

/backport to stable21

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants