Import Data from Safari iOS #26914

Brandon-T · 2024-12-06T16:26:02Z

Security Review

https://github.com/brave/reviews/issues/1815

Summary

Add Password Importer API to Brave-Core and a Swift wrapper around it
Add History Importer API to Brave-Core and a Swift wrapper around it
Refactor Bookmarks Import API in Swift to be async-await

Resolves brave/brave-browser#42727

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

github-actions · 2024-12-06T16:30:43Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

kylehickinson

Quick glance feedback (haven't looked at implementation details yet):

Needs unit tests
Everything under ios/browser/api should be Obj-C wrappers, please move all of the actual importer logic to separate directory/directories. Try to match Chromium's directory structure in this regard
Is it possible to share this logic with desktop so that they can get Safari import via exported zip as well? (Perhaps with a layered component)

kdenhartog · 2024-12-09T03:42:05Z

components/password_manager/core/browser/import/csv_safari_password.cc

+  }
+
+  size_t field_idx = 0;
+  CSVFieldParser parser(row);


Seems that the parser here considers the grammar simple enough that it doesn't require using a memory safe language or separate process.

I believe this the solves for the rule of 2 issue here as well.

kdenhartog · 2024-12-09T03:42:37Z

in general this seems ok to me, but I think it would be useful for @stoletheminerals to give this one a deeper look.

mkarolin

Not clear if we need a header in DEPS files, but for now ++

thypon

requires test

ios/brave-ios/Sources/Brave/Frontend/Sync/BraveCore/ZipImporter/ZipImporter.swift

kylehickinson

Couple more notes

kylehickinson · 2025-02-20T21:59:40Z

ios/brave-ios/Sources/DataImporter/DataImporterStrings.swift

+
+    public static let personalImportTitle = NSLocalizedString(
+      "personalImportTitle",
+      tableName: "DataImporter",


You don't need a table name for all these strings, since its in a separate target

kylehickinson · 2025-02-20T22:20:07Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterStateView.swift

+      if kind == .passwordConflict {
+        Button {
+          dismiss()
+        } label: {
+          Label {
+            Text(Strings.close)
+          } icon: {
+            Image(braveSystemName: "leo.close")
+              .foregroundColor(Color(braveSystemName: .iconDefault))
+              .padding(8)
+          }
+          .labelStyle(.iconOnly)
+        }
+        .background(Color(braveSystemName: .materialSeparator), in: Circle())
+        .frame(maxWidth: .infinity, alignment: .trailing)
+      }


This shouldn't be part of the VStack, it should be in a overlay(alignment: .topTrailing)

It also probably shouldn't be part of the ScrollView

This view doesn't have a scroll-view and isn't embedded in one.

kylehickinson · 2025-02-20T22:21:45Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+  @ObservedObject
+  private var model = DataImportModel()


This should be a @StateObject if you're going to create it in the View

kylehickinson · 2025-02-20T22:22:00Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+  @State
+  private var showSuccessView = false
+
+  @State
+  private var showFailureView = false
+
+  @State
+  private var showLoadingView = false
+
+  @State
+  private var showMainView = true


As per DM these should all be removed

kylehickinson · 2025-02-20T22:24:33Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+  @ViewBuilder
+  private func mainView() -> some View {


Doesn't need to be a func, can just be private var mainView: some View

kylehickinson · 2025-02-20T22:25:59Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+        .sheet(
+          isPresented: $model.isLoadingProfiles,


These sheets are explicitly associated with the main view only it seems? Is that correct? If for instance the import state is any causing any of the other views to appear the sheets will be dismissed, should these go on the container view instead?

kylehickinson · 2025-02-20T22:31:19Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+                .osAvailabilityModifiers({
+                  if #available(iOS 16.4, *) {
+                  } else {
+                    $0.background(.thickMaterial)
+                  }
+                })


As Per DM you can't leave one condition empty, need to return the view provided as the closure is a ViewBuilder

kylehickinson · 2025-02-20T22:32:06Z

ios/browser/api/password/importer/DEPS

@@ -0,0 +1,3 @@
+include_rules = [
+  "+brave/components/password_manager/core/browser/import/safari_password_importer.h"
+]


EOF newline missing

kylehickinson · 2025-02-20T22:40:37Z

ios/brave-ios/Sources/DataImporter/UI/DataImporterView.swift

+      if model.importState == .success {
+        DataImporterStateView(kind: .success, model: model) {


There's something weird here about creating an entire switch across import states all to display the same View (DataImporterStateView) with almost identical modifiers with the only thing changing is Buttons? You also seem to combine both making this reusable and a "View only" kind of component and also passing in the model which means you're now checking specific things related to the logic of the feature.

Either you should have one view and you only pass in the model and adjust inside directly based on importState, or you don't pass in the model and make sure you can pass in all necessary info from here.

You also probably want to find a way to do this without multiple if-else causing the creation of completely separate views for each state

Switched them all to separate views instead of one shared view.

kylehickinson · 2025-02-20T23:00:38Z

This final page also doesnt match design

kylehickinson · 2025-02-21T14:14:53Z

ios/brave-ios/Sources/DataImporter/Models/DataImportModel.swift

+        Task.delayed(bySeconds: 1.0) { @MainActor in
+          self.importState = .dataConflict
+        }


You're already in an async method, just do try await Task.sleep(...) directly in it

kylehickinson · 2025-02-21T14:15:00Z

ios/brave-ios/Sources/DataImporter/Models/DataImportModel.swift

-        self.importState = .dataConflict
+        Task.delayed(bySeconds: 1.0) { @MainActor in
+          self.importState = .dataConflict
+        }


Same as above

github-actions · 2025-02-21T15:04:44Z

[puLL-Merge] - brave/brave-core@26914

Description

This PR adds support for importing Safari data into Brave on iOS. It includes functionality to import bookmarks, browsing history, and passwords from Safari export files, with careful handling of conflicts and data validation. The change also includes a new UI for guiding users through the import process.

Security Hotspots

Password handling in safari_password_importer.cc and related files - Ensure passwords are properly encrypted in memory and securely cleared after use
File handling in ZipImporter.swift - Check for zip bombs and malicious file paths
CSV parsing in csv_safari_password.cc - Validate input to prevent buffer overflows or injection attacks
IPC communication in csv_safari_password_parser_service.cc - Ensure proper sandboxing and validation of messages

Changes

Changes

By filename:

New Components:

components/password_manager/core/browser/import/: New Safari password import infrastructure
- safari_password_importer.{h,cc}: Core importer implementation
- csv_safari_password.{h,cc}: CSV file parsing for passwords
- safari_import_results.{h,cc}: Results handling

iOS Changes:

ios/brave-ios/Sources/DataImporter/: New iOS UI for data import
- Added import flow UI components
- Multi-profile support
- Error handling and success states
- Tutorial for users

Build System:

Updated BUILD.gn files to support new components
Added dependencies for password importing

sequenceDiagram
    participant User
    participant UI
    participant Importer
    participant CSVParser
    participant Storage

    User->>UI: Initiates import
    UI->>Importer: Import request
    Importer->>CSVParser: Parse Safari export
    CSVParser-->>Importer: Parsed data
    
    alt Has Conflicts
        Importer->>UI: Show conflict resolution
        User->>UI: Resolve conflicts
        UI->>Importer: Resolution choice
    end

    Importer->>Storage: Store imported data
    Storage-->>UI: Import complete
    UI->>User: Show success/failure

Add Password Importer Add History Importer Signed-off-by: Brandon <[email protected]>

Signed-off-by: Brandon <[email protected]>

Add bottom sheet and hook up importer Add Importer View Add FilePicker Add Conflict handling Add Conflict View Add Success View Add Error View Finish all UI and refactor all code to its own package. Refactor Brave-Core.

Fix scrollViews, sheet backgrounds, layouts. Remove BraveSheet.

…ansparency and padding on conflicts view.

…veCore

…flicker

Brandon-T self-assigned this Dec 6, 2024

Brandon-T requested a review from a team as a code owner December 6, 2024 16:26

github-actions bot added the puLL-Merge label Dec 6, 2024

github-actions bot added the needs-security-review label Dec 6, 2024

github-actions bot assigned kdenhartog and thypon Dec 6, 2024

kylehickinson requested changes Dec 6, 2024

View reviewed changes

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from ea6722c to f1a9c7d Compare December 6, 2024 18:28

Brandon-T requested a review from a team as a code owner December 6, 2024 18:28

Brandon-T requested a review from kylehickinson December 6, 2024 18:30

Brandon-T removed the puLL-Merge label Dec 6, 2024

kdenhartog reviewed Dec 9, 2024

View reviewed changes

kdenhartog assigned stoletheminerals Dec 9, 2024

mkarolin approved these changes Dec 9, 2024

View reviewed changes

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from f1a9c7d to 67c2c7b Compare December 10, 2024 17:53

github-actions bot added the puLL-Merge label Dec 10, 2024

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch 2 times, most recently from c21ba1c to da8e55e Compare December 18, 2024 16:59

diracdeltas requested a review from stoletheminerals December 18, 2024 20:25

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch 2 times, most recently from 66c8b79 to c72538a Compare January 22, 2025 18:58

thypon requested changes Jan 22, 2025

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Sync/BraveCore/ZipImporter/ZipImporter.swift Outdated Show resolved Hide resolved

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from caddb70 to fa9e252 Compare February 19, 2025 16:42

stoletheminerals approved these changes Feb 19, 2025

View reviewed changes

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch 2 times, most recently from b890b94 to 5c92a3a Compare February 20, 2025 19:55

nuo-xu approved these changes Feb 20, 2025

View reviewed changes

kylehickinson requested changes Feb 20, 2025

View reviewed changes

kylehickinson reviewed Feb 21, 2025

View reviewed changes

Brandon-T added 19 commits February 21, 2025 12:35

Import Data from Safari iOS

98c12da

Add Password Importer Add History Importer Signed-off-by: Brandon <[email protected]>

Update for iOS 18.2 RC

d065884

Signed-off-by: Brandon <[email protected]>

Fix AccessingSecurityScopedResource

83b420a

Add import view

ac73dad

Add bottom sheet and hook up importer Add Importer View Add FilePicker Add Conflict handling Add Conflict View Add Success View Add Error View Finish all UI and refactor all code to its own package. Refactor Brave-Core.

Localized strings. Localized URLs. Address feedback. Fix copyrights.

e3707a9

Addressed most feedback (excluding spacing)

f9c3266

Fix scrollViews, sheet backgrounds, layouts. Remove BraveSheet.

Use a Javascript Unzipper instead of Chromium's

7fc21c9

Rename JSUnzip

78ce143

Fix compiling with Cr134. Use jszip from Chromium.

75eed4b

Fix presubmit

2db0b0c

Fix button sizes

0687c1f

Fix conflicts not showing due to Chromium function call order. Fix tr…

1804652

…ansparency and padding on conflicts view.

Cleanup unused branch

900d879

Addressed feedback

223b5be

Move password importer to only happen inside the Init callback in Bra…

272033b

…veCore

Move navigation bar changes to the ZStack

bdc7468

Add a delay for the loading screen so you don't see a navigation bar …

e5a978d

…flicker

Fix data importer fonts and fixed password importer logs

a2d13c4

Addressed feedback

f05f0d1

Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from 987e4e5 to f05f0d1 Compare February 21, 2025 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import Data from Safari iOS #26914

Import Data from Safari iOS #26914

Brandon-T commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024

kylehickinson left a comment •

edited

Loading

kdenhartog Dec 9, 2024

kdenhartog commented Dec 9, 2024

mkarolin left a comment

thypon left a comment

kylehickinson left a comment

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

Brandon-T Feb 21, 2025 •

edited

Loading

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

kylehickinson Feb 20, 2025

Brandon-T Feb 21, 2025 •

edited

Loading

kylehickinson commented Feb 20, 2025

kylehickinson Feb 21, 2025

kylehickinson Feb 21, 2025

github-actions bot commented Feb 21, 2025

Changes

		if model.importState == .success {
		DataImporterStateView(kind: .success, model: model) {

Import Data from Safari iOS #26914

Are you sure you want to change the base?

Import Data from Safari iOS #26914

Conversation

Brandon-T commented Dec 6, 2024 • edited Loading

Security Review

Summary

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Dec 6, 2024

kylehickinson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog commented Dec 9, 2024

mkarolin left a comment

Choose a reason for hiding this comment

thypon left a comment

Choose a reason for hiding this comment

kylehickinson 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

Brandon-T Feb 21, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brandon-T Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

kylehickinson commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2025

Description

Security Hotspots

Changes

Brandon-T commented Dec 6, 2024 •

edited

Loading

kylehickinson left a comment •

edited

Loading

Brandon-T Feb 21, 2025 •

edited

Loading

Brandon-T Feb 21, 2025 •

edited

Loading