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.

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

Enable username&password passed in URL #1521

Merged
merged 6 commits into from
Mar 9, 2023
Merged

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Mar 2, 2023

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
CC:

Description:

  • Adds support to pass basic auth username/password in URL

Steps to test this PR:
URLs for testing:

  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

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

This doesn't appear to work. I created the following URL, pasted it in and hit enter and it loaded the SERP.

https://user:pass@authenticationtest.com/HTTPAuth/

But I created a bookmark with this URL, used the fire button, then opened the link with the bookmark and it worked.

Also, what are the changes about the rulesCompiledCondition about - just code clean up?

@@ -242,7 +243,6 @@ class TabViewController: UIViewController {
}
}

private var rulesCompiledCondition: RunLoop.ResumeCondition? = RunLoop.ResumeCondition()
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this have to do with this PR?

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 was adding urlProvidedBasicAuthCredential property and noticed this rulesCompiledCondition is not used anywhere, I was hesitating to add an extra var to the class but having this removed sounds fair

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough :)

@mallexxx
Copy link
Collaborator Author

mallexxx commented Mar 3, 2023

@brindy I‘m not sure there‘s a double check for URL validity first in OmniBar.swift:401 and then basically the same procedure (BSK URLExtension: URL.isValid) in webUrl(from text: String), so I‘ve just removed the check for user == nil, seems working now

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

@mallexxx after using the fire button I am still logged in when testing with https://user:pass@authenticationtest.com/HTTPAuth/ - is that expected?

[Edit: I tested this on macOS and it works as expected - after the fire button you are logged out]

@mallexxx
Copy link
Collaborator Author

mallexxx commented Mar 7, 2023

I guess that is expected, most probably credential cash is not cleaned in iOS

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Looks like we're caching credentials in prod too, I'll create a separate task for that. LGTM!

@brindy brindy assigned mallexxx and unassigned brindy Mar 7, 2023
mallexxx added a commit to duckduckgo/macos-browser that referenced this pull request Mar 9, 2023
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**
@mallexxx mallexxx merged commit 36902d7 into develop Mar 9, 2023
@mallexxx mallexxx deleted the alex/fix-credential-url branch March 9, 2023 13:47
ayoy pushed a commit to duckduckgo/macos-browser that referenced this pull request Mar 10, 2023
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**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants