brave-core icon indicating copy to clipboard operation
brave-core copied to clipboard

[Skus] - Propagate Skus Errors to the point of usage

Open Brandon-T opened this issue 10 months ago • 2 comments

Summary

  • SkusSDK now returns a skus::SkusResult which contains a skus::SkusResultCode code and a std::string message for all skus calls.
  • The Rust side now calls the C++ code directly instead of using extern "C" bindings. This allows us to remove the into_raw and C pointer as the first argument.
  • Got rid of all the different types and callbacks for once single type: RustSequencedCallback, so simplify the code greatly.

Resolves https://github.com/brave/brave-browser/issues/37347

This PR completes: https://github.com/brave/brave-core/pull/22290

Submitter Checklist:

  • [x] I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • [x] There is a ticket for my issue
  • [x] Used Github auto-closing keywords in the PR description above
  • [x] Wrote a good PR/commit description
  • [x] Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • [x] Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • [x] 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
  • [x] 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:
    • [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
    • [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
    • [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
    • [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
    • [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
    • [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
    • [ ] https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

Brandon-T avatar Apr 04 '24 20:04 Brandon-T

@rillian I chatted with @Brandon-T and we have a path forward that eliminates the UniquePtr wrapping of SkusResult as well as generally improves the safety by changing the callback definitions such that they are handled by cxx ( they're manually written FFI functions not subject to cxx generation right now. )

evq avatar Apr 05 '24 20:04 evq

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

Here's my review of the pull request:

Description

This PR changes the way errors are handled and returned in the Skus (SKU management) system. It introduces a new SkusResult struct that contains both a result code and a message, replacing the previous approach of returning just a string. This change is propagated throughout the codebase, affecting both C++ and Rust components.

Changes

Changes

  1. components/skus/common/skus_sdk.mojom:

    • Added new SkusResultCode enum and SkusResult struct.
    • Updated all method signatures to return SkusResult instead of string.
  2. components/skus/browser/rs/cxx/src/errors.rs:

    • Refactored error handling to use the new SkusResult struct.
    • Implemented conversion from Rust errors to SkusResult.
  3. components/skus/browser/rs/cxx/src/lib.rs:

    • Updated Rust FFI definitions to use the new SkusResult struct.
  4. components/skus/browser/skus_context_impl.cc:

    • Updated callback handling to work with SkusResult.
  5. components/ai_chat/core/browser/ai_chat_credential_manager.cc:

    • Updated method signatures and error handling to use SkusResult.
  6. browser/ai_chat/ai_chat_settings_helper.cc:

    • Updated method signatures to use SkusResultPtr.
  7. components/brave_vpn/browser/brave_vpn_service.cc:

    • Updated method signatures and error handling to use SkusResultPtr.
  8. components/skus/renderer/skus_js_handler.cc:

    • Updated JavaScript bindings to handle SkusResult.
  9. iOS-specific changes:

    • Updated BraveSkusManager.swift and BraveSkusSDK.swift to work with the new error handling mechanism.

Possible Issues

  1. The error messages in BraveSkusSDK.swift are now being logged, which might expose sensitive information if not properly sanitized.

Security Hotspots

  1. The new error handling mechanism in iOS components (BraveSkusSDK.swift) includes error messages in logs. Ensure that these messages do not contain sensitive information that could be exploited if logs are exposed.

Overall, this change improves error handling and provides more detailed error information throughout the Skus system. However, care should be taken to ensure that error messages do not expose sensitive information, especially in logs.

github-actions[bot] avatar Jun 27 '24 04:06 github-actions[bot]