Skip to content

fix(SessionService): sanitize displayName to utf8 encoding#8405

Open
silverkszlo wants to merge 1 commit intostable32from
fix/sanitize-utf8-encoding-displayname
Open

fix(SessionService): sanitize displayName to utf8 encoding#8405
silverkszlo wants to merge 1 commit intostable32from
fix/sanitize-utf8-encoding-displayname

Conversation

@silverkszlo
Copy link
Copy Markdown
Collaborator

📝 Summary

A user reported that they can not open text/md files and were getting the following error message: "Malformed UTF-8 characters, possibly incorrectly encoded", while most other users could open the same file without a problem.

This PR uses EncodingService to convert display names from external backends (e.g. LDAP/AD) to UTF-8 before including them in JSON responses.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

Signed-off-by: silver <s.szmajduch@posteo.de>
@silverkszlo silverkszlo force-pushed the fix/sanitize-utf8-encoding-displayname branch from 46c903a to 6b33c6e Compare March 24, 2026 13:06
Copy link
Copy Markdown
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Interesting corner case and I wonder whether this would need to be applied in other places as well. Searching for [dD]isplayName revealed at least lib/Notification/Notifier.php and lib/Controller/UserApiController.php.

But fine with merging as is. A comment in the code would be nice to make it clear for the future why we do the extra sanitizing there.

@mejo-
Copy link
Copy Markdown
Member

mejo- commented Mar 24, 2026

Ahh, I just now realized that this is supposed to be merged to stable32 directly. Any particular reason why the fix is not targeting main first (and backported from there)?

@silverkszlo
Copy link
Copy Markdown
Collaborator Author

Ahh, I just now realized that this is supposed to be merged to stable32 directly. Any particular reason why the fix is not targeting main first (and backported from there)?

No good reason, I just started developing in 32 as the user reported their issue in that version and then I have just noticed, after creating the PR, that it would have made more sense to apply that in main directly. I'll add the change to the other branches soon.

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.

2 participants