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

decode entities in toast message #1640

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Dec 21, 2023

Description

Decodes toast message entities, which are encoded before they're sent from the server. See also silverstripe/silverstripe-framework#11105

Side bonus : this is much safer than using jquery

Manual testing steps

Go to model admin to a record with a ' in the name or any utf8 character like ö or an emoji
Click save
Discover the utf8 encoded ' that is not displayed properly in the toast message

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@lekoala lekoala mentioned this pull request Dec 21, 2023
2 tasks
@GuySartorelli
Copy link
Member

Hiya, First off, thank you for this contribution.

We added a new pull request template to this repository yesterday - but it looks like your PR description isn't using it. Did the template show up when you created this pull request?

@lekoala
Copy link
Contributor Author

lekoala commented Dec 21, 2023

hey @GuySartorelli it did show but i have to admit i didn't read it :-/ it seems to me that since everything was already pretty much explained in the linked issue that felt like doing the work twice and i was already quite busy getting the two pr ready
it took me already longer than expected to pin down what was exactly happening since it's a combined issue between the data not being url encoded in X-Status and how the content is currently escaped :-)

@GuySartorelli
Copy link
Member

I've added the template back in - please add any manual testing steps, or if there aren't any that are specific to this PR, please point back at the reproduction steps in the issue.

Please also tick all of the boxes that you have done, and do whatever is still not done based on the checklist.

@lekoala
Copy link
Contributor Author

lekoala commented Dec 21, 2023

Hi @GuySartorelli. While i understand the need to raise the quality level of the PR to avoid burdening the team, sometimes it feels like a real pain to get across some changes. Ultimately, it's up to the team to determine how/where things go (eg: NOT using the current method of escaping text that is not XSS safe), or to refactor the utility to do so somewhere that makes sense so that i can be used across the whole project.
Basically, it feels like i've done my part of the job :-) i had an issue, and i fixed it and explained how i did it
the steps are described in the issue already, and it gives the wider context
i'll add them here also since i was there anyway

@GuySartorelli
Copy link
Member

The idea here is that we've added a new template to make things faster - by following the template, there's a higher degree of certainty that all of the small things have been done, and that there's enough context for someone (me in this case) to come along and review the work.

This is a new process we're trying, and I'd really appreciate your help in testing the process by following it. Frankly I'm not going to spend my time reviewing this or the related PR this side of christmas if the process isn't being followed, because if the process isn't followed it increases the amount of effort that goes into reviewing the PRs.

@lekoala
Copy link
Contributor Author

lekoala commented Dec 21, 2023

:-) yup i understand that fully

but it's pretty much the same thing for me, it's not really easy to explain to a customer that it took half a day to solve some badly escaped characters due to the open source nature of the project

=> the main issue remaining are:
-> i didn't run the build process because i didn't check yet how to do that (but should be pretty easy for anyone from the team if they want to give it a run)
-> check why the text was escaped using jquery when it's not safe
-> probably refactor the escape function into some util function that can be used elsewhere

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

A few things here I'm unsure about.

Please also update the commit message to include FIX at the start (see commit message guidelines)

client/src/legacy/LeftAndMain.js Outdated Show resolved Hide resolved
client/src/legacy/LeftAndMain.js Outdated Show resolved Hide resolved
var statusMessage = function(text, type) {
text = jQuery('<div/>').text(text).html(); // Escape HTML entities in text
jQuery.noticeAdd({text: text, type: type, stayTime: 5000, inEffect: {left: '0', opacity: 'show'}});
jQuery.noticeAdd({text: decodeEntities(text), type: type, stayTime: 5000, inEffect: {left: '0', opacity: 'show'}});
Copy link
Member

Choose a reason for hiding this comment

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

On line 191 we have a message that is being passed into a decodeURIComponent function - it seems likely there's a risk of double-decoding there, if that's even a thing. Should we remove the call to decodeURIComponent on that line?

statusMessage(decodeURIComponent(msg), msgType);

Copy link
Member

Choose a reason for hiding this comment

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

The same thing is happening in

if(msg) statusMessage(decodeURIComponent(msg), (status === 'success') ? 'success' : 'error');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as i'm aware, there is only a risk of double encoding, not double decoding but i may be wrong here
at any rate, "it seems to work" on my end with my changes. i know that's not a very satisfactory answer :-)

Copy link
Member

@GuySartorelli GuySartorelli Jan 15, 2024

Choose a reason for hiding this comment

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

I guess the thing I'm worried about (and it is an extreme edge case so maybe we can ignore it? I'd want feedback from others in @silverstripe/core-team though) is if someone intentionally wants the encoded form.

e.g. a page called "Are &quot; and &#39; the same thing?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure either, i just did what needed in order to fix all my use cases :)

Copy link
Member

Choose a reason for hiding this comment

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

I've just tried creating a page called Are &quot; and ' and &#39; the same thing? both with this (and the framework) PR, and without them.
In both cases, the toast displays like this:
Screenshot from 2024-01-19 12-08-34

So this PR is still not handling some characters (& specifically in this case) correctly.
Can you please look into that?

@lekoala lekoala force-pushed the patch-5 branch 2 times, most recently from 802906e to d921e36 Compare January 17, 2024 14:25
@lekoala
Copy link
Contributor Author

lekoala commented Jan 17, 2024

hi @GuySartorelli

so i have made a small update

  • renamed the function to be more accurate about what it does (the main goal is to escape html, the added bonus is that it helps with things like single quotes)
  • i don't see any risk of "double decoding", the decodeURIComponent is basically decoding what does "rawurlencode" on the php side (hence, the other PR)
  • this will be safer than using jQuery for escaping html

@GuySartorelli
Copy link
Member

It's worth notice that I cannot reproduce the original issue with ' or ö being rendered incorrectly in toast messages (at least for saving a page, maybe other toasts are different?) with framework 5.1.x-dev and admin 2.1.x-dev - I've tried in both firefox and chromium.

I do see a problem with & being incorrectly escaped to &amp; though, so there is definitely a problem.

@lekoala
Copy link
Contributor Author

lekoala commented Jan 19, 2024

This is what i got on the latest version

image

windows/apache/chrome latest version

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 23, 2024

Ahh, I see! By scrutinising the screenshot I realised that's a DataObject in some gridfield, probably in a modeladmin. I had been testing pages in the sitetree which apparently handle this differently. I can confirm that using a modeladmin, I see the same results that you do.

I think any fix for this will need to unify those two approaches - i.e. whatever code sitetree is used for decoding/sanitising/etc toast messages should be used my gridfields, and vice-versa.

@lekoala
Copy link
Contributor Author

lekoala commented Feb 9, 2024

@GuySartorelli so i finally got some time to get to the bottom of this

Here is the situation

Indeed, in the cms it's working fine. This is the current code:

https://github.com/silverstripe/silverstripe-cms/blob/cb46fd8aac90b6a1f08542b92cefac841f64a469/code/Controllers/CMSMain.php#L1779-L1795

It works, because the title is not escaped and rawurlencoded

We could do the same thing and I'll be happy to update my PR silverstripe/silverstripe-framework#11105 accordingly and drop this one (ie: remove the escaping, and add the rawurlencode)

It does raise the question of :

  • why was it deemed necessary to escape the title in the gridfield (and not in the cms) ?
  • why use an unsafe escape method using JQuery().text
    This could be part of another PR, while we do the quick fix (align to CMSMain behaviour) so that at least international users (and smiley lovers) can get proper toasts in the meantime

@GuySartorelli
Copy link
Member

why was it deemed necessary to escape the title in the gridfield (and not in the cms) ?

Not sure - there's nothing in the PR that shows why it would be necessary. I suspect it was just a copy-paste from somewhere doing something different-but-similar.

why use an unsafe escape method using JQuery().text
This could be part of another PR, while we do the quick fix (align to CMSMain behaviour) so that at least international users (and smiley lovers) can get proper toasts in the meantime

I strongly suspect it just wasn't known to be unsafe - I wouldn't have known if you hadn't mentioned it.

@GuySartorelli
Copy link
Member

I've merged the framework PR which solves the majority of the toast problems.

I'd support swapping out JQuery().text - whether you want to rework this PR to include that, do it separately, or just open an issue for someone else to look into, either way is fine.

For this PR, if you're up for it, could you please tackle the problem where & is incorrectly decoded into &amp;?

@lekoala
Copy link
Contributor Author

lekoala commented Feb 14, 2024

I've made a small update to use innerText, it all seem to work nicely with the latest version of the framework and the updated decode method

Even crazy input like

image

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Works great! Just one small change request.

I tested out the title About Us &'😅<script></script> in the pages admin and in a model admin.

Works with every scenario in the pages admin.
Works with saving in the model admin, but other actions (publishing, unpublishing, archiving) transform the emoji:
Published Company "About Us &'ð��<script></script>"

client/src/legacy/LeftAndMain.js Outdated Show resolved Hide resolved
@lekoala
Copy link
Contributor Author

lekoala commented Feb 15, 2024

@GuySartorelli yes, it seems indeed the Versioned gridfield has the same issue
PR silverstripe/silverstripe-versioned#438

And var name updated.

@GuySartorelli
Copy link
Member

Rebased and built JS. Once CI is green I'll merge.
Thank you so much for working on this!

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Works great locally. Thanks again!

@GuySartorelli GuySartorelli merged commit 5075154 into silverstripe:2.1 Feb 19, 2024
12 checks passed
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