Closed
Bug 1297630
Opened 8 years ago
Closed 7 years ago
Cert error page is not displayed properly in iframes (content.js assumes error page is toplevel)
Categories
(Firefox :: General, defect, P3)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: pauly, Assigned: johannh)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxprivacy])
Attachments
(2 files)
[Affected versions]:
- FX 45, 48, 51.0a1 (2016-08-23)
[Affected platforms]:
- all
[Steps to reproduce]:
1. Open data:text/html,<iframe src="https://wrong.host.badssl.com/" height="500"></iframe>
2. Click on the 'Advanced' button
[Expected result]:
- "Add Exception" button should be displayed
- Clicking on the blue error code expands below the cert error details
[Actual result]:
- "Add Exception" button is not displayed
- Clicking on the blue error code displays 2 buttons with "copy text to clipboard"
[Regression range]:
- not a regression
Reporter | ||
Updated•8 years ago
|
Summary: Security exception is not displayed properly in iframes → Cert error page is not displayed properly in iframes
Comment 1•8 years ago
|
||
Mark, was there some kind of deliberate decision to not make this work in iframes or was that just an oversight?
![]() |
||
Comment 2•8 years ago
|
||
The "Add Exception" button is deliberately not shown in iframes to prevent pages tricking the user into adding exceptions via clickjacking. I don't see any harm in making the error code/details functionality work like normal, though.
Comment 3•8 years ago
|
||
I don't recall a deliberate decision (that's not to say one wasn't made).
Reports will be sent for these pages (even if the content.js part to enable / disable automatic reporting isn't working) if the user has previously enabled automatic reporting since there is now necko code that sends those reports (previously, we relied on the content.js stuff to send all of the reports).
Personally, I'm not too concerned if the reporting UI isn't displayed in iframes.
Flags: needinfo?(mgoodwin)
Comment 4•8 years ago
|
||
Sounds like it would be nice to fix, but I think it is too late to be concerned about beta at this point.
Updated•8 years ago
|
Whiteboard: [fxprivacy][triage]
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Assignee | ||
Comment 6•7 years ago
|
||
The fixes to this problem are most likely happening in individual bugs, like bug 1422811, so we might want to use this bug to track that work.
Depends on: 1422811
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
This is mostly done to unblock bug 1415279, which would end up breaking iframe cert errors even more. I hope Franziskus can just rebase on top of my changes to fix that. I know this isn't a very clean solution, but I'm not sure we want to wait until we have figured out which of the different chrome - content communication mechanisms is the thing we want to use (and have ported everything to use that)...
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8959114 [details]
Bug 1297630 - Test certificate errors in iframes.
https://reviewboard.mozilla.org/r/227990/#review233862
Mostly just some nits, though a bit curious about the autofocus thing.
::: browser/base/content/test/about/browser_aboutCertError.js:9
(Diff revision 1)
> "use strict";
>
> // This is testing the aboutCertError page (Bug 1207107).
>
> const GOOD_PAGE = "https://example.com/";
> +const GOOD_PAGE_2 = "https://example.org/";
If there's a dummy page we can use in this directory, I'd prefer that over loading the root of the mochitest server, which has oodles of contents and is therefore much slower to load, esp. on debug.
::: browser/base/content/test/about/browser_aboutCertError.js:69
(Diff revision 1)
> - let {entries} = JSON.parse(ss.getTabState(tab));
> + let {entries} = JSON.parse(ss.getTabState(tab));
> - is(entries.length, 1, "there is one shistory entry");
> + is(entries.length, 1, "there is one shistory entry");
>
> - info("Clicking the go back button on about:certerror");
> + info("Clicking the go back button on about:certerror");
> - await ContentTask.spawn(browser, null, async function() {
> - let doc = content.document;
> + await ContentTask.spawn(browser, {frame: useFrame}, async function({frame}) {
> + let doc = frame ? content.document.getElementById("frame").contentDocument : content.document;
Subtle duplication of the `frame` ID here - given that this is always an iframe, could just `querySelector("iframe")` ? Don't mind much either way though if you have strong reasons to want to use an ID.
::: browser/base/content/test/about/browser_aboutCertError.js:73
(Diff revision 1)
> - await ContentTask.spawn(browser, null, async function() {
> - let doc = content.document;
> + await ContentTask.spawn(browser, {frame: useFrame}, async function({frame}) {
> + let doc = frame ? content.document.getElementById("frame").contentDocument : content.document;
> +
> - let returnButton = doc.getElementById("returnButton");
> + let returnButton = doc.getElementById("returnButton");
> + if (!frame) {
> - is(returnButton.getAttribute("autofocus"), "true", "returnButton has autofocus");
> + is(returnButton.getAttribute("autofocus"), "true", "returnButton has autofocus");
Hm, do we deliberately not want to focus this in frames?
::: browser/base/content/test/about/browser_aboutCertError.js:293
(Diff revision 1)
> - "Correct error message found");
> + "Correct error message found");
> - is(message.tagName, "a", "Error message is a link");
> + is(message.tagName, "a", "Error message is a link");
>
> - message = await ContentTask.spawn(browser, null, async function() {
> - let doc = content.document;
> + message = await ContentTask.spawn(browser, {frame: useFrame}, async function({frame}) {
> + let doc = frame ? content.document.getElementById("frame").contentDocument : content.document;
> + let win = frame ? content.document.getElementById("frame").contentWindow : content.window;
You won't need `win`, you can get the docshell off `doc.docShell`
::: browser/base/content/test/about/browser_aboutCertError.js:302
(Diff revision 1)
> + let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
> + .getInterface(Ci.nsIWebNavigation)
> + .QueryInterface(Ci.nsIDocShell);
Nit: doc.docShell;
::: browser/base/content/test/about/browser_aboutCertError.js:361
(Diff revision 1)
> - let doc = content.document;
> + let doc = frame ? content.document.getElementById("frame").contentDocument : content.document;
> + let win = frame ? content.document.getElementById("frame").contentWindow : content.window;
Same nits for only needing `doc`
::: browser/base/content/test/about/browser_aboutCertError.js:371
(Diff revision 1)
> + let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
> + .getInterface(Ci.nsIWebNavigation)
> + .QueryInterface(Ci.nsIDocShell);
Nit: doc.docShell;
Attachment #8959114 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8959115 [details]
Bug 1297630 - Make certificate error pages work properly in iframes.
https://reviewboard.mozilla.org/r/227992/#review233866
Ugh, I'd forgotten how terrible this code was. Security checks in random places. I had 2 comments where I pointed out missing checks, only to realize that they happened elsewhere it's just REALLY NOT OBVIOUS when we check what where. And stuff. So the sooner we switch to *anything* more sane, the better. </rant>
Thank you for writing this!
::: commit-message-ecb8c:11
(Diff revision 1)
> +I did not update communication for Captive Portal pages, since those require
> +one-way broadcasting from chrome to content, which is not supported in this model.
> +We can fix this in the future.
Do we have a follow-up bug for this? If not, can you file one?
Then can we include the follow-up bug ID in this commit message please.
::: browser/base/content/content.js:247
(Diff revision 1)
> - return content.document.documentURI.startsWith("about:certerror");
> + return doc.documentURI.startsWith("about:certerror");
> },
>
> receiveMessage(msg) {
> - if (!this.isAboutCertError) {
> + if (msg.name == "CertErrorDetails") {
> + let win = WebNavigationFrames.findDocShell(msg.data.frameId, docShell);
It's uh, pretty interesting that there's a method called `findDocShell` that returns a window rather than a docshell. But OK, that's not your problem here...
::: browser/base/content/content.js:248
(Diff revision 1)
> },
>
> receiveMessage(msg) {
> - if (!this.isAboutCertError) {
> + if (msg.name == "CertErrorDetails") {
> + let win = WebNavigationFrames.findDocShell(msg.data.frameId, docShell);
> + if (!this.isAboutCertError(win.document)) {
In any case, it can return null if the frame is gone. We should catch that here and also early return.
::: browser/base/content/content.js:264
(Diff revision 1)
> + let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
> + .getInterface(Ci.nsIWebNavigation)
> + .QueryInterface(Ci.nsIDocShell);
Nit: `win.document.docShell`
::: browser/base/content/content.js:285
(Diff revision 1)
> notAfter /= 1000;
> return {notBefore, notAfter};
> },
>
> onCertErrorDetails(msg) {
> - let div = content.document.getElementById("certificateErrorText");
> + let win = WebNavigationFrames.findDocShell(msg.data.frameId, docShell);
We just did this in the calling method. Can we just pass the window by ref?
::: browser/base/content/content.js:331
(Diff revision 1)
> - content.document.getElementById("errorShortDesc")
> + doc.getElementById("errorShortDesc")
> .style.display = "none";
> - content.document.getElementById("wrongSystemTimePanel")
> + doc.getElementById("wrongSystemTimePanel")
> .style.display = "block";
Nit: unwrap these too please.
::: browser/base/content/content.js:362
(Diff revision 1)
> - content.document.getElementById("errorShortDesc")
> + doc.getElementById("errorShortDesc")
> .style.display = "none";
> - content.document.getElementById("wrongSystemTimeWithoutReferencePanel")
> + doc.getElementById("wrongSystemTimeWithoutReferencePanel")
> .style.display = "block";
And these.
::: browser/base/content/content.js:584
(Diff revision 1)
> + let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
> - .getInterface(Ci.nsIWebNavigation)
> + .getInterface(Ci.nsIWebNavigation)
> - .QueryInterface(Ci.nsIDocShell);
> + .QueryInterface(Ci.nsIDocShell);
`win.document.docShell`
Attachment #8959115 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959114 [details]
Bug 1297630 - Test certificate errors in iframes.
https://reviewboard.mozilla.org/r/227990/#review233862
> Hm, do we deliberately not want to focus this in frames?
I would say not, in an iframe. Feel free to raise a bug if you feel strongly about this but I don't think we should change that behavior in this bug either.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959115 [details]
Bug 1297630 - Make certificate error pages work properly in iframes.
https://reviewboard.mozilla.org/r/227992/#review233866
> Do we have a follow-up bug for this? If not, can you file one?
>
> Then can we include the follow-up bug ID in this commit message please.
Done, thanks
> It's uh, pretty interesting that there's a method called `findDocShell` that returns a window rather than a docshell. But OK, that's not your problem here...
Turns out I was confusing this and it does return a docShell, but docShells also have a document getter.
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f9f56aeb495
Test certificate errors in iframes. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a09f27b6dceb
Make certificate error pages work properly in iframes. r=Gijs
Comment 19•7 years ago
|
||
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/72f6a225e5e6
Backed out changeset a09f27b6dceb for bc failures on content/test/about/browser_aboutNetError.js and en-us failures on functional/security/test_ssl_disabled_error_page.py on a CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/1365d9214e5b
Backed out changeset 1f9f56aeb495 for bc failures on content/test/about/browser_aboutNetError.js and en-us failures on functional/security/test_ssl_disabled_error_page.py on a CLOSED TREE
Comment 20•7 years ago
|
||
Backed out changeset 1f9f56aeb495 (bug 1297630) for bc failures on content/test/about/browser_aboutNetError.js and en-us failures on functional/security/test_ssl_disabled_error_page.py on a CLOSED TREE
Backout link: https://hg.mozilla.org/integration/autoland/rev/1365d9214e5b2054a07e53d4597acf918ff647e1
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a09f27b6dceb0b3f8f43468489ee3eac2c4e5a86
Log link:https://treeherder.mozilla.org/logviewer.html#?job_id=168535385&repo=autoland&lineNumber=2196
[task 2018-03-16T15:38:44.053Z] 15:38:44 INFO - TEST-PASS | browser/base/content/test/about/browser_aboutNetError.js | Should be showing error page - true == true -
[task 2018-03-16T15:38:44.054Z] 15:38:44 INFO - Buffered messages finished
[task 2018-03-16T15:38:44.055Z] 15:38:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutNetError.js | prefResetButton has autofocus - null == "true" -
[task 2018-03-16T15:38:44.056Z] 15:38:44 INFO - Stack trace:
[task 2018-03-16T15:38:44.057Z] 15:38:44 INFO - resource://testing-common/content-task.js line 50 > eval:null:11
[task 2018-03-16T15:39:28.463Z] 15:39:28 INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 21•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36bfb71a88ea
Test certificate errors in iframes. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9c83861ca1a1
Make certificate error pages work properly in iframes. r=Gijs
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jhofmann)
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36bfb71a88ea
https://hg.mozilla.org/mozilla-central/rev/9c83861ca1a1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 25•7 years ago
|
||
I have reproduced this bug with Nightly 51.0a1 (2016-08-24) on Windows 10, 64 Bit!
This bug's fix is verified with latest Nightly!
Build ID : 20180419224145
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [bugday-20180418]
Reporter | ||
Comment 26•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo) from comment #2)
> The "Add Exception" button is deliberately not shown in iframes to prevent
> pages tricking the user into adding exceptions via clickjacking.
So, is this still expected?
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #26)
> (In reply to David Keeler [:keeler] (use needinfo) from comment #2)
> > The "Add Exception" button is deliberately not shown in iframes to prevent
> > pages tricking the user into adding exceptions via clickjacking.
> So, is this still expected?
Yeah.
Flags: needinfo?(jhofmann)
Reporter | ||
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•