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

[smerriman]

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.

[swissspidy]

Duplicate of #35239.

[johnbillion]

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.

[swissspidy]

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.

[smerriman]

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?

[swissspidy]

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.

[newsplugin.com]

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.

[swissspidy]

@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.

[smerriman]

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.

[Mte90]

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

[swissspidy]

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

[Mte90]

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?

[swissspidy]

@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.

[smerriman]

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.

[Mte90]

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.

[smerriman]

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.)

[iandunn]

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.

[swissspidy]

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.

[iandunn]

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.

[swissspidy]

@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?

[azaozz]

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 :)

[azaozz]

#47363 was marked as a duplicate.

[azaozz]

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

[iandunn]

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.

[garrett-eclipse]

FYI #49558 proposes removing noreferrer now that Firefox supports noopener.

[SergeyBiryukov]

Patch by @timhavinga from #47363

[afercia]

Quickly testing 39097.diff​, not sure it does anything? I may be missing something though. Also, quoting from ticket:47363#comment:10

To keep this as secure as possible when opening in a new tab or window the link should probably have the noopener noreferrer attributes.

I guess this parts need to be addressed.

Just noting it's 4 years this issue is waiting for a solution. It would be nice to fix it, sooner or later :) Pinging the Embeds component maintainers @swissspidy @azaozz

[SergeyBiryukov]

Replying to afercia:

Quickly testing 39097.diff​, not sure it does anything? I may be missing something though.

Yeah, testing with Ctrl-clicking in Chrome on Windows 10, it doesn't seem to have any effect.

Moving to 5.6 for further testing.

[audrasjb]

WP 5.6 Beta 1 is in 5 days, so let's move this ticket to Future Release.

[SergeyBiryukov]

Testing this again, 39097.diff​ mostly works, but needs some adjustment to check for the correct modifiers.

39097.2.diff​ works as expected. To test:

1. npm run build.
2. Embed a post on your local install in another post.
3. Try Ctrl/Cmd + click on any link in the embed iframe. The link should open in a new tab.

This only affects Ctrl/Cmd + click. Right-clicking a link in the embed iframe and then selecting "Open link in a new tab" already works in my testing in both Chrome and Firefox, so it doesn't look like any changes are needed for that.

[garrett-eclipse]

Refresh to correct doc comment spelling and introduce shiftKey and altKey to the modifier exclusion list to support 'open in new window' and 'save link as' features respectively.

[garrett-eclipse]

Testing the second diff worked as @SergeyBiryukov indicated and was just going to update the spelling of catched => caught but saw other implementations also covering the altKey and ShiftKey so looked into them and found they're valid cases as well so added them in 39097.3.diff.
altKey (option key on mac) will 'save the link as'.
shiftKey will 'open a new window'.

Sorry to remove the commit but would like my change tested on Windows as well since I only have a Mac.

[SergeyBiryukov]

39097.3.diff​ looks good to me and works as expected. Thanks for these corrections!

[audrasjb]

That's so cool, thanks.

I tested 39097.3.diff on my side and Cmd + click works well on Mac OSX + Chrome or Firefox.

[audrasjb]

(added the workflow keyword by mistake)

[SergeyBiryukov]

In 49202:

Embeds: Only catch clicks from the primary mouse button in the click handler, without any modifier keys.

This ensures that Ctrl/Cmd + click to open a link in the embed iframe in a new tab works as expected.

Props timhavinga, garrett-eclipse, smerriman, swissspidy, johnbillion, Mte90, iandunn, azaozz, afercia, audrasjb, SergeyBiryukov.
Fixes #39097.