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

Escape HTML in profile name preview in profile settings #9446

Merged
merged 5 commits into from Dec 7, 2018

Conversation

Projects
None yet
3 participants
@pawelngei
Copy link
Contributor

pawelngei commented Dec 6, 2018

Addresses #9343 . Additionally falls back to the default profile name if user deletes their custom one.

Right now the data about the default name is set in a display: none span, but it can be provided to JavaScript in some other way.

Personally I'd rewrite this view to React some day, but we should be good for now.

@@ -9,6 +9,7 @@
= image_tag account.avatar.url, alt: '', width: 48, height: 48, class: 'u-photo'

.display-name
%span{:id=>"default_account_display_name", :style=>"display:none;"}= display_name(account, custom_emojify: true)

This comment has been minimized.

@Gargron

Gargron Dec 6, 2018

Member

Use id: "" style, the => style is outdated

This comment has been minimized.

@nightpool

nightpool Dec 6, 2018

Collaborator

since the default display name is always just the username, we should display that instead. this will give incorrect results for users trying to unset their display name

This comment has been minimized.

@pawelngei

pawelngei Dec 6, 2018

Author Contributor

Is the username anywhere else on the page for JS to fetch, or should we provide it some other way through Ruby?

if (name) {
name.innerHTML = emojify(target.value);
if (target.value) {
name.textContent = emojify(target.value);

This comment has been minimized.

@nightpool

nightpool Dec 6, 2018

Collaborator

textContent = emojify doesn't work, since emojify returns img tags for custom emoji

This comment has been minimized.

@pawelngei

pawelngei Dec 6, 2018

Author Contributor

👍

This comment has been minimized.

@pawelngei

pawelngei Dec 7, 2018

Author Contributor

Okay, I think there may be a wider bug, but even on master I cannot see the emojis in this field. I checked out https://writing.exchange/ and https://chaos.social/ and it doesn't work there, either.

@@ -9,6 +9,7 @@
= image_tag account.avatar.url, alt: '', width: 48, height: 48, class: 'u-photo'

.display-name
%span{:id=>"default_account_display_name", :style=>"display:none;"}= display_name(account, custom_emojify: true)

This comment has been minimized.

@nightpool

nightpool Dec 6, 2018

Collaborator

since the default display name is always just the username, we should display that instead. this will give incorrect results for users trying to unset their display name

@@ -9,6 +9,7 @@
= image_tag account.avatar.url, alt: '', width: 48, height: 48, class: 'u-photo'

.display-name
%span{id: "default_account_display_name", style: "display:none;"}= display_name(account, custom_emojify: true)

This comment has been minimized.

@nightpool

nightpool Dec 6, 2018

Collaborator

since the default display name is always just the username, we should display that instead. this will give incorrect results for users trying to unset their display name

@pawelngei

This comment has been minimized.

Copy link
Contributor Author

pawelngei commented Dec 7, 2018

I've started looking at https://github.com/tootsuite/mastodon/blob/master/app/javascript/mastodon/features/emoji/emoji.js and there's something seriously wrong with it. Does it work for anyone? I tested it on my local instance, chaos.social and writing.exchange and it doesn't emojify the profile.

From what I see the extremely convoluted emojify function never gets past the first while.

I'd like to refactor that function, but first I'd like to know if it works for anybody, anywhere..

@pawelngei

This comment has been minimized.

Copy link
Contributor Author

pawelngei commented Dec 7, 2018

Okay, after several hours of debugging I have no idea how emojify works and how to test custom emojis. I welcome you to try fixing that and testing what kind of escaping works properly.

@pawelngei

This comment has been minimized.

Copy link
Contributor Author

pawelngei commented Dec 7, 2018

Managed to handle the emojified unicode, but I have no idea if this will work for custom icons as well or how to test them.

@Gargron

Gargron approved these changes Dec 7, 2018

Addressed

@Gargron Gargron merged commit 5c7f641 into tootsuite:master Dec 7, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment