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

Late escape site blocks #37880

Merged
merged 6 commits into from
Jan 17, 2022
Merged

Late escape site blocks #37880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b675cf
Site logo first pass
getdave Jan 11, 2022
44bfdcd
First pass site tagline
getdave Jan 11, 2022
d23bd4d
First pass site title
getdave Jan 11, 2022
d1b494e
Force heading level to be int in site title
getdave Jan 11, 2022
322e0b2
Escape HTML attr value not entire string
getdave Jan 12, 2022
c794dff
Fix lint error
getdave Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-library/src/site-logo/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function render_block_core_site_logo( $attributes ) {
// Add the link target after the rel="home".
// Add an aria-label for informing that the page opens in a new tab.
$aria_label = 'aria-label="' . esc_attr__( '(Home link, opens in a new tab)' ) . '"';
$custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . $attributes['linkTarget'] . '"' . $aria_label, $custom_logo );
$custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . esc_attr( $attributes['linkTarget'] ) . '"' . $aria_label, $custom_logo );
Copy link
Member

Choose a reason for hiding this comment

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

This is under a '_blank' === $attributes['linkTarget'] check, so we already know what the value is, and it's hardcoded without any user-input involved. Escaping this string is not necessary, it can just be _blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, if data is coming from a variable I believe we should escape at point of output. Who's to say what the variable might contain in future. It's hardcoded for now but that's no guarantee it will stay that way.

This is minor overhead for the sake of long term peace of mind and resilience against XSS.

Copy link
Member

Choose a reason for hiding this comment

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

I agree... But why not just make it target="_blank"? It can't be anything else. That way we make it easier to read, and at the same time it's a performance micro-optimization 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look again. If we can hard code inline when we build the markup then let's do that,

Copy link
Contributor Author

@getdave getdave Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I've looked again. Yes this block of code is behind the following conditional:

if ( $attributes['isLink'] && '_blank' === $attributes['linkTarget'] ) {

Whilst I recognise this is how it is now, that could easily change in the future.

What if there's a refactor and the $custom_logo markup ends up outside this conditional? What if the developer forgets to escape the linkTarget as part of the refactor?

I appreciate this is a "what if" scenario but given the low overhead (how many site logo blocks are going to be on one page?) I believe it is better to be safe than sorry when it comes to XSS. I believe it's a good trade off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the next step here? Is the addition of this esc_attr() a blocker? If it absolutely is I will revert the change.

}

$classnames = array();
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/site-tagline/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function render_block_core_site_tagline( $attributes ) {
return sprintf(
'<p %1$s>%2$s</p>',
$wrapper_attributes,
$site_tagline
esc_html( $site_tagline )
);
}

Expand Down
8 changes: 4 additions & 4 deletions packages/block-library/src/site-title/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,20 @@ function render_block_core_site_title( $attributes ) {
$aria_current = is_home() || ( is_front_page() && 'page' === get_option( 'show_on_front' ) ) ? ' aria-current="page"' : '';

if ( isset( $attributes['level'] ) ) {
$tag_name = 0 === $attributes['level'] ? 'p' : 'h' . $attributes['level'];
$tag_name = 0 === $attributes['level'] ? 'p' : 'h' . (int) $attributes['level'];
}

if ( $attributes['isLink'] ) {
$link_attrs = array(
'href="' . get_bloginfo( 'url' ) . '"',
'rel="home"',
'href="' . esc_url( get_bloginfo( 'url' ) ) . '"',
'rel="' . esc_attr( 'home' ) . '"',
$aria_current,
);
if ( '_blank' === $attributes['linkTarget'] ) {
$link_attrs[] = 'target="_blank"';
$link_attrs[] = 'aria-label="' . esc_attr__( '(opens in a new tab)' ) . '"';
}
$site_title = sprintf( '<a %1$s>%2$s</a>', implode( ' ', $link_attrs ), $site_title );
$site_title = sprintf( '<a %1$s>%2$s</a>', implode( ' ', $link_attrs ), esc_html( $site_title ) );
}
$wrapper_attributes = get_block_wrapper_attributes( array( 'class' => $align_class_name ) );

Expand Down