Skip to content

Commit

Permalink
Enable username&password passed in URL (#1006)
Browse files Browse the repository at this point in the history 
Task/Issue URL:
https://app.asana.com/0/414235014887631/1204080079383830/f
Tech Design URL:
https://app.asana.com/0/481882893211075/1204065038916791/f
Security Triage URL:
https://app.asana.com/0/1199892415909552/1204067721102886/f
BSK PR: duckduckgo/BrowserServicesKit#245
iOS PR: duckduckgo/iOS#1521
CC: @brindy 

**Description**:
- Adds support to pass basic auth username/password in URL

**Steps to test this PR**:
1. Validate login/password passed in URL is used for basic
authentication and is saved per session
2. Validate login/password passed in URL is not displayed in the UI or
saved in browsing history
3. Validate when clicking links containing username/password
(user:pass@domain.com), the credentials aren‘t displayed in UI but are
used during basic auth
4. Validate if invalid credentials are provided then auth dialog is
displayed

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
**When ready for review, remember to post the PR in MM**
  • Loading branch information
@mallexxx
mallexxx authored Mar 9, 2023
1 parent 104f341 commit e4097a5
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 13 deletions.
2 changes: 1 addition & 1 deletion DuckDuckGo.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8445,7 +8445,7 @@
repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
requirement = {
kind = exactVersion;
version = 48.0.2;
version = 48.1.1;
};
};
AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {
Expand Down
4 changes: 2 additions & 2 deletions DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/duckduckgo/BrowserServicesKit",
"state" : {
"revision" : "c38de5453fa8892259075d9e7af8b43c7a692b44",
"version" : "48.0.2"
"revision" : "f54a72c805447918391b5e2fa20fee368773417b",
"version" : "48.1.1"
}
},
{
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/Bookmarks/View/AddBookmarkModalViewController.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ final class AddBookmarkModalViewController: NSViewController {
let title: String?

init?(_ tab: Tab) {
guard case let .url(url, userEntered: _) = tab.content else {
guard case let .url(url, credential: _, userEntered: _) = tab.content else {
return nil
}
self.url = url
Expand Down
40 changes: 34 additions & 6 deletions DuckDuckGo/Browser Tab/Model/Tab.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
// swiftlint:disable file_length

import Cocoa
import Common
import WebKit
import os
import Combine
Expand All @@ -27,7 +28,6 @@ import Navigation
import TrackerRadarKit
import ContentBlocking
import UserScript
import Common
import PrivacyDashboard

protocol TabDelegate: ContentOverlayUserScriptDelegate {
Expand All @@ -49,7 +49,7 @@ final class Tab: NSObject, Identifiable, ObservableObject {

enum TabContent: Equatable {
case homePage
case url(URL, userEntered: Bool = false)
case url(URL, credential: URLCredential? = nil, userEntered: Bool = false)
case privatePlayer(videoID: String, timestamp: String?)
case preferences(pane: PreferencePaneIdentifier?)
case bookmarks
Expand All @@ -67,6 +67,9 @@ final class Tab: NSObject, Identifiable, ObservableObject {
return .preferences(pane: preferencePane)
} else if let privatePlayerContent = PrivatePlayer.shared.tabContent(for: url) {
return privatePlayerContent
} else if let url, let credential = url.basicAuthCredential {
// when navigating to a URL with basic auth username/password, cache it and redirect to a trimmed URL
return .url(url.removingBasicAuthCredential(), credential: credential, userEntered: userEntered)
} else {
return .url(url ?? .blankPage, userEntered: userEntered)
}
Expand Down Expand Up @@ -119,7 +122,7 @@ final class Tab: NSObject, Identifiable, ObservableObject {

var url: URL? {
switch self {
case .url(let url, userEntered: _):
case .url(let url, credential: _, userEntered: _):
return url
case .privatePlayer(let videoID, let timestamp):
return .privatePlayer(videoID, timestamp: timestamp)
Expand All @@ -139,7 +142,7 @@ final class Tab: NSObject, Identifiable, ObservableObject {

var isUserEnteredUrl: Bool {
switch self {
case .url(_, userEntered: let userEntered):
case .url(_, credential: _, userEntered: let userEntered):
return userEntered
default:
return false
Expand Down Expand Up @@ -470,7 +473,9 @@ final class Tab: NSObject, Identifiable, ObservableObject {
if content.isUrl, !webView.isLoading {
self.addVisit(of: url)
}
if content != self.content {
if self.content.isUrl, self.content.url == url {
// ignore content updates when tab.content has userEntered or credential set but equal url
} else if content != self.content {
self.content = content
}
}
Expand Down Expand Up @@ -769,7 +774,7 @@ final class Tab: NSObject, Identifiable, ObservableObject {
@MainActor
private var contentURL: URL {
switch content {
case .url(let value, userEntered: _):
case .url(let value, credential: _, userEntered: _):
return value
case .privatePlayer(let videoID, let timestamp):
return .privatePlayer(videoID, timestamp: timestamp)
Expand Down Expand Up @@ -1215,6 +1220,15 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
// send this event only when we're interrupting loading and showing extra UI to the user
webViewDidReceiveUserInteractiveChallengePublisher.send()

// when navigating to a URL with basic auth username/password, cache it and redirect to a trimmed URL
if case .url(let url, .some(let credential), userEntered: let userEntered) = content,
url.matches(challenge.protectionSpace),
challenge.previousFailureCount == 0 {

self.content = .url(url, userEntered: userEntered)
return .credential(credential)
}

let (request, future) = BasicAuthDialogRequest.future(with: challenge.protectionSpace)
self.userInteractionDialog = UserDialog(sender: .page(domain: challenge.protectionSpace.host), dialog: .basicAuthenticationChallenge(request))
do {
Expand Down Expand Up @@ -1242,6 +1256,20 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
@MainActor
func decidePolicy(for navigationAction: NavigationAction, preferences: inout NavigationPreferences) async -> NavigationActionPolicy? {

// when navigating to a URL with basic auth username/password, cache it and redirect to a trimmed URL
if let mainFrame = navigationAction.mainFrameTarget,
let credential = navigationAction.url.basicAuthCredential {

return .redirect(mainFrame) { navigator in
var request = navigationAction.request
// credential is removed from the URL and set to TabContent to be used on next Challenge
self.content = .url(navigationAction.url.removingBasicAuthCredential(), credential: credential, userEntered: false)
// reload URL without credentials
request.url = self.content.url!
navigator.load(request)
}
}

if let policy = privatePlayer.decidePolicy(for: navigationAction, in: self) {
return policy
}
Expand Down
4 changes: 2 additions & 2 deletions DuckDuckGo/Navigation Bar/View/AddressBarButtonsViewController.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ final class AddressBarButtonsViewController: NSViewController {
}

switch selectedTabViewModel.tab.content {
case .url(let url, userEntered: _):
case .url(let url, credential: _, userEntered: _):
guard let host = url.host else { break }

let isNotSecure = url.scheme == URL.NavigationalScheme.http.rawValue
Expand Down Expand Up @@ -797,7 +797,7 @@ final class AddressBarButtonsViewController: NSViewController {
let selectedTabViewModel = tabCollectionViewModel.selectedTabViewModel else { return }

switch selectedTabViewModel.tab.content {
case .url(let url, userEntered: _):
case .url(let url, credential: _, userEntered: _):
// Don't play the shield animation if mouse is over
guard !privacyEntryPointButton.isAnimationViewVisible else {
break
Expand Down
2 changes: 1 addition & 1 deletion Unit Tests/Permissions/WebViewMock.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ final class WebViewMock: WKWebView {
internal func setURL(_ url: URL) {
self._protocol = url.scheme!
self._host = url.host!
self._port = url.port ?? (url.scheme == "https" ? 443 : 80)
self._port = url.port ?? url.navigationalScheme?.defaultPort ?? 0
}

class func new(url: URL) -> WKSecurityOriginMock {
Expand Down

0 comments on commit e4097a5

Please sign in to comment.