1461195 - CSP/XFO error pages should offer an option to visit the page directly

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[bug.zilla]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180503143129

Steps to reproduce:

Visited a page (in an RSS reader)


Actual results:

Page was blocked by Firefox due to Content Security Policy 


Expected results:

IE displays the warning but also offers an option to visit the page directly:
https://imgur.com/a/0U9dry3

Comment 1

[Bodea Daniel [:danibodea]]

Hello! :)

I have attempted to reproduce this issue, but it seems like I do not have all the details needed to reproduce it.
Which RSS reader were you using and which page were you visiting when the issue reproduced?

Thank you.

Comment 2

[bug.zilla]

Hi,

struggling to recreate this now.
If you are able to recreate it somehow, then please leave it open.

Otherwise, feel free to close.

Thanks

Comment 3

[bug.zilla]

I've managed to reproduce it:

(1) Go to inoreader's demo page: https://www.inoreader.com/u/demo

(2) Search for "telegraph" in the top left search field

(3) Add the top RSS (or any of the telegraph.co.uk ones)

(4) Click on the eye icon on the top right and select column view

(5) Select any article and then either press q or click on the globe icon below the headline in the right panel

Here are the respective error messages from firefox and IE:
https://imgur.com/a/jKL0pWk

Comment 4

[Bodea Daniel [:danibodea]]

I can reproduce this issue on Windows 10 x64 using the repro steps given in the comment above.

Comment 5

[Daniel Veditz [:dveditz]]

One warning: if we do this we MUST NOT send same-site cookies when opened as a fresh tab. This technique could actually be used as part of a social-engineering attack to get a user to open a URL in a way that the framing page can't do on its own. Should be easy -- we already have to handle this correctly when a page does a window.open() on a page. We just need to make sure it behaves like that.

And write tests for it!

Comment 6

[Johann Hofmann [:johannh]]

Updated (simpler) STR:

• Go to https://johann-hofmann.com/frame-tester.html
• Enter https://google.com into the input field to get the error for X-Frame options
• Enter https://www.telegraph.co.uk into the input field to get the error for CSP

For these two error pages, we want to offer the possibility to open the page directly, to do that, we could do that by offering an option like the one shown in the screenshot in comment 0 on the page.

The important part is outlined in comment 5. If we add a link to the page, we need to ensure that we're not opening it with the system principal, so that SameSite=Strict cookies are not set. You can use https://samesite-cookies.glitch.me/ to quickly test whether or not you're loading SameSite cookies. If a link from the error page is sending SameSite cookies, we could fall back to calling window.open() instead, which will hopefully not.

The more challenging part may be writing a test for this. To verify SameSite=Strict cookies aren't loading, we'll need to use a server script in our test.

Comment 7

[:ingrid]

I would like to take on this bug.

Comment 8

[Johann Hofmann [:johannh]]

Absolutely, thanks for helping out!

Comment 9

[:ingrid]

Attachment: Bug 1461195 - CSP/XFO error pages should offer an option to visit the page directly r=johannh

Comment 10

[Johann Hofmann [:johannh]]

Attachment: Error Page

Hi Betsy, could you help us out with a small ad-hoc copy request on this bug?

For context, some sites choose to forbid being embedded as an iframe on another site because of security reasons. If a site does not want to be embedded but some other sites tries to do it anyway, we show an error page to the user saying that unfortunately we can't show this site here.

However, the user could of course visit the site in a new tab or a new window directly. To make that easier we want to add a new link to the error page that offers the user that possibility. Now we need one string for that link.

In our WIP patch, we currently say "Open in a New Window" (see screenshot). A longer more explanatory version would be "Open this page in a new window instead".

A final difficulty is that for many users it will open in a new tab instead of a new window, because there's a preference that controls that. I suppose we should have two different strings depending on which way the pref is set? Can we avoid that somehow?

Thanks!

Comment 11

[Betsy Mikel [:betsymi]]

Hi Johann, we don't think this is a simple case of the casing on that particular string. In the message itself, I don't think we communicate well to users why opening this page in a different window will solve the problem. Because we're asking users to take an action here, the call to action should be a button, not hyperlinked text. Buttons use title case.

Firefox Can't Open This Page

<name of site> will not allow Firefox to display the page if another site has embedded it using an X-Frame. To see this page, you need to open it in a new window.

[ Open Site in New Window ]

Comment 12

[Johann Hofmann [:johannh]]

Thank you Betsy! Ingrid, does that make sense to you? Do you need any additional information?

Comment 13

[:ingrid]

Is the new text meant to be added to the error page or substitute parts/the whole old text? For clarity, could it be possible to have a version of the entire text, how it should be displayed on the page? Should the same text be displayed on the error page for Content Security Policy? Thank you!

Comment 14

[Johann Hofmann [:johannh]]

I think this is supposed to replace the “Nightly prevented this page...” description text. Right, Betsy?

Also, reading the copy again, we should probably replace the word “X-Frame” with “iframe” or something like that, or just remove the “using an X-Frame” part completely from the sentence. That’s probably best. It’s just a confusing technical term that doesn’t really add value. Any thoughts?

Thanks

Comment 15

[Betsy Mikel [:betsymi]]

Yes, it is replacement text. Thanks! Am fine to remove the technical term as well.

Comment 16

[:ingrid]

Ok! So the new text would look like this, right? Thanks!

Blocked by X-Frame-Options Policy

An error occurred during a connection to <name of site>.

<Browser> Can't Open This Page

<name of site> will not allow <Browser> to display the page if another site has embedded it. To see this page, you need to open it in a new window.

[ Open Site in New Window ]

Comment 17

[Johann Hofmann [:johannh]]

That looks correct to me.

Comment 18

[Betsy Mikel [:betsymi]]

Sorry, no it would just say

<Browser> Can't Open This Page

<name of site> will not allow <Browser> to display the page if another site has embedded it. To see this page, you need to open it in a new window.

[ Open Site in New Window ]

Comment 19

[Johann Hofmann [:johannh]]

We can then probably consolidate the two strings for XFO and CSP into a single one. Given that it has the same effect I think that's much more user-friendly. Developers will have details in the developer console, though I wonder if we should think about showing an "advanced" information panel. Not in this bug, though.

Comment 20

[:ingrid]

Attachment: Open Site In New Window With a Button

Is this the way you imagined it? :)

Comment 21

[Johann Hofmann [:johannh]]

Looks like a huge improvement in terms of usability to me! Let's needinfo Betsy for feedback but I think we don't have to block on that :)

Thank you!

Comment 22

[bug.zilla]

A suggestion, if I may.
Could we add something to let the user know that this is done to protect their security?
Otherwise, it might give the impression that Firefox is somehow broken.

Thanks

Comment 23

[Johann Hofmann [:johannh]]

I feel like this is a good idea but Betsy is the master of content here so I'll let her speak to that.

I could imagine just changing it to "To protect your security, <site> will not allow Firefox to display the page if another site has embedded it.

(We could also have a "Learn More" link pointing to SUMO since that will likely lead to some folks wondering what it's about, but that's a separate bug)

Comment 24

[Betsy Mikel [:betsymi]]

Yes, I am good with that change. Screenshot looks good!

Comment 25

[Johann Hofmann [:johannh]]

Ingrid, are you stuck on anything? I feel like this bug may be more important than we initially assumed, so I wonder if there's anything I can do to help you drive it forward?

Comment 26

[:ingrid]

Thank you, Johann! The tests are still troubling me a bit. But I just submitted a patch to let you know where I currently am.

While trying to follow your suggestions for the tests :

a) Do we have the correct error page
b) Is the link visible (BrowserTestUtils.is_visible)
c) When we click the link, does a new window open with the correct URL?
d) If your server code tries to set some SameSite=Strict cookies and displays them on load, are they displayed in the new window?

I encounter the problem that the tests
a. start timing out when I add additional checks and the element I am trying to check is
not being found, because it is not loaded yet. I tried setting a timeout or using the
BrowserTestUtils.browserLoaded and it worked when I ran the test alone, but not
when I ran the whole test folder.
b. the tests behave differently when ran on their own, as opposed to when I run the whole folder,
creating race conditions, for instance that the tabs are not getting removed anymore.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01b0133a4f6e
CSP/XFO error pages should offer an option to visit the page directly r=johannh,fluent-reviewers,flod

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/01b0133a4f6e