Add rules for GetVisibleEntry, InProcessBrowserTest and WebContentsUserData and browser dependency inversion

Author: bridiver

assets/semgrep_rules/client/browser-dependency-inversion.cc

@@ -0,0 +1,8 @@
+// ruleid:  browser-dependency-inversion
+chrome::FindBrowserWithTab(web_contents);
+// ruleid:  browser-dependency-inversion
+FindBrowserWithTab(web_contents);
+// ruleid:  browser-dependency-inversion
+BrowserView::GetBrowserViewForNativeWindow();
+// ruleid:  browser-dependency-inversion
+void MyClass::MyMethod(Browser* browser, bool test) { }

assets/semgrep_rules/client/browser-dependency-inversion.yaml

@@ -0,0 +1,30 @@
+rules:
+  - id: browser-dependency-inversion
+    metadata:
+      author: Brian Johnson <[email protected]>
+      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
+      assignees: |
+        goodov
+        cdesouza-chromium
+        bridiver
+      category: correctness
+    message: |
+      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
+    severity: INFO
+    languages:
+      - cpp
+    pattern-either:
+      - patterns:
+        - pattern: $FUNC(...)
+        - metavariable-regex:
+            metavariable: $FUNC
+            regex: ^(chrome::)?(FindTabbedBrowser|FindAnyBrowser|FindBrowserWithProfile|FindAllTabbedBrowsersWithProfile|FindAllBrowsersWithProfile|FindBrowserWithID|FindBrowserWithWindow|FindBrowserWithActiveWindow|FindBrowserWithTab|FindBrowserWithGroup|FindBrowserWithUiElementContext|FindLastActiveWithProfile|FindLastActive|BrowserView::GetBrowserViewForNativeWindow|BrowserView::FindBrowserWindowWithWebContents)$
+      - patterns:
+        - pattern: $RETURN $FUNC(..., Browser* $BROWSER, ...) { ... }

assets/semgrep_rules/client/get-visible-entry.cc

@@ -0,0 +1,11 @@
+// ruleid: get-visible-entry
+web_contents->GetController().GetVisibleEntry();
+
+// ruleid: get-visible-entry
+web_contents->GetVisibleURL();
+
+// ok: get-visible-entry
+web_contents->GetLastCommittedURL();
+
+// ok: get-visible-entry
+web_contents->GetController().GetLastCommittedEntry();

assets/semgrep_rules/client/get-visible-entry.yaml

@@ -0,0 +1,23 @@
+rules:
+  - id: get-visible-entry
+    metadata:
+      author: Brian Johnson <[email protected]>
+      references:
+        - https://github.com/brave/brave-browser/wiki/Security-reviews
+      confidence: MEDIUM
+      source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/get-visible-entry.yaml
+      assignees: |
+        thypon
+        diracdeltas
+        bridiver
+      category: security
+    message: |
+      $FUNC usages should be vet by the security-team. Most of the time you want the last committed entry/url
+    severity: INFO
+    languages:
+      - cpp
+    patterns:
+      - pattern: $OBJ.$FUNC(...)
+      - metavariable-regex:
+            metavariable: $FUNC
+            regex: ^(GetVisibleEntry|GetVisibleURL)$

assets/semgrep_rules/client/in-process-browser-test.cc

@@ -0,0 +1,7 @@
+// ruleid: in-process-browser-test
+class MyTest : public InProcessBrowserTest {
+}
+
+// ok: in-process-browser-test
+class MyTest : public PlatformBrowserTest {
+}

assets/semgrep_rules/client/in-process-browser-test.yaml

@@ -0,0 +1,20 @@
+rules:
+  - id: in-process-browser-test
+    metadata:
+      author: Brian Johnson <[email protected]>
+      source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/in-process-browser-test.yaml
+      assignees: |
+        goodov
+        cdesouza-chromium
+        bridiver
+      category: correctness
+    pattern: |
+      class $CLASS : public InProcessBrowserTest
+    message: "Most browser tests should be PlatformBrowserTest so they can run on android. "
+    languages:
+      - generic
+    paths:
+      include:
+        - "*.cc"
+        - "*.h"
+    severity: WARNING

assets/semgrep_rules/client/web-contents-user-data.cc

@@ -0,0 +1,9 @@
+// ruleid: web-contents-user-data
+class MyTest : public WebContentsUserData {
+}
+// ruleid: web-contents-user-data
+class MyTest : public content::WebContentsUserData {
+}
+// ok: web-contents-user-data
+class MyTest : public WebContentsObserver {
+}

assets/semgrep_rules/client/web-contents-user-data.yaml

@@ -0,0 +1,27 @@
+rules:
+  - id: web-contents-user-data
+    metadata:
+      author: Brian Johnson <[email protected]>
+      source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/in-process-browser-test.yaml
+      assignees: |
+        goodov
+        cdesouza-chromium
+        bridiver
+      category: correctness
+      references:
+        - https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
+    pattern-either:
+      - pattern: public content::WebContentsUserData
+      - pattern: public WebContentsUserData
+    message: |
+      Prefer dependency injection
+
+      References:
+      - https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
+    languages:
+      - generic
+    paths:
+      include:
+        - "*.cc"
+        - "*.h"
+    severity: INFO