PDF: Check the loaded URL in sendScriptingMessage_().

PDF: Check the loaded URL in sendScriptingMessage_().

BUG=653749
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[Lei Zhang, 2016-10-09 00:52:40]

Description was changed from

==========
PDF: Check the loaded URL in sendScriptingMessage_().
==========

to

==========
PDF: Check the loaded URL in sendScriptingMessage_().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Lei Zhang, 2016-10-09 00:52:55]

Description was changed from

==========
PDF: Check the loaded URL in sendScriptingMessage_().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
PDF: Check the loaded URL in sendScriptingMessage_().

BUG=653749
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Lei Zhang, 2016-10-09 00:53:09]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 00:53:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-09 01:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-09 01:46:47]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-10-09 10:49:50]

rob@robwu.nl changed reviewers:
+ rob@robwu.nl

[robwu, 2016-10-09 10:49:51]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:379: actual_url_ = response_url_str;
Testing for URL equality may be too strict. Checking origins ought to be
sufficient, so how about renaming |actual_url_| to |actual_origin_| and using
the following instead:

  actual_origin_ = url::Origin(GURL(response_url_str)).Serialize();

[Lei Zhang, 2016-10-11 00:09:01]

thestig@chromium.org changed reviewers:
+ raymes@chromium.org

[Lei Zhang, 2016-10-11 00:09:02]

+raymes for review
+random net/ folks for thoughts on this redirection business.

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:382: if (actual_url_ != response_url_str)
I was wondering about this as well. Would a web server ever redirect to
different URLs / origins for different chunks of a file? For load balancing
perhaps?

[Lei Zhang, 2016-10-11 00:11:09]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Lei Zhang, 2016-10-11 00:11:09]

+tsepez for review as well.

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:379: actual_url_ = response_url_str;
On 2016/10/09 10:49:50, robwu wrote:
> Testing for URL equality may be too strict. Checking origins ought to be
> sufficient, so how about renaming |actual_url_| to |actual_origin_| and using
> the following instead:
> 
>   actual_origin_ = url::Origin(GURL(response_url_str)).Serialize();

Ack. We can certainly do that if desired.

[raymes, 2016-10-11 04:26:23]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:338: request.SetFollowRedirects(true);
thestig: Can we just set this to false? I think this should solve all our
cross-origin problems. The MimeHandler should handle all of the redirection
before the URL gets to the plugin.

[Lei Zhang, 2016-10-11 04:45:59]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:338: request.SetFollowRedirects(true);
On 2016/10/11 04:26:22, raymes wrote:
> thestig: Can we just set this to false? I think this should solve all our
> cross-origin problems. The MimeHandler should handle all of the redirection
> before the URL gets to the plugin. 

Well, I imagine before we had MimeHandler, we needed this for the PDF Viewer to
follow redirects. Now that there's another layer on top... maybe not? Do there
exist web servers that would behave as Rob's?

[raymes, 2016-10-11 05:24:28]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:338: request.SetFollowRedirects(true);
On 2016/10/11 04:45:59, Lei Zhang wrote:
> On 2016/10/11 04:26:22, raymes wrote:
> > thestig: Can we just set this to false? I think this should solve all our
> > cross-origin problems. The MimeHandler should handle all of the redirection
> > before the URL gets to the plugin. 
> 
> Well, I imagine before we had MimeHandler, we needed this for the PDF Viewer
to
> follow redirects. Now that there's another layer on top... maybe not? Do there
> exist web servers that would behave as Rob's?

You mean where the first request doesn't redirect but subsequent range requests
do? I'm not sure but it seems kinda strange. It sort of implies the resource
underneath is changing after the first request.

[raymes, 2016-10-11 06:22:37]

From a security standpoint I think it's good to not to have to reason about
redirects inside the plugin. The PDF should just load the URL that is
passed to it.

On Tue, 11 Oct 2016 at 16:24 <raymes@chromium.org> wrote:

>
> https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
> File pdf/document_loader.cc (right):
>
>
>
https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
> pdf/document_loader.cc:338: request.SetFollowRedirects(true);
> On 2016/10/11 04:45:59, Lei Zhang wrote:
> > On 2016/10/11 04:26:22, raymes wrote:
> > > thestig: Can we just set this to false? I think this should solve
> all our
> > > cross-origin problems. The MimeHandler should handle all of the
> redirection
> > > before the URL gets to the plugin.
> >
> > Well, I imagine before we had MimeHandler, we needed this for the PDF
> Viewer to
> > follow redirects. Now that there's another layer on top... maybe not?
> Do there
> > exist web servers that would behave as Rob's?
>
> You mean where the first request doesn't redirect but subsequent range
> requests do? I'm not sure but it seems kinda strange. It sort of implies
> the resource underneath is changing after the first request.
>
> https://codereview.chromium.org/2407683002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2016-10-11 16:10:47]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:382: if (actual_url_ != response_url_str)
Seems unlikely, but we'll find out.  Want to add a UMA stat?

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.h
File pdf/document_loader.h (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.h#newco...
pdf/document_loader.h:108: std::string actual_url_;
nit: maybe a comment that explains how this differs from the above.

[mmenke, 2016-10-11 17:30:53]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-10-11 17:30:54]

[+jkarlin, +gavinp]:  We fall over in some creative and exciting way if only a
subset of range requests are redirected, right?

[Lei Zhang, 2016-10-11 19:07:31]

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:338: request.SetFollowRedirects(true);
On 2016/10/11 05:24:28, raymes wrote:
> On 2016/10/11 04:45:59, Lei Zhang wrote:
> > On 2016/10/11 04:26:22, raymes wrote:
> > > thestig: Can we just set this to false? I think this should solve all our
> > > cross-origin problems. The MimeHandler should handle all of the
redirection
> > > before the URL gets to the plugin. 
> > 
> > Well, I imagine before we had MimeHandler, we needed this for the PDF Viewer
> to
> > follow redirects. Now that there's another layer on top... maybe not? Do
there
> > exist web servers that would behave as Rob's?
> 
> You mean where the first request doesn't redirect but subsequent range
requests
> do? I'm not sure but it seems kinda strange. It sort of implies the resource
> underneath is changing after the first request.

Yes, I agree it is probably a bit crazy, but I'd like some input from networking
folks regarding this to help reassure me that this is crazy and we should just
flip the bit to false.

BTW, this along with ppapi/tests/test_url_loader.cc are the only callers of
SetFollowRedirects(). Though ppapi::URLRequestInfoData has its follow_redirects
member initiailized to true.

[nasko, 2016-10-12 00:47:46]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-10-12 00:47:47]

A small drive-by.

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/2407683002/diff/1/pdf/document_loader.cc#newc...
pdf/document_loader.cc:382: if (actual_url_ != response_url_str)
On 2016/10/11 16:10:47, Tom Sepez wrote:
> Seems unlikely, but we'll find out.  Want to add a UMA stat?

Why not DumpWithoutCrashing? If it is never hit, it has no overhead. But if it
hits, I imagine you would want to know what happened.

[Lei Zhang, 2016-10-13 00:37:12]

On 2016/10/11 17:30:54, mmenke wrote:
> [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if only a
> subset of range requests are redirected, right?

Still seeking comments from networking folks.

The alternative of not following redirects in the plugin is
https://codereview.chromium.org/2409423004

[raymes, 2016-10-13 00:44:44]

On 2016/10/13 00:37:12, Lei Zhang wrote:
> On 2016/10/11 17:30:54, mmenke wrote:
> > [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if only
a
> > subset of range requests are redirected, right?
> 
> Still seeking comments from networking folks.
> 
> The alternative of not following redirects in the plugin is
> https://codereview.chromium.org/2409423004

I pinged jkarlin and he said gavinp was the best person to ask.

[Lei Zhang, 2016-10-13 00:58:17]

On 2016/10/13 00:44:44, raymes wrote:
> I pinged jkarlin and he said gavinp was the best person to ask.

Or we can take the commit it and see what happens approach? :)

[raymes, 2016-10-13 01:24:46]

On 2016/10/13 00:58:17, Lei Zhang wrote:
> On 2016/10/13 00:44:44, raymes wrote:
> > I pinged jkarlin and he said gavinp was the best person to ask.
> 
> Or we can take the commit it and see what happens approach? :)

mmenke didn't seem strongly opposed to the idea so I feel ok with it :)

[gavinp, 2016-10-13 20:07:57]

On 2016/10/11 17:30:54, mmenke wrote:
> [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if only a
> subset of range requests are redirected, right?

If a response to a range request isn't a 206, then it looks like we trash the
whole entry (treat it as a whole new update). Then, the normal handling for
creating a new cache request follows.

So I think we're good here, yes.

[Lei Zhang, 2016-10-13 22:03:11]

On 2016/10/13 20:07:57, gavinp wrote:
> On 2016/10/11 17:30:54, mmenke wrote:
> > [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if only
a
> > subset of range requests are redirected, right?
> 
> If a response to a range request isn't a 206, then it looks like we trash the
> whole entry (treat it as a whole new update). Then, the normal handling for
> creating a new cache request follows.
> 
> So I think we're good here, yes.

To be explicit, you are saying responding with 302 for a range request is ok,
and we should not go with https://codereview.chromium.org/240942300 ?

If that's the case, can you also comment on the validity of a server responding
with 302s but redirecting to different URLs?

[raymes, 2016-10-18 00:58:38]

On 2016/10/13 22:03:11, Lei Zhang wrote:
> On 2016/10/13 20:07:57, gavinp wrote:
> > On 2016/10/11 17:30:54, mmenke wrote:
> > > [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if
only
> a
> > > subset of range requests are redirected, right?
> > 
> > If a response to a range request isn't a 206, then it looks like we trash
the
> > whole entry (treat it as a whole new update). Then, the normal handling for
> > creating a new cache request follows.
> > 
> > So I think we're good here, yes.
> 
> To be explicit, you are saying responding with 302 for a range request is ok,
> and we should not go with https://codereview.chromium.org/240942300 ?
> 
> If that's the case, can you also comment on the validity of a server
responding
> with 302s but redirecting to different URLs?

ping: gavinp?

I think we should land https://codereview.chromium.org/2409423004/ and see how
we go.

[gavinp, 2016-10-25 18:05:07]

On 2016/10/18 00:58:38, raymes wrote:
> On 2016/10/13 22:03:11, Lei Zhang wrote:
> > On 2016/10/13 20:07:57, gavinp wrote:
> > > On 2016/10/11 17:30:54, mmenke wrote:
> > > > [+jkarlin, +gavinp]:  We fall over in some creative and exciting way if
> only
> > a
> > > > subset of range requests are redirected, right?
> > > 
> > > If a response to a range request isn't a 206, then it looks like we trash
> the
> > > whole entry (treat it as a whole new update). Then, the normal handling
for
> > > creating a new cache request follows.
> > > 
> > > So I think we're good here, yes.
> > 
> > To be explicit, you are saying responding with 302 for a range request is
ok,
> > and we should not go with https://codereview.chromium.org/240942300 ?
> > 
> > If that's the case, can you also comment on the validity of a server
> responding
> > with 302s but redirecting to different URLs?
> 
> ping: gavinp?
> 
> I think we should land https://codereview.chromium.org/2409423004/ and see how
> we go.

Yes. Go ahead.

[Lei Zhang, 2016-10-25 18:07:05]

On 2016/10/25 18:05:07, gavinp wrote:
> Yes. Go ahead.

Thanks. Will close this CL then.

[Lei Zhang, 2016-10-25 18:07:58]

Description was changed from

==========
PDF: Check the loaded URL in sendScriptingMessage_().

BUG=653749
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
PDF: Check the loaded URL in sendScriptingMessage_().

BUG=653749
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========