1368110 - Create child redirect target (new) channel using parent target channel's original URI

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Honza Bambas (:mayhemer)]

Reason: URI of a channel may not always be the URI we are actually redirecting to in case the channel's result principal URI is turned by the protocol handler to an actual principal that may be different (e.g. in case of a moz-extension: URL leading to a jar: URI, and there are other examples).

OriginalURI during the redirect preprocessing (before opening the new channel or getting the final veto result) reflects the URI we've been originally redirected to (i.e. the Location header value, or the target URI obtained any other way).

This also makes sure that security checking on the child process is made correctly, mainly after we land bug 1367814.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=325ac293772a6e865468e9a9d5758928ab0b9c7b

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8871845 [details] [diff] [review]
v1

This should go through :bz as well.  I think this is enough to fulfill bug 1256122 comment 85.  If the way we construct the channel is identical to how we have done that on the parent process, then we don't need to carry result principal URI, since it MUST result to the same value on child process.

Comment 3

[Jason Duell]

Comment on attachment 8871845 [details] [diff] [review]
v1

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

Honza: bz isn't accepting review requests right now :) so either just land this or try to get ahold of him by other means...

Comment 4

[Honza Bambas (:mayhemer)]

I know, I'll hold, this belongs to the 'sec check URI' group of bugs.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8871845 [details] [diff] [review]
v1

Boris, this should address your comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1256122#c85

It's based on assumption that the child protocol handler will create the channel the same way as he parent side protocol - resulting in channel with the same URI, originalURI and result principal URI on its load info.  Hence, there is no need to send all those down, the originalURI is enough.

There is also a comment in the code: https://bugzilla.mozilla.org/attachment.cgi?id=8871845&action=diff#a/netwerk/protocol/http/HttpChannelParent.cpp_sec2 to explain it.

(Formatting nits will be fixed before landing.)

Thanks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8871845 [details] [diff] [review]
v1

> HttpChannelChild::Redirect1Begin(const uint32_t& registrarId,
>+  nsresult rv;

Why move this around?

If you do, please fix the indent; on the SetupRedirect call; it's broken right now.

> HttpChannelParent::StartRedirect(uint32_t registrarId,
>+  nsresult rv;

Again, why move this?

r=me assuming all this code is running before the parent process calls SetOriginalURI on the post-redirect channel to set it to the same thing as the pre-redirect originalURI.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (vacation until 7/7) from comment #6)
> Comment on attachment 8871845 [details] [diff] [review]
> v1
> 
> > HttpChannelChild::Redirect1Begin(const uint32_t& registrarId,
> >+  nsresult rv;
> 
> Why move this around?

I always place nsresult rv declaration as the first in the method to prevent hidden in-block redeclaration bugs (not caught by any tool chain we use.)

> r=me assuming all this code is running before the parent process calls
> SetOriginalURI on the post-redirect channel to set it to the same thing as
> the pre-redirect originalURI.

Yes!  The channel on the parent and on the child (here) are both created before the redirect vetoing consultation (the callback wants to have the channel as a arg), therefor before we change the original URI.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> Needs rebasing.

Ups, sorry for that.  The patch lingered for too long...

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1

rebased, and to make sure everything's still right:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b3e11d4f3574bbb78b7b2c2cc39b432cbeef4e

Comment 11

[Honza Bambas (:mayhemer)]

(based on result of the try push)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9c8ba8d349
Create child redirect target (new) channel using parent target channel's original URI. r=jduell, r=bz

Comment 13

[Carsten Book [:Tomcat]]

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