Closed Bug 1192666 Opened 4 years ago Closed 4 years ago

nsIPrincipal.origin generates invalid URI value for ipv6 addressees

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1189070
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.
Attachment #8645500 - Flags: review?(bobbyholley)
Assignee: nobody → michael
Attachment #8645500 - Flags: review?(bobbyholley) → review?(bugs)
Added a new test case to the test_origin tests
Attachment #8645500 - Attachment is obsolete: true
Attachment #8645500 - Flags: review?(bugs)
Attachment #8645888 - Flags: review?(bugs)
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?
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.
Attachment #8645888 - Attachment is obsolete: true
Attachment #8645888 - Flags: review?(bugs)
Attachment #8645907 - Flags: review?(bugs)
Attachment #8645907 - Flags: review?(bugs) → review+
(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!
Blocks: 1193416
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
Attachment #8645907 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c72acf657631
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
This blocks the uplift of bug 1189070.
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.
Attachment #8645907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(michael)
(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.
Flags: needinfo?(michael)
(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!
Blocks: 1195415
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.