Closed Bug 1189073 Opened 4 years ago Closed 4 years ago

Adding localhost:9091 cookie exceptions fails with console error

Categories

(Firefox :: Preferences, defect)

42 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- unaffected
firefox42 + fixed

People

(Reporter: mozilla, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
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]
Blocks: 1165263
Component: Untriaged → Preferences
Keywords: regression
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...
Blocks: 1173523
No longer blocks: 1165263
(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://.
Attachment #8640747 - Flags: review?(michael)
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.
Attachment #8640747 - Flags: review?(michael) → review-
(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!
Is there a reason why the status is unconfirmed after triaging?
Flags: needinfo?(ehsan)
(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 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 :)
Attachment #8640747 - Flags: review- → review+
Tracking since this is a regression. Let's get this into nightly!
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.  :-)
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/2c75751cea07
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.