1192666 - nsIPrincipal.origin generates invalid URI value for ipv6 addressees

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

The value produced by nsIPrincipal.origin for an ipv6 URI (like `http://[2001:db8::ff00:42:8329]`) incorrectly doesn't include the `[]` characters around the hostname. This is necessary with ipv6 URIs due to ports being ambiguous without it. In fact, the output of `originNoSuffix` for ipv6 URIs is not a valid URI, and thus some code (such as BrowserUtils's principalFromOrigin) will not function correctly.

We probably want to use something similar to `NS_GenerateHostPort` which was introduced in bug 694576 to account for the fact that URIs for ipv6 hosts don't include the `[]` in the host property.

Comment 1

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

Attachment: Emit '[]' around origin strings for ipv6 origins.

I took a jab at implementing this. I was considering re-using the logic from NS_GenerateHostPort, but I wasn't sure if we wanted to strip the part of the ipv6 URI after the '%' like that code does, so I left that part out.

Comment 2

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

Attachment: Emit '[]' around origin strings for ipv6 origins

Added a new test case to the test_origin tests

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8645888 [details] [diff] [review]
Emit '[]' around origin strings for ipv6 origins

Add test also without any port.


What does principal.baseDomain return for ipv6 case?

Comment 4

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

Attachment: Emit '[]' around origin strings for ipv6 origins

principal.baseDomain returns the ipv6 address without the enclosing []s. Should it inclode those?
If it should, it might be nice to do that in a follow up bug.

Comment 5

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #4)
> principal.baseDomain returns the ipv6 address without the enclosing []s.
> Should it inclode those?
> If it should, it might be nice to do that in a follow up bug.

Please file a follow-up for that.  Thanks!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c72acf657631

Comment 7

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

Comment on attachment 8645907 [details] [diff] [review]
Emit '[]' around origin strings for ipv6 origins

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: Permissions for ipv6 addresses will not function correctly in the permission manager (for example, they will not be re-loaded correctly after closing & restarting firefox, and they may not be looked up correctly). Other components may also fail when interacting with ipv6 addresses.
[Describe test coverage new/current, TreeHerder]: There are tests for this change, and the logic is the same as in other places in the code where ipv6 URIs are handled.
[Risks and why]: There could be code which depends on the old behavior for the origin string, however I think that that is unlikely. 
[String/UUID change made/needed]: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c72acf657631

Comment 9

[(no longer active)]

This blocks the uplift of bug 1189070.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8645907 [details] [diff] [review]
Emit '[]' around origin strings for ipv6 origins

Blocks the uplift of a serious bug, has a test, taking it.

Comment 11

[Bobby Holley (:bholley)]

Sorry being late to the game here - swamped last week.

Wouldn't it make more sense for to use ->GetHostPort on the URI rather than duplicating the '[' logic manually?

Comment 12

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

(In reply to Bobby Holley (Mostly Out This Week) from comment #11)
> Sorry being late to the game here - swamped last week.
> 
> Wouldn't it make more sense for to use ->GetHostPort on the URI rather than
> duplicating the '[' logic manually?

That would make more sense. I'll file & fix that in a follow up bug.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #12)
> That would make more sense. I'll file & fix that in a follow up bug.

Rad - thanks Michael!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41bd412c3964