Closed Bug 1277686 Opened 3 years ago Closed 3 years ago

Reject manifests with any "incognito" property other than "spanning"

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox48 verified, firefox49 verified, firefox50 verified)

VERIFIED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: kmag, Assigned: bsilverberg, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

Extensions which specify this property with a value that we don't support are doing to wind up in an environment they don't expect, and may wind up causing private browsing data to leak.

For the moment, we should continue to default to "spanning" mode, since we don't have any support for other modes, but explicitly specifying another mode should be an error.
Mentor: kmaglione+bmo
Whiteboard: [good first bug]
> but explicitly specifying another mode should be an error.

That's a little bit harsh, isn't it? I'd expect the extension authors then to change the value to "spanning" maybe without doing further changes, which isn't any better. 

Is it possible to "show" these extensions only non-incognito mode windows and tabs?
"possible", but that doesn't seem like a good first bug. This is a really quick and easy way to signal to developers what's going to happen and reduce the number of surprises for users and developers. Seems like a good idea to me, the work to continue on improving the modes can continue.
I agree. It would be possible to only show extensions non-incognito data, yes, but that would be the same as implementing `"incognito": "not_allowed"`, which is much more work than simply validating the property.

For the moment, I'm more concerned about giving developers consistent behavior.
This should be implemented by adding an optional "incognito" property to the manifest schema[1].

It should be an enumerated string property, with one choice: "spanning"

We should also add an xpcshell test[2] to check the validation. This test should serve as a good blueprint for that:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_content_security_policy.js

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Iteration: --- → 50.1
Kris, at the moment it looks like I have some time available this week to work on new bugs. Would you like me to take this one off your hands?
Flags: needinfo?(kmaglione+bmo)
Sure, but this will also need a separate version of the patch with `"onError": "warn"` for uplift to Aurora and Beta.
Assignee: kmaglione+bmo → bob.silverberg
Flags: needinfo?(kmaglione+bmo)
Attachment #8761553 - Flags: review?(kmaglione+bmo)
Status: NEW → ASSIGNED
Comment on attachment 8761553 [details] [diff] [review]
Version of the patch for uplift to Aurora and Beta

Review of attachment 8761553 [details] [diff] [review]:
-----------------------------------------------------------------

Let's land this version in m-c first, and create a separate bug to land the stricter version once it's baked for a couple of weeks.
Attachment #8761553 - Flags: review?(kmaglione+bmo) → review+
Attachment #8761548 - Attachment is obsolete: true
Attachment #8761548 - Flags: review?(kmaglione+bmo)
Blocks: 1280027
For now we will land the version that generates an warning. I filed bug 1280027 as a follow-up to change the warning to an error, which we will land in the future.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/cf14f77f4f1d
Issue a warning for manifests with any "incognito" property other than "spanning", r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf14f77f4f1d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attached patch Patch for Aurora uplift (obsolete) — Splinter Review
This one is for uplift to aurora
Attachment #8761553 - Attachment is obsolete: true
(In reply to Bob Silverberg [:bsilverberg] from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=16d8cc0fd731

Aurora try
(In reply to Bob Silverberg [:bsilverberg] from comment #18)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=68cfd16c2c21

beta try
Something seems to have gone horribly wrong with my pushes to try of the patch on top of aurora and beta. I'm trying again, but perhaps I'm not doing it right. 

The new push on top of beta is https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5f2fcf1381.

The new push on top of aurora is https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4440b7ba01e.
Attachment #8763245 - Attachment is obsolete: true
(In reply to Bob Silverberg [:bsilverberg] from comment #24)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb853a07853

New aurora try
(In reply to Bob Silverberg [:bsilverberg] from comment #26)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7265ff440e79

New beta try
Comment on attachment 8763619 [details] [diff] [review]
Patch for Aurora uplift

Approval Request Comment
[Feature/regressing bug #]: bug 1277686
[User impact if declined]: We would like to prevent extensions from using an unsupported value of "incognito".
[Describe test coverage new/current, TreeHerder]: A new test was written to support this change, which is included in the patch. Treeherder results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb853a07853
[Risks and why]: This is a very small change, with a test included, therefore can be considered low risk.
[String/UUID change made/needed]: None.
Attachment #8763619 - Flags: approval-mozilla-aurora?
(In reply to Bob Silverberg [:bsilverberg] from comment #28)
> [Risks and why]: This is a very small change, with a test included,
> therefore can be considered low risk.

To add a bit more detail: This patch adds some metadata so that an unsupported
setting in a WebExtension manifest triggers an error console warning, which
should be visible to developers. In nightlies, unsupported values for this
flag will soon trigger an error, rather than a warning, so uplifting this
patch is likely to prevent add-ons from breaking as Firefox 50 rides the
trains, as well as alerting developers that they won't be getting the behavior
that they expect as a result of setting this flag.
Comment on attachment 8763619 [details] [diff] [review]
Patch for Aurora uplift

Approval Request Comment
[Feature/regressing bug #]: bug 1277686
[User impact if declined]: We would like to prevent extensions from using an unsupported value of "incognito".
[Describe test coverage new/current, TreeHerder]: A new test was written to support this change, which is included in the patch. Treeherder results at 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7265ff440e79
[Risks and why]: This is a very small change, with a test included, therefore can be considered low risk.
[String/UUID change made/needed]: None.

Also see Kris' additional comment above.
Attachment #8763619 - Flags: approval-mozilla-beta?
Comment on attachment 8763619 [details] [diff] [review]
Patch for Aurora uplift

OK, taking it to improve the webextension support
taking it, should be in 48 beta 3
Attachment #8763619 - Flags: approval-mozilla-beta?
Attachment #8763619 - Flags: approval-mozilla-beta+
Attachment #8763619 - Flags: approval-mozilla-aurora?
Attachment #8763619 - Flags: approval-mozilla-aurora+
Verified fixed on Firefox 51.0a1 (2016-08-24), Firefox 50.0a2 (2016-08-25), Firefox 49 beta 6 (20160822111414) and Firefox 48.0.2 (20160823121617) under Windows 10 64-bit.

The following warning is successfully thrown in Browser Console:
“1472127171356	addons.webextension.EarthViewIcaTest@csrf.jp	WARN	Loading extension 'EarthViewIcaTest@csrf.jp': Reading manifest: Error processing incognito: Invalid enumeration value "split"”
I know its an old bug, but could we get a page about the incognito manifest property please?
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.