Malicious site protection - extract special error page navigation logic #3624

alessandroboron · 2024-11-26T18:02:56Z

Task/Issue URL: https://app.asana.com/0/1206329551987282/1208836682251611/f
Tech Design URL: https://app.asana.com/0/1206329551987282/1207273224076495/f

Description:

This PR extracts the navigation logic for special error pages in its own class.

This PR focuses on creating the the entites used to encapsulate the logic for SSL and Malicious site protection

The implementation of the logic for Malicious site protection will follow in an upcoming PR.

Steps to test this PR:

Ensure all the SpecialErrorPage tests pass

Manual tests
SSL Tests from SSL PR

Steps to test this PR:

Go to badssl.com
Select expired certificate
Our custom special error page should open.

at this point you should be able to go back, and once you go back you should be able to go forward
please validate if design looks fine, and if it doesn't break in landscape
please validate if design supports light/dark modes
please validate if design supports translations (in order to test it you will most likely have to re-enter the page)
please validate the privacy shield icon in navigation bar is invisible.

Tap on leave site.

you should be taken back (if it was possible) or the tab should be closed if this was first page in navigation

Tap on advanced and accept the risk.

please validate that expired.badssl.com has loaded (red background)
please validate that privacy shield icon reappeared but has red dot on top of it
once you go to privacy dashboard you should see information about incorrect certificate
please validate you can go back to badssl.com site

Definition of Done (Internal Only):

Does this PR satisfy our Definition of Done?

Copy Testing:

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

Orientation Testing:

Portrait
Landscape

Device Testing:

OS Testing:

iOS 15
iOS 16
iOS 17

Theme Testing:

Light theme
Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

github-actions · 2024-11-26T18:04:33Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against b77eb5a

alessandroboron · 2024-11-27T09:11:49Z

DuckDuckGo/MaliciousSiteProtection/MaliciousSiteProtectionManager.swift

+
+import Foundation
+
+final class MaliciousSiteProtectionManager: MaliciousSiteDetecting {


Implementation of this class will come in a subsequent PR.
This was used to mock the BSK library and assert that the special error page was rendered.

alessandroboron · 2024-11-27T09:25:30Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+import SpecialErrorPages
+import WebKit
+
+enum MaliciousSiteProtectionNavigationResult: Equatable {


This class will encapsulate MaliciousSiteProtection logic to evaluate if a site is malicious and will handle its internal state to bypass URLs.

alessandroboron · 2024-11-27T09:26:14Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+SSL.swift

+    }
+
+    func errorPageVisited(errorType: SSLErrorType) {
+        Pixel.fire(pixel: .certificateWarningDisplayed(errorType.rawParameter))


We could inject the fire function and add tests to ensure Pixels are fired.

tbh I don't mind leaving it as it is for now

alessandroboron · 2024-11-27T09:26:32Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+SSL.swift

+import BrowserServicesKit
+import SpecialErrorPages
+import Core
+


This class encapsulate the SSL logic

alessandroboron · 2024-11-27T09:27:31Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler.swift

+import WebKit
+import SpecialErrorPages
+import Core
+


This class will orchestrate the navigation logic of features that require a Special Error Page

alessandroboron · 2024-11-27T09:32:02Z

DuckDuckGoTests/SpecialErrorPage/SSLErrorPageNavigationHandlerTests.swift

+@testable import SpecialErrorPages
+@testable import DuckDuckGo
+
+final class SSLSpecialErrorPageTests: XCTestCase {


These tests were initially using the TabViewController. I split them in two test classes, SSLSpecialErrorPageTests and SpecialErrorPageNavigationHandlerIntegrationTests.

We could also convert this class using SwiftTesting so we have all test suites related to Special Error Pages. I can make a follow up task at the end of the project.

would be great :)

I update the existing tests to use Swift Testing

jaceklyp

what a nice piece of code, you're definitely leaving it in much better shape than it was, the other comments are just minor;
note: haven't tested it yet

jaceklyp · 2024-11-28T13:35:09Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+SSL.swift

+
+    func makeNewRequestURLAndSpecialErrorDataIfEnabled(error: NSError) -> SSLSpecialError?
+
+    func errorPageVisited(errorType: SSLErrorType)


let's remove empty lines

jaceklyp · 2024-11-28T13:47:05Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+SSL.swift

+    }
+
+    func errorPageVisited(errorType: SSLErrorType) {
+        Pixel.fire(pixel: .certificateWarningDisplayed(errorType.rawParameter))


tbh I don't mind leaving it as it is for now

jaceklyp · 2024-11-28T14:03:11Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces.swift

+// MARK: - SpecialErrorPageNavigationDelegate
+
+/// A delegate for handling navigation actions related to special error pages.
+protocol SpecialErrorPageNavigationDelegate: AnyObject {


can we move this protocol above BaseSpecialError protocol where it is already used?

jaceklyp · 2024-11-28T14:05:37Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces.swift

@@ -0,0 +1,131 @@
+//
+//  SpecialErrorPageInterfaces.swift


I don't think we should put all these protocols under this umbrella file - for me personally it makes things confusing - I'm fine with putting separate files into group though named like this

Sounds good to me!

jaceklyp · 2024-11-28T14:25:06Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces.swift

+    func setUserScript(_ userScript: SpecialErrorPageUserScript?)
+}
+
+typealias SpecialErrorPageNavigationHandling = BaseSpecialErrorPageNavigationHandling & WebViewNavigationHandling & SpecialErrorPageUserScriptDelegate


perhaps we can rename these to:
SpecialErrorPageNavigationHandling -> SpecialErrorPageManaging
BaseSpecialErrorPageNavigationHandling -> SpecialErrorPageContextHandling/SpecialErrorPageFlowHandling (as it is mostly responsible for state handling and setup)? Base prefix doesn't tell much

Yes I wasn’t fan of “Base” as well but couldn’t find a better name. :)

jaceklyp · 2024-11-28T14:25:42Z

DuckDuckGoTests/SpecialErrorPage/SSLErrorPageNavigationHandlerTests.swift

+@testable import SpecialErrorPages
+@testable import DuckDuckGo
+
+final class SSLSpecialErrorPageTests: XCTestCase {


would be great :)

jaceklyp · 2024-11-28T14:26:39Z

DuckDuckGoTests/SpecialErrorPage/SpecialErrorPageNavigationHandlerIntegrationTests.swift

+import SpecialErrorPages
+@testable import DuckDuckGo
+
+@Suite("Special Error Pages - SSL Integration Tests", .serialized)


jaceklyp · 2024-11-29T09:18:06Z

what a nice piece of code, you're definitely leaving it in much better shape than it was, the other comments are just minor; note: haven't tested it yet

update: tested and it works well

alessandroboron · 2024-11-29T13:29:31Z

what a nice piece of code, you're definitely leaving it in much better shape than it was, the other comments are just minor; note: haven't tested it yet

Thanks @jaceklyp, I will address the comments! I will re-request a review soon :)

…ic (#3624) Task/Issue URL: https://app.asana.com/0/1206329551987282/1208836682251611/f Tech Design URL: https://app.asana.com/0/1206329551987282/1207273224076495/f **Description**: This PR extracts the navigation logic for special error pages in its own class. This PR focuses on creating the the entites used to encapsulate the logic for SSL and Malicious site protection

alessandroboron force-pushed the alessandro/malicious-site-protection-navigation branch 3 times, most recently from b1ff305 to 2a77dee Compare November 26, 2024 20:31

alessandroboron added 5 commits November 27, 2024 10:03

Add SpecialErrorPage navigation handlers

62fe8f5

Update TabViewController to use SpecialErrorPageNavigationHandler

f11301e

Update and add tests for SpecialErrorPageNavigationHandler

43d6335

Fix unit tests crash

4d4f428

Fix lint warnings

c3815f5

alessandroboron force-pushed the alessandro/malicious-site-protection-navigation branch from 2a77dee to c3815f5 Compare November 27, 2024 09:06

alessandroboron commented Nov 27, 2024

View reviewed changes

Refactor Special Error model

cdf5546

alessandroboron commented Nov 27, 2024

View reviewed changes

alessandroboron requested a review from jaceklyp November 27, 2024 09:36

jaceklyp requested changes Nov 28, 2024

View reviewed changes

Fix PR comments

b77eb5a

alessandroboron requested a review from jaceklyp November 29, 2024 14:16

jaceklyp approved these changes Nov 29, 2024

View reviewed changes

alessandroboron merged commit 02670bc into alessandro/malicious-site-protection Dec 2, 2024
16 checks passed

alessandroboron deleted the alessandro/malicious-site-protection-navigation branch December 2, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Malicious site protection - extract special error page navigation logic #3624

Malicious site protection - extract special error page navigation logic #3624

alessandroboron commented Nov 26, 2024

github-actions bot commented Nov 26, 2024 •

edited

Loading

alessandroboron Nov 27, 2024

alessandroboron Nov 27, 2024

alessandroboron Nov 27, 2024

jaceklyp Nov 28, 2024

alessandroboron Nov 27, 2024

alessandroboron Nov 27, 2024 •

edited

Loading

alessandroboron Nov 27, 2024 •

edited

Loading

jaceklyp Nov 28, 2024

alessandroboron Nov 29, 2024

jaceklyp left a comment

jaceklyp Nov 28, 2024

jaceklyp Nov 28, 2024

jaceklyp Nov 28, 2024

jaceklyp Nov 28, 2024

alessandroboron Nov 29, 2024

jaceklyp Nov 28, 2024

alessandroboron Nov 29, 2024

jaceklyp Nov 28, 2024

jaceklyp Nov 28, 2024

jaceklyp commented Nov 29, 2024

alessandroboron commented Nov 29, 2024


		import Foundation

		final class MaliciousSiteProtectionManager: MaliciousSiteDetecting {


		func makeNewRequestURLAndSpecialErrorDataIfEnabled(error: NSError) -> SSLSpecialError?

		func errorPageVisited(errorType: SSLErrorType)

Malicious site protection - extract special error page navigation logic #3624

Malicious site protection - extract special error page navigation logic #3624

Conversation

alessandroboron commented Nov 26, 2024

Internal references:

github-actions bot commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandroboron Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

alessandroboron Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaceklyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaceklyp commented Nov 29, 2024

alessandroboron commented Nov 29, 2024

github-actions bot commented Nov 26, 2024 •

edited

Loading

alessandroboron Nov 27, 2024 •

edited

Loading

alessandroboron Nov 27, 2024 •

edited

Loading