Make WordPress Core

Opened 15 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#12056 closed enhancement (fixed)

target="_blank" being stripped from Profile Bio and Category Description

Reported by: lovewpmu's profile lovewpmu Owned by: audrasjb's profile audrasjb
Milestone: 6.8 Priority: normal
Severity: normal Version: 2.9.2
Component: Formatting Keywords: has-patch needs-testing-info has-unit-tests has-screenshots
Focuses: Cc:

Description

Many apologies if this is a duplicate. I have searched but did not find it yet posted.

I noticed that target="_blank" is being stripped from my "a href" tags my profile "Biographical Info" field even though the "a href" with the URL and closing tag still remain. It happens every time I save my profile.

This was independently verified.

It is a regular wordpress install running 2.9.1 (not wordpressmu, etc.).

My original thread can be found here:
http://wordpress.org/support/topic/355388?replies=1

Attachments (5)

12056.diff (324 bytes) - added by nofearinc 11 years ago.
sample target addition to $allowedtags
Testing-pullrequest7792.png (274.7 KB) - added by gaellebesson 2 months ago.
Testing PullRequest 7792
test-PR-7792.png (168.8 KB) - added by nuryko 2 months ago.
Test PR 7792
Before-Testing-pullrequest7792.mov (8.6 MB) - added by maximemeganck 2 months ago.
Before testing the PR 7792
After-Testing-pullrequest7792.mp4 (3.2 MB) - added by maximemeganck 2 months ago.
After testing the PR 7792

Change History (57)

#1 @miqrogroove
15 years ago

The target attribute is still valid as of XHTML 1.0 Transitional, but you will probably have to use onclick instead. Probably neither are allowed to non-administrators.

#2 @lovewpmu
15 years ago

there is no reason why someone who is filling out their bio with links to the relevant online info should need to code in javascript. in fact, most people using wordpress -- ie. the common user -- probably cannot. i believe the links on the HTML generated for posts, _blank is used for the "Open in New Window" option.

this should be standardized and completely independent from whether one is or is not an administrator.

#3 @miqrogroove
15 years ago

It does seem to be using the same filters for admins. That's a bit surprising.

#4 @miqrogroove
15 years ago

It's set up as a default filter for pre_user_description so you could override that with a plugin.

You might have a case for enabling unfiltered bios for administrators, but beyond that I think the "it's allowed in posts" argument isn't going to fly. WordPress explicitly filters bios the same way as comments instead of posts.

#5 @nacin
15 years ago

  • Milestone changed from 2.9.2 to Future Release

#6 @bsutcliffe
15 years ago

  • Cc bsutcliffe added
  • Keywords html strip profile bio category description link target added
  • Summary changed from target="_blank" being stripped from Profile Bio to target="_blank" being stripped from Profile Bio and Category Description
  • Version set to 2.9.2

This also occurs in category descriptions. If I add a link to one of my category descriptions and specify target="_blank", the target reference gets stripped out when saving. Unlike bios, this filter definitely should not be applied.

#7 @bsutcliffe
15 years ago

  • Cc bsutcliffe removed
  • Keywords html strip profile bio category description link target removed

#8 @wojtek.szkutnik
15 years ago

  • Keywords gsoc added

#9 @sjefen6
14 years ago

Images, lists and class specifications are also being stripped away from category descriptions. What is the reasoning for being so strict with category descriptions?

@nofearinc
11 years ago

sample target addition to $allowedtags

#10 @nofearinc
11 years ago

Adding the target to the anchor array of $allowedtags would solve that globally in kses.php after the default filters are applied in default-filters.php, but I guess there might be a security risk with adding a frame target to external location? I've uploaded a sample proof of concept above.

Not sure if there is a way to globally allow a given value for an attribute in $allowedtags.

#11 @nacin
11 years ago

  • Component changed from General to Formatting
  • Type changed from defect (bug) to enhancement

This is "intentional" but we may be able to fix this now. kses can now make changes based on a particular filter. Otherwise changing $allowedtags would open this up for comments.

#12 @miqrogroove
10 years ago

  • Keywords kses added

#13 @harmr
8 years ago

+1 for enabling href/target by default for $allowedtags

#14 @blogitsolutions
7 years ago

+1 for enabling _blank (ticket is now 8 years old)

Last edited 7 years ago by blogitsolutions (previous) (diff)

#15 @spacedmonkey
7 years ago

  • Keywords has-patch dev-feedback added
  • Milestone changed from Future Release to 5.0

#16 @stefahn
7 years ago

+1 for letting us open links in the author bio in a new tab.

#17 @johnbillion
6 years ago

  • Milestone changed from 5.0 to Future Release

#18 @ctrlaltdelete
6 years ago

Awesome.

Last edited 6 years ago by ctrlaltdelete (previous) (diff)

#19 @arvindwp
5 years ago

Not yet resolved? facing same issue. Author social bio should not cause visitor to mve away from website. Any fix?

#20 @nofearinc
5 years ago

Just tested my 2013 patch above and it still works here. Once applied, you can save target="_blank" to your bio and it no longer gets stripped.

#21 @stefahn
5 years ago

Who will merge this patch into master?

This ticket was mentioned in PR #6866 on WordPress/wordpress-develop by @nirajgirixd.


8 months ago
#22

## Description

This PR ensures that target="_blank" is preserved in the Biographical Info and Category Description fields to maintain the intended link behavior.

## Changes Made

  • Add target attribute for anchor tag in $allowedtags in wp-includes/kses.php.

## Trac ticket: https://core.trac.wordpress.org/ticket/12056

@martin.krcho commented on PR #6866:


8 months ago
#23

This change will affect the behaviour of all kses_* functions. I am not sure if that is desired.

@nirajgirixd commented on PR #6866:


8 months ago
#24

@martinkrcho You are right; changing the $allowedtags would indeed affect the behaviour of all kses_* functions, which might not be ideal. I will look for an alternative solution.

@nirajgirixd commented on PR #6866:


5 months ago
#25

@martinkrcho Could you please take a look at the PR and let me know if the new changes are satisfactory?

@martin.krcho commented on PR #6866:


5 months ago
#26

@nirajgiriXD I prefer this approach to the previous one. It affects only a limited number of filters.

Would you be able to provide unit tests or provide instructions for manual testing (in the Trac ticket)?

#27 @martin.krcho
5 months ago

  • Keywords needs-testing-info needs-unit-tests added

@nirajgirixd commented on PR #6866:


5 months ago
#28

@martinkrcho I've added the testing instructions along with some screenshots to the PR description.

#29 @nirajgirixd
4 months ago

I've submitted a PR to resolve this issue, and it has already been approved. Could someone please review it and merge it into the trunk? If any further changes are needed, please feel free to suggest them.

cc: @nacin, @spacedmonkey, @johnbillion

#30 @spacedmonkey
4 months ago

  • Milestone changed from Future Release to 6.8

@nirajgirixd any change like this would need a unit test to validate the change doesn’t break anything. Hopefully we can get this change in the next release of WordPress.

#31 @nirajgirixd
4 months ago

@spacedmonkey Thank you for the feedback.

This ticket was mentioned in PR #7792 on WordPress/wordpress-develop by @sukhendu2002.


3 months ago
#32

  • Keywords has-unit-tests added; needs-unit-tests removed

## Preserve target="_blank" in Biographical Info and Category Description

### Description
This pull request ensures that the target="_blank" attribute is preserved when adding links in the Biographical Info and Category Description fields. Previously, this attribute was being stripped by the kses sanitization process.

Additionally, new unit tests have been added to verify the preservation of the target="_blank" attribute in these specific contexts.

### Changes Made

  • Added the target attribute to the allowed tags in the user_description and pre_term_description contexts in wp_kses_allowed_html().
  • Added the following new unit tests:
    • test_target_attribute_preserved_in_user_description()
    • test_target_attribute_preserved_in_term_description()
    • test_target_attribute_preserved_in_user_description_with_other_attributes()
    • test_target_attribute_preserved_in_term_description_with_other_attributes()

#33 @audrasjb
2 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Thanks for the PR! Self assigning for final review.

@gaellebesson
2 months ago

Testing PullRequest 7792

#36 @gaellebesson
2 months ago

Hello, this is my first contribution to WordPress Core from the Whodunit Contributor Day :)

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7792

Environment

  • OS: macOS
  • Web Server: WordPress Playground
  • PHP: Playground Default
  • WordPress: current version
  • Browser: Chrome
  • Theme: Twenty Twenty-Five
  • Active Plugins: None

Actual Results

  • I pasted the following code into the biographical field
    <a href="" target="_blank">Lien externe </a>
    
  • The "target Blank" was preserved


  • ✅ Issue resolved with patch.

Additional Notes

  • See the screenshot above
Last edited 2 months ago by gaellebesson (previous) (diff)

@nuryko
2 months ago

Test PR 7792

#37 @nuryko
2 months ago

Test Report

Hi !
Here another test from the Whodunit Contributor Day !

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7792

Environment

  • OS: macOS
  • Web Server: WordPress Playground
  • PHP: Playground default
  • WordPress: current version
  • Browser: Arc 1.72.1 (Chromium)
  • Theme: Twenty Twenty-Five
  • Active Plugins: None

Actual Results

  • Pasted in category description :
    <a href=“” target=“_blank”>This is a test</a>
    
  • target=“_blank” preserved
  • ✅ Issue resolved with patch.

Additional Notes

  • See the screenshot above
Last edited 2 months ago by nuryko (previous) (diff)

@guillaumeturpin commented on PR #7792:


2 months ago
#38

I just checked the implementation and tests

LGTM 🎉

@maximemeganck
2 months ago

Before testing the PR 7792

@maximemeganck
2 months ago

After testing the PR 7792

#39 @audrasjb
2 months ago

  • Keywords commit has-screenshots added; dev-feedback removed
  • Status changed from reviewing to accepted

Looks like we're good to go.

#40 @ranafge
2 months ago

Test Report


This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7792

Environment
OS: macOS
Web Server: WordPress Playground
PHP: Playground Default
WordPress: current version
Browser: Chrome
Theme: Twenty Twenty-Five
Active Plugins: None

Actual Results


I pasted the following code into the biographical field

<a href="" target="_blank">Lien externe </a>

The "target Blank" was preserved


✅ Issue resolved with patch.


#41 @azaozz
2 months ago

  • Focuses accessibility added
  • Keywords commit removed

If I'm not mistaken links with target=_blank are not particularly good for accessibility reasons (can be disorienting for screen reader users). For that reason they should be used very sparingly, only when absolutely necessary. Should there be a warning about that for the new places they are allowed? Also, can't remember if rel=noopener is still needed for some browsers, think there was a ticket about that.

Marking this for a quick review by the accessibility team.

#42 follow-up: @audrasjb
2 months ago

Hello @azaozz,

This is not about explicitly encouraging people to use such an attribute (like the block editor does within its UI without informing people that it can be a poor user experience) but mostly about allowing it in the code provided by authors (they provide directly the HTML code, here). On a bio profile field, it can make sense to allow it (see comments above).

Concerning the browser implementation:

Note: Setting target="_blank" on <a>, <area> and <form> elements implicitly provides the same rel behavior as setting rel="noopener" which does not set window.opener.

Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener

Therefore I think we are definitely good to go with the current changeset.

Last edited 2 months ago by audrasjb (previous) (diff)

#43 @audrasjb
2 months ago

Let's keep the accessibility workflow keyword for now, but I'm not convinced it is directly an accessibility issue. Or if it is, let's start adding a link to target:blank best practices all over the Block Editor :)

#44 in reply to: ↑ 42 @azaozz
2 months ago

Replying to audrasjb:

This is not about explicitly encouraging people to use

Right, but target=_blank is not in the default KSES filters. I.e. there is some effort to discourage people from using it. Afaik the reason seems to be that a lot of people tend to use this incorrectly seemingly because of some old/marginal/incompetent SEO advice?

Therefore I think we are definitely good to go with the current changeset.

Right, adding of noopener was removed from WP some time ago.

Let's keep the accessibility workflow keyword for now

Sure, thought it would be nice to ping the Accessibility team for a quick opinion.

Last edited 2 months ago by azaozz (previous) (diff)

#45 follow-up: @joedolson
6 weeks ago

  • Focuses accessibility removed

Overall, I'd prefer not to allow target=_blank any more than absolutely necessary; it mostly introduced accessibility problems. However, when it comes to content creation, the reality is that we have no way of knowing how that content will be used - it's hypothetically possible it could be in a context where opening in a new tab is important.

I don't see any particularly good reason to change this, but I don't have a particularly good reason to block it on the accessibility front, either.

Removing the accessibility focus, as this doesn't impact the accessibility of anything produced directly in WordPress, only the user's content output.

#46 @audrasjb
6 weeks ago

Thanks for the feedback @joedolson :)

#47 @rinkalpagdar
6 weeks ago

Test Report

Description

This report validates whether the indicated patch works as expected.

PR tested: https://github.com/WordPress/wordpress-develop/pull/7792

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.25
  • Server: nginx/1.27.2
  • Database: mysqli (Server: 8.0.40 / Client: mysqlnd 8.2.25)
  • Browser: Chrome 131.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

#48 in reply to: ↑ 45 @azaozz
5 weeks ago

  • Keywords gsoc kses removed

Replying to joedolson:

I don't see any particularly good reason to change this, but I don't have a particularly good reason to block it on the accessibility front, either.

Thanks for having a look! Yea perhaps I'm overthinking this; it is up to the author/site owner to decide what they want to do and why, so there's no good reason to block it.

@sukhendu2002 commented on PR #7792:


5 weeks ago
#49

@martinkrcho, Thanks for the suggestion! I’ve updated the tests to use data providers accordingly.

#50 @mikinc860
5 weeks ago

Test Report

This report validates whether the indicated patch works as expected.
Tested Patch: https://github.com/WordPress/wordpress-develop/pull/7792

Environment

OS: Windows 11
Web Server: WordPress Playground
PHP: Playground Default
WordPress: current version
Browser: Chrome
Theme: Twenty Seventeen
Active Plugins: None

Actual Results

✅ Issue resolved with a patch.

#51 @audrasjb
4 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 59677:

Formatting: Preserve target="_blank" in Biographical Info and Category Description.

This changeset ensures the target="_blank" attribute is preserved when adding links in the Biographical Info and Category Description fields. Previously, this attribute was being stripped by the KSES sanitization process.

Additionally, new unit tests have been added to verify the preservation of the target="_blank" attribute in these specific contexts.

Props lovewpmu, miqrogroove, bsutcliffe, sjefen6, nofearinc, nacin, harmr, blogitsolutions, stefahn, nirajgirixd, martinkrcho, spacedmonkey, sukhendu2002, audrasjb, gaellebesson, nuryko, guillaumeturpin, maximemeganck, ranafge, azaozz, joedolson, rinkalpagdar, mikinc860.
Fixes #12056.

Note: See TracTickets for help on using tickets.