Closed Bug 1585588 Opened 3 years ago Closed 3 years ago

Firefox Nightly Screenshot Function Fails: Operation is insecure.

Categories

(Firefox :: Screenshots, defect, P1)

71 Branch
All
Unspecified
defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 blocking verified

People

(Reporter: ice2jafar, Assigned: aswan, NeedInfo)

References

(Regression)

Details

(Keywords: rca-needed, regression)

Attachments

(2 files)

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

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.

Component: Untriaged → Screenshots
Keywords: regression

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Regressed by: 1467970

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

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"
Flags: needinfo?(ianb)

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

Duplicate of this bug: 1586528

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

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

Priority: -- → P1
Severity: normal → major

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

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

Flags: qe-verify?
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe4675749b69
Fix screenshots cross-doc adoption r=_6a68

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Assignee: nobody → andrew.swan

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

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?

Flags: needinfo?(andrew.swan)
Flags: needinfo?(andrew.swan)

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

Flags: qe-verify? → qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Hardware: Unspecified → All

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.

Keywords: rca-needed

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

Flags: needinfo?(tmaity)

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?

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?

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

Flags: needinfo?(tmaity)

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

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.