Perform redirect checks before OnReceivedRedirect in //net.
Right now, if URLRequest sees a redirect, it first signals
OnReceivedRedirect, gives the caller a chance to reject the redirect
and, if okay, it carries on to URLRequest::Redirect.
URLRequest::Redirect then performs additional checks and potentially
rejects the redirect. This ordering has two quirks:
First, new_url.is_valid() is checked very late, but most consumers'
checks presumably are going to check the URLs. This means that we reject
invalid redirect URLs based not on //net code, but whatever consumer
logic happens to notice.
In //content, this is ChildProcessSecurityPolicyImpl, which
interprets the renderer as trying to fetch somewhere questionable and
just cancels the request. This results in an aborted request and no
visible error, which is confusing, particularly for navigations.
Second, URLRequest::Delegate gets called in an odd order. If we don't
think request->url() should be allowed to redirect to new_url, whatever
error we surface should be associated with request->url(), not new_url.
That is, new_url did not misbehave necessarily. request->url() did for
daring to utter such an obviously invalid URL.
In particular, this is relevant for browser navigation logic, which
needs to associate the error page with the right URL. If the error page
is associate with new_url, reloading will skip the check, which is
problematic.
We do actually handle this properly. request->url() is not updated until
later, but since Chrome is a complex multi-threaded multi-process IPC
monster, the URLRequest is not immediate accessible. Instead, code likes
to update the working URL when it receives OnReceivedRedirect.
//net's current behavior is incompatible with this. In a normal
redirect situation, we might signal:
OnReceivedRedirect(new_url) // url() => old_url
OnResponseStarted(net_error) // url() => new_url.
But in this situation, we'll signal:
OnReceivedRedirect(new_url) // url() => old_url
OnResponseStarted(net_error) // url() => old_url!
This is weird. This CL fixes this so it looks like:
OnResponseStarted(net_error) // url() => old_url
This means redirects to unparseable URLs simply fail with
ERR_INVALID_REDIRECT (previously //net used ERR_INVALID_URL, but since
the error page will commit at the old URL, ERR_INVALID_REDIRECT seems
clearer). It also means that code which receives OnReceivedRedirect can
eagerly update the URL, provided, of course, it orders it correctly with
its own redirect checks. But the accept/reject signal may be propagated
on-path, whereas a two-phase check (first consumer, then //net) requires
potentially two round-trips.
Unfortunately, this change does as a result require checking in newly
failing test expectations for the redirect to data URL variant of issue
#707185. Before this CL, whether we produced an opaque-redirect filtered
response for an invalid redirect depended on whether the check happened
in //net or outside. This now makes them treated similarly. Later, we
can see about fixing both cases. (One thought is to teach //net about
manual redirect mode, but this is subtle because //net persists info
in the cache based on what it believes is and isn't a redirect. Another
is to check the response object in OnResponseStarted and, if it's
redirect-shaped, produce a filtered response wrapping an error response.
This probably will involve adding a boolean to
ResourceRequestCompletionStatus.)
BUG=462272,723796,707185
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2917133002
Cr-Commit-Position: refs/heads/master@{#477371}
16 files changed
