WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 19 months ago

#43280 closed defect (bug) (fixed)

Add rel="noopener" to target="_blank" links by default in image widget

Reported by: audrasjb Owned by: pento
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.4
Component: Widgets Keywords: has-patch commit has-unit-tests
Focuses: Cc:

Description

Core previously introduced rel="noopener" attribute to target="_blank" links –which is fine– but I think image widget should also add this attribute to image link.
I know there is already a "Link rel" text input but I think we should add rel="noopener" attribute by default when "Open link in a new tab" option is checked and "Link rel" is empty.

Attachments (6)

43280.diff (634 bytes) - added by audrasjb 3 years ago.
Adds default rel noopener attribute when using target blank in media widget and Link rel field is empty
43280.2.diff (644 bytes) - added by audrasjb 3 years ago.
Adds both noopener and noreferrer values and refreshes the patch against trunk
43280.3.diff (1.4 KB) - added by welcher 20 months ago.
Adds some unit tests
43280.4.diff (1.2 KB) - added by welcher 20 months ago.
Removes needless parameter from tests setup
43280.5.diff (1.1 KB) - added by audrasjb 20 months ago.
Use wp_targeted_link_rel() to add rel attributes
Capture d’écran 2019-03-21 à 21.23.41.png (47.9 KB) - added by audrasjb 20 months ago.
Rel attributes are added with wp_targeted_link_rel() function so they are always here in front

Download all attachments as: .zip

Change History (27)

@audrasjb
3 years ago

Adds default rel noopener attribute when using target blank in media widget and Link rel field is empty

#1 @audrasjb
3 years ago

  • Keywords has-patch added

#3 @audrasjb
3 years ago

Related: #43290 (menu walker)

#4 @welcher
3 years ago

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

As per my comment in #43290, I'm thinking we should also add the noreferrer here as well to be consistent

#5 @welcher
3 years ago

  • Milestone changed from Awaiting Review to 5.0

@audrasjb
3 years ago

Adds both noopener and noreferrer values and refreshes the patch against trunk

#6 @pento
2 years ago

  • Milestone changed from 5.0 to 5.1

#7 @welcher
22 months ago

Patch looks good to me - thanks for updating @audrasjb!

#8 @pento
22 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from 5.1 to 5.2
  • Type changed from defect (bug) to enhancement

Similar to #43290, this change is good to land in 5.2. It could do with some unit tests.

#9 @desrosj
20 months ago

@welcher @audrasjb are you able to add some unit tests for this before 5.2 beta 1 in a little under 2 days time?

@welcher
20 months ago

Adds some unit tests

#10 @audrasjb
20 months ago

  • Keywords commit added

Thanks @welcher you rock!

#11 @audrasjb
20 months ago

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

@welcher
20 months ago

Removes needless parameter from tests setup

#12 @afercia
20 months ago

As mentioned on #46421, I'd suggest to investigate on the actual usefulness of noreferrer:

#13 @audrasjb
20 months ago

@afercia I think both of the rel attributes should be added, in order to keep some consistency with https://developer.wordpress.org/reference/functions/wp_targeted_link_rel/
If we want to remove noreferrer, I believe we should remove it from that function :-)

#14 @afercia
20 months ago

@audrasjb right. Then, keeping the patch as is (same for the other related ones) seems to most appropriate thing to do. I'd suggest to open a separate ticket for discussion though.
(also, noting those functions lack a docblock @since tag)

#15 @afercia
20 months ago

For consistency with images added in the post content, I'd think the behavior should be different.

In the post content, images get noopener noreferrer even if users set their own value. Also, when reopening the details modal, the "rel" field is populated with noopener noreferrer plus any custom value previously added:

http://cldup.com/3gBfUq6uDV.png

Instead, with this patch noopener noreferrer are added only if the field is empty. Also, when reopening the details modal the field is empty.

#16 @audrasjb
20 months ago

@afercia You're right. We should use the same behaviour as post_content. I'll try to have a patch ready in the next few hours. Otherwise, we should punt it to the next release.

@audrasjb
20 months ago

Use wp_targeted_link_rel() to add rel attributes

@audrasjb
20 months ago

Rel attributes are added with wp_targeted_link_rel() function so they are always here in front

#17 @audrasjb
20 months ago

@afercia with 4328.5.diff, the rel atributes are added with wp_targeted_link_rel() so they are always added in front-end.

#18 @afercia
20 months ago

@audrasjb personally I like this new approach. Not my area of expertise though. I think there are two options:

  • change this ticket (and the other related ones) to type: bug, as this is about security hardening and it was missed in the original ticket #43187. Having a few more days may help asking a review from the ones involved in #43187 who have better knowledge on this issue
  • punt to 5.3 :)

#19 @audrasjb
20 months ago

  • Type changed from enhancement to defect (bug)

Ok, thanks for your feedback :)
I'm changing both #43290 and #43280 to type "bug" to see if we can have a review in the next few days or if we have to punt them to 5.3.
(I think #46421 if ready to ship by the way)

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

#20 @pento
19 months ago

  • Owner changed from audrasjb to pento
  • Status changed from assigned to accepted

#21 @pento
19 months ago

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

In 45144:

Widgets: Add rel="noopener noreferrer" to links with target="_blank" in the Image widget.

Props audrasjb, welcher, afercia.
Fixes #43280.

Note: See TracTickets for help on using tickets.