Re: Changing the Navigated URL in a NavigationThrottle

[Charlie Reis]

[-google.com addresses, +chromium.org addresses]

On Tue, Jul 31, 2018 at 1:15 PM Charlie Reis <cr...@google.com> wrote:

Camille, do you have a sense for whether there's anything technical preventing the NavigationThrottle code from simulating a redirect?  I think Nasko mentioned something about it being hard to simulate from outside the network stack, but otherwise I'm not opposed to the idea.

(This isn't the first feature that has wanted the ability to redirect, so it seems potentially desirable.  I think the closest equivalent is the Web Request API, though I've heard it's quite complex and isn't a good model for how to do a redirect.)

Charlie

On Tue, Jul 31, 2018 at 11:30 AM Robert Ogden <rober...@google.com> wrote:

Hi all,

I'd like to add some new functionality in a content::NavigationThrottle and am determining the best way to do it. This new functionality would allow an embedder to redirect the main frame request of a navigation to a specified new URL, optionally with new/additional/conditional request headers. 

One way to do this that does not require any //content changes, is to have a NavigationThrottle return content::NavigationThrottle::CANCEL and then PostTask a new navigation to WebContents::OpenURL.

Another proposed solution is to create a new ThrottleAction like CHANGE_URL or REDIRECT to do the same thing (this would require other state in ThrottleCheckResult). This would require tricky internal changes to //content but seems potentially useful down the road.

What are you thoughts?

Thanks

Robert

--

Robert Ogden

rober...@google.com

[Alex Moshchuk]

Looks like there's some precedent in doing a NavigationThrottle::CANCEL combined with OpenURL for at least NTP and PDF loading.  I think supporting a new ThrottleAction to do a redirect would be useful, but it'll likely involve thinking through some questions, for example which URLs the throttle is allowed to redirect to, since normally the network stack blocks things like redirects to data: URLs, but that might be safe/needed from a throttle (e.g., in the PDF case above).

On Wed, Aug 1, 2018 at 2:55 PM Robert Ogden <rober...@chromium.org> wrote:

Any thoughts on this? The core workings of our feature is blocked behind this discussion and I'd like to make it into M70. Hoping to have some decision by the end of the week, but we can talk on GVC if that helps.

I did some of the exploration Charlie mentioned to Camille in the last email. It looks like it is reasonably straightforward to offer a redirect action on WillStartRequest since the URLLoader in NavigationRequest can just be given the new URL, but in WillRedirectRequest, content can't push the new URL to the URLLoader in the network stack, so we'd need to reset the URLLoader with a new one anyways. I'm sure there are several gotchas that follow from that, like where to pick up on checking throttles and how to update state.

I'm very tempted by the cancel and OpenURL option as it doesn't require any content changes which could have many delicate edge cases.

Thanks

Robert

[Robert Ogden]

jam@ just let me know that URLLoaderThrottle now supports redirecting to a new URL and I'm going working with him to support cross-origin. This just landed in https://chromium-review.googlesource.com/c/chromium/src/+/1159821 so I'm going to let this thread become stale and pursue that instead.

Thanks all!

Robert

[Nasko Oskov]

Please be sure to not allow arbitrary redirects, as those can violate security properties of the browser. 

[Robert Ogden]

jam's idea won't work after all, so I'm back to this.

> Please be sure to not allow arbitrary redirects, as those can violate security properties of the browser.

No, we will only ever redirect to a Google-owned domain.

So far I haven't heard much so I'll assume that there aren't any really strong feelings on this. I agree it'd be nice to support a REDIRECT ThrottleCheckResult, but may be very involved to do, and my team is still gunning for M70. As Alex mentioned, there is already a precedent for using CANCEL and OpenURL. Although I'm happy to create a bug to look into supporting this again in the future.

If there aren't any strong objections, I'd like to move forward with the CANCEL and OpenURL option. I'll give this until morning on Monday August 6, Seattle time, to give Camille a chance to jump in.

Thanks

Robert

[Robert Ogden]

Hello all, again.

After landing some code for my feature, we've discovered that the above approach doesn't work well. Specifically, the cancel and open pattern resets the PLM harness, breaking the accuracy of the metrics that it collects. We also note that it would probably have a similar effect on UKM which uses navigation IDs, but haven't dug into that very far since the PLM breakage alone is a deal breaker.

I've written a proof of concept CL that adds a CHANGE_URL ThrottleAction and a small NavigationThrottle that uses it to demonstrate the effect. https://chromium-review.googlesource.com/c/chromium/src/+/1178895

I'd appreciate it if somebody could take an informal pass on this and let me know if the general approach and public API changes are acceptable. If so, I'll invest more time on this CL to make it merge-worthy.

Thanks!

Robert

[Robert Ogden]

Friendly ping :)

[Nasko Oskov]

I took a quick look over the CL. What bothers me a bit is that on the CHANGE_URL action we completely lose the state of the original URLLoader and create a new one. We can look at what state needs to be carried over, but an example would be redirects encountered by the initial loader before we reach CHANGE_URL. 

On Mon, Aug 20, 2018 at 12:24 PM Robert Ogden <rober...@chromium.org> wrote:

Friendly ping :)

On Thu, Aug 16, 2018 at 2:08 PM Robert Ogden <rober...@chromium.org> wrote:

Hello all, again.

After landing some code for my feature, we've discovered that the above approach doesn't work well. Specifically, the cancel and open pattern resets the PLM harness, breaking the accuracy of the metrics that it collects. We also note that it would probably have a similar effect on UKM which uses navigation IDs, but haven't dug into that very far since the PLM breakage alone is a deal breaker.

I've written a proof of concept CL that adds a CHANGE_URL ThrottleAction and a small NavigationThrottle that uses it to demonstrate the effect. https://chromium-review.googlesource.com/c/chromium/src/+/1178895

I'd appreciate it if somebody could take an informal pass on this and let me know if the general approach and public API changes are acceptable. If so, I'll invest more time on this CL to make it merge-worthy.

Thanks!

Robert

On Fri, Aug 3, 2018 at 11:34 AM Robert Ogden <rober...@chromium.org> wrote:

jam's idea won't work after all, so I'm back to this.

Can you give us a bit of context why it didn't work? Is it completely out of the picture to support this by adding redirect-like functionality to URLLoader that is caused externally?

 

> Please be sure to not allow arbitrary redirects, as those can violate security properties of the browser.

No, we will only ever redirect to a Google-owned domain.

This is only a promise for your own feature. If we add CHANGE_URL functionality in throttles, it has to support any feature misusing it by mistake.

[Robert Ogden]

Thanks! I've noted that state in the original URLLoader needs to be pushed to the new one.

Replied to other comments inline below.

On Tue, Aug 21, 2018 at 9:41 AM Nasko Oskov <na...@chromium.org> wrote:

I took a quick look over the CL. What bothers me a bit is that on the CHANGE_URL action we completely lose the state of the original URLLoader and create a new one. We can look at what state needs to be carried over, but an example would be redirects encountered by the initial loader before we reach CHANGE_URL. 

On Mon, Aug 20, 2018 at 12:24 PM Robert Ogden <rober...@chromium.org> wrote:

Friendly ping :)

On Thu, Aug 16, 2018 at 2:08 PM Robert Ogden <rober...@chromium.org> wrote:

Hello all, again.

After landing some code for my feature, we've discovered that the above approach doesn't work well. Specifically, the cancel and open pattern resets the PLM harness, breaking the accuracy of the metrics that it collects. We also note that it would probably have a similar effect on UKM which uses navigation IDs, but haven't dug into that very far since the PLM breakage alone is a deal breaker.

I've written a proof of concept CL that adds a CHANGE_URL ThrottleAction and a small NavigationThrottle that uses it to demonstrate the effect. https://chromium-review.googlesource.com/c/chromium/src/+/1178895

I'd appreciate it if somebody could take an informal pass on this and let me know if the general approach and public API changes are acceptable. If so, I'll invest more time on this CL to make it merge-worthy.

Thanks!

Robert

On Fri, Aug 3, 2018 at 11:34 AM Robert Ogden <rober...@chromium.org> wrote:

jam's idea won't work after all, so I'm back to this.

Can you give us a bit of context why it didn't work? Is it completely out of the picture to support this by adding redirect-like functionality to URLLoader that is caused externally?

 

jam@'s idea only works when the original url (not a redirect) is what is changed. So say I want to trigger on https://m.foo.com and the user types in foo.com. foo.com redirects to https://m.foo.com, but would not see the change. Further more, jam's feature only supports same-origin.  

 

> Please be sure to not allow arbitrary redirects, as those can violate security properties of the browser.

No, we will only ever redirect to a Google-owned domain.

This is only a promise for your own feature. If we add CHANGE_URL functionality in throttles, it has to support any feature misusing it by mistake.

 

How about allowing arbitrary redirects to domains that satisfy IsGoogleHostname or are already in the redirect chain (in case my feature needs to bypass after an attempted redirection)

[Nasko Oskov]

On Tue, Aug 21, 2018 at 12:54 PM Robert Ogden <rober...@chromium.org> wrote:

Thanks! I've noted that state in the original URLLoader needs to be pushed to the new one.

Replied to other comments inline below.

On Tue, Aug 21, 2018 at 9:41 AM Nasko Oskov <na...@chromium.org> wrote:

I took a quick look over the CL. What bothers me a bit is that on the CHANGE_URL action we completely lose the state of the original URLLoader and create a new one. We can look at what state needs to be carried over, but an example would be redirects encountered by the initial loader before we reach CHANGE_URL. 

On Mon, Aug 20, 2018 at 12:24 PM Robert Ogden <rober...@chromium.org> wrote:

Friendly ping :)

On Thu, Aug 16, 2018 at 2:08 PM Robert Ogden <rober...@chromium.org> wrote:

Hello all, again.

After landing some code for my feature, we've discovered that the above approach doesn't work well. Specifically, the cancel and open pattern resets the PLM harness, breaking the accuracy of the metrics that it collects. We also note that it would probably have a similar effect on UKM which uses navigation IDs, but haven't dug into that very far since the PLM breakage alone is a deal breaker.

I've written a proof of concept CL that adds a CHANGE_URL ThrottleAction and a small NavigationThrottle that uses it to demonstrate the effect. https://chromium-review.googlesource.com/c/chromium/src/+/1178895

I'd appreciate it if somebody could take an informal pass on this and let me know if the general approach and public API changes are acceptable. If so, I'll invest more time on this CL to make it merge-worthy.

Thanks!

Robert

On Fri, Aug 3, 2018 at 11:34 AM Robert Ogden <rober...@chromium.org> wrote:

jam's idea won't work after all, so I'm back to this.

Can you give us a bit of context why it didn't work? Is it completely out of the picture to support this by adding redirect-like functionality to URLLoader that is caused externally?

 

jam@'s idea only works when the original url (not a redirect) is what is changed. So say I want to trigger on https://m.foo.com and the user types in foo.com. foo.com redirects to https://m.foo.com, but would not see the change. Further more, jam's feature only supports same-origin.  

 

> Please be sure to not allow arbitrary redirects, as those can violate security properties of the browser.

No, we will only ever redirect to a Google-owned domain.

This is only a promise for your own feature. If we add CHANGE_URL functionality in throttles, it has to support any feature misusing it by mistake.

 

How about allowing arbitrary redirects to domains that satisfy IsGoogleHostname or are already in the redirect chain (in case my feature needs to bypass after an attempted redirection)

This very much decreases the usability and functionality of the CHANGE_URL code. I think we should be safe enough to restrict it to same-scheme redirects, which is what the current network stack (prior to Network Service) actually does. Each protocol handler (looked up by the scheme) can answer IsSafeRedirectTarget and handlers for most schemes return false in their overrides. 

[Chris Thompson]

Resurrecting this thread as my team is working on a feature that would benefit from being able to perform redirects from a NavigationThrottle.

A quick summary of what we're trying to do: We want to (optionally) upgrade main-frame navigations to HTTPS, and then trigger an interstitial if the server only supports HTTP. To do this with a URLLoaderThrottle runs into a few issues, as we'll likely need to coordinate state between the NavigationThrottle that handles the interstitial and the URLLoaderThrottle that can perform the actual upgrades, as well as coordinating an allowlist of sites that shouldn't trigger the upgrades or interstitial. If we could instead keep all of this logic in a single NavigationThrottle, I think that that could simplify our implementation significantly.

As far as constraints on the redirects go, our redirects would technically be non-same-scheme, but we would be same-ProtocolHandler at least. It looks like currently IsSafeRedirectTarget() isn't handled in ProtocolHandlers anymore (HttpProtocolHandler is the only remaining one and it just uses the default "return true" implementation), and the only ones I could find are ChromeContentRendererClient::IsSafeRedirectTarget() (handling extensions) and in content/'s url_utils.cc (handling other cases). It seems like a NavigationThrottle CHANGE_URL/REDIRECT result could check both directly.

Would people still be amenable to us pursuing this direction? Are there things that have significantly changed here since 2018 that we should be aware of?

Thanks!

- Chris

[Chris Thompson]

+navigation-dev as this is navigation specific.

I briefly checked in with Nasko about this, and it seems like adding redirect support to NavigationThrottle will be the cleaner way to implement what we want, so I'd like to work through what requirements we need to handle in the CHANGE_URL action.

Another interesting complication that diverges from the original use case in this thread is that we want to fallback when the upgrade fails (so "restarting" the pre-redirect URL). One idea I had was allowing a CHANGE_URL action from NavigationThrottle::WillFailRequest() (so that the navigation and loader can keep all of their params and state vs. restarting the navigation from scratch), but from my understanding of the navigation state logic this would require changing NavigationRequest::CheckStateTransition() to allow transitioning from WILL_FAIL_REQUEST to WILL_REDIRECT_REQUEST, which also seems like a significant change. Opinions very welcome on the feasibility of that change, or ideas for alternate ways to redirect back on failure :-)

I'm starting to work on drafting an updated version of Robert's proof-of-concept CL and I'll link that here once it's uploaded.

Thanks!

- Chris

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/69ba2531-dac3-4688-91b1-bd79bd772cd7n%40chromium.org.

[Chris Thompson]

Thanks for the suggestion and code pointers Ryan -- that seems like another plausible alternative for handling the redirect part of our feature.

For interstitials, they are created by returning an error page as part of a NavigationThrottle ThrottleCheckResult so being able to do both the redirect and the interstitial handling in the same NavigationThrottle would make our implementation logic simpler.

On Tue, Jun 8, 2021 at 10:56 AM Ryan Sturm <ryan...@chromium.org> wrote:

On Tue, Jun 8, 2021 at 10:53 AM Ryan Sturm <ryan...@google.com> wrote:

Just adding some context around the direction LitePages went with (now removed due to feature being removed).

LitePages landed on using a navigation URLLoader interceptor to insert redirects. This lived between content navigation and the network stack, essentially. The model was:

1. Determine if we should try to fetch an alternative URL
2. On success, serve redirect to alternative URL, on failure, fallback to regular network behavior
3. Second interceptor serves the stored response from (1/2) to the redirect URL

https://chromium-review.googlesource.com/c/chromium/src/+/2161311/20/chrome/browser/previews/previews_lite_page_redirect_url_loader.cc

SearchPrefetch also uses a similar model without serving the redirect:

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/prefetch/search_prefetch/search_prefetch_url_loader_interceptor.cc;drc=d27ca1ceb51060e616fb202ead6e6d1b4e02f865;bpv=1;bpt=1;l=71

I don't have a lot of context about how interstitials are served, but it seems like you could use this model to serve the content and separately determine when the interstitial needs to be served at a higher layer.

Accidentally sent a reply from @google.com address, re-sending with @chromium (see above).

[Chris Thompson]

Following up specifically on the question about changing the state machine checks for NavigationRequest::CheckStateTransition(), as it impacts our ability to use the new action if we add support for it: Are there fundamental issues with allowing a state transition from WILL_FAIL_REQUEST --> WILL_REDIRECT_REQUEST? If that's a no-go, are there preferred ways in navigation code to "restart" a navigation without losing important state? (For URLLoader/Throttle code, my understanding is restarting the ThrottlingURLLoader via RestartWithURLResetAndFlags() should work, but there isn't a similar affordance on NavigationURLLoader.)