Detect cross-origin visit request attempts

[jayohms]

This is the corresponding PR to the same turbo-android issue outlined in hotwired/turbo-android#325

The core turbo.js library does not permit cross-origin visit requests (i.e. a link that redirects to an external domain). These requests call the adapter method visitRequestFailedWithStatusCode(visit, statusCode) for all platforms.

To handle this in the browser adapter, non-HTTP status code failures update the window.location so the browser can handle the top-level redirect. The browser adapter does not actually know whether a cross-origin redirect was attempted, since the CORS policy restricts requests to the same-origin, but updating the window.location directly bypasses needing this knowledge.

The mobile adapters, however, can't just update the window.location — a new visit needs to be proposed so the native app can decide how to handle the external url request. This PR finds any potential cross-origin redirect locations when a visit request fails with a non-HTTP status code and proposes the external redirect location as a new visit.

Fixes #200

Test with the demo branch: hotwired/turbo-native-demo#34

[jayohms]

Is this the best way to handle popping the current visitable ViewController from the backstack?

[pfeiffer]

Could this be calling a delegate instead? This would allow customization of the behavior here and the Session would be less dependent on the app being structured in a specific way, ie. the Session doesn't really 'push' or 'pop' any controllers today - controllers just need to implement Visitable.

[jayohms]

Yeah, I figured that'd probably be where we ended up, makes sense 👍

[pfeiffer]

An alternative would be to implement a method in Visitable that provides the behavior as in the PR, but could be overwritten and customized in the controller that implements Visitable, ie for. controllers not living in a UINavigationController, eg.:

// Session
func webView(_ bridge: WebViewBridge, didProposeVisitToCrossOriginRedirect location: URL) {
   activatedVisitable?.removeDueToCrossOriginRedirect()
}

// Visitable
func removeDueToCrossOriginRedirect() {
  // Sensible default, but could be customized in controller implementing Visitable
  visitableViewController.navigationController?.popViewController(animated: false)
}

[joemasilotti]

Would it be better to perform the visit with a REPLACE action instead of manually manipulating the backstack?

[jayohms]

I don't think a replace visit fits the model here very well. The redirect url is for an external domain, so there won't be an in-app visit that takes place. We need to drop the current visitable from the backstack before opening the redirect url in an external app/browser, since it'll be displaying the progress indicator when the cross-origin redirect is seen.

[joemasilotti]

Ah, gotcha. Then I think we should find a way to delegate to the navigator to perform the stack manipulation.

[joemasilotti]

After digging into the PR, I'm leaning more towards @pfeiffer's recommendation of an additional delegate callback on SessionDelegate. This would require the developer to do the popping manually but we can handle that for them in Turbo Navigator. I like this approach because if the developer isn't using a navigation controller then they have full control of what to do.

But more importantly, if the redirected screen was opened in a modal then we need to dismiss not pop the new (blank) view controller from the screen. This will be a one-liner in Turbo Navigator because we already have logic that handles those situations.

[svara]

Very good points @joemasilotti! I agree and I like the suggestion of an additional delegate callback on SessionDelegate.

[jayohms]

@joemasilotti @svara @olivaresf Can you give this a review and let me know if the way to pop the backstack for a visitable that leads to a cross-origin redirect is the best impementation? (Updated demo web branch linked in the PR description.)

[joemasilotti]

After digging into the PR, I'm leaning more towards @pfeiffer's recommendation of an additional delegate callback on SessionDelegate. This would require the developer to do the popping manually but we can handle that for them in Turbo Navigator. I like this approach because if the developer isn't using a navigation controller then they have full control of what to do.

But more importantly, if the redirected screen was opened in a modal then we need to dismiss not pop the new (blank) view controller from the screen. This will be a one-liner in Turbo Navigator because we already have logic that handles those situations.

[pfeiffer]

Added a PR in #203 implementing the session delegate for handling cross-origin redirects.

[joemasilotti]

Closed in favor of #203.