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

Import Data from Safari iOS #26914

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Brandon-T
Copy link
Contributor

@Brandon-T Brandon-T commented Dec 6, 2024

Security Review

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:

Test Plan:

@Brandon-T Brandon-T added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-teamcity Do not run builds in TeamCity labels Dec 6, 2024
@Brandon-T Brandon-T self-assigned this Dec 6, 2024
@Brandon-T Brandon-T requested a review from a team as a code owner December 6, 2024 16:26
Copy link
Contributor

github-actions bot commented Dec 6, 2024

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.

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

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)

@Brandon-T Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from ea6722c to f1a9c7d Compare December 6, 2024 18:28
@Brandon-T Brandon-T requested a review from a team as a code owner December 6, 2024 18:28
}

size_t field_idx = 0;
CSVFieldParser parser(row);
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

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

@Brandon-T Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from f1a9c7d to 67c2c7b Compare December 10, 2024 17:53
@Brandon-T Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from 67c2c7b to c21ba1c Compare December 11, 2024 21:03
Add Password Importer
Add History Importer

Signed-off-by: Brandon <[email protected]>
Signed-off-by: Brandon <[email protected]>
@Brandon-T Brandon-T force-pushed the feature/HistoryBookmarksImportSafari branch from c21ba1c to da8e55e Compare December 18, 2024 16:59
Copy link
Contributor

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

Description

This PR introduces Safari password import functionality to the Brave browser on iOS. It adds new components for importing bookmarks, history, and passwords from Safari, including support for importing from zip files. The changes include new utility classes, importers, and modifications to existing UI components to support these import operations.

Possible Issues

  1. The import process is not atomic - if an error occurs during import, some data may have already been imported.
  2. There's no progress indicator for long-running import operations, which could lead to a poor user experience for large imports.
  3. The error handling in some areas (e.g., zip file extraction) could be more robust.

Security Hotspots

  1. File system access: The code interacts with the file system to read import files. Ensure proper validation of file paths and contents to prevent potential directory traversal or arbitrary file read vulnerabilities.
  2. Password handling: Importing passwords involves handling sensitive data. Ensure that passwords are not logged or stored insecurely during the import process.
  3. JSON parsing: The history import functionality parses JSON data. Validate and sanitize this input to prevent potential injection attacks or crashes due to malformed data.
Changes

Changes

  1. components/password_manager/core/browser/import/:
    • Added new files for Safari password import functionality, including CSV parsing and data structures.
  2. components/password_manager/services/csv_password/:
    • Introduced new service for parsing CSV files containing Safari passwords.
  3. ios/brave-ios/Sources/Brave/Frontend/:
    • Modified existing UI components to support new import functionality.
  4. ios/browser/api/:
    • Added new APIs for history and password importing, as well as zip file extraction.
  5. ios/BUILD.gn:
    • Updated build configuration to include new components.
sequenceDiagram
    participant User
    participant UI
    participant Importer
    participant FileSystem
    participant Parser
    participant DataStore

    User->>UI: Initiate Safari data import
    UI->>Importer: Start import process
    Importer->>FileSystem: Read import file
    FileSystem-->>Importer: Raw file data
    Importer->>Parser: Parse data (CSV/JSON)
    Parser-->>Importer: Structured data
    Importer->>DataStore: Store imported data
    DataStore-->>Importer: Import result
    Importer-->>UI: Update import status
    UI-->>User: Display import result
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-teamcity Do not run builds in TeamCity CI/skip-windows-x64 Do not run CI builds for Windows x64 needs-security-review puLL-Merge unused-CI/skip-linux-x64 Do not run CI builds for Linux x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] - Export API for Importing Passwords and History from Safari
6 participants