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

Post Title: fix special chars #18616

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Nov 19, 2019

Description

This commit ensures setting rightly of the title content, escaping and unescaping from special chars.

How has this been tested?

Add special chars in the post title. Save. Check that the chars are not rightly escaped in the front-end.
It's possible to inject javascript from the title of the post:

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

This commit ensures setting rightly of the title content, escaping and unescaping from special chars.
@retrofox retrofox requested review from aduth and youknowriad and removed request for talldan November 19, 2019 18:02
@retrofox retrofox added the [Type] Bug An existing feature does not function as intended label Nov 19, 2019
@retrofox retrofox merged commit c21dbfe into master Nov 19, 2019
@retrofox retrofox deleted the update/fix-post-title-special-chars branch November 19, 2019 22:05
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
@aduth
Copy link
Member

aduth commented Jan 29, 2020

See: #19898

I'm not sure about this change. What does it mean "setting rightly"? What issue is this fixing?

It's possible to inject javascript from the title of the post:

In the future, if there's a suspected security issue, I would recommend to follow the reporting process detailed at:

https://github.com/WordPress/gutenberg/blob/master/SECURITY.md

However, in this case, it's entirely intentional that a title can include HTML if the user has the correct permissions to be able to be able to do so.

See also: https://make.wordpress.org/core/handbook/testing/reporting-security-vulnerabilities/#why-are-some-users-allowed-to-post-unfiltered-html

Users with Administrator or Editor roles are allowed to publish unfiltered HTML in post titles, post content, and comments, and upload HTML files to the media library. WordPress is, after all, a publishing tool, and people need to be able to include whatever markup they need to communicate.

(Emphasis mine)


In #19898, I have recommended to revert this pull request. Would you foresee there being any unintended consequences of a revert?

@retrofox
Copy link
Contributor Author

Hi, @aduth. Thanks for taking over the issue and giving feedback.

In #19898, I have recommended to revert this pull request. Would you foresee there being any unintended consequences of a revert?

After reading your comments and the more context about this, I think it's logical just reverting the PR. To be honest I think misunderstood the issue and consequently its solution. My bad :-(

We didn't get too much feedback on that moment so we proceeded to merge it. Glad to help with the revert PR, or helping in whatwever we need to go ahead.

@aduth
Copy link
Member

aduth commented Jan 29, 2020

No worries @retrofox . If anything, I just wanted to be extra clear what the expectations were, in case there would be any potential breakage from doing a revert. I've got a branch in progress already. There are a few other additional changes to account for, such as #19187. I can ping you when it's ready, in case you can help with the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants