WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 6 weeks ago

#39097 reopened defect (bug)

Links in embeds can't be opened in a new tab

Reported by: smerriman Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: needs-patch needs-testing
Focuses: accessibility Cc:

Description

While reading https://make.wordpress.org/core/2016/12/05/wordpress-4-7-field-guide/, which contains many embeds, I wanted to open up all items of interest in new tabs so I could read through them.

In Firefox, mousewheel-clicking, ctrl-clicking, or right clicking a link in an embed immediately opens the link in the same tab (in the last case, without the normal right click menu appearing at all).

In Chrome, mousewheel-clicking does nothing; ctrl-clicking opens it in the same tab, while right clicking does work.

This appears to all be caused by the sandbox attribute on the iframe.

Not allowing the link to open in a new tab seems very wrong, especially when multiple embeds are in a post.

Change History (28)

#1 @swissspidy
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #35239.

#2 @johnbillion
3 years ago

  • Keywords needs-patch needs-testing added
  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

This is not actually a duplicate of #35239. The bug reported is that it's not possible to use ctrl/cmd click to open a link in an embed in a new tab.

This behaviour can be seen in the 4.7 field guide post at https://make.wordpress.org/core/2016/12/05/wordpress-4-7-field-guide/ where the bug was also reported in the comments.

#3 @swissspidy
3 years ago

Cmd+click and why it's not possible right now has been discussed in exactly this ticket, even though the inital question was about making that the default behaviour. Anyway, worth a read.

#4 @smerriman
3 years ago

Yep, the other ticket contains details on the same issue. This feels like a pretty major flaw of embeds though.

Website owners often add target="_blank" to try to "keep people on their site" - this is of course a bad thing to do as discussed in the other ticket. However, having a feature which *forces* someone off their site, highly unexpectedly, and disables the right click context menu feels even worse. I can't imagine any site owner would want to use embeds if they knew this would be the result.

I'm sure there were good reasons for hijacking the links in the way done currently, but most uses of iframes would never require this (think, say, Twitter embeds) - does the gain outweigh the cost?

Last edited 3 years ago by smerriman (previous) (diff)

#5 @swissspidy
3 years ago

I'm sure there were good reasons for hijacking the links in the way done currently, but most uses of iframes would never require this (think, say, Twitter embeds) - does the gain outweigh the cost?

The problem is, when you embed tweets you know they come from a trusted source. With oEmbed discovery, you can basically embed any website, which makes it quite dangerous. Using JavaScript trickery, one could easily trick visitors into visiting sites they didn't intend to visit. That's why currently you can only click on links leading to the same host.

In #35239 I shared an idea on how to resolve this and enable opening links in a new tab by using cmd/ctrl click and why it's not possible right now. Hence my point that this ticket here is a duplicate.

#6 @swissspidy
3 years ago

  • Version set to 4.4

#7 @newsplugin.com
2 years ago

What about the right button? I understand that there may be problems when opening links, but I see no reason why right button default functionality of context menu need to be disabled. Ever.

#8 @swissspidy
2 years ago

@newsplugin.com As far as I can tell, there's no problem with the context menu. You should be able to right click -> open in new tab without problems. Browsers will handle this differently and won't execute our JavaScript in that case. The problem is just with cmd/ctrl + click.

#9 @smerriman
2 years ago

Nope, it still definitely affects right clicking, as mentioned in my original ticket. In Firefox, the behaviour appears to have changed slightly; I now see the context menu appear, but the link starts loading instantly at the same time, so I quickly end up on the wrong page.

#10 @Mte90
2 years ago

I tried to replicate right now but seems that oembed are disabled in the page on the first comment.

#11 @swissspidy
2 years ago

@Mte90 You can simply embed a post on your local blog in another post to test embeds.

#12 @Mte90
20 months ago

After a little bit of testing seems that to enable from an iframe to open a link in a new tab is something that need to be changed in the content of the link of the iframe.
For a purpose of security need a little bit of javascript hacking. So the only way is to inject a little bit of JS in every page that contain the embed, is something fine on WordPress?

#13 @swissspidy
20 months ago

@Mte90 I'm not sure I fully understand your proposal. The problem with this ticket right now is that embeds need to change their postMessage() calls in a way that they're not backward compatible with the current way. Instead of just sending the URL being clicked, they also need to send whether the tab should open in a new window or not. However, opening links in a new window via JavaScript is usually blocked by browsers. That's why this tickets sounds more like a wontfix to me.

If you want to completely allow shift-clicks for links on your site, you'd probably have to remove the sandbox attributes from the iframe HTML and adjust the JS to not use postMessage() for the links.

#14 @smerriman
20 months ago

Even if you're fine with the idea of disabling shift-click or control click, the broken right-click context menu in Firefox surely still has to be considered a bug that needs fixing. I am still able to replicate this - right clicking shows the context menu, but immediately starts loading the link I right-clicked on.

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

#15 @Mte90
20 months ago

Yes, we need to hack from javascript and changing the embed itself (sorry for my explanation).
Also for the fix probably of Firefox that (on right click) open the link (happen also to me) require a fix in the embed.
Probably is better to open a ticket for the make website and not on WordPress because is something that we cannot handle so quite easily from the page.

#16 @smerriman
20 months ago

Also, I'm not actually sure why the issue of browsers blocking links from opening in a new window is an issue.

If the browser allows it, everything works fine.
If the browser disallows it, nothing happens.

Nothing happening is better than something completely unexpected happening.

(Or probably not even nothing; an alert that a new window popping up has been blocked, which you can probably then unblock.)

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

#17 @iandunn
5 months ago

I encountered this bug today and I agree that it's a really bad UX, and an example of "breaking the web". Right-clicking, middle-clicking, and command-clicking are basic browser features that users expect to work, and I'm skeptical that there are ever any application features which justify breaking them.

Background

I want to make sure I understand what's going on here correctly, though. This is what I've gathered:

  1. The raw HTML rendered by the host server just displays a standard link to the embed source
  2. To provide better UX, that link is then transformed into a rich preview via JavaScript
  3. The rich preview is rendered on the embed source's server, because that's the way the oEmbed protocol is designed
  4. Because the preview comes from a remote source, the host server can't trust it, and has to put it in a sandboxed <iframe>
  5. The allow-top-navigation value for the sandbox attribute was rejected because, when combined with allow-scripts, it can be used to navigate away from the page programmatically, which would a vector for phishing attacks. Related discussions: 1, 2.
  6. Because the <iframe> is sandboxed, the browser will not normally open links inside it when they're clicked on.
  7. So, instead, the rich preview running on the embed source server sends a postMessage via JavaScript to the host server. The host server receives and authenticates that message, and then opens the link via the JavaScript running on the host server.
  8. The host server opens the link in the current window (via window.top.location.href = data.value), because opening in a new window would require window.open, which is blocked browsers unless it's the result of a direct user action.

Is that all accurate?

Potential Solutions

Here's some rough ideas, some/all of them may be unfeasible, but hopefully they'll spark something useful.

  • Could we remove the postMessage JS entirely, and instead use a value for the sandbox attribute which would allow links to work, without introducing security side-effects? allow-top-navigation was rejected, but it doesn't seem like allow-top-navigation-by-user-activation was considered.
  • Could we add allow-top-navigation but remove allow-scripts and the postMessage JS? It seems like the only remaining JS might be the sharing button, which could be redesigned to not require JS. Perhaps do something similar to the navigation menus that only use CSS.
  • Could we use allow-popups or allow-popups-to-escape-sandbox in some way? Maybe have linkClickHandler detect if it's a middle/command click, and do a window.open(). It seems like this would bypass popup blockers because it'd be triggered by a direct user action. We'd probably have to use the escape-sandbox variant, because the other would be blocked by X-FRAME-OPTIONS in many cases.
  • Maybe the WP oEmbed provider can supply the WP oEmbed consumer with the raw data via JSON, and the consumer can sanitize and then render the HTML? In that case we might not need the iframe at all, or could at least use looser permissions. It seems like the desire to allow JavaScript was so that 3rd-party embeds could embed "cool stuff", but I'm assuming we don't need that for a WP post. We could still allow that for 3rd-party embeds, and have this special solution for WP embeds. There may be back-compat concerns here, though.
  • Maybe the postMessage from the source server to the host server could tell the host server if it was a middle/cmd click. If it is, then the host could open it in a new tab. This probably wouldn't work since it wouldn't be interpreted by the browser as a user interaction, though.
  • Assuming that won't work, there may be some other approach where the source server can behave differently based on if the link was clicked on with a middle/cmd click.
  • Are there examples of other embeds solving this problem (inside or outside of WP)? It doesn't seem completely unique to us, so there may be other ideas we haven't thought of.

#18 follow-up: @swissspidy
5 months ago

That is a pretty accurate background summary, yes.

Could we remove the postMessage JS entirely, and instead use a value for the sandbox attribute which would allow links to work, without introducing security side-effects? allow-top-navigation was rejected, but it doesn't seem like allow-top-navigation-by-user-activation was considered.

To be fair, allow-top-navigation-by-user-activation was added in to Chrome in 2018, 3 years after we added oEmbed to WordPress. So this was not a thing at the time. Happy to give that a try now.

It seems like the only remaining JS might be the sharing button, which could be redesigned to not require JS.

Not really an option a) because JS is required to improve the accessibility of the menu and b) postMessage is needed so that the iframe can inform the host site of its size, so that it can be resized accordingly (responsiveness).

Could we use allow-popups or allow-popups-to-escape-sandbox in some way?

Happy to give that option a try as well.

Maybe the WP oEmbed provider can supply the WP oEmbed consumer with the raw data via JSON, and the consumer can sanitize and then render the HTML?

There were plenty of discussions back in 2015 when we worked on oEmbed. The consensus was that the embedded site should be in control of the content and layout of the embed. It was not about being able to add "cool stuff".

Maybe the postMessage from the source server to the host server could tell the host server if it was a middle/cmd click. If it is, then the host could open it in a new tab.

That is what we have explored in #35239. Please read through that ticket to see why that isn't feasible. tl:dr: back compat and user interaction.

Are there examples of other embeds solving this problem

Not that I am currently aware of. WordPress is rather restrictive with embeds compared to others.

#19 @iandunn
5 months ago

Thanks for the detailed response :)

I looked into browser support for allow-top-navigation-by-user-activation, but unfortunately it looks like IE, Edge and Firefox haven't added support for it yet :(
With Edge moving to adopt Blink, though, they'll get it as part of that.

For future reference, Chrome added it in v58, and Safari in 11.1 (13605.1.33.1.2) / 10.13.4.

So, it seems like it'll be a few years before that's a viable option :(

Hopefully there's something else we can do in the meantime.

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


2 months ago

#21 @afercia
2 months ago

  • Focuses accessibility added

#22 @swissspidy
2 months ago

@iandunn #47363 just came to my attention. The patch actually seems reasonable, because when you deliberately perform a CMD + click / right click, then this should not necessarily go through the whole protection layers. Thoughts?

#23 in reply to: ↑ 18 @azaozz
2 months ago

Replying to swissspidy:

Could we use allow-popups or allow-popups-to-escape-sandbox in some way?

Happy to give that option a try as well.

Don't think this is a good idea. There are other problems with opening popups (reference to the parent window, etc.) that we "don't want to deal with" :)

The patch actually seems reasonable, because when you deliberately perform a CMD + click / right click, then this should not necessarily go through the whole protection layers.

True, but it may need to have noopener noreferrer there :)

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

#24 @azaozz
2 months ago

#47363 was marked as a duplicate.

#25 @azaozz
2 months ago

There is a proposed patch by @timhavinga to enable opening in a new tab/window: https://core.trac.wordpress.org/attachment/ticket/47363/47363.patch

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


2 months ago

#27 @iandunn
2 months ago

I haven't had time to test yet, but the patch looks good at first glance. I also like the idea of adding noopener noreferrer.

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


6 weeks ago

Note: See TracTickets for help on using tickets.