Closed Bug 1456485 Opened 6 years ago Closed 6 years ago

Firefox Screenshots no longer works on PDF pages

Categories

(WebExtensions :: General, defect, P1)

x86_64
All
defect

Tracking

(firefox-esr52 unaffected, firefox-esr6061+ verified, firefox59 unaffected, firefox60+ wontfix, firefox61+ verified, firefox62+ verified)

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ verified
firefox59 --- unaffected
firefox60 + wontfix
firefox61 + verified
firefox62 + verified

People

(Reporter: cmuntean, Assigned: kmag)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached image pdf pages.gif
[Affected versions]:
- Nightly 61.0a1 (Build ID: 20180424013604)
- Beta 60.0b14 (Build ID: 20180419200216)

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Prerequisites]:
- Have a clean Firefox profile.
- Screenshots onboarding tour was already visited.

[Steps to reproduce]:
1. Open the latest Nightly build with the profile from prerequisites.
2. Navigate to a PDF page (eg: http://www.pdf995.com/samples/pdf.pdf).
3. Click the "Take a Screenshot" button from "Page Actions" menu.
4. Observe the behavior. 

[Expected result]:
- The Screenshots overlay is displayed and you can make a selection.

[Actual results]:
- "We can't screenshot this page" error is displayed. 

[Regression]:
- The issue is not reproducible on latest Firefox 59.0.2 release. Considering this I have found the regression window using the mozregression tools. Here are the results:
- Last good revision: c8514e653b71479ba22a307f58f79f563ed80e9d
- First bad revision: 07ab807639ee42a407a9bdb0d374206c0f17678d
- Pushlog: https://goo.gl/aLWFGg

Looks like Bug 1436482 introduced this issue, but I am not sure if this should be fixed on the Screenshots or Nightly side.

[Notes]:
- Attached a screen recording with the issue.
Kris can you please take a look at this issue?
Flags: needinfo?(kmaglione+bmo)
I suppose we may as well make system extensions immune to site restrictions.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
[Tracking Requested - why for this release]:
Serious feature regression affecting upcoming esr.

David, this seems pretty serious regression.
Flags: needinfo?(ddurst)
We hope to have this ride along with a dot release, still in progress.
Guys, since this is relevant to extensions developers: https://discourse.mozilla.org/t/extensions-on-pdfjs-pages/28441

Since not my account not any account from my team mates has access to the linked bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1436482

Would anyone care to explain more about #1436482 and how this affects extensions, where/if it is documented?
Comment on attachment 8974600 [details]
Bug 1456485: Part 1 - Support unrestricted matching in MatchPattern.

https://reviewboard.mozilla.org/r/242960/#review248850

::: dom/chrome-webidl/MatchPattern.webidl:131
(Diff revision 1)
>     */
>    boolean ignorePath = false;
> +
> +  /**
> +   * If true, the set of schemes this pattern can match is restricted to
> +   * those accessible by WebExtensions.

"non-system" or "unprivileged" webextensions

::: dom/chrome-webidl/MatchPattern.webidl:133
(Diff revision 1)
> +
> +  /**
> +   * If true, the set of schemes this pattern can match is restricted to
> +   * those accessible by WebExtensions.
> +   */
> +  boolean restrictSchemes = true;

I would probably invert this to `allowAllSchemes` and default to false.

Virtually every condition in this and the other patch set it and use it as the negation anyway.

::: toolkit/components/extensions/MatchPattern.cpp:315
(Diff revision 1)
>    }
>  
>    RefPtr<nsAtom> scheme = NS_AtomizeMainThread(StringHead(aPattern, index));
>    if (scheme == nsGkAtoms::_asterisk) {
>      mSchemes = AtomSet::Get<WILDCARD_SCHEMES>();
> -  } else if (permittedSchemes->Contains(scheme) || scheme == nsGkAtoms::moz_extension) {
> +  } else if (!aRestrictSchemes ||

This seems a bit too permissive, I would feel better with a whitelist.

Even though it would still need an explicit permission  in the manifest, I'm not sure everyone writing Mozilla extensions will be familiar with security implications of every possible scheme.

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:125
(Diff revision 1)
> +  pass({url: "about:foo", pattern: ["about:foo*"], options: {restrictSchemes: false}});
> +  pass({url: "about:foobar", pattern: ["about:foo*"], options: {restrictSchemes: false}});
> +  pass({url: "resource://foo/bar", pattern: ["resource://foo/bar"], options: {restrictSchemes: false}});
> +  fail({url: "resource://fog/bar", pattern: ["resource://foo/bar"], options: {restrictSchemes: false}});
> +  fail({url: "about:foo", pattern: ["about:meh"], options: {restrictSchemes: false}});
>  });

I believe the general `<all_urls>` or `*://*` won't work for restricted schemes without the explicit match pattern for that scheme (even with `restrictedSchemes: false`).

Please add a test verifying that.
(In reply to paolo.caminiti from comment #7)
> Would anyone care to explain more about #1436482 and how this affects
> extensions, where/if it is documented?

Not in a public bug, no. Sorry.
Comment on attachment 8974600 [details]
Bug 1456485: Part 1 - Support unrestricted matching in MatchPattern.

https://reviewboard.mozilla.org/r/242960/#review248850

> I would probably invert this to `allowAllSchemes` and default to false.
> 
> Virtually every condition in this and the other patch set it and use it as the negation anyway.

I think `restrictSchemes` makes more sense in this case. `allowAllSchemes` implies that some themes are restricted, which seems a bit odd.

> This seems a bit too permissive, I would feel better with a whitelist.
> 
> Even though it would still need an explicit permission  in the manifest, I'm not sure everyone writing Mozilla extensions will be familiar with security implications of every possible scheme.

The main purpose of this is not for WebExtensions. In any case, it doesn't matter much how permissive it is, since we don't allow content scripts to access system principal documents in any case.
Priority: -- → P1
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

https://reviewboard.mozilla.org/r/242962/#review248864

::: toolkit/components/extensions/Extension.jsm:619
(Diff revision 1)
> +            let matcher = new MatchPattern(perm, {restrictSchemes, ignorePath: true});
>  
> -          perm = matcher.pattern;
> +            perm = matcher.pattern;
> -          originPermissions.add(perm);
> +            originPermissions.add(perm);
> +          } catch (e) {
> +            Cu.reportError(`Invalid host permission: ${perm}`);

Hm, do we surface these manifest warnings in `about:debugging`, similar to Chrome?

::: toolkit/components/extensions/Extension.jsm:811
(Diff revision 1)
>  
>      this.apiManager = this.getAPIManager();
>      await this.apiManager.lazyInit();
>  
>      this.webAccessibleResources = manifestData.webAccessibleResources.map(res => new MatchGlob(res));
> -    this.whiteListedHosts = new MatchPatternSet(manifestData.originPermissions);
> +    this.whiteListedHosts = new MatchPatternSet(manifestData.originPermissions, {restrictSchemes: !this.hasPermission("mozillaAddons")});

So, this is using the logic that was previously in Extension, moved to ExtensionData.  But now ExtensionData is missing the `mozillaAddons` permission stripping from [1].

This could still technically all be correct (like, only matters when created as an Extension), but it's really hard to follow, can you move that logic from [1] to ExtensionData as well?

1) https://searchfox.org/mozilla-central/rev/eb6c521/toolkit/components/extensions/Extension.jsm#1438

::: toolkit/components/extensions/extension-process-script.js:338
(Diff revision 1)
>          webAccessibleResources = extension.webAccessibleResources.map(host => new MatchGlob(host));
>        }
>  
> +      let restrictSchemes = !extension.permissions.has("mozillaAddons");
> +
> +      let contentScripts = [];

Looks unused.

::: toolkit/components/extensions/test/xpcshell/test_ext_unknown_permissions.js:27
(Diff revision 1)
>    const {WebExtensionPolicy} = Cu.getGlobalForObject(ChromeUtils.import("resource://gre/modules/Extension.jsm", {}));
>  
>    let policy = WebExtensionPolicy.getByID(extension.id);
>    Assert.deepEqual(Array.from(policy.permissions).sort(), ["activeTab", "http://*/*"]);
>  
> -  Assert.deepEqual(extension.extension.manifest.optional_permissions, ["https://example.com/"]);
> +  Assert.deepEqual(extension.extension.manifest.optional_permissions, ["chrome://favicon/", "https://example.com/"]);

Wait, so does requesting this optional permission work now?  If not, that needs a test.
Comment on attachment 8974600 [details]
Bug 1456485: Part 1 - Support unrestricted matching in MatchPattern.

https://reviewboard.mozilla.org/r/242960/#review249036

r=me, provided the `about:blank` situation has a simple/sane answer.

::: toolkit/components/extensions/MatchPattern.cpp:331
(Diff revision 1)
>     ***************************************************************************/
>    offset = index + 1;
>    tail.Rebind(aPattern, offset);
>  
> +  if (scheme == nsGkAtoms::about) {
> +    // about: URIs don't have hosts, so just treat the host as a wildcard and

How does this now interact with `about:blank` documents?
Attachment #8974600 - Flags: review+
Comment on attachment 8974600 [details]
Bug 1456485: Part 1 - Support unrestricted matching in MatchPattern.

https://reviewboard.mozilla.org/r/242960/#review249036

> How does this now interact with `about:blank` documents?

It doesn't. We match based on principal URI, and about:blank is never used as a principal URI.
(In reply to paolo.caminiti from comment #7)
> Would anyone care to explain more about #1436482 and how this affects
> extensions, where/if it is documented?

https://www.mozilla.org/en-US/security/advisories/mfsa2018-11/#﷒0﷓ is the public description.
Flags: needinfo?(ddurst)
Attachment #8974600 - Flags: review?(aswan)
Attachment #8974601 - Flags: review?(aswan)
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

https://reviewboard.mozilla.org/r/242962/#review248864

> Hm, do we surface these manifest warnings in `about:debugging`, similar to Chrome?

Sometimes. Sort of. Generally only hard errors. It could be better, but I'm not super familiar with the about:debugging side of things.

> Wait, so does requesting this optional permission work now?  If not, that needs a test.

No. Unprivileged extension MatchPatterns will never accept chrome: URLs, which is already tested.
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

https://reviewboard.mozilla.org/r/242962/#review248864

> So, this is using the logic that was previously in Extension, moved to ExtensionData.  But now ExtensionData is missing the `mozillaAddons` permission stripping from [1].
> 
> This could still technically all be correct (like, only matters when created as an Extension), but it's really hard to follow, can you move that logic from [1] to ExtensionData as well?
> 
> 1) https://searchfox.org/mozilla-central/rev/eb6c521/toolkit/components/extensions/Extension.jsm#1438

I probably can, but I'd rather that be another bug, since that code runs with a different Extension object, with different information, at runtime rather than install time, and thus that change is much riskier.
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

https://reviewboard.mozilla.org/r/242962/#review249922

r=me for this uplift, but let's make sure to simplify the logic of base class depending on checks in derived class in a followup bug.  It's a footgun waiting to happen.

::: toolkit/components/extensions/Extension.jsm:513
(Diff revision 2)
>  
>    canUseExperiment(manifest) {
>      return this.experimentsAllowed && manifest.experiment_apis;
>    }
>  
> +  // eslint-disable-next-line complexity

Please note this for the followup as well.
Attachment #8974601 - Flags: review?(tomica) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c259ed0e0d1cecf8fdc6092b8714e880e3169d5
Bug 1456485: Part 1 - Support unrestricted matching in MatchPattern. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/364fa52d097e335146f94cbc98238c77b7789d61
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes. r=zombie
https://hg.mozilla.org/mozilla-central/rev/8c259ed0e0d1
https://hg.mozilla.org/mozilla-central/rev/364fa52d097e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
We should probably have a QA pass on this before doing anything with uplifts.
Flags: qe-verify+
Flags: in-testsuite+
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Firefox: 62.0a1, Build ID: 20180524100118

I have retested this issue on latest Nightly 62.0a1 (Build ID: 20180524100118) and the issue is still reproducible. Firefox Screenshots is still not working on PDF pages. 
Tested on Windows 7 x64, Windows 10 x64, Mac 10.13 and Arch Linux.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Does the add-on (Firefox Screenshots) has the "mozillaAddons" permission?
Target Milestone: mozilla62 → ---
With the "mozillaAddons" permission it will also work on addons.mozilla.org?
Flags: needinfo?(kmaglione+bmo)
In addition to pdf's this bug also applies to taking screenshots in reader view. Doesn't seem to be mentioned so I thought I would report it.
https://i.imgur.com/GRG0I7g.png
(In reply to kernp25 from comment #25)
> Does the add-on (Firefox Screenshots) has the "mozillaAddons" permission?

Yes, https://github.com/mozilla-services/screenshots/issues/4495

It's unclear to me when that will land in the product. Given Milestone 13 (62-1), sometime in the 62 cycle (if it hasn't already)? Wil, is that correct?
Flags: needinfo?(wclouser)
(In reply to henry from comment #27)
> In addition to pdf's this bug also applies to taking screenshots in reader
> view. Doesn't seem to be mentioned so I thought I would report it.
> https://i.imgur.com/GRG0I7g.png

Please file a new bug for this and CC me to it. I want to make sure it doesn't get lost in this report :)
Flags: needinfo?(henry)
The change to Screenshots will be landing in m-c this week.
Flags: needinfo?(wclouser)
Thanks Wil.
Flags: needinfo?(kmaglione+bmo)
(In reply to Wil Clouser [:clouserw] from comment #30)
> The change to Screenshots will be landing in m-c this week.

Oops, forgot to ask -- do you have a bug to track that, so we can make sure its tracked and gets uplifted to 61? Thanks.
Flags: needinfo?(wclouser)
Screenshots not working in reader mode is a known issue: https://github.com/mozilla-services/screenshots/issues/3092
Flags: needinfo?(henry)
I ran a Try push with the upstream Screenshots mozillaAddons permission change included, but I still can't screenshot PDFs or pages in Reader Mode :(
https://hg.mozilla.org/try/rev/a5a87a2e8726cbda32c08bcb48fa830361a8c1c5
https://queue.taskcluster.net/v1/task/VTKNm9opT129w4Q5I66QgA/runs/0/artifacts/public/build/target.zip

Am I doing something wrong? :)
Depends on: 1465544
Thanks Barry.  The bug for landing the screenshots patch is https://bugzilla.mozilla.org/show_bug.cgi?id=1465544

re: comment 34: Jared is trying the same.  He said he didn't see it fixed either.  I'll ask him to comment here, but it sounds like we might need another patch on this bug.
Flags: needinfo?(wclouser)
I'm seeing the "Missing host permission for the tab" error with the mozillaAddons permission added on top of current central.
Hmm, I might be doing this wrong - I see the mozillaAddons permission is getting stripped out at startup. Let me mess with the about:config settings and try again. Sorry for the spam.
I still get the "Missing host permission for the tab" error if I set the relevant prefs on the command line:

|./mach run --setpref extensions.legacy.enabled=true --setpref xpinstall.signatures.required=false|
Not being able to add `mozillaAddons` might be related to signing, even with legacy.enabled, and I _really_ hope that's unrelated to this bug, and that didn't regress here (can you verify please).

But the other issue is probably that you need to add the "resource://pdf.js" or perhaps "resource://*" permission as well.
> Not being able to add `mozillaAddons` might be related to signing, even with legacy.enabled, and I _really_ hope that's unrelated to this bug, and that didn't regress here (can you verify please).

Screenshots is working fine for regular websites on my local build of m-c, so I don't think this is an issue. Feel free to double-check yourself using the incantation from comment 38, and let me know if you see something strange.

> But the other issue is probably that you need to add the "resource://pdf.js" or perhaps "resource://*" permission as well.

The docs[1] specifically state that the resource scheme is not supported. Are the docs incorrect? Are there special docs for system addons that I should be consulting? I'm aware of [2], but it doesn't discuss webextensions API differences.



[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Match_patterns#Invalid_match_patterns
[2] https://firefox-source-docs.mozilla.org/toolkit/mozapps/extensions/docs/addon-manager/SystemAddons.html
Flags: needinfo?(tomica)
> Not being able to add `mozillaAddons` might be related to signing, even with legacy.enabled, and I _really_ hope that's unrelated to this bug, and that didn't regress here (can you verify please).

I may not have been totally clear - when I start my local build using the comment 38 incantation, I *don't* see the "mozillaAddon permission stripped" log line in the browser console, so I think that should be fine.
> The docs[1] specifically state that the resource scheme is not supported.
> Are the docs incorrect? Are there special docs for system addons that I
> should be consulting? I'm aware of [2]

That's exactly what this bug is about, see comment 2, and the path commit messages
Flags: needinfo?(tomica)
We heavily use inline pdf's and Firefox in our workplace, a few of use use mouse gestures. We've notice the change in #1436482 broke the ability to just use mouse gestures from within pdf documents, can this be fixed/reconsidered?
It turns out the mozillaAddons permission _is_ getting stripped out, but I only see the line in the browser console on the first run after building.

So, I'm not sure how to test this patch. Maybe the author/reviewer could do some of this testing, as the changes related to Screenshots currently seem to be one or two lines in the webextension manifest?

Also, adding the "resource://pdf.js/*" permission disables pdf.js completely, and instead causes PDF links to be handled by the system handler. This seems unexpected to me, and might be a separate bug.
I also see "Stripping mozillaAddons permission from screenshots@mozilla.org" with this patch reverted. I don't think this patch regresses that part of the behavior.

I still don't know how to test this patch, though.
FWIW, Nightlies including bug 1465544 are also still not working. Do we have any clues yet why the mozillaAddons permissions seems to be unable to stick?
Flags: needinfo?(tomica)
The issue is that Screenshots is still an embedded web extension (seems like it's the last one, what's the blocker for that?), and so it's not getting proper flags to mark it as privileged.

Filed bug 1466349 to fix this.
Flags: needinfo?(tomica)
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #44)
> Also, adding the "resource://pdf.js/*" permission disables pdf.js
> completely, and instead causes PDF links to be handled by the system
> handler. This seems unexpected to me, and might be a separate bug.

I'm unable to reproduce this, can you please provide a STR?
Depends on: 1466349
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

> Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1436482

> [User impact if declined]:
Firefox Screenshots feature doesn't work on PDF pages.

> [Is this code covered by automated tests?]:
Yes.

> [Has the fix been verified in Nightly?]:
No, needs followup bug 1466349 for the complete fix.

> [Needs manual test from QE? If yes, steps to reproduce]: 
Yes.

> [List of other uplifts needed for the feature/fix]:
Bug 1466349.

> [Is the change risky?]:
Somewhat.

> [Why is the change risky/not risky?]:
It only enables privileged (mozilla-signed) extensions to access protected pages.

> [String changes made/needed]:
None.
Attachment #8974601 - Flags: approval-mozilla-beta?
The remaining work for this happened in bug 1466349. Re-closing accordingly.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8974601 [details]
Bug 1456485: Part 2 - Allow extensions with the mozillaAddons permission to match restricted schemes.

Fixes so Screenshots works again with various privileged pages like the PDF Viewer and Reader Mode. Approved for 61.0b12.
Attachment #8974601 - Flags: approval-mozilla-esr60+
Attachment #8974601 - Flags: approval-mozilla-beta?
Attachment #8974601 - Flags: approval-mozilla-beta+
I have verified this issue on the latest Nightly (62.0a1 Build ID: 20180606083531) build and the issue is no longer reproducible. Firefox Screenshots works now on PDF pages.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.
Hey Kris, I took a crack at rebasing this for ESR60, but my Try push is hitting test_ext_contentscript_restrictSchemes.js xpcshell failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=182162747&repo=try

Here's a link to the push (the patches at least build, which required a bit of effort too):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d329736a0b66b8bf25668187e1e564c75680d2
Flags: needinfo?(kmaglione+bmo)
Sorry, there was another change to our test utilities that this was depending on.
Flags: needinfo?(kmaglione+bmo)
I have tested this issue on latest Beta (61.0b12, Build ID: 20180607135512) and latest ESR (60.0.3 - Build ID: 20180607182801), but the issue is still reproducible.
In order to test the issue on ESR, I have used the builds from the following treeherder link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&fromchange=efbcc205a0d3562691bf62bb0cb55bb7e891e8ff
Tested on Windows 7 x64, Windows 10 x64, Mac 10.13 and Arch Linux.

I have also installed the latest Screenshots 33.0.0 dev version on latest Firefox Developer Edition (62.0b12), but the issue is still reproducible. However, it is not reproducible on latest Nightly 62.0a1 build.

All dependencies bugs have been fixed and uplifted in Beta and ESR (bug 1465544 and bug 1466349). Is there another patch that needs to be uplifted in Beta and ESR in order to fix this issue? Kris, Tomislav can you please take a look at this?
Flags: needinfo?(tomica)
Flags: needinfo?(kmaglione+bmo)
This probably depends on bug 1467113
Flags: needinfo?(kmaglione+bmo)
Depends on: 1467113
Just tested a build from Beta tip and screenshotting is still not working.
Flags: needinfo?(kmaglione+bmo)
Bah. It looks like I accidentally fixed another bug in the midst of my refactorings.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1467924
Flags: needinfo?(tomica)
I have verified this issue on the latest Beta (61.0b14 Build ID: 20180614135649) and the issue is no longer reproducible.
Tested on Windows 7 x64, Windows 10 x64, Mac Os 10.13 and Arch Linux.

The issue is still reproducible on Firefox ESR 60.0 because of bug 1467924 that should also be uplifted in ESR 60 (see bug 1467924 comment 10).
Product: Toolkit → WebExtensions
I have verified the issue on latest ESR (60.1.0, Build ID 20180619173714) and the issue is no longer reproducible. Tested on Windows 10 x64, Windows 7 x64, Mac 10.13 and Arch Linux 4.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: