Fix Javascript URL navigation on iOS

Fix unit tests + formatting
brave · Mar 4, 2024 · 6240e38 · 6240e38
1 parent 262d41e
commit 6240e38
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)
  }
  }