Add rules for GetVisibleEntry, InProcessBrowserTest and WebContentsUs… #736
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,8 @@ | |||
| // ruleid: browser-dependency-injection | |||
| chrome::FindBrowserWithTab(web_contents); | |||
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag
Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/browser-dependency-inversion.yaml
Cc @goodov @cdesouza-chromium @bridiver
| // ruleid: browser-dependency-injection | ||
| chrome::FindBrowserWithTab(web_contents); | ||
| // ruleid: browser-dependency-injection | ||
| FindBrowserWithTab(web_contents); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag
Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/browser-dependency-inversion.yaml
Cc @goodov @cdesouza-chromium @bridiver
| // ruleid: browser-dependency-injection | ||
| FindBrowserWithTab(web_contents); | ||
| // ruleid: browser-dependency-injection | ||
| BrowserView::GetBrowserViewForNativeWindow(); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag
Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/browser-dependency-inversion.yaml
Cc @goodov @cdesouza-chromium @bridiver
| // ruleid: browser-dependency-injection | ||
| BrowserView::GetBrowserViewForNativeWindow(); | ||
| // ruleid: browser-dependency-injection | ||
| void MyClass::MyMethod(Browser* browser, bool test) { } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag
Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/browser-dependency-inversion.yaml
Cc @goodov @cdesouza-chromium @bridiver
| @@ -0,0 +1,14 @@ | |||
| // ruleid: get-visible-entry | |||
| web_contents->GetController().GetVisibleEntry(); | |||
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] GetVisibleEntry usages should be vet by the security-team. Most of the time you want the last committed entry/url
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/get-visible-entry.yaml
Cc @thypon @diracdeltas @bridiver
| web_contents->GetController().GetVisibleEntry(); | ||
|
|
||
| // ruleid: get-visible-entry | ||
| web_contents->GetVisibleURL(); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] GetVisibleURL usages should be vet by the security-team. Most of the time you want the last committed entry/url
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/get-visible-entry.yaml
Cc @thypon @diracdeltas @bridiver
| @@ -0,0 +1,9 @@ | |||
| // ruleid: web-contents-user-data | |||
| class MyTest : public WebContentsUserData { | |||
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Prefer dependency injection
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/in-process-browser-test.yaml
Cc @goodov @cdesouza-chromium @bridiver
| class MyTest : public WebContentsUserData { | ||
| } | ||
| // ruleid: web-contents-user-data | ||
| class MyTest : public content::WebContentsUserData { |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Prefer dependency injection
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/in-process-browser-test.yaml
Cc @goodov @cdesouza-chromium @bridiver
| @@ -0,0 +1,7 @@ | |||
| // ruleid: in-process-browser-test | |||
| class MyTest : public InProcessBrowserTest { | |||
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Most browser tests should be PlatformBrowserTest so they can run on android.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/in-process-browser-test.yaml
Cc @goodov @cdesouza-chromium @bridiver
…erData and browser dependency inversion
…erData and browser dependency inversion