1189073 - Adding localhost:9091 cookie exceptions fails with console error

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150514102509

Steps to reproduce:

1. Open cookie exception dialog.
2. Type "localhost:9091" in the entry field and press the access button.


Actual results:

Nothing.  Except an error message in the console.


Expected results:

URI should have been added to the list for http://localhost:9091.

Is this even necessary?  Cookie permissions have never cared about ports, and cookies themselves don't care about ports (maybe they should, but they don't and probably never will), so seems unnecessary to force multiple exceptions for the same thing.

Or maybe this needs the cute migration code that goes through the history and generates every matching URI?  Surely not such a massive burden for a single user-supplied host.

Comment 1

[(no longer active)]

The permission manager is not specific to cookies, and while ports technically don't matter for cookies, they do for other purposes.

I can reproduce the error.  I get this in the terminal from a debug build:

JavaScript error: chrome://browser/content/preferences/permissions.js, line 121: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrincipal.origin]

Comment 2

[(no longer active)]

This is fun!  We parse "localhost:9091" as a URL with scheme "localhost".

Trying to find a way to deal with this that doesn't include special casing...

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #2)
> This is fun!  We parse "localhost:9091" as a URL with scheme "localhost".
> 
> Trying to find a way to deal with this that doesn't include special casing...

That is pretty gross... We could look at the "host" segment of the URI, and if it is all numerical, then we assume it was meant to be parsed as a port? I'm not sure.

We could also try to catch that exception on the origin attribute accessor, and use it to determine if we need to prepend the http://.

Comment 4

[(no longer active)]

Attachment: Handle entering 'localhost:12345' into the cookie exceptions dialog correctly

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8640747 [details] [diff] [review]
Handle entering 'localhost:12345' into the cookie exceptions dialog correctly

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

::: browser/components/preferences/permissions.js
@@ +92,5 @@
>        try {
>          uri = Services.io.newURI(input_url, null, null);
> +        principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
> +        // If we have ended up with an unknown scheme, the following will throw.
> +        principal.origin;

I'm not sure if this is what we want to do, and if it is, I think that we should word the comment a bit differently.

The reason why principal.origin is important is that internally within the permission manager, we need the origin to be available in order to store the permission. Due to the changes in bug 1172080, the origin property is now only valid for some schemes, due to security concerns, which happens to have the desirable effect here.

I would be OK with us using this as a check for whether the URI is one which we would want to store permissions on, but I would definitely want to make the comment more clear.

(I should state that I am not sure of a better mechanism for accomplishing our goals here than to use the system in the origin getter, but it might still be worth looking into).

@@ +97,3 @@
>        } catch(ex) {
>          uri = Services.io.newURI("http://" + input_url, null, null);
> +        principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);

It might be a good idea to try to access .origin in this branch too, such that if it fails after adding the http:// then it produces an error prompt for the user instead of failing quietly.

Comment 6

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #5)
> Comment on attachment 8640747 [details] [diff] [review]
> Handle entering 'localhost:12345' into the cookie exceptions dialog correctly
> 
> Review of attachment 8640747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/permissions.js
> @@ +92,5 @@
> >        try {
> >          uri = Services.io.newURI(input_url, null, null);
> > +        principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
> > +        // If we have ended up with an unknown scheme, the following will throw.
> > +        principal.origin;
> 
> I'm not sure if this is what we want to do, and if it is, I think that we
> should word the comment a bit differently.
> 
> The reason why principal.origin is important is that internally within the
> permission manager, we need the origin to be available in order to store the
> permission. Due to the changes in bug 1172080, the origin property is now
> only valid for some schemes, due to security concerns, which happens to have
> the desirable effect here.
> 
> I would be OK with us using this as a check for whether the URI is one which
> we would want to store permissions on, but I would definitely want to make
> the comment more clear.

Yes, that is what I'm trying to do.  Any suggestions for a better comment?

> (I should state that I am not sure of a better mechanism for accomplishing
> our goals here than to use the system in the origin getter, but it might
> still be worth looking into).

Not sure if I can parse this part...  What do you mean by "the system"?

> @@ +97,3 @@
> >        } catch(ex) {
> >          uri = Services.io.newURI("http://" + input_url, null, null);
> > +        principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
> 
> It might be a good idea to try to access .origin in this branch too, such
> that if it fails after adding the http:// then it produces an error prompt
> for the user instead of failing quietly.

That is a good idea, will do!

Comment 7

[Kate Glazko]

Is there a reason why the status is unconfirmed after triaging?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #6)
> Yes, that is what I'm trying to do.  Any suggestions for a better comment?

How about:

The origin accessor on the principal object will throw if the principal doesn't have a canonical origin representation. This will help catch cases where the URI parser parsed something like `localhost:8080` as having the scheme `localhost`, rather than being an invalid URI. A canonical origin representation is required by the permission manager for storage, so this won't prevent any valid permissions from being entered by the user.

> Not sure if I can parse this part...  What do you mean by "the system"?

I'm not sure either anymore. Ignore that part :S.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8640747 [details] [diff] [review]
Handle entering 'localhost:12345' into the cookie exceptions dialog correctly

r+ with the new comment and getter in the catch branch :)

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression. Let's get this into nightly!

Comment 11

[(no longer active)]

(In reply to Kate Glazko from comment #7)
> Is there a reason why the status is unconfirmed after triaging?

Not really, some devs (myself included) completely ignore that field since it doesn't map to our development process.  :-)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c75751cea07

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c75751cea07