1560570 - FeaturePolicy should be considered when permissions.query() is called

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Andrea Marchesini [:baku]]

There are many feature-policy 'features' which match 'permissions'. We should consider the feature-policy state when permissions.query() is called. For instance, if feature-policy denies 'geolocation', |permissions.query({name:'geolocation'})| should be resolved with a 'deny' PermissionStatus.

Comment 1

[Andrea Marchesini [:baku]]

:nhnt11, do you want to take this bug?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Sure, I can take this. I have a high-level question before I start writing a patch: it seems like our FeaturePolicy features are defined separately from our permissions. Do we have a mapping of permission -> feature? If they are identical, is the "identicality" enforced somewhere?

Depending on the answer, this patch might involve implementing that mapping. I'll do some digging but wanted to put the question here in case you can save me some archaeology :)

Comment 3

[Andrea Marchesini [:baku]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #2)

Sure, I can take this. I have a high-level question before I start writing a patch: it seems like our FeaturePolicy features are defined separately from our permissions. Do we have a mapping of permission -> feature? If they are identical, is the "identicality" enforced somewhere?

No. This is part of the issue we want to fix. I would suggest to extend this:
https://searchfox.org/mozilla-central/rev/0b7007a23bc16c857f894140e12f307bfeef2fdd/dom/security/featurepolicy/FeaturePolicyUtils.cpp#18-21
If you introduce a "const char* permissionName" (null for all the features without a associated permission), then, you can add a static function in FeaturePolicyUtils to obtain the feature-name from a permission name (and/or viceversa).

You can do this mapping in the permissionUtils.cpp too: https://searchfox.org/mozilla-central/rev/0b7007a23bc16c857f894140e12f307bfeef2fdd/dom/permission/PermissionUtils.cpp#12

But I prefer to have it in FeaturePolicyUtils.cpp. Maybe there is a better approach :)

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1560570 - Consult FeaturePolicy when testing a permission. r=baku

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

The attachment is just a WIP for initial feedback. It adds permission names when relevant in the feature map, and exposes a function that gets the feature name for a given permission. It uses this to check FeaturePolicy when testing a permission, and if the feature is not allowed, sets DENY_ACTION. I tested this locally by forcing FeaturePolicyUtils::IsFeatureAllowed to always return false and then testing geolocation permission.

Things to consider:

1. TESTS!!!!!
2. Baku said we should verify that the permission API is exposed to workers.
3. We should expose something (like a console error) to the frontend to indicate that the permission was auto-denied due to feature policy (probably useful for developers)
4. I wonder if there's any utility in creating a new explicit permission action to indicate FeaturePolicy denial.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Nihanth, can you please make this bug block the main feature-policy one whenever you get a chance.

Comment 7

[Thomas Nguyen (:tnguyen)]

Hi Nihanth, are you still looking at this? Can I take this bug?

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Yeah, feel free, I never managed to clear my head enough to resume this patch... Thanks and sorry! I unassigned myself.

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1560570- FeaturePolicy should be considered when permissions.query() is called

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa175de96c82
FeaturePolicy should be considered when permissions.query() is called r=baku,johannh

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19199 for changes under testing/web-platform/tests

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 13

[Stefan Hindli [:stefan_hindli]]

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

Comment 14

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot