-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Security reviews
yan edited this page Dec 14, 2024
·
21 revisions
If a code change (pull request, commit, etc.) satisfies ANY of the following, it requires a security/privacy review before it can be merged (even if behind a flag or only for certain channels). We prefer to review things at the design/spec stage and then again at the implementation stage.
- It is a major feature that touches many parts of the code or required work from multiple teams.
- [Browser-only] It modifies or adds network requests, or it rewrites URLs (including internal requests like those with the
brave://
scheme). - It is related to money/BAT.
- It involves cryptography (including anything which generates a random number, a random series of bytes, or the like) or distributes cryptography that is not fully open source.
- It adds new dependencies, vendors, service providers, integrations, or plugins. Update (9/2023): Please choose "Third-Party Dependency Review" when opening these in brave/reviews which requires a security review and principal engineer review in brave-core.
- It is related to sensitive user information such as cookies, passwords, and private browsing data.
- It changes the amount of data collected by Brave or a third party — including making any logs which may be sent to Brave or a third party.
- [Browser-only] It adds a new extension that is built-in or has special privileges in Brave.
- It changes any security/privacy messaging in our products (warning messages, security icons, etc.) or introduces non-trivial changes to any security/privacy-related UI elements, such as the URL bar (which displays security state and origin), the certificate viewer, interstitial pages, devtools, and shields.
- It adds a new channel or modifies an existing channel for distributing software produced by Brave, including software updates.
- [Browser-only] It has to do with a new window API. Please see https://docs.google.com/document/d/1BfJbLCvwToPqXHzsKqQgDuluwVDtGhzFR1BclqVzqaA/edit first.
- [Browser-only] Adds any "content scripts" which run in the main world or an isolated world, AKA any scripts which interact with in-page JavaScript.
- It adds a new way of collecting data from A/B tests (not through Griffin or Crash Reports).
- It saves data on a user's device and/or later retrieves it for purposes that are not strictly necessary (such as analytics). Further guidance on this is coming but please message Yan/Pat in the meantime if you need clarity.
- [Browser-only] It involves adding or updating mojo APIs / IPC, or adding new WebUIs such as chrome:// or chrome-untrusted:// pages.
If you are not sure whether you need a security review, you should probably ask for one just to play it safe.
Security reviews can be filed by staff here: https://github.com/brave/security/issues
Outside contributors should ask a staff to file a security issue for them if needed.