Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917133002:
Perform redirect checks before OnReceivedRedirect in //net. (Closed)
DescriptionPerform 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}
Committed: https://chromium.googlesource.com/chromium/src/+/d894710be3b56a15871376c5979615053dddcd70
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : add an extra assertion #Patch Set 4 : fix tests #Patch Set 5 : fix test #Patch Set 6 : test expectations #Patch Set 7 : rebase #
Total comments: 5
Patch Set 8 : mmenke comments #
Total comments: 4
Patch Set 9 : nasko comments #Messages
Total messages: 52 (37 generated)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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(ERR_UNSAFE_REDIRECT) // 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, provide, of course, it does not reject the redirect itself. But the accept/reject signal may be propagated on-path, whereas a two-phase check (first consumer, then //net) requires potentially two round-trips. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the request headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the request headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 ==========
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the request headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 ==========
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. BUG=462272,723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid "opaque-redirect filtered response"s did work did so largely on accident. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid "opaque-redirect filtered response"s did work did so largely on accident. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. FIXME: Actually, (1) does not work because the net stack does not read or cache redirect bodies. I suspect (4) is the only truly feasible option. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. FIXME: Actually, (1) does not work because the net stack does not read or cache redirect bodies. I suspect (4) is the only truly feasible option. BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. FIXME: Actually, (1) does not work because the net stack does not read or cache redirect bodies. I suspect (4) is the only truly feasible option. FIXME: Alternatively, what might work is ResourceLoader passes down the error, but combined with the HTTP status code or perhaps just a boolean for whether it was during a redirect. We plumb that all the way to ResourceError and, in DidFail, create an opaque filtered response wrapping the error. This is nasty, but it should work! BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. One note: this change seemingly conflicts with issue #707185, where the spec requires that, in some modes, the Fetch API return an "opaque-redirect filtered response" if the status code is remotely redirect-smelling, deferring all actual processing of the redirect to later, if ever. We don't have this behavior today, and it seems mildly questionable. Nonetheless, there are a few possible answers: 1. URLRequest takes a boolean to simply skip redirect processing altogether. We will return a "redirect" response as if it were the final response, and that's the end of it. The navigation code may end up a little funny as, unlike the spec, we do expect the normal request-processing logic to process requests. But it should be doable. (We'd want, from the perspective of Service Workers, navigation to make manual redirect fetches but, from the perspective of URLRequest, the redirect processing should not be skipped.) 2. I believe, in OnResponseStarted, even in the error case, the response headers are still available. So the Fetch manual redirect logic may first check if it is redirect-smelling and, if yes, create a opaque-redirect filtered response wrapper just the error. This seems fine as such response objects are opaque and don't let you know if there was secretly an error or not. 3. Don't worry about this. This is only the case when one tries to redirect to an invalid URL or a data URL or so. 4. The spec prescribes unreasonably fragile (security-wise) behavior and should be changed. Unfortunately, this change does as a result require checking in newly failing test expectations for the redirect to data URL variant of the above case. We do now have consistent handling, which is valuable, but it tentatively went in the direction away from what the spec says. One of the above options will need to be explored but, in the meantime, the few cases where invalid opaque-redirect filtered responses worked did so largely on accident. FIXME: Actually, (1) does not work because the net stack does not read or cache redirect bodies. I suspect (4) is the only truly feasible option. FIXME: Alternatively, what might work is ResourceLoader passes down the error, but combined with the HTTP status code or perhaps just a boolean for whether it was during a redirect. We plumb that all the way to ResourceError and, in DidFail, create an opaque filtered response wrapping the error. This is nasty, but it should work! BUG=462272,723796,707185 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 ==========
davidben@chromium.org changed reviewers: + horo@chromium.org, mmenke@chromium.org, nasko@chromium.org
mmenke: For whether this is sound from a //net perspective. Directories to sign off on: chrome/browser/net components/error_page net nasko: For //content and whether this works for the navigation side of things. Directories to sign off on: content horo: For Fetch since I am sadly regressing the invalid redirect corner case. :-( Directories to sign off on: third_party/WebKit
On 2017/06/05 19:08:21, davidben wrote: > horo: For Fetch since I am sadly regressing the invalid redirect corner case. > :-( > Directories to sign off on: > third_party/WebKit Oh, also this file, since I'm tweaking code under one of your TODOs: content/browser/service_worker/service_worker_browsertest.cc
https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:468: URLRequestStatus::FromError(redirect_valid)); Why not use OnDone()? https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:744: if (!IsSafeRedirect(new_url)) { Doesn't RDH/CRDHD currently allow redirects to schemes the network stack doesn't handle? (Like email:foo, or whatever weird third party schemes are registered locally?) I think this check breaks that.
https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:468: URLRequestStatus::FromError(redirect_valid)); On 2017/06/05 19:25:15, mmenke wrote: > Why not use OnDone()? Done. https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:744: if (!IsSafeRedirect(new_url)) { On 2017/06/05 19:25:15, mmenke wrote: > Doesn't RDH/CRDHD currently allow redirects to schemes the network stack doesn't > handle? (Like email:foo, or whatever weird third party schemes are registered > locally?) I think this check breaks that. Yup, this is exactly why the spec is split up in this bizarre way. :-) Is SafeRedirect lets through anything it doesn't understand, probably to keep this exact case working. https://cs.chromium.org/chromium/src/net/url_request/url_request_job_factory_... I suppose that too is sort of weird. The nuisance is if we decide to order the other way, we actually need two signals out of URLRequestJob (so //content knows it's okay to update the URL), which means two process/IPC hops.
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Want to think about this a bit more tomorrow, but it seems fine. https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/2917133002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:744: if (!IsSafeRedirect(new_url)) { On 2017/06/05 19:44:17, davidben wrote: > On 2017/06/05 19:25:15, mmenke wrote: > > Doesn't RDH/CRDHD currently allow redirects to schemes the network stack > doesn't > > handle? (Like email:foo, or whatever weird third party schemes are registered > > locally?) I think this check breaks that. > > Yup, this is exactly why the spec is split up in this bizarre way. :-) > > Is SafeRedirect lets through anything it doesn't understand, probably to keep > this exact case working. > https://cs.chromium.org/chromium/src/net/url_request/url_request_job_factory_... > > I suppose that too is sort of weird. The nuisance is if we decide to order the > other way, we actually need two signals out of URLRequestJob (so //content knows > it's okay to update the URL), which means two process/IPC hops. Ah, you're right. I had thought the reason we informed the Delegate before the check was because this check failed, but I was wrong.
On 2017/06/05 19:52:01, mmenke wrote: > Want to think about this a bit more tomorrow, but it seems fine. Feel free to grab me at some point if you want to chat. This thing is... kind of a mess. :-(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content/ LGTM. Thanks for getting this done! https://codereview.chromium.org/2917133002/diff/140001/content/browser/browse... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/2917133002/diff/140001/content/browser/browse... content/browser/browser_side_navigation_browsertest.cc:425: // navigation logic is no longer needed. Can you add "See https://crbug.com/723796." so these can be easily tracked? https://codereview.chromium.org/2917133002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2917133002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6870: EXPECT_EQ(redirect_to_blank_url, Let's rename this, redirect_to_unsafe_redirect is probably more descriptive and reflects what it really does.
+CC: yhirano@ lgtm
On 2017/06/05 19:54:21, davidben wrote: > On 2017/06/05 19:52:01, mmenke wrote: > > Want to think about this a bit more tomorrow, but it seems fine. > > Feel free to grab me at some point if you want to chat. This thing is... kind of > a mess. :-( Did we decide to switch to the two callback approach or not? I can't remember. :)
On 2017/06/06 15:59:18, mmenke wrote: > On 2017/06/05 19:54:21, davidben wrote: > > On 2017/06/05 19:52:01, mmenke wrote: > > > Want to think about this a bit more tomorrow, but it seems fine. > > > > Feel free to grab me at some point if you want to chat. This thing is... kind > of > > a mess. :-( > > Did we decide to switch to the two callback approach or not? I can't remember. > :) I think we decided this version was simpler. I dunno. Did you prefer the other one? Two callbacks means we need OnRequestRedirect + OnBeforeRedirect and for, ultimately, even the fetch and network services to expose two of them.
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
https://codereview.chromium.org/2917133002/diff/140001/content/browser/browse... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/2917133002/diff/140001/content/browser/browse... content/browser/browser_side_navigation_browsertest.cc:425: // navigation logic is no longer needed. On 2017/06/06 00:27:38, nasko (slow) wrote: > Can you add "See https://crbug.com/723796.%22 so these can be easily tracked? Done. https://codereview.chromium.org/2917133002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2917133002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6870: EXPECT_EQ(redirect_to_blank_url, On 2017/06/06 00:27:38, nasko (slow) wrote: > Let's rename this, redirect_to_unsafe_redirect is probably more descriptive and > reflects what it really does. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm good, either way. LGTM!
On 2017/06/06 17:34:35, mmenke_google.com wrote: > I'm good, either way. LGTM! LGTM from the right account, this time. Multilogin. :(
The CQ bit was unchecked by davidben@chromium.org
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, horo@chromium.org Link to the patchset: https://codereview.chromium.org/2917133002/#ps160001 (title: "nasko comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496771463763000,
"parent_rev": "500f75b2f92268e040cd4cd3bb69f827d95ba6ea", "commit_rev":
"4f8e54501b2f2a9e7a25e353688441302acc896a"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496771463763000,
"parent_rev": "bc44f1e38fe9de4a00be9891afbdedbed610cd9f", "commit_rev":
"d894710be3b56a15871376c5979615053dddcd70"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/d894710be3b56a15871376c59796... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/d894710be3b56a15871376c59796... |
