1328163 - support protocol-agnostic cookie whitelisting

Status: NEW

(Core :: Networking: Cookies, enhancement, P3)

Description

[Patrick Hubenthal]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

Added mozilla.org to cookie whitelist.


Actual results:

http://mozilla.org appeared in whitelist.


Expected results:

Either:
a. mozilla.org appears in whitelist
or:
b. http://mozilla.org *and* https://mozilla.org appear in whitelist

Comment 1

[Patrick Hubenthal]

I'd like to be able to allow domains for cookies regardless of which protocol I'm using to connect to them, without having to add them to the whitelist twice (once with http and once with https), as I currently have to do.

Other users seem to want this too, e.g.:
https://www.reddit.com/r/firefox/comments/3x9vy9/cookie_whitelisting_could_be_much_easier_and/
http://www.ghacks.net/2016/08/12/firefox-49-http-passwords-on-https-sites/#comment-3954852

ALSO related: Anthony Howe's suggestion, at https://bugzilla.mozilla.org/show_bug.cgi?id=543755#c6, to provide "finer granularity of cookie blocking choices, like:

a) block all cookies regardless of HTTP/HTTPS (current model)
b) block all cookies over HTTP, but allow over HTTPS
c) block all cookies over HTTPS, unless 'Secure' attribute set."

Comment 2

[Taylor Fields]

This is relevant to my interests as I brought it up on the Mozilla #introduction IRC a few days ago.

I have been poking around in extensions/cookie to understand how cookie permissions are currently done, but am still unable to pinpoint where exactly the protocols are compared. It should not be difficult to modify the appropriate check once found.

The most relevant function seems to be CommonTestPermission in nsPermissionManager.cpp, but I do not understand it well.

There is also a function, MatchesURI in nsPermission.cpp, that looked relevant, but I could not find it called anywhere else in the code base.

Comment 3

[Amy Chung [:Amy]]

Hi Patrick,
Would you mind to try your problem on 51.0b10?
(You can download it from https://ftp.mozilla.org/pub/firefox/releases/51.0b10/)
I can reproduce your problem on 50, but I found this problem which doesn't occur on 51.0b10.
Thanks!

Comment 4

[Patrick Hubenthal]

Thanks, Amy! I'll try it this evening and post my results here.

Comment 5

[Patrick Hubenthal]

I just tested, and unfortunately I'm still experiencing the same problem on 51.0b10.

Comment 6

[Amy Chung [:Amy]]

Attachment: Screen Shot 2017-01-04 at 4.21.09 PM.png

Hi Patrick,
Would I confirm the reproducing steps to you?
1. Opened Preferences.
2. Selected Privacy
3. Chose Firefox will to "Use custom settings for history".
4. Opened Exceptions.
5. Typed "mozilla.org" on "Address of website"
6. Chose "Allow".
7. Saved Change
8. Opened Exceptions and confirm "mozilla.org" on the site list.

I got the right result(as shown in the attachment) after following the steps above.
And my firefox version is 51.0b10 (64-bit).

Thanks!

Comment 7

[Patrick Hubenthal]

Thank you for listing the steps you took, Amy – I can confirm that they are the same steps I took. But I see from your screenshot that there has been a misunderstanding.

When I type "mozilla.org" into the "Address of website" box in the "Exceptions" interface, then "http://mozilla.org" is added to the site list. This is the case for me with both FF 50 and FF 51, and it's the way cookie exceptions have worked in Firefox for some time now.

However, what I want is different. I want to be able to type "mozilla.org" into the "Address" box and have BOTH "http://mozilla.org" and "https://mozilla.org" added to the site list.

In other words, I'm not reporting a bug; I'm suggesting an improvement – or at least what I think would be an improvement. I'm sorry I didn't make that more clear at the beginning. I see now that the "Importance" tag for this ticket can be set to "Enhancement," so I'll do that now.

Comment 8

[Amy Chung [:Amy]]

Hi Patrick,
Thanks for your suggestions! 
I will take a look at this problem and improve the whitelist function.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Taylor Fields from comment #2)
> The most relevant function seems to be CommonTestPermission in
> nsPermissionManager.cpp, but I do not understand it well.

We cannot change how the PermissionManager functions; it is used in many places for different permissions that most definitely care about scheme. If you want both schemes to be whitelisted then both will have to be stored in the permission manager.

For cookies, at least, it may be reasonable to have the UI create both entries if the user does not specify a scheme. Be careful in modifying the exception dialog UI because I believe it is shared code with permissions where assuming both schemes would not be correct or safe (some permissions are not allowed for http: for example). Might need to add a flag to the dialog creation that says whether it should behave this way or not.

(On the other hand allowing insecure http: to set cookies can be used by a MITM to attack secure sites. Evolving web standards are moving against letting insecure and secure sites share cookies, and even against having insecure sites at all. That's a long battle, of course.)

Comment 10

[Amy Chung [:Amy]]

Attachment: Part1: implementation

Hi Jared,
Would you help me to review my patch?
I added a function as below:
1. User enters the domain name on "Exception - cookie".
2. The whitelist(or blocked list depending on user chooses which function) shows "http://domain_name" and "https://domain_name" at the same time.

About test cases, I would like to create three functions to test blocked situation, allowed situation and allowed session situation.
1. In blocked situation, I will set the expired cookies on http://domain_name and https://domain_name then confirm the cookies doesn't set in DB.
2. In allowed situation, I will set cookies on http://domain_name and https://domain_name then confirm the cookies all exist.
3. In allowed session situation, I will set session cookies and expired cookie on http://domain_name and https://domain_name.
   Then I will confirm the expired cookie can't set in DB, and the session cookies exist.
Would you give me your suggestions about this test cases?

Thanks!

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8827967 [details] [diff] [review]
Part1: implementation

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

Sorry for the slow turn-around-time here. I thought maybe you were still working on this as it was just marked for feedback. Your test cases sound good to me.

::: browser/components/preferences/permissions.js
@@ +101,5 @@
>        } catch (ex) {
> +        let uri_http;
> +        let uri_https;
> +        uri_http  = Services.io.newURI("http://" + input_url, null, null);
> +        uri_https = Services.io.newURI("https://" + input_url, null, null);

Should we at some point stop adding http:// by default? In other words, if I approved this patch to add both schemes, would we later want to remove adding the http: scheme due to its inherent insecurity?

@@ +119,5 @@
>      var capabilityString = this._getCapabilityString(aCapability);
>  
>      // check whether the permission already exists, if not, add it
> +    var permissionExists = false;
> +    var capabilityExists = false;

You can leave these variables as 'let' instead of changing them to 'var'. We are using 'let' in most new code and we would normally just change the other variables to be declared with 'let' instead of removing 'let' in favor of 'var'.

Comment 12

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 13

[Shian-Yow Wu [:swu]]

Is bug 1246096 a dup of this bug?

Comment 14

[Patrick Hubenthal]

I don't think so. The reporter of that bug says at https://bugzilla.mozilla.org/show_bug.cgi?id=1246096#c12 that they're not concerned about the scheme.

(I don't even think that's a real bug, actually: If you look at the attachment to https://bugzilla.mozilla.org/show_bug.cgi?id=1246096#c6 you can see that the problem is really that the reporter was overlooking the difference between `google.com` and `www.google.com`, as Nika Layzell points out in https://bugzilla.mozilla.org/show_bug.cgi?id=1246096#c9.)