1585588 - Firefox Nightly Screenshot Function Fails: Operation is insecure.

Status: VERIFIED FIXED

(Firefox :: Screenshots, defect, P1)

Description

[ice2jafar]

Attachment: 1002190506[1].jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

I have installed this 3 times coming to the same issue.
After install. Simply clicking on "Take a screenshot" activates but doesn't show prompts.

Actual results:

When clicked on from the url bar for the first time a message pops up stating.

"Whoa! Firefox Screenshots went haywire.
We're not sure what happened. Care to try again or take a shot of a different page?
The operation is insecure.
via 2d365d63-1626-4576-9ef6-d0b17ecce525

After which no prompts show up and you're unable to access any of the page you're wanting to screenshot until you go back in the url tab and "deselect" Take a Screenshot.

Expected results:

After selecting Take a Screenshot, the usual prompts allowing you to pick the size and dimensions of the target area. This issue didn't start happening until the nightly update within the past week.

Comment 1

[ice2jafar]

Sidenote: Installing any other Screenshot add-ons fail to capture "Sectional"/"Selected Area" screenshots. Only Full page screenshot functionality still works on said add-ons.

Comment 2

[ice2jafar]

Sidenote: Installing any other Screenshot add-ons fail to capture "Sectional"/"Selected Area" screenshots. Only Full page screenshot functionality still works on said add-ons.

Comment 3

[Danny Colin [:sdk]]

I can confirm that Firefox Screenshot doesn't work on Nightly 71 (tested on Linux)

Here's the info returned by mozregression:

 9:58.96 INFO: Last good revision: a3d9784a3b3326edc67c95699227b5fa684183d9
 9:58.96 INFO: First bad revision: 858991b684efa86e069eb0a919d640781ccb12dd
 9:58.96 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a3d9784a3b3326edc67c95699227b5fa684183d9&tochange=858991b684efa86e069eb0a919d640781ccb12dd

I also got a few 9:28.36 WARNING: Skipping build deba67add7d2: Unable to find build info using the taskcluster so mozregression failed to pinpoint the exact falty commit. However, it has narrowed the search.

Comment 4

[Andrew Swan [:aswan]]

Here's the code in question which is no longer supported since bug 1467970:
https://searchfox.org/mozilla-central/rev/2f29d53865cb895bf16c91336cc575aecd996a17/browser/extensions/screenshots/onboarding/slides.js#51-54

Comment 5

[Andrew Swan [:aswan]]

[Tracking Requested - why for this release]:
Screenshots is pretty badly broken.

Comment 6

[Andrew Swan [:aswan]]

The patch below appears to fix the issue but:

1. The fact that no tests broke when something so basic is broken is concerning
2. This code defines some html as a js string constant (https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/build/onboardingHtml.js) and manually parses is and injects it into an iframe. There are no comments in the surrounding code explaining why it works this way and I can't find the screenshots repo anywhere under github.com/mozilla/* to look at history there.

Ian, can you shed some light on why this code is written this way? It seems like it could be simplified but given point #1 above I wouldn't feel comfortable making a change since it seems like there aren't tests in place that would detect regressions...

Anyway, here's a quick-and-dirty fix:

diff --git a/browser/extensions/screenshots/onboarding/slides.js b/browser/extensions/screenshots/onboarding/slides.js
--- a/browser/extensions/screenshots/onboarding/slides.js
+++ b/browser/extensions/screenshots/onboarding/slides.js
@@ -44,7 +44,7 @@ this.slides = (function() {
       iframe.addEventListener("load", catcher.watchFunction(() => {
         doc = iframe.contentDocument;
         assertIsBlankDocument(doc);
-        const parsedDom = (new DOMParser()).parseFromString(
+        const parsedDom = (new iframe.contentWindow.DOMParser()).parseFromString(
           html,
           "text/html"

Comment 7

[jscher2000]

(In reply to Andrew Swan [:aswan] from comment #6)

... I can't find the screenshots repo anywhere under github.com/mozilla/* to look at history there.

In case it's still helpful, it's under services:

https://github.com/mozilla-services/screenshots/

Comment 9

[jscher2000]

I opened an issue for that change on Github, but I have no idea when it would release:

https://github.com/mozilla-services/screenshots/issues/5396

Comment 10

[Pascal Chevrel:pascalc]

Screenshot is a differentiator feature for Firefox, marking as blocking the release.

Comment 11

[Andrew Swan [:aswan]]

Attachment: Bug 1585588 Fix screenshots cross-doc adoption

Comment 12

[Andrew Swan [:aswan]]

(In reply to jscher2000 from comment #9)

I opened an issue for that change on Github, but I have no idea when it would release:

I thought I heard at some point that screenshots was going to migrate from github to mozilla-central, though I can't find any written record of that anywhere. Anyhow, given the urgency and the fact that there hasn't been a non-pontoon update to the github repo in many months, I think we should just land the fix here.

Comment 13

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Please note, the breakage is in the onboarding slides, so this bug only affects users (profiles) that have never used Screenshots before. (At least, I'm unable to reproduce in a profile that has used Screenshots prior to current Nightly.)

:aswan, it should be fine to land the fix directly in tree. We've previously opened github issues to upstream fixes landed directly in m-c, like mass refactorings.

As far as the future plans (m-c vs github), Ian (already ni'd) was the owner and can speak to timelines.

I can't see the secure bug 1467970, but the proposed change looks fine. I will R+ presently.

There's just one automated test, but it seems that it should have failed: https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe4675749b69
Fix screenshots cross-doc adoption r=_6a68

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fe4675749b69

Comment 16

[ice2jafar]

Thank you for addressing this so promptly.
I haven't found any issues since todays update.

Comment 17

[Sean Feng [:sefeng]]

We have a concern whether the security change we made in bug 1467970 will break more extensions, and it looks like we missed some key concepts about content scripts.

aswan: Do you mind provide more info about what was the situation in this bug? What was the things that broken and What setup was fixed?

Comment 18

[Tomislav Jovanovic :zombie]

I provided details in https://bugzilla.mozilla.org/show_bug.cgi?id=1467970#c43.

Comment 19

[Andrew Swan [:aswan]]

I can't see bug 1467970 or Tom's comment but this was due to Screenshots doing the thing I described in comment 6. I'm not sure why screenshots was written that way and the question/needinfo I asked there was never answered. Regardless, I don't have any data but my instinct is that this is not a common enough pattern to cause concern. In any case, the fix for affected code is pretty simple...

Comment 20

[Cristian Fogel [:cfogel]]

Confirmed issue with 71.0a1 (2019-10-02).
Fix verified on 71.0b8 and 72.0a1 (2019-11-10) on Windows 10, macOS 10.13.6, Ubuntu 16.04.

If there is something extra that needs verification please feel free to reopen based on the reply to the question from comment 17.

Comment 21

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.

Comment 22

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I'm not really sure what the root cause would be for this bug, suggestions welcome.

Comment 23

[Sean Feng [:sefeng]]

The root cause was when I was implementing bug 1467970, I missed the case that this operation was valid for web extensions.

In addition to that, a test case should preexist to catch it, so maybe Design Error and Testing Error?

Comment 24

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sure, that makes sense. I'm curious if multiple root causes are supported. Do you think it might also be reasonable to consider missing or incomplete internal API docs to be the root cause?

Comment 25

[Tania Maity (:tmaity)]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #24)

Sure, that makes sense. I'm curious if multiple root causes are supported. Do you think it might also be reasonable to consider missing or incomplete internal API docs to be the root cause?

You can enter two root causes, let me know if you face any issues. If there are too many root causes, please select the most significant one(s).

Comment 26

[Sean Feng [:sefeng]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #24)

Do you think it might also be reasonable to consider missing or incomplete internal API docs to be the root cause?

Sounds reasonable to me :).