-
Notifications
You must be signed in to change notification settings - Fork 904
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
Issue 4328: Proxying requests for sb-ssl.google.com for file proxy checks through sb-ssl.brave.com #2377
Conversation
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.
LGTM, but I would also add a new unit test for this if possible.
347a866
to
e24dba0
Compare
brave::ResponseCallback callback; | ||
GURL::Replacements replacements; | ||
replacements.SetHostStr(kBraveSafeBrowsingFileCheckProxy); | ||
GURL expected_url(url.ReplaceComponents(replacements)); |
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.
minor: i think this test would be more precise if it tested for the literal expected string, like https://sb-ssl.brave.com/...
instead of url.ReplaceComponents(replacements)
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.
done!
…ecks through sb-ssl.brave.com auditors: @riastradh-brave, @diracdeltas
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.
lgtm assuming tests pass. thanks for taking this
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.
LGTM
fix brave/brave-browser#4328
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
<DATA_DIR>/Safe Browsing
testsafebrowsing.appspot.com
- Verify that the download checks work correctlysb-ssl.google.com
using a proxy (Fiddler for windows/linux and little snitch for mac-os)Reviewer Checklist:
After-merge Checklist:
changes has landed on.