Closed gweiying closed 8 months ago
LGTM!
* agree that it looks ok to just set the cache time to 300s, no real need to follow the given `expireTime` so closely * minor question, non-blocking: `SOCIAL_ENGINEERING_EXTENDED_COVERAGE` wasn't there before and so it looks like we don't need to add it right away, but i'm wondering if it's good to add in eventually? * previously `isThreatBulk` could handle bulk requests all at once (up to 500), but now it sends individual requests for each URL, should we test that bulk uploads are okay too? * in the future, refactoring to use "web risk" instead of "safe browsing" would be nice!
Good points! Thanks for the quick review.
Added in SOCIAL_ENGINEERING_EXTENDED_COVERAGE
as part of the threat types covered, I agree that it's good to add and err on the safe side!
Also ran tests on bulk upload and updated the test list above, thanks for flagging out.
Context
Replaces Google Safe Browsing with Google Web Risk API, which has no daily quotas on number of links scanned.
Notes
MALWARE
SOCIAL_ENGINEERING
UNWANTED_SOFTWARE
SOCIAL_ENGINEERING_EXTENDED_COVERAGE
MALWARE
SOCIAL_ENGINEERING
UNWANTED_SOFTWARE
POTENTIALLY_HARMFUL_APPLICATION
Web Risk return:
Tests
Tests on staging
Deploy Notes
Before deploying,
After deployment, tests on production, 3 environments
New environment variables: None, reusing existing Safe Browsing API keys