-
Notifications
You must be signed in to change notification settings - Fork 857
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
Android safebrowsing safetynet #16080
Conversation
android/java/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java
Outdated
Show resolved
Hide resolved
d8ed596
to
577da44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd only suggest a small change to the flag description to make it clear where the list is coming from.
8a213a7
to
ef8ff4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java++
|
Should we default the setting to ENABLED by default? (ex: the step 2 in your test plan; |
Concern above discussed offline. Downloaded and sideloaded, works great! 😄 Great job @SergeyZhukovsky! |
ef8ff4a
to
87f3335
Compare
87f3335
to
a786b35
Compare
Preference preference = getPreferenceScreen().findPreference(PREF_SAFE_BROWSING); | ||
if (preference != null) { | ||
preference.setOnPreferenceClickListener((pref) -> { | ||
if (!ChromiumPlayServicesAvailability.isGooglePlayServicesAvailable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is not the recommended way to do this, but we'll start with that and then revise it once we add the P3A question. The alternative is to do a dummy lookup for something like safebrowsing.google.com
and then check for failure.
android/java/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java
Outdated
Show resolved
Hide resolved
android/java/org/chromium/components/safe_browsing/BraveSafeBrowsingApiHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
there are two minor questions
44bfbd4
to
e8488bc
Compare
that's been addressed, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src changes look ok
Resolves brave/brave-browser#8705
Security review: https://github.com/brave/security/issues/1118
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
brave://flags/#brave-safe-browsing
is EnabledSafe Browsing
is enabled inSettings->Brave Shields & privacy