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.

Sign up for GitHub

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

Jump to bottom

Fix: Display HTML in titles of latest posts block. #13622

Merged
merged 1 commit into from
Feb 19, 2019
Merged

Fix: Display HTML in titles of latest posts block. #13622

merged 1 commit into from
Feb 19, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

Fixes: #13603

This PR makes sure if the title of a post contains HTML tags, the HTML tags are rendered correctly in the frontend and in the editor of latest posts block.

How has this been tested?

Create a new post write "Hello World!" in the title.
Publish the post.
Create another post add the latest posts block there. Verify the HTML in the title of the post we created before is correctly displayed in the block.
Publish the post verify the HTML is also correctly displayed on the frontend.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Package] Block library /packages/block-library [Block] Latest Posts Affects the Latest Posts Block labels Jan 31, 2019
@jorgefilipecosta jorgefilipecosta self-assigned this Jan 31, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
@gziolo gziolo requested review from Soean and ajitbohra February 5, 2019 16:18
@jorgefilipecosta jorgefilipecosta requested a review from a team February 5, 2019 17:40
@mapk
Copy link
Contributor

mapk commented Feb 6, 2019

I just tested this by adding some <em> tags around a word in my title and when viewing on the frontend, my word was italicized without showing any HTML tags. Great work! ✅

html-heading

@gziolo gziolo requested review from aduth and ellatrix February 6, 2019 06:20
gziolo
gziolo reviewed Feb 6, 2019
View reviewed changes
packages/block-library/src/latest-posts/edit.js Outdated
@@ -174,7 +175,12 @@ class LatestPostsEdit extends Component {
>
{ displayPosts.map( ( post, i ) =>
<li key={ i }>
<a href={ post.link } target="_blank">{ decodeEntities( post.title.rendered.trim() ) || __( '(Untitled)' ) }</a>
<RichText.Content
Copy link
Member

Choose a reason for hiding this comment

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

I’m not certain that RichText.Content is a perfect fit here. It looks like all you need is to wrap the title with RawText component from element package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gziolo we don't have a RawText component in the element package but I assumed you wanted to refer the RawHTML component and I updated the code to use it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, RawHTML :)

@jorgefilipecosta jorgefilipecosta force-pushed the fix/display-html-in-titles-of-latest-posts-block branch from c2c1f8d to 4b0a4bd Compare February 11, 2019 16:00
@jorgefilipecosta jorgefilipecosta force-pushed the fix/display-html-in-titles-of-latest-posts-block branch 2 times, most recently from 29f00e6 to 54f7420 Compare February 11, 2019 17:23
gziolo
gziolo reviewed Feb 12, 2019
View reviewed changes
packages/block-library/src/latest-posts/index.php
@@ -37,7 +37,7 @@ function render_block_core_latest_posts( $attributes ) {
$list_items_markup .= sprintf(
'<li><a href="%1$s">%2$s</a>',
esc_url( get_permalink( $post ) ),
esc_html( $title )
$title
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe? This is also used to render it on the frontend.

Copy link
Member

Choose a reason for hiding this comment

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

Is it safe? This is also used to render it on the frontend.

I think it's good to be prudent of, but in this instance I believe it's okay, given the following security consideration notes:

https://codex.wordpress.org/Function_Reference/the_title#Security_considerations

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing. That makes this code completely valid.

aduth
aduth reviewed Feb 18, 2019
View reviewed changes
packages/block-library/src/latest-posts/edit.js
<RawHTML>
{
post.title.rendered.trim() ||
__( '(Untitled)' )
Copy link
Member

Choose a reason for hiding this comment

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

Depending whether we consider translation as trusted strings†, it would be unsafe to allow this to be inserted in RawHTML

If the latter is unavoidable, and since translations should not be considered trusted strings, be sure to sanitize the result before echoing.

https://codex.wordpress.org/I18n_for_WordPress_Developers#HTML

† I've heard conflicting reports that they are "safe". In this case, it seems easy enough to just consider whether the title exists before deciding to output as RawHTML.

{
	post.title.rendered.trim() ?
		<RawHTML>{ post.title.rendered }</RawHTML> :
		__( '(Untitled)' )
}

packages/block-library/src/latest-posts/index.php
@@ -37,7 +37,7 @@ function render_block_core_latest_posts( $attributes ) {
$list_items_markup .= sprintf(
'<li><a href="%1$s">%2$s</a>',
esc_url( get_permalink( $post ) ),
esc_html( $title )
$title
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe? This is also used to render it on the frontend.

I think it's good to be prudent of, but in this instance I believe it's okay, given the following security consideration notes:

https://codex.wordpress.org/Function_Reference/the_title#Security_considerations

gziolo
gziolo approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This code looks good. I would suggest addressing the comment from @aduth https://github.com/WordPress/gutenberg/pull/13622/files#r257818347 before merging.

Thanks for working on this tricky issue 🙇

@jorgefilipecosta jorgefilipecosta force-pushed the fix/display-html-in-titles-of-latest-posts-block branch from 54f7420 to cc2bd02 Compare February 19, 2019 09:45
@jorgefilipecosta jorgefilipecosta merged commit b2861d5 into master Feb 19, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/display-html-in-titles-of-latest-posts-block branch February 19, 2019 10:04
@jorgefilipecosta
Copy link
Member Author

Thank you @aduth and @gziolo for the reviews and important security checks.

aduth
aduth reviewed Feb 19, 2019
View reviewed changes
packages/block-library/src/latest-posts/edit.js
</li>
) }
{ displayPosts.map( ( post, i ) => {
const titleTrimmed = post.title.rendered.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we trim the title?

@ellatrix ellatrix mentioned this pull request Mar 4, 2019
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@jorgefilipecosta @youknowriad
Fix: Display HTML in titles of latest posts block. (#13622)
3ca7834
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@jorgefilipecosta @youknowriad
Fix: Display HTML in titles of latest posts block. (#13622)
cf972bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@gziolo gziolo gziolo approved these changes

@Soean Soean Awaiting requested review from Soean

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@ellatrix ellatrix Awaiting requested review from ellatrix

@noisysocks noisysocks Awaiting requested review from noisysocks

@notnownikki notnownikki Awaiting requested review from notnownikki

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@jorgefilipecosta jorgefilipecosta

Labels
[Block] Latest Posts Affects the Latest Posts Block [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
5.2 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
4 participants
@jorgefilipecosta @mapk @gziolo @aduth