Closed Bug 744936 Opened 8 years ago Closed 6 years ago

Security review for In-content preferences

Categories

(mozilla.org :: Security Assurance: Review Request, task, minor)

task
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: curtisk, Assigned: dveditz)

References

()

Details

(Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][score:0::Low][Fx])

Attachments

(1 obsolete file)

Who is/are the point of contact(s) for this review?
    
Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
    
Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
    
Does this request block another bug? If so, please indicate the bug number This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)

Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?

Are there any portions of the project that interact with 3rd party services?

Will your application/service collect user data? If so, please describe 

If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):

Desired Date of review (if known from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and whom to invite.
Assignee: nobody → dveditz
Status: NEW → ASSIGNED
I don't think we need to do a formal security review.

The feature basically moves the preferences from a dialog to a chrome URL, and followed the same practices as about:permissions and about:addons.
Whiteboard: [pending secreview] → [pending secreview][triage needed]
I don't think it needs a formal sec review either, fwiw - for the same reasons Jared described. Additionally, this is also only the first pass for in-content preferences, and really only moves the existing preferences from a dialog to an in-content page. The second pass, which will involve a re-think of how the preferences are organised, might benefit from a sec review - as it will involve re-organising security-sensitive preferences.


(In reply to Curtis Koenig [:curtisk] from comment #0)
> Who is/are the point of contact(s) for this review?

Jared Wein <jwein@mozilla.com> and Blair McBride <bmcbride@mozilla.com>

> Please provide a short description of the feature / application (e.g.
> problem solved, use cases, etc.):

Move preferences from a popup dialog to an in-content (ie, loaded in a tab) UI.

> Please provide links to additional information (e.g. feature page, wiki) if
> available and not yet included in feature description:

https://wiki.mozilla.org/In-content_preferences

> Does this request block another bug? If so, please indicate the bug number

No.

> This review will be scheduled amongst other requested reviews. What is the
> urgency or needed completion date of this review?

Not urgent, it will be landed disabled.

> Please answer the following few questions: (Note: If you are asked to
> describe anything, 1-2 sentences shall suffice.)
> 
> Does this feature or code change affect Firefox, Thunderbird or any product
> or service the Mozilla ships to end users?

Only Firefox for desktop.

> Are there any portions of the project that interact with 3rd party services?

No.

> Will your application/service collect user data? If so, please describe 

No.

> Desired Date of review (if known from
> https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html)
> and whom to invite.

TBA.
Whiteboard: [pending secreview][triage needed] → [pending secreview][triage needed 2012.05.02]
What was the outcome of this triage?
Summary: In-content preferences → Security review for In-content preferences
:jaws - we had to move triage around and Dan was unable to attend. And I wanted him to weigh in on where he thinks we need to go with this. So lets just ask him here in-bug

:dveditz - do you agree with comment 2 that we can skip this? If so just close this resolved.
I am not sure if this is the right place to post
but would love to bring this to your notice

Extra click(+ distractions) to reach same sub menu with new implementation are making it so hard to use

In older implementation to switch between say General and Advanced, 
user can click the respected icon at the top of the dialog. However now to do so , one has to click back button , then see the sub-menu he/she has to go and click it again , this adds 1 more click and distracts from the motive IMHO.
one cannot jump from one options page to another, without first going back & this slows the user down.
it should just like the addons manager or the current in-window type where we have the buttons always visible together , (and the current one selected like we have it now).
This make the process just like it is in older versions(except in-content)
I am not sure if this is the right place to post
but would love to bring this to your notice

Extra click(+ distractions) to reach same sub menu with new implementation are making it so hard to use

In older implementation to switch between say General and Advanced, 
user can click the respected icon at the top of the dialog. However now to do so , one has to click back button , then see the sub-menu he/she has to go and click it again , this adds 1 more click and distracts from the motive IMHO.
one cannot jump from one options page to another, without first going back & this slows the user down.
it should just like the addons manager or the current in-window type where we have the buttons always visible together , (and the current one selected like we have it now).
This make the process just like it is in older versions(except in-content)
magnumarchonbasileus - this bug is for tracking activities related to the security of the feature. I don't see a relation to the security of the feature or it's proposed implementation in your comments. Bug 718011 is a meta bug for this feature (which this bug blocks), you might want to check through that set of bugs*  for the appropriate place to add this.


* (https://bugzilla.mozilla.org/showdependencytree.cgi?id=718011&hide_resolved=0)
If a querystring is implemented such as about:preferences?section=tabs or similar will that have security implications? Sorry I don't know if there's a bug filed about it yet.
(In reply to Mardeg from comment #8)
> If a querystring is implemented such as about:preferences?section=tabs or
> similar will that have security implications? Sorry I don't know if there's
> a bug filed about it yet.

No. Adding a query string to a chrome URI has no security implications, IMO. But there's also no concrete plans for ever doing that. Bug 741047, which solves a similar (or same?) problem, doesn't do it via the URL.

At one stage we were talking about maybe implementing a custom URL scheme for these types of UIs, like firefox:preferences/tabs, firefox:addons/appearance, etc. That would certainly require a sec review if we ever did it.
Attachment #623125 - Attachment is obsolete: true
Risk/Priority Ranking Exercise https://wiki.mozilla.org/Security/RiskRatings

Priority: N/A

Operational: 0 - N/A
User: 0 - N/A
Privacy: 0 - N/A
Engineering: 1 - Minor
Reputational: 0 - N/A

Priority Score: 0
Severity: normal → minor
Whiteboard: [pending secreview][triage needed 2012.05.02] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][score:0::Low]
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][score:0::Low] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][score:0::Low][Fx]
This move would have security implications.

Displaying the prefs in the same place and the same way Firefox displays Web pages would be a dangerous confusion.

As a user :

* The prefs dialog is in my home, I have to be careful when I am there. This dialog controls the behaviour of my computer.

* On the other hand, a Web page is in the outside world. It is a sandbox. I can relax. If I mess around in a Web page, it will not break my computer.

Confusing these things would be *a bad idea*.

You may find more detailed developments here :

https://bugzilla.mozilla.org/show_bug.cgi?id=584942#c17
(In reply to Blair McBride [:Unfocused] from comment #9)
> No. Adding a query string to a chrome URI has no security implications, IMO.

It can, actually. Not if it's just switching a view, but there's potential danger if the parameters perform some action. Treat chrome URLS like HTTP GET actions and keep them "idempotent" as required by specs and you should be OK. Luckily web content can't load chrome urls, but other applications (or even "apps") may be able to.

> At one stage we were talking about maybe implementing a custom URL scheme
> for these types of UIs, like firefox:preferences/tabs,
> firefox:addons/appearance, etc. That would certainly require a sec review if
> we ever did it.

It would simply because we're nervous about new schemes, but the use-case and proposal sounds reasonable.

about:preferences is not marked URI_SAFE_FOR_UNTRUSTED_CONTENT so we're good here.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.