Closed Bug 1172028 Opened 5 years ago Closed 5 years ago

Only fully-signed add-ons should be offered as sideloaded.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 + fixed
firefox41 + fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(2 files)

No description provided.
Assignee: nobody → dtownsend
[Tracking Requested - why for this release]:

This was an error when we implemented add-on signing, sideloaded add-ons should only be usable if fully signed.
Flags: qe-verify-
Flags: firefox-backlog+
Whiteboard: [hijacking][fxsearch]
Rank: 9
Priority: -- → P1
Attached patch patchSplinter Review
This makes add-ons marked with foreignInstall unusable unless fully signed. It also increases the cases where we set that flag by including the staging directory unless there is a cached json manifest there, which broke a few of our tests suites!
Attachment #8616728 - Flags: review?(dveditz)
Comment on attachment 8616728 [details] [diff] [review]
patch

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

r=dveditz
Attachment #8616728 - Flags: review?(dveditz) → review+
Tracking for 40 and 41 because it is affected.
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=70776f034a7c since this or the other cset caused a perma failure in xperf like https://treeherder.mozilla.org/logviewer.html#?job_id=3392932&repo=fx-team
Flags: needinfo?(dtownsend)
Attached patch talos patchSplinter Review
The tp5n test overrides the default talos setting for extensions.autoDisableScopes causing Firefox to refuse to enable the talos extensions on startup with the changes in this patch. Clearing this solves it and I don't think there would be any ill-effects from this.

Interestingly it also clears extensions.enabledScopes which means the default theme is disabled so we're actually doing slightly different file I/O during startup in the xperf run than a real build does, might even be displaying different icons depending on the Windows version. Removing that causes the test to fail with a new file being accessed so I left it for now.
Flags: needinfo?(dtownsend)
Attachment #8621168 - Flags: review?(jmaher)
Comment on attachment 8621168 [details] [diff] [review]
talos patch

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

thanks, I will land this on talos and we can update talos.json and reland.
Attachment #8621168 - Flags: review?(jmaher) → review+
Relanded with the updated talos revision: https://hg.mozilla.org/integration/fx-team/rev/75bc56b719ce
https://hg.mozilla.org/mozilla-central/rev/75bc56b719ce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8616728 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Add-on signing
[User impact if declined]: Preliminarily signed add-ons will be able to be sideloaded which wasn't the intention.
[Describe test coverage new/current, TreeHerder]: On m-c since last week with automated tests
[Risks and why]: Low risk, this is a straightforward change
[String/UUID change made/needed]: None
Attachment #8616728 - Flags: approval-mozilla-aurora?
Comment on attachment 8616728 [details] [diff] [review]
patch

Potential source of trouble, taking the fix.
Attachment #8616728 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I just uplifted m-b talos.json desktop pin to point to the same as m-c, and m-a in order to pick up a fix from: https://bugzil.la/1171159

sanity check, does m-b require https://bugzilla.mozilla.org/attachment.cgi?id=8616728 too if I did: https://hg.mozilla.org/releases/mozilla-beta/diff/2f8d779efaa1/testing/talos/talos.json
Flags: needinfo?(dtownsend)
(In reply to Jordan Lund (:jlund) from comment #15)
> I just uplifted m-b talos.json desktop pin to point to the same as m-c, and
> m-a in order to pick up a fix from: https://bugzil.la/1171159
> 
> sanity check, does m-b require
> https://bugzilla.mozilla.org/attachment.cgi?id=8616728 too if I did:
> https://hg.mozilla.org/releases/mozilla-beta/diff/2f8d779efaa1/testing/talos/
> talos.json

We're not intending on uplifting this to beta so it shouldn't need it.
Flags: needinfo?(dtownsend)
You need to log in before you can comment on or make changes to this bug.