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

Fixed escaping issue #4452

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nazmulhudadev
Copy link

The variable $login_header_text escaped by esc_html at https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-login.php#L209

I'm no sure that is it should be escaped by esc_html or any other escaping function. So, I'm open for any suggestion to make changes.

Trac ticket: https://core.trac.wordpress.org/ticket/58305

The variable $login_header_text escaped by esc_html at https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-login.php#L209

I am open for any suggestion.
@@ -206,7 +206,7 @@ function login_header( $title = 'Log In', $message = '', $wp_error = null ) {

?>
<div id="login">
<h1><a href="<?php echo esc_url( $login_header_url ); ?>"><?php echo $login_header_text; ?></a></h1>
<h1><a href="<?php echo esc_url( $login_header_url ); ?>"><?php echo esc_html( $login_header_text ); ?></a></h1>
Copy link
Contributor

@costdev costdev May 14, 2023

Choose a reason for hiding this comment

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

I'm not sure esc_html() is the right function to use here. This is inside an a element which has a transparent content model:

Transparent, but there must be no interactive descendant, a element descendant, or descendant with the tabindex attribute specified.
ref

As it's transparent, valid content matches that of its parent, a h1 element (with exceptions as noted above).

The h1 element has a phrasing content model.

Escaping all HTML could therefore break BC if folks have used something like:

get_bloginfo( 'title' ) . '<sup>&trade;</sup>'

That said, it doesn't mean we need to support everything here, but it doesn't seem like we should be blocking something like the simple example above.

We may find esc_js() or a selective KSES function to be of use here. Curious what others think may be appropriate.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for reviewing the changes.

Yes, I had the same thought as you, and I also wanted to use esc_js() or a selective KSES function. However, I was uncertain about which one would be more appropriate.

@SergeyBiryukov
Copy link
Member

Hi there, thanks for the PR!

We may find esc_js() or a selective KSES function to be of use here. Curious what others think may be appropriate.

It looks like esc_js() is generally not used outside of JavaScript context, as it escapes text strings specifically for echoing in JS, and is intended to be used for inline JS.

wp_kses() with a list of allowed tags might be more appropriate, but I wonder what would that solve in practice, as if someone has access to add arbitrary code via the login_headertext filter, they can use pretty much any other filter to execute any code, for example login_message directly below. So I'm not sure if escaping is necessary here. Curious to see what others think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants