Closed
Bug 1224950
Opened 9 years ago
Closed 9 years ago
Limit Message-Using Remote New Tab to Nightly and Aurora
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
People
(Reporter: oyiptong, Assigned: oyiptong)
References
Details
(Keywords: addon-compat)
Attachments
(4 files)
(deleted),
text/x-review-board-request
|
mconley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mconley
:
review+
|
Details |
(deleted),
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This tracks adding a build flag that limits the implementation of the Remote NewTab page using RemotePageListener to Nightly and Aurora due to a privilege escalation concern with RemotePageListener.
We will implement a set of API's instead of sending messages. We'll go beyond Aurora after removing our dependency on messages.
Comment 1•9 years ago
|
||
To be clear, message passing itself is not inherently a problem. The specific problems are:
(1) Running unprivileged content in an <iframe> of a privileged document.
(2) That RemotePageManager has not been audited or hardened to interact with untrusted code.
We could certainly fix (2), but my impression was that we weren't doing that because it wasn't what we were planning to ship anyway. My concern was that we were pressing ahead with shipping the interim solution, which may not have had proper security auditing.
Assignee | ||
Comment 2•9 years ago
|
||
Ok. Sounds good. We are fixing both, in bug 1218996 and bug 1218998.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → oyiptong
Assignee | ||
Updated•9 years ago
|
Summary: Add build flag to keep meesaging-based remote new tab page in Nightly and Aurora → Limit Message-Using Remote New Tab to Nightly and Aurora
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Attachment #8688754 -
Flags: review?(mconley)
Assignee | ||
Comment 5•9 years ago
|
||
A try-server build with the RELEASE_BUILD flag set can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade074496609
Comment 6•9 years ago
|
||
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
https://reviewboard.mozilla.org/r/25401/#review22947
I built a release build locally, and it seemed to behave properly, and I could not access about:remote-newtab despite setting the pref, so that bit works.
I found a few places where you can use AppConstants. Otherwise, this looks great. Thanks for doing this! :D
::: browser/app/profile/firefox.js:1350
(Diff revision 1)
> -// activates the remote-hosted newtab page
> +// if true, it activates the remote-hosted newtab page
> +#ifndef RELEASE_BUILD
> pref("browser.newtabpage.remote", false);
> +#endif
Best to move the comment within the ifdef
::: browser/components/nsBrowserGlue.js:29
(Diff revision 1)
> +#ifndef RELEASE_BUILD
```
if (!AppConstants.RELEASE_BUILD) {
...
}
```
::: browser/components/nsBrowserGlue.js:854
(Diff revision 1)
> +#ifndef RELEASE_BUILD
```
if (!AppConstants.RELEASE_BUILD) {
...
}
```
::: browser/components/nsBrowserGlue.js:1180
(Diff revision 1)
> +#ifndef RELEASE_BUILD
```
if (!AppConstants.RELEASE_BUILD) {
...
}
```
::: browser/components/nsBrowserGlue.js:2573
(Diff revision 1)
> +#ifndef RELEASE_BUILD
```
if (!AppConstants.RELEASE_BUILD) {
...
}
```
Attachment #8688754 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://reviewboard.mozilla.org/r/25401/#review22947
> Best to move the comment within the ifdef
Was just following what other ifdefs were doing. I don't have any strong feelings either way.
> ```
> if (!AppConstants.RELEASE_BUILD) {
> ...
> }
> ```
There is extensive use of ifdef in nsBrowserGlue.js and it does get in the list of pre-processed JS files. I'm following what other pieces of code is doing.
AppConstants is actually only used once:
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js#893
I would err on the side of using ifndef. Doubly so since this is a file which runs on startup.
Comment 8•9 years ago
|
||
(In reply to Olivier Yiptong [:oyiptong] from comment #7)
> https://reviewboard.mozilla.org/r/25401/#review22947
>
> > Best to move the comment within the ifdef
>
> Was just following what other ifdefs were doing. I don't have any strong
> feelings either way.
Without doing that, the comment in the final output after pre-processing just kinda floats around not being applied to anything, which is kinda weird. Folks don't normally look at processed files, but they've started showing up in DXR, which is why I mention it.
>
> > ```
> > if (!AppConstants.RELEASE_BUILD) {
> > ...
> > }
> > ```
>
> There is extensive use of ifdef in nsBrowserGlue.js and it does get in the
> list of pre-processed JS files. I'm following what other pieces of code is
> doing.
> AppConstants is actually only used once:
>
> https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/
> dist/bin/browser/components/nsBrowserGlue.js#893
>
> I would err on the side of using ifndef. Doubly so since this is a file
> which runs on startup.
We've only recently (last couple of months?) introduced AppConstants, as a way of (slowly) moving our JS over to something that can be linted properly[1]. The reason you're probably seeing only one usage here is because we just haven't gotten around to introducing AppConstants in that file.
But we should avoid, if we can, introducing more pre-processor directives.
If this truly results in some kind of startup regression, I'm willing to relent.
[1]: https://mail.mozilla.org/pipermail/firefox-dev/2015-February/002689.html
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> We've only recently (last couple of months?) introduced AppConstants, as a
> way of (slowly) moving our JS over to something that can be linted
> properly[1]. The reason you're probably seeing only one usage here is
> because we just haven't gotten around to introducing AppConstants in that
> file.
>
> But we should avoid, if we can, introducing more pre-processor directives.
>
> If this truly results in some kind of startup regression, I'm willing to
> relent.
>
> [1]: https://mail.mozilla.org/pipermail/firefox-dev/2015-February/002689.html
Got it. Makes sense. I'll add the AppConstants checks.
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8688754 -
Attachment description: MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley → MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r=mconley
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25401/diff/1-2/
Assignee | ||
Comment 13•9 years ago
|
||
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
Attachment #8689360 -
Flags: review?(mconley)
Comment 14•9 years ago
|
||
Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
https://reviewboard.mozilla.org/r/25573/#review23131
Thanks!
Attachment #8689360 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d87a6c6accf1161b5086d648944ec77e873a0494
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f69fd164a031deb79cfe3cc6fb914047364ab5ec
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r=mconley
Comment 16•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d87a6c6accf1
https://hg.mozilla.org/mozilla-central/rev/f69fd164a031
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for nightly: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f69fd164a031&selectedJob=17567943. testing triggering a release build and the tests/code didn't execute. tried a try-server release build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e01057d707c
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none
Attachment #8688754 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for nightly: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f69fd164a031&selectedJob=17567943. testing triggering a release build and the tests/code didn't execute. tried a try-server release build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e01057d707c
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none
Attachment #8689360 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•9 years ago
|
||
Note: Both of these patches need to be applied for a build to be successful.
status-firefox44:
--- → affected
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
This has been on Nightly for a few days so seems safe. The patch is also removing code until the right mitigation is put into place. Let's uplift to Aurora44.
Attachment #8688754 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
Aurora44+
Attachment #8689360 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 22•9 years ago
|
||
this has problems during uplift to aurora:
warning: conflicts while merging browser/app/profile/firefox.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/about/AboutRedirector.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/newtab/moz.build! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/nsBrowserGlue.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
could you take a look? Thanks
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25401/diff/2-3/
Attachment #8688754 -
Attachment description: MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r=mconley → MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25573/diff/1-2/
Comment 27•9 years ago
|
||
Hey oyiptong - I don't think you need r+ for a straight rebase, which this looks like it was (the interdiff shows no actual changes to your patch). Can you confirm?
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 28•9 years ago
|
||
Ah, there were some (minor) changes. Perhaps reviewboard isn't showing them?
Flags: needinfo?(oyiptong)
Comment 29•9 years ago
|
||
I'm fine reviewing a Splinter patch, in that case.
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8694233 -
Flags: review?(mconley)
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8694234 -
Flags: review?(mconley)
Comment 32•9 years ago
|
||
Comment on attachment 8694233 [details] [diff] [review]
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora
Review of attachment 8694233 [details] [diff] [review]:
-----------------------------------------------------------------
So I manually compared this patch against the one posted in MozReview, and I think they're equivalent, which is probably why MozReview showed an empty interdiff. Or am I somehow missing some fiddling you had to do during the rebase?
::: browser/components/nsBrowserGlue.js
@@ +846,5 @@
> NewTabUtils.init();
> NewTabUtils.links.addProvider(DirectoryLinksProvider);
> AboutNewTab.init();
>
> + if(!AppConstants.RELEASE_BUILD) {
Space after if. I guess I missed this when I reviewed the Nightly patch. :/
@@ +1171,5 @@
>
> CustomizationTabPreloader.uninit();
> WebappManager.uninit();
>
> + if(!AppConstants.RELEASE_BUILD) {
Space after if
Attachment #8694233 -
Flags: review?(mconley) → review+
Comment 33•9 years ago
|
||
Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js
Review of attachment 8694234 [details] [diff] [review]:
-----------------------------------------------------------------
Same as above.
Attachment #8694234 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley
Removing uplift request, as the patch was made for m-c and the repos have diverged. Flagging aurora-specific bug instead
Attachment #8688754 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley
Removing uplift request, as the patch was made for m-c and the repos have diverged. Flagging aurora-specific bug instead
Attachment #8689360 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8694233 [details] [diff] [review]
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora
Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b1994914f9
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none
Attachment #8694233 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js
Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b1994914f9
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none
Attachment #8694234 -
Flags: approval-mozilla-aurora?
Comment 38•9 years ago
|
||
Hey oyiptong - pretty sure you got approvals on the equivalent patches. You just rebased them. I don't think you need further approval. Pretty sure you're good to go here.
Assignee | ||
Comment 39•9 years ago
|
||
awesome!
Attachment #8694233 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js
rebased. aurora44+
Attachment #8694234 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 41•9 years ago
|
||
bugherder uplift |
Comment 42•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3228f68ffc38
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/54819a857281
status-b2g-v2.5:
--- → fixed
Comment 43•9 years ago
|
||
Not only was the NewTabUtils deprecation notice from bug 1204983 not mentioned on the Firefox 44 for developers page but there's no mention of this removal there either...
status-b2g-v2.5:
fixed → ---
Keywords: addon-compat
Assignee | ||
Comment 44•9 years ago
|
||
It was an oversight on my part. Please check bug 1240559, comment 10 for a solution for 44 and 45.
NewTabURL.jsm is back in 46 without a deprecation notice.
Comment 45•9 years ago
|
||
Thanks for the update.
You need to log in
before you can comment on or make changes to this bug.
Description
•