Closed Bug 1331769 Opened 3 years ago Closed 2 years ago

Consider more clever checking for host permission changes

Categories

(Toolkit :: Add-ons Manager, defect, P2)

51 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox58 --- verified

People

(Reporter: aswan, Assigned: Kwan)

References

Details

(Whiteboard: [permissions], triaged)

Attachments

(4 files)

In the proposed patch for 1317470 we simply compute differences in the permissions a webextension declares in its manifest.  But this means that changing from a permission for "<all_urls>" to "https://*.somedomain.com" or from "https://*.somedomain.com" to "https://host.somedomain.com" will trigger prompts for new permissions when in fact those are examples of actually reducing the permissions of the extension.
If this is a big deal we should address it but I'm reluctant to write a bunch of extra code for this until we have reason to think its worth doing.
Priority: -- → P5
Whiteboard: [permissions] → [permissions], triaged
Duplicate of this bug: 1373434
Carrying over P2 from bug 1373434
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Priority: P5 → P2
Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188098

Looks good (and thanks again!) but this also should have tests.
Attachment #8910944 - Flags: review?(aswan) → review-
Some questions about testing:
at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"] narrowing.  Would it be worth adding another extension to test other examples, e.g.:
["*://*.example.com/*", "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"] ->
["http://*.example.com/*", "http://subdomain.example2.com/*", "http://www.example3.com/path/*", "https://*.example4.com/*"]

Do the extensions need to be signed?  The tests seem to work fine without them being, but it appears the other extensions in the directory are.
If so, is signing them locally with web-ext sign <keys> fine, or do they need special signing? (especially as they mention the mozilla domain in their IDs)
(In reply to Ian Moody [:Kwan] from comment #7)
> Some questions about testing:
> at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"]
> narrowing.  Would it be worth adding another extension to test other
> examples, e.g.:
> ["*://*.example.com/*", "http://*.example2.com/*",
> "http://www.example3.com/*", "https://*/*"] ->
> ["http://*.example.com/*", "http://subdomain.example2.com/*",
> "http://www.example3.com/path/*", "https://*.example4.com/*"]

I do think some additional tests would be good here, though this is really about testing MatchPatternSet.subsumes().  I think ideally we would put those tests in:
http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_MatchPattern.js
It would be good to have systematic tests for specific cases where subsumes() should return true but also cases where it should return false (e.g., "domain.com" -> "*.domain.com").
Anyhow, that can happen in a follow-up bug (if its something you're interested in working on, that would be great!)

> Do the extensions need to be signed?  The tests seem to work fine without
> them being, but it appears the other extensions in the directory are.
> If so, is signing them locally with web-ext sign <keys> fine, or do they
> need special signing? (especially as they mention the mozilla domain in
> their IDs)

Since the tests in this directory were first created we landed changes that let us disable signing in the test environment, which means that they don't need to be signed, but the xpinstall.signatures.required preference needs to be set to false or the tests will fail when this code gets to beta.
And since the extensions don't need to be signed, that means we can skip checking in xpis altogether and create extensions on the fly with `AddonTestUtils.createTempWebExtensionFile()`.  That should make the tests much more readable since they don't rely on the contents of zip files.  I'd like to get all the tests in this directory switched over to that method but switching them all is beyond the scope of this bug.  I'll leave it up to you if you prefer to make that change now or just land the xpis and let them get converted when the rest of the directory gets converted.  Of course, if you want to work on the follow-up to convert the existing tests, that would be great!
(In reply to Andrew Swan [:aswan] from comment #8)
> (In reply to Ian Moody [:Kwan] from comment #7)
> > Some questions about testing:
> > at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"]
> > narrowing.  Would it be worth adding another extension to test other
> > examples, e.g.:
> > ["*://*.example.com/*", "http://*.example2.com/*",
> > "http://www.example3.com/*", "https://*/*"] ->
> > ["http://*.example.com/*", "http://subdomain.example2.com/*",
> > "http://www.example3.com/path/*", "https://*.example4.com/*"]
> 
> I do think some additional tests would be good here, though this is really
> about testing MatchPatternSet.subsumes().  I think ideally we would put
> those tests in:
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/xpcshell/test_MatchPattern.js
> It would be good to have systematic tests for specific cases where
> subsumes() should return true but also cases where it should return false
> (e.g., "domain.com" -> "*.domain.com").
> Anyhow, that can happen in a follow-up bug (if its something you're
> interested in working on, that would be great!)
Yep, I've filed bug 1403224 and I'll submit a patch.

> > Do the extensions need to be signed?  The tests seem to work fine without
> > them being, but it appears the other extensions in the directory are.
> > If so, is signing them locally with web-ext sign <keys> fine, or do they
> > need special signing? (especially as they mention the mozilla domain in
> > their IDs)
> 
> Since the tests in this directory were first created we landed changes that
> let us disable signing in the test environment, which means that they don't
> need to be signed, but the xpinstall.signatures.required preference needs to
> be set to false or the tests will fail when this code gets to beta.
> And since the extensions don't need to be signed, that means we can skip
> checking in xpis altogether and create extensions on the fly with
> `AddonTestUtils.createTempWebExtensionFile()`.  That should make the tests
> much more readable since they don't rely on the contents of zip files.  I'd
> like to get all the tests in this directory switched over to that method but
> switching them all is beyond the scope of this bug.  I'll leave it up to you
> if you prefer to make that change now or just land the xpis and let them get
> converted when the rest of the directory gets converted.  Of course, if you
> want to work on the follow-up to convert the existing tests, that would be
> great!
Okay I've added setting the signatures.required pref to false to the patch.
While not having to check in unreadable files is a definite plus, I'll just leave them as XPIs for now since changing the tests enough to create these ones on the fly seems as much work as changing the test to create all three (legacy and update_perms).  Additionally I'm not sure how to serve generated files in a mochitest to the updater, is there some equivalent to xpcshell's createHttpServer?
Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188864
Attachment #8910944 - Flags: review?(aswan) → review+
Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188866
Comment on attachment 8911595 [details]
Bug 1331769 - Test that there is no prompt for origin permission narrowing.

https://reviewboard.mozilla.org/r/183018/#review188868
Attachment #8911595 - Flags: review?(aswan) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1b732f1db720
Properly check whether new origin permissions are a subset of old origin permissions. r=aswan
https://hg.mozilla.org/integration/autoland/rev/e48250ab7382
Test that there is no prompt for origin permission narrowing. r=aswan
Keywords: checkin-needed
Attached image hostPrompted.gif
I observed for the extension hostpermissionstest0 if you upgrade from v1.1 to v1.2 will display the permission string for <all_urls>, is this expected?

Checked on Firefox 58.0a1 (20171002220204) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

Extension used:
https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissionstest0/versions/
v1.0 “<all_urls>” -> v1.1 “*://*.example.com/*” -> v1.2 "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"
Flags: needinfo?(moz-ian)
(In reply to CosminB from comment #17)
> Created attachment 8914672 [details]
> hostPrompted.gif
> 
> I observed for the extension hostpermissionstest0 if you upgrade from v1.1
> to v1.2 will display the permission string for <all_urls>, is this expected?
Isn't that because "https://*/*" is also presented as all websites?  I don't think the prompt makes protocol distinctions to avoid confusing the average user, it only mentions domains.  In fact that result should have been true before this patch, as far as I can deduce.
Flags: needinfo?(moz-ian)
Attached image hostNotPrompt.gif
Yes, you are right Ian Moody.
Thanks for the help!

This issue is verified as fixed on Firefox 58.0a1 (20171002220204) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

Upgrading to a new version, the host permissions will not trigger a new prompt. 

Extension used:
https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissionstest0/versions/
v1.0 “<all_urls>” -> 
v1.1 “*://*.example.com/*” -> 
v1.2 "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"

https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissiont/versions/
v1.0 "*://*.example.com/*", "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*" -> 
v1.1 "http://*.example.com/*", "http://subdomain.example2.com/*", "http://www.example3.com/path/*", "https://*.example4.com/*" 

Please see the attached video.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1426066
Duplicate of this bug: 1441693
You need to log in before you can comment on or make changes to this bug.