Closed Bug 1375485 Opened 2 years ago Closed 2 years ago

urls or <all_urls> can be requested as "permissions" instead of "origins"

Categories

(WebExtensions :: General, defect, P5)

56 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: cbadescu, Assigned: Kwan, Mentored)

References

Details

(Keywords: good-first-bug, regression, Whiteboard: [triaged])

Attachments

(4 files, 2 obsolete files)

[Affected versions]:
- Firefox 56.0a1 (2017-06-21) 
 
[Affected platforms]:
- Windows 7 64-bit
- Ubuntu 16.04 32-bit
 
[Steps to reproduce]:
1.Install the attached WebExtension.
2.Click on the icon of the Webextension.
3.Request for optional permissions.
 
[Expected results]:
- The host permissions that are not included in “origins” are not silently granted.
 
[Actual results]:
- The host permissions that are not included in “origins” are silently granted.
 
[Regression range]
Last good revision: 2cf23132f3b1aba5ed6cfcd2b9cdaabb5cccc0f5
First bad revision: def0cd765925f547ab60ac5647253578e4a56e26
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2cf23132f3b1aba5ed6cfcd2b9cdaabb5cccc0f5&tochange=def0cd765925f547ab60ac5647253578e4a56e26
 
[Additional notes]:
- This issue reproduces constantly.
This is another entry in the list of api quirks we have inherited from google -- when calling permissons.requst(), origin permissions are distinguished from regular permissions but when listed in the optional_permissions property in the manifest both types of permissions are included in a single list.  Inside request(), we just check that everything being requested is present in optional_permissions, we don't actually check that they are of the correct type.

I've updated the title to be more clear, since we don't actually grant host permissions if they are requested in "permissions" instead of "origins".  I think the impact here is just annoyance to extension authors who write code like this by mistake.  I guess the fix is just to validate the arguments to request() using classifyPermission()
Summary: Host permissions and <all_urls> can be silently granted if they are not included in “origins” → urls or <all_urls> can be requested as "permissions" instead of "origins"
Can you help find an owner for this bug? Or is it something for your general backlog? Do you have a way to indicate priority for your team? (P1, P2, etc) Thanks.
Flags: needinfo?(amckay)
Sorry about that liz.
Mentor: aswan
Flags: needinfo?(amckay)
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: [triaged]
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Comment on attachment 8910507 [details]
Bug 1375485 - Expose classifyPermissions on ExtensionData.

https://reviewboard.mozilla.org/r/181926/#review188092

::: toolkit/components/extensions/Extension.jsm:451
(Diff revision 1)
> +   *          "origin" to indicate this is a host/origin permission.
> +   *          "api" to indicate this is an api permission
> +   *                (as used for webextensions experiments).
> +   *          "permission" to indicate this is a regular permission.
> +   */
> +  classifyPermission(perm) {

please make this static
Attachment #8910507 - Flags: review?(aswan) → review+
Comment on attachment 8910508 [details]
Bug 1375485 - Validate permission types in permissions.request.

https://reviewboard.mozilla.org/r/181928/#review188094

The code looks good (and thank you!) but can we please have a test before landing this.  This should be easily testable from xpcshell.
Attachment #8910508 - Flags: review?(aswan) → review-
While writing tests for this I've changed my mind about the best way to fix it.  There is already permissions validation carried out using the schemas, which catch invalid permissions and permissions requested as origins.  This bug is actually caused by an error in the schemas.  They reuse the same definition of (Optional)Permission for both the permissions API and the manifest.
Instead the manifest schema should use a separate (Optional)PermissionOrOrigin type for its (optional_)permissions field, and (Optional)Permission should be limited to actual permissions.

New patch (plus test) incoming shortly.
Attachment #8910507 - Attachment is obsolete: true
Attachment #8910508 - Attachment is obsolete: true
Comment on attachment 8911495 [details]
Bug 1375485 - Update the extension manifest schema to distinguish between permissions and origins.

https://reviewboard.mozilla.org/r/182948/#review188540

very nice, thanks!
Attachment #8911495 - Flags: review?(aswan) → review+
Comment on attachment 8911496 [details]
Bug 1375485 - Add test to verify that requesting an origin or permission in the wrong field throws an error.

https://reviewboard.mozilla.org/r/182950/#review188544

Looks good.
Heads up though, this is going to conflict with the patches on bug 1402066.
Attachment #8911496 - Flags: review?(aswan) → review+
(In reply to Andrew Swan [:aswan] from comment #12)
> Comment on attachment 8911496 [details]
> Bug 1375485 - Add test to verify that requesting an origin or permission in
> the wrong field throws an error.
> 
> https://reviewboard.mozilla.org/r/182950/#review188544
> 
> Looks good.
> Heads up though, this is going to conflict with the patches on bug 1402066.

Thanks for the heads up.  I'll wait for 1402066 to land, then rebase on top of it (assuming it does within the next week or so).
Looks great, thanks.  Land away when you have a good try run.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d3efdc909cb9
Update the extension manifest schema to distinguish between permissions and origins. r=aswan
https://hg.mozilla.org/integration/autoland/rev/4c664d3d37fa
Add test to verify that requesting an origin or permission in the wrong field throws an error. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3efdc909cb9
https://hg.mozilla.org/mozilla-central/rev/4c664d3d37fa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this something that can ride the 58 train or did you want to request Beta approval for 57?
Flags: needinfo?(moz-ian)
Flags: in-testsuite+
I don't _think_ there's a need for uplift, it doesn't really affect anything user facing as far as I know, just makes it clearer to developers by making something that silently failed before an explicit fail now.  Though given that it could break some ext code that doesn't catch the fail, perhaps it's worth having in place for 57 and the start of the webext only world.  Andrew?

I would be more inclined to request uplift for bug 1331769 however, as that is user facing, though how often it'd be an actual issue is another matter.
Flags: needinfo?(moz-ian) → needinfo?(aswan)
I think letting this ride the trains is fine.
Flags: needinfo?(aswan)
Attached file Requests.zip
This issue is verified as fixed on Firefox 58.0a1 (20171001220301) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

An error message is displayed in the browser console if  you request a permission/origin in the wrong field.

Please see the attached screenshots.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.