Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jan 25, 2019

While looking into nextcloud/android#3487 I noticed, that the apps which use the most famous RichObjectStrings (Activity and Notifications) have an inconsistent handling in terms of HTML encoding. Or to phrase it better, neither cared and it worked for most, because the providing app happened to escape the HTML (sometimes).

So now we simply define, that ROS should not be HTML escaped, similar to their plain-text variants. Instead the viewing part is responsible to escape the HTML properly, which was provided before the ROS rendering happens.

⚠️ The rendering PRs below need to be merged/backported before, otherwise we create a stored XSS in the comments app. ⚠️

I checked all apps, and I only could find HTML handling in comments (for activities only, notifications are fine) and announcementcenter (both fixed for upcoming)


Client devs please confirm that this works as intended on your side then:

You can test this, by mentioning your user @test1 in a comment:

@test1 <strong>this should not be bold</strong>
This is on a new line

<em>And this is two lines away and NOT italic</em>

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@tobiasKaminsky
Copy link
Member

As mentioned on IRC, this is working on Android.
But as soon as I enable this PR, I do get html tags parsed after a refresh of the web site:

  • open details on any file
  • enter a comment with html tag
  • see that they are not parsed
  • refresh site
  • see html tags parsed, e.g. text is bold

@nickvergessen
Copy link
Member Author

Which is expected as per the bold warning in the PR description

@tobiasKaminsky
Copy link
Member

Hm. I assumed that if I enable all three PRs, that then everything would work…
Then sorry for the noise ;-)

@marinofaggiana
Copy link
Member

Sure, no problem !

@rullzer
Copy link
Member

rullzer commented Jan 30, 2019

@nickvergessen so since we only have notifications on the client. We'd need to validate that we escape the html right?

@nickvergessen
Copy link
Member Author

@rullzer yeah basically make sure all HTML is shown as plain text <strong>.
Markup should only be done via the Rich message stuff.

@rullzer
Copy link
Member

rullzer commented Jan 30, 2019

I'll try to verify this evening then

@rullzer
Copy link
Member

rullzer commented Feb 1, 2019

ros

Properly escaped on desktop.,

@MorrisJobke MorrisJobke merged commit 668b706 into master Feb 7, 2019
@MorrisJobke MorrisJobke deleted the bugfix/noid/unify-html-encoding-handling-with-other-ros-apps branch February 7, 2019 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants