Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Detect cross-origin visit request attempts #201

Closed
wants to merge 2 commits into from
Closed

Detect cross-origin visit request attempts #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3398de4
Initial work to detect cross-origin redirect visit requests
jayohms Mar 28, 2024
171ceab
Remove the current visitable from the backstack before opening the cr…
jayohms Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Source/Session/Session.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,13 @@ extension Session: WebViewDelegate {
delegate?.session(self, didProposeVisit: proposal)
}

func webView(_ bridge: WebViewBridge, didProposeVisitToCrossOriginRedirect location: URL) {
// Remove the current visitable from the backstack since it
// resulted in a visit failure due to a cross-origin redirect.
activatedVisitable?.visitableViewController.navigationController?.popViewController(animated: false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pfeiffer pfeiffer Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

openExternalURL(location)
}

func webView(_ webView: WebViewBridge, didStartFormSubmissionToLocation location: URL) {
delegate?.sessionDidStartFormSubmission(self)
}
Expand Down
1 change: 1 addition & 0 deletions Source/WebView/ScriptMessage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ extension ScriptMessage {
case pageLoadFailed
case errorRaised
case visitProposed
case visitProposedToCrossOriginRedirect
case visitProposalScrollingToAnchor
case visitProposalRefreshingPage
case visitStarted
Expand Down
3 changes: 3 additions & 0 deletions Source/WebView/WebViewBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import WebKit

protocol WebViewDelegate: AnyObject {
func webView(_ webView: WebViewBridge, didProposeVisitToLocation location: URL, options: VisitOptions)
func webView(_ webView: WebViewBridge, didProposeVisitToCrossOriginRedirect location: URL)
func webViewDidInvalidatePage(_ webView: WebViewBridge)
func webView(_ webView: WebViewBridge, didStartFormSubmissionToLocation location: URL)
func webView(_ webView: WebViewBridge, didFinishFormSubmissionToLocation location: URL)
Expand Down Expand Up @@ -129,6 +130,8 @@ extension WebViewBridge: ScriptMessageHandlerDelegate {
delegate?.webViewDidInvalidatePage(self)
case .visitProposed:
delegate?.webView(self, didProposeVisitToLocation: message.location!, options: message.options!)
case .visitProposedToCrossOriginRedirect:
delegate?.webView(self, didProposeVisitToCrossOriginRedirect: message.location!)
case .visitProposalScrollingToAnchor:
break
case .visitProposalRefreshingPage:
Expand Down
29 changes: 27 additions & 2 deletions Source/WebView/turbo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,18 @@
this.loadResponseForVisitWithIdentifier(visit.identifier)
}

visitRequestFailedWithStatusCode(visit, statusCode) {
this.postMessage("visitRequestFailed", { identifier: visit.identifier, statusCode: statusCode })
async visitRequestFailedWithStatusCode(visit, statusCode) {
// Turbo does not permit cross-origin fetch redirect attempts and
// they'll lead to a visit request failure. Attempt to see if the
// visit request failure was due to a cross-origin redirect.
const redirect = await this.fetchFailedRequestCrossOriginRedirect(visit, statusCode)
const location = visit.location.toString()

if (redirect != null) {
this.postMessage("visitProposedToCrossOriginRedirect", { location: redirect.toString(), identifier: visit.identifier })
} else {
this.postMessage("visitRequestFailed", { location: location, identifier: visit.identifier, statusCode: statusCode })
}
}

visitRequestFinished(visit) {
Expand Down Expand Up @@ -174,6 +184,21 @@
}

// Private

async fetchFailedRequestCrossOriginRedirect(visit, statusCode) {
// Non-HTTP status codes are sent by Turbo for network
// failures, including cross-origin fetch redirect attempts.
if (statusCode <= 0) {
try {
const response = await fetch(visit.location, { redirect: "follow" })
if (response.url != null && response.url.origin != visit.location.origin) {
return response.url
}
} catch {}
}

return null
}

postMessage(name, data = {}) {
data["timestamp"] = Date.now()
Expand Down