Make WordPress Core

Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#40916 closed enhancement (fixed)

Add "noreferrer" to comments in dashboard

Reported by: cybr's profile Cybr Owned by: davidbaumwald's profile davidbaumwald
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch commit
Focuses: accessibility, administration Cc:

Description

Within /wp-admin/edit-comments.php, one might check out a posted link before verifying the comment's legitimacy.

While doing so, we're sending the linked site that their comment got through; plausibly opening yourself to complications.

Whatever the reason may be, I don't think we should give spammers any information.

To resolve this, simply add rel=noreferrer to those links.

Attachments (8)

class-wp-comments-list-table.diff (1.1 KB) - added by adam3128 7 years ago.
Conditional check for comment status. If not approved, add noreferrer to anchor tags shown on dashboard.
class-wp-comments-list-table-v2.diff (628 bytes) - added by adam3128 7 years ago.
Version 2 of solution with target blank and noopener
40916.diff (3.9 KB) - added by Cybr 7 years ago.
wp_unbind_links() function, attached to comments through JIT filter
class-wp-comments-list-table-v3.diff (963 bytes) - added by erayalakese 7 years ago.
A small modification to reduce code repeat
40916.patch (3.8 KB) - added by andraganescu 5 years ago.
Refreshed patch 40916.diff
40916.2.2.diff (836 bytes) - added by audrasjb 3 years ago.
Comments: In the admin, open comment author links in a new tab, and add a noopener rel attribute.
Capture d’écran 2021-09-26 à 15.22.40.png (128.7 KB) - added by audrasjb 3 years ago.
works fine
40916.rel.diff (738 bytes) - added by sabernhardt 3 years ago.
adding rel attribute without target="_blank", including comment

Download all attachments as: .zip

Change History (26)

#1 @johnbillion
7 years ago

  • Keywords needs-patch good-first-bug added
  • Version trunk deleted

@adam3128
7 years ago

Conditional check for comment status. If not approved, add noreferrer to anchor tags shown on dashboard.

#2 @adam3128
7 years ago

Hey guys,

First ticket I've contributed code towards the core for. Attached a diff file which I think resolves this issue.

Any issues, just let me know.

Hope this helps,

Adam

#3 @adam3128
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

#4 @Cybr
7 years ago

We might also wish to add target=_blank and add rel="noopener noreferrer" to optimize flow and prevent window hijacking thereof.

Ref: https://wordpress.org/support/topic/remove-rel-noopener-noreferrer-in-wordpress-4-7-4/#post-9057251

I think we shouldn't go away from the admin area in the first place, also.

@adam3128 I think the "approved" status check is redundant, even though it's a nice touch. Visitors comment for the front-end, after all.

@adam3128
7 years ago

Version 2 of solution with target blank and noopener

#5 @adam3128
7 years ago

  • Keywords 2nd-opinion removed

Hey @Cybr,

Thanks for the feedback above. That sounds good to me.

I've attached the new revision with your comments taken on board.

Any issues, just let me know.

Adam

@Cybr
7 years ago

wp_unbind_links() function, attached to comments through JIT filter

#6 @Cybr
7 years ago

  • Keywords needs-testing 2nd-opinion added

The file I've attached incorporates @adam3128's changes (author links).
Also including a filter that applies to all comments' contents on the wp-admin/edit-comments.php screen.

The filter catches all links through a new function (wp_unbind_links()) and removes all formats of target and rel attributes, finally replacing them with empty strings without fail. Even rel="something example'" gets caught.

From there it adds target=_blank rel="noopener noreferrer" to each found link.

The code has been tested and works on PHP 5.2.16 and PHP 7.1.5.

What needs to be determined is the placement of the filter.
i.e. Do we want to catch all new comments, all current comments, or only the comments on the said admin screen?

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

@erayalakese
7 years ago

A small modification to reduce code repeat

#7 @DrewAPicture
7 years ago

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

Assigning ownership to mark the good-first-bug as "claimed".

#8 @pratikkry
5 years ago

any progress on this?

@andraganescu
5 years ago

Refreshed patch 40916.diff

#9 @andraganescu
5 years ago

  • Keywords good-first-bug 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

@Cybr I took the liberty and refreshed your patch as it didn't apply cleanly via the grunt patch command.

@erayalakese @adam3128 I think @Cybr 's patch is more complete solving for more than the author's URL, but any link in the comment. Also in the other patches the check for approved authors seems not needed (and did not seem to be working in my testing either) as all these links should be treated the same.

About @Cybr 's question on:

What needs to be determined is the placement of the filter.
i.e. Do we want to catch all new comments, all current comments, or only the comments on the said admin screen?

I think we should stick to this screen for now.

@pratikkry as you seem interested into this ticker perhaps you have a bit of time to test this latest patch?

@audrasjb
3 years ago

Comments: In the admin, open comment author links in a new tab, and add a noopener rel attribute.

#10 @audrasjb
3 years ago

  • Focuses accessibility added
  • Keywords needs-testing removed
  • Milestone changed from Future Release to 5.9

In 40916.2.2.diff, I suggest to simplify the approach proposed in the previous patch, and to make in more accessible by adding a screen-reader-text to the link.

Moving for 5.9 consideration.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

This ticket was mentioned in Slack in #accessibility by alexstine. View the logs.


3 years ago

#13 @joedolson
3 years ago

I'm strongly opposed to adding unnecessary links with target=blank to the admin. Using noopener would be fine to protect from hijacking the window, but opening in a new window disrupts the user flow and prevents the user from making a choice.

Since these links are potentially untrusted, I would consider it reasonable to add an interstitial before visiting those links - but automatically opening in a new window just makes thing less predictable.

If target=blank is to be kept, it needs to be done in an accessible way: that means that this needs to be communicated to all users, not just users of screen readers. That means either visible by default or capable of becoming visible on focus/touch interactions. The screen reader text alone is not sufficient.

@sabernhardt
3 years ago

adding rel attribute without target="_blank", including comment

#14 @sabernhardt
3 years ago

I added another option, to disallow referrer information without opening links in a new tab. It includes noopener, too, in case the user chooses to open in a new tab.

#53843 may remove the rel="noopener" attribute from other links in the future, so the patch has a comment to discourage editing this at the same time as those.

#15 @audrasjb
3 years ago

  • Keywords commit added

As per the Accessibility team bug scrub, 40916.rel.diff looks like the best option we have. I tested this new patch and it looks like it does the job.

I think we're safe to commit it into 5.9. Thanks @sabernhardt for the refresh 👍

#16 @davidbaumwald
3 years ago

  • Owner changed from adam3128 to davidbaumwald
  • Status changed from assigned to accepted

#17 @davidbaumwald
3 years ago

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

In 52007:

Comments: Add noopener noreferrer to author links in list table.

When viewing the listing of all comments, author links previously passed referrer information to untrusted URLs. This change adds noreferrer to each author link, as well as noopener to prevent the passing of information about the parent window.

Props cybr, adam3128, erayalakese, andraganescu, audrasjb, joedolson, sabernhardt. 
Fixes #40916.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.