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

Malicious site protection address bar and privacy dashboard changes #3718

Open
wants to merge 4 commits into
base: alessandro/malicious-site-protection-navigation-detection-async
Choose a base branch
from

Conversation

alessandroboron
Copy link
Contributor

@alessandroboron alessandroboron commented Dec 11, 2024

Task/Issue URL: https://app.asana.com/0/1206329551987282/1208959082985728/f
Tech Design: https://app.asana.com/0/1206329551987282/1207273224076495/f
CC: @not-a-rootkit

Description:

This PR addresses the following:

  1. Updates the Privacy Icon to use the globe asset when visiting special error pages (SSL error included).
  2. Updates the Privacy icon to use the alert asset when the user accepts the risk of visiting a malicious page.
  3. Show an updated privacy dashboard for phishing and malware special error pages.

Steps to test this PR:

Scenario 1 - Special Error Pages show globe icon in the address bar
Ensure that the below URLs show the globe icon in the Address Bar as per Figma Design

  1. http://privacy-test-pages.site/security/badware/phishing.html
  2. http://privacy-test-pages.site/security/badware/malware.html
  3. https://expired.badssl.com/ (or any other SSL error URL)

Scenario 2 - Malicious Sites show alert icon in the address bar

  1. Navigate to the below websites
    a. http://privacy-test-pages.site/security/badware/phishing.html
    b. http://privacy-test-pages.site/security/badware/malware.html
  2. When the special error page is shown, tap “Advanced” button and then tap the “Accept Risk adn Visit Site” button.
  3. Ensure that the address bar show an alert as per Figma Design

Scenario 3 - Malicious Sites show updated privacy dashboard

  1. Navigate to the below websites
    a. http://privacy-test-pages.site/security/badware/phishing.html
    b. http://privacy-test-pages.site/security/badware/malware.html
  2. When the special error page is shown, tap “Advanced” button and then tap the “Accept Risk adn Visit Site” button.
  3. Tap the alert icon to open the Privacy Dashboard.
  4. Ensure that the Privacy dashboard inform the user of deceptive website

Scenario 4 - Accepting visiting SSL Insecure website (Bonus Point)

  1. Navigate to one of the malicious website.
  2. Accept navigating to the website.
  3. Navigate to https://expired.badssl.com/.
  4. Ensure the globe icon is shown instead of the alert icon.
  5. Accept the risk of visiting the site.
  6. Ensure the shield icon is shown instead of the alert icon.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

Copy link

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

@github-actions github-actions bot added the stale label Dec 19, 2024
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-navigation-detection-async branch from b6dbf5c to 5c07c05 Compare December 19, 2024 12:42
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-address-bar-and-privacy-dashboard branch from 95a56b5 to 014ea13 Compare December 19, 2024 12:46
@github-actions github-actions bot removed the stale label Dec 20, 2024
Comment on lines +305 to +308
if privacyInfo.isSpecialErrorPageVisible {
showCustomIcon(icon: .specialError)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

how does it look like for ssl error page now? I remember decision was not to show any icon, are we good there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this change we didn’t show any icon. We agreed to show the same icon for the special error pages, SSL included.

case daxLogo, shield, shieldWithDot
case daxLogo, shield, shieldWithDot, alert

fileprivate var staticImage: UIImage? {
Copy link
Contributor

Choose a reason for hiding this comment

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

why static?

Copy link
Contributor Author

@alessandroboron alessandroboron Jan 7, 2025

Choose a reason for hiding this comment

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

Probably not a great name if it raises questions. As shield and shieldWithDot are animated I was looking for a name that would give the idea of a “static” image without animation. I’ll find a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe static makes sense in this context, even the imageView was named staticImageView. Let me know if you can think of a better name

@@ -1064,6 +1064,7 @@ class TabViewController: UIViewController {
let privacyInfo = PrivacyInfo(url: url,
parentEntity: entity,
protectionStatus: makeProtectionStatus(for: host),
malicousSiteThreatKind: specialErrorPageNavigationHandler.currentThreatKind,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo; malicous

Copy link
Contributor Author

@alessandroboron alessandroboron Jan 10, 2025

Choose a reason for hiding this comment

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

Do you mind if I do this fix as a separate PR? I noticed it's an existing typo made in few different places. I will fix it in BSK and then make a PR for iOS and macOS. But because this PR is pointing at specific branch for privacy dashboard it’s a bit tricky to do this fix unless I merge that one first. As I’m merging this PR in my main feature branch there are no issues to merge the PR pointing at that branch. I will ensure it points at exact version before ship review.

Copy link

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

@github-actions github-actions bot added the stale label Dec 28, 2024
Copy link

github-actions bot commented Jan 4, 2025

This PR has been closed after 14 days of inactivity. Feel free to reopen it if you plan to continue working on it or have further discussions.

@github-actions github-actions bot closed this Jan 4, 2025
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-navigation-detection-async branch from 5c07c05 to c3dbd41 Compare January 9, 2025 01:22
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-address-bar-and-privacy-dashboard branch from 014ea13 to b4d4dd0 Compare January 9, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants