Fix #36092: Fix Javascript URL navigation on iOS (#22244)

- Block Javascript URL navigation on iOS
brave · Mar 5, 2024 · 3aaa5cb · 3aaa5cb
1 parent 6b835d9
commit 3aaa5cb
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 41 deletions.
diff --git a/...nd/Browser/Toolbars/BottomToolbar/Menu/Bookmarks/AddEditBookmarkTableViewController.swift b/...nd/Browser/Toolbars/BottomToolbar/Menu/Bookmarks/AddEditBookmarkTableViewController.swift
@@ -280,7 +280,7 @@ class AddEditBookmarkTableViewController: UITableViewController {
     switch mode {
     case .addBookmark(_, _):
       guard let urlString = bookmarkDetailsView.urlTextField?.text,
-        let url = URL(string: urlString) ?? urlString.bookmarkletURL
+        let url = URL(string: urlString) ?? URL.bookmarkletURL(from: urlString)
       else {
         return earlyReturn()
       }
@@ -317,7 +317,7 @@ class AddEditBookmarkTableViewController: UITableViewController {
       }
     case .editBookmark(let bookmark):
       guard let urlString = bookmarkDetailsView.urlTextField?.text,
-        let url = URL(string: urlString) ?? urlString.bookmarkletURL
+        let url = URL(string: urlString) ?? URL.bookmarkletURL(from: urlString)
       else {
         return earlyReturn()
       }
@@ -358,7 +358,7 @@ class AddEditBookmarkTableViewController: UITableViewController {
 
     case .editFavorite(let favorite):
       guard let urlString = bookmarkDetailsView.urlTextField?.text,
-        let url = URL(string: urlString) ?? urlString.bookmarkletURL
+        let url = URL(string: urlString) ?? URL.bookmarkletURL(from: urlString)
       else {
         return earlyReturn()
       }

diff --git a/...es/Brave/Frontend/Browser/Toolbars/BottomToolbar/Menu/Bookmarks/BookmarkDetailsView.swift b/...es/Brave/Frontend/Browser/Toolbars/BottomToolbar/Menu/Bookmarks/BookmarkDetailsView.swift
@@ -73,8 +73,8 @@ class BookmarkDetailsView: AddEditHeaderView, BookmarkFormFieldsProtocol {
       .forEach(contentStackView.addArrangedSubview)
 
     var url = url
-    if url?.isBookmarklet == true {
-      url = url?.removingPercentEncoding
+    if let urlString = url, URL.bookmarkletURL(from: urlString) != nil {
+      url = urlString.removingPercentEncoding
     } else if let url = url, let favUrl = URL(string: url) {
       faviconImageView.loadFavicon(siteURL: favUrl, isPrivateBrowsing: isPrivateBrowsing)
     }
@@ -94,7 +94,7 @@ class BookmarkDetailsView: AddEditHeaderView, BookmarkFormFieldsProtocol {
   // MARK: - Delegate actions
 
   @objc func textFieldDidChange(_ textField: UITextField) {
-    if textField.text?.isBookmarklet == true {
+    if let text = textField.text, URL.bookmarkletURL(from: text) != nil {
       delegate?.correctValues(validationPassed: validateCodeFields())
     } else {
       delegate?.correctValues(validationPassed: validateFields())

diff --git a/ios/brave-ios/Sources/Shared/BookmarkValidation.swift b/ios/brave-ios/Sources/Shared/BookmarkValidation.swift
@@ -18,8 +18,8 @@ public struct BookmarkValidation {
 
   public static func validateBookmarklet(title: String?, url: String?) -> Bool {
     guard let url = url else { return validateTitle(title) }
-    if !url.isBookmarklet { return false }
-    guard let javascriptCode = url.bookmarkletCodeComponent else {
+    guard let bookmarklet = URL.bookmarkletURL(from: url) else { return false }
+    guard let javascriptCode = bookmarklet.bookmarkletCodeComponent else {
       return false
     }
 

diff --git a/ios/brave-ios/Sources/Shared/Extensions/URLExtensions.swift b/ios/brave-ios/Sources/Shared/Extensions/URLExtensions.swift
@@ -412,38 +412,39 @@ extension URL {
 
 extension URL {
   public var isBookmarklet: Bool {
-    return self.absoluteString.isBookmarklet
-  }
-
-  public var bookmarkletCodeComponent: String? {
-    return self.absoluteString.bookmarkletCodeComponent
-  }
-}
-
-extension String {
-  public var isBookmarklet: Bool {
-    let url = self.lowercased()
-    return url.hasPrefix("javascript:") && !url.hasPrefix("javascript:/")
+    if let scheme = self.scheme {
+      return scheme == "javascript"
+    }
+    return self.absoluteString.hasPrefix("javascript:")
   }
 
   public var bookmarkletCodeComponent: String? {
     if self.isBookmarklet {
-      if let result = String(self.dropFirst("javascript:".count)).removingPercentEncoding {
-        return result.isEmpty ? nil : result
+      // Chrome, Safari, Desktop all treat anything after `javascript:` including `//` as the component!
+      // So `javascript:/` has a component of `/`
+      // `javascript://test` has a component of `//test`
+      if self.absoluteString.hasPrefix("javascript:") {
+        if let result = String(self.absoluteString.dropFirst("javascript:".count))
+          .removingPercentEncoding
+        {
+          return result.isEmpty ? nil : result
+        }
       }
     }
     return nil
   }
 
-  public var bookmarkletURL: URL? {
-    if self.isBookmarklet,
-      let escaped = self.addingPercentEncoding(withAllowedCharacters: .urlAllowed)
+  public static func bookmarkletURL(from string: String) -> URL? {
+    if string.hasPrefix("javascript:"),
+      let escaped = string.addingPercentEncoding(withAllowedCharacters: .urlAllowed)
     {
       return URL(string: escaped)
     }
     return nil
   }
+}
 
+extension String {
   public func removeSchemeFromURLString(_ scheme: String?) -> String {
     guard let scheme = scheme else {
       return self

diff --git a/ios/brave-ios/Tests/BraveSharedTests/NSURLExtensionsTests.swift b/ios/brave-ios/Tests/BraveSharedTests/NSURLExtensionsTests.swift
@@ -697,46 +697,52 @@ class NSURLExtensionsTests: XCTestCase {
   }
 
   func testIsBookmarkletURL() {
-    let goodURLs = [
+    let javascriptURLs = [
       "javascript:",
+      "javascript://",
       "javascript:void(window.close(self))",
       "javascript:window.open('https://brave.com')",
     ]
 
-    let badURLs = [
-      "javascript:/",
-      "javascript://",
-      "javascript://something",
+    let nonJavascriptURLs = [
+      "https://brave",
+      "https://brave.com",
+      "https://javascript://",
+      "javascript//",
     ]
 
-    goodURLs.forEach {
-      XCTAssertNotNil($0.bookmarkletURL)
+    javascriptURLs.forEach {
+      XCTAssertNotNil(URL.bookmarkletURL(from: $0))
     }
 
-    badURLs.forEach {
-      XCTAssertNil($0.bookmarkletURL)
+    nonJavascriptURLs.forEach {
+      XCTAssertNil(URL.bookmarkletURL(from: $0))
     }
   }
 
   func testIsBookmarkletURLComponent() {
-    let goodURLs = [
+    let javascriptURLs = [
+      "javascript:/",
+      "javascript://",
+      "javascript://something",
       "javascript:void(window.close(self))",
       "javascript:window.open('https://brave.com')",
+      "javascript://%0a%0dalert('UXSS')",
     ]
 
     let badURLs = [
       "javascript:",
-      "javascript:/",
-      "javascript://",
-      "javascript://something",
+      "https://test",
+      "javascript//test",
+      "javascript/test",
     ]
 
-    goodURLs.forEach {
-      XCTAssertNotNil($0.bookmarkletCodeComponent)
+    javascriptURLs.forEach {
+      XCTAssertNotNil(URL.bookmarkletURL(from: $0)?.bookmarkletCodeComponent)
     }
 
     badURLs.forEach {
-      XCTAssertNil($0.bookmarkletCodeComponent)
+      XCTAssertNil(URL.bookmarkletURL(from: $0)?.bookmarkletCodeComponent)
     }
   }