Add support for SafeBrowsing via request #1332
Conversation
Brandon-T
requested review from
jhreis,
iccub,
kylehickinson,
danishjafri88 and
jumde
and removed request for
jhreis
| @discardableResult | ||
| public static func threatMatches(urls: [URL], session: URLSession = .shared, _ completion: @escaping (Response, Error?) -> Void) throws -> URLSessionDataTask { | ||
|
|
||
| let apiKey = "AIzaSyDeglae_dQQyQuRNk1jPq5R5--jBy21H5o" |
There was a problem hiding this comment.
As a rule of thumb, never include API keys in the source code. We need to use the safe-browsing proxy for this. See: https://github.com/brave/handbook/blob/master/development/aws.md - This applies for all keys.
|
Please open a security review for this issue, we need multiple eyes on this feature to make sure its implemented correctly. Thanks for such a quick turn around on this @Brandon-T |
| case unknown = "PLATFORM_TYPE_UNSPECIFIED" | ||
|
|
||
| /// Threat posed to Windows. | ||
| case windows = "WINDOWS" |
There was a problem hiding this comment.
Do we need other platforms here? Just iOS should work.
|
@Brandon-T - test suite for safe browsing: https://testsafebrowsing.appspot.com/ - Tests with malicious sub-resources are not working correctly. |
|
|
||
| @discardableResult | ||
| public static func threatMatches(urls: [URL], session: URLSession = .shared, _ completion: @escaping (Response, Error?) -> Void) throws -> URLSessionDataTask { | ||
|
|
There was a problem hiding this comment.
This is using the Lookup API, we should use the recommended Update API to reduce the performance impact and not sending hash for every URL to the google servers: https://developers.google.com/safe-browsing/v4/update-api
|
Hey, these are my own personal API keys that I created on my own account. I can revoke them at any given time as they are only attached to my account. It was only included so we can test the PR.. Was not aware we had a proxy but I’ll be rewriting this PR then because there’s no available Swift APIs or Libraries for “Update API”, etc.. I will open a new PR tomorrow with everything rewritten and working with the proxy instead of my own keys. |
Fixes #483
There are two ways to implement safe-browsing. One is via a request to get the list and the other is to download the sb database and query against it via hashes and hash prefixes.
The ticket stated "via proxy" so I proxy the requests instead of checking against hashes.
NOTE: I am using my own API key atm for testing it.. It's included in this PR so others can test it without making one.. However, before MERGING, I need to get one from somewhere or perhaps the CI/CD will manage the keys? Not sure.
We will probably also want to get a design for the "Blocked" screen for when a request is blocked?
Submitter Checklist:
Fix #123: This fixes the shattered coffee cup!(orNo Bug: <message>if no relevant ticket)NSLocalizableString()Test Plan:
Screenshots:
Reviewer Checklist:
QA/(Yes|No)release-notes/(include|exclude)bug/enhancement