WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 8 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:
PR Number:

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 22 months 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 21 months ago.
Adds both noopener and noreferrer values and refreshes the patch against trunk
43280.3.diff (1.4 KB) - added by welcher 8 months ago.
Adds some unit tests
43280.4.diff (1.2 KB) - added by welcher 8 months ago.
Removes needless parameter from tests setup
43280.5.diff (1.1 KB) - added by audrasjb 8 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 8 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
22 months ago

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

#1 @audrasjb
22 months ago

  • Keywords has-patch added

#3 @audrasjb
22 months ago

Related: #43290 (menu walker)

#4 @welcher
21 months 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
21 months ago

  • Milestone changed from Awaiting Review to 5.0

@audrasjb
21 months ago

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

#6 @pento
13 months ago

  • Milestone changed from 5.0 to 5.1

#7 @welcher
10 months ago

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

#8 @pento
10 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
8 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
8 months ago

Adds some unit tests

#10 @audrasjb
8 months ago

  • Keywords commit added

Thanks @welcher you rock!

#11 @audrasjb
8 months ago

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

@welcher
8 months ago

Removes needless parameter from tests setup

#12 @afercia
8 months ago

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

#13 @audrasjb
8 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
8 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
8 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
8 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
8 months ago

Use wp_targeted_link_rel() to add rel attributes

@audrasjb
8 months ago

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

#17 @audrasjb
8 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
8 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
8 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 8 months ago by audrasjb (previous) (diff)

#20 @pento
8 months ago

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

#21 @pento
8 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.