brave-core
brave-core copied to clipboard
[Skus] - Propagate Skus Errors to the point of usage
Summary
- SkusSDK now returns a
skus::SkusResult
which contains askus::SkusResultCode code
and astd::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 theinto_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
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issue - [x] Checked the PR locally:
- [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:
@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. )
[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
-
components/skus/common/skus_sdk.mojom
:- Added new
SkusResultCode
enum andSkusResult
struct. - Updated all method signatures to return
SkusResult
instead of string.
- Added new
-
components/skus/browser/rs/cxx/src/errors.rs
:- Refactored error handling to use the new
SkusResult
struct. - Implemented conversion from Rust errors to
SkusResult
.
- Refactored error handling to use the new
-
components/skus/browser/rs/cxx/src/lib.rs
:- Updated Rust FFI definitions to use the new
SkusResult
struct.
- Updated Rust FFI definitions to use the new
-
components/skus/browser/skus_context_impl.cc
:- Updated callback handling to work with
SkusResult
.
- Updated callback handling to work with
-
components/ai_chat/core/browser/ai_chat_credential_manager.cc
:- Updated method signatures and error handling to use
SkusResult
.
- Updated method signatures and error handling to use
-
browser/ai_chat/ai_chat_settings_helper.cc
:- Updated method signatures to use
SkusResultPtr
.
- Updated method signatures to use
-
components/brave_vpn/browser/brave_vpn_service.cc
:- Updated method signatures and error handling to use
SkusResultPtr
.
- Updated method signatures and error handling to use
-
components/skus/renderer/skus_js_handler.cc
:- Updated JavaScript bindings to handle
SkusResult
.
- Updated JavaScript bindings to handle
-
iOS-specific changes:
- Updated
BraveSkusManager.swift
andBraveSkusSDK.swift
to work with the new error handling mechanism.
- Updated
Possible Issues
- The error messages in
BraveSkusSDK.swift
are now being logged, which might expose sensitive information if not properly sanitized.
Security Hotspots
- 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.