[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

Mark, was there some kind of deliberate decision to not make this work in iframes or was that just an oversight?

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.

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.

Sounds like it would be nice to fix, but I think it is too late to be concerned about beta at this point.

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.

My bug!

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 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;

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`

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

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

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

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

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

https://hg.mozilla.org/mozilla-central/rev/36bfb71a88ea https://hg.mozilla.org/mozilla-central/rev/9c83861ca1a1

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

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

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