From e901ffdfe9871d676183ca9aa7146b764f09b64b Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 13 Nov 2026 22:25:45 -0500 Subject: [PATCH 1/3] fix(scanner): Make per-language enabled-rule allowlists take effect OpenGrep prefixes the ID of every rule it loads from a --config path with the path's directories, so the --exclude-rule flags built from bare rule IDs never matched and every bundled rule ran regardless of _enabled_rules. The scanner now passes OpenGrep a temporary copy of the rules file holding only the enabled rules. Enabled IDs that match no rule are logged as a warning, and an allowlist that matches nothing runs every rule for the language. The _enabled_rules defaults are now empty in action.yml and connectors.yaml, so every bundled rule runs by default, as scans already did. The Action defaults had drifted from the CLI defaults and named seven rule IDs that no longer exist. A test now rejects default lists that name unknown rule IDs. Also: - Skip erlang.yml and objective-c.yml in every all-languages expansion; OpenGrep supports neither language. - Remove the temporary custom-rule and filtered-rule directories when the scan ends. - Add an end-to-end test that runs the real OpenGrep binary. Fixes #135 --- CHANGELOG.md | 23 +++ action.yml | 60 +++--- docs/github-action.md | 27 ++- docs/parameters.md | 14 +- socket_basics/connectors.yaml | 60 +++--- socket_basics/core/config.py | 9 +- .../core/connector/opengrep/__init__.py | 88 ++++++--- .../core/connector/opengrep/custom_rules.py | 10 - tests/test_action_inputs_consistency.py | 28 +++ tests/test_opengrep_custom_rules.py | 179 +++++++++++++++++- tests/test_opengrep_rule_filter_e2e.py | 109 +++++++++++ 11 files changed, 473 insertions(+), 134 deletions(-) create mode 100644 tests/test_opengrep_rule_filter_e2e.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b2dde4e..4c7faa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Upgrade notes +- **`_enabled_rules` now takes effect.** If you set it in a workflow, + on the command line or in the Socket Basics dashboard, scans for that language + now run only the listed rules, where they previously ran every bundled rule, + so expect fewer SAST findings. Clear the list or set + `all_rules_enabled: 'true'` to keep running every rule. The step log warns + about listed IDs that match no rule. See the Fixed entry below. - **Socket Tier 1 needs outbound access to `api.socket.dev` and `storage.googleapis.com`.** Socket npm CLI 1.9.0 and later download the Coana reachability engine from Socket instead of npm, with no npm fallback. @@ -44,6 +50,23 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed - TruffleHog 3.99.8 is built on Go 1.27.3, clearing the CVE-2026-111943 finding noted in 3.4.1. +- `_enabled_rules` now narrows the SAST scan. OpenGrep prefixes the + ID of every rule it loads from a rules file with the file's directories, so + the `--exclude-rule` flags built from bare rule IDs never matched and every + bundled rule ran. The scanner now passes OpenGrep a copy of the rules file + that holds only the enabled rules. Enabled IDs that match no rule are logged + as a warning; if none match, every rule for that language runs (#135). +- **Behavior change:** the `_enabled_rules` defaults are now empty in + both `action.yml` and `connectors.yaml`, so every bundled rule for an enabled + language runs. That is what scans already did, because the filter above never + applied, so default scan results do not change. The previous Action defaults + had drifted from the CLI defaults and named seven rule IDs that no longer + exist; with the filter working, they would have cut JavaScript scans from 59 + rules to 6. To narrow a scan, set `_enabled_rules` explicitly. +- An all-languages SAST scan no longer loads `erlang.yml` or + `objective-c.yml`. OpenGrep supports neither language and reported an + `UnknownLanguageError` for each file. +- Temporary files written for custom SAST rules are removed after the scan. ## [3.4.1] - 2026-10-21 diff --git a/action.yml b/action.yml index b9a38f3..d43b881 100644 --- a/action.yml +++ b/action.yml @@ -177,9 +177,9 @@ inputs: required: false default: "" c_enabled_rules: - description: "Comma-separated list of C SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C SAST rules to run (default: empty, which runs every bundled C rule)" required: false - default: "c-buffer-overflow,c-format-string-vulnerability,c-command-injection,c-sql-injection,c-use-after-free,c-hardcoded-secrets,c-weak-crypto,c-ssl-issues" + default: "" c_sast_enabled: description: "Enable C SAST scanning" required: false @@ -197,9 +197,9 @@ inputs: required: false default: "" cpp_enabled_rules: - description: "Comma-separated list of C++ SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C++ SAST rules to run (default: empty, which runs every bundled C++ rule)" required: false - default: "c-buffer-overflow,c-format-string-vulnerability,c-command-injection,c-sql-injection,c-use-after-free,c-hardcoded-secrets,c-weak-crypto,c-ssl-issues,cpp-unsafe-cast,cpp-memory-management" + default: "" cpp_sast_enabled: description: "Enable C++ SAST scanning" required: false @@ -209,9 +209,9 @@ inputs: required: false default: "" csharp_enabled_rules: - description: "Comma-separated list of C# SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C# SAST rules to run (default: empty, which runs every bundled C# rule)" required: false - default: "dotnet-sql-injection,dotnet-command-injection,dotnet-code-injection,dotnet-unsafe-deserialization,dotnet-ldap-injection,dotnet-hardcoded-credentials,dotnet-ssl-bypass,dotnet-xxe-vulnerability,dotnet-xpath-injection" + default: "" csharp_sast_enabled: description: "Enable C# SAST scanning" required: false @@ -229,9 +229,9 @@ inputs: required: false default: "" dotnet_enabled_rules: - description: "Comma-separated list of .NET SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the .NET SAST rules to run (default: empty, which runs every bundled .NET rule)" required: false - default: "dotnet-sql-injection,dotnet-command-injection,dotnet-code-injection,dotnet-unsafe-deserialization,dotnet-ldap-injection,dotnet-hardcoded-credentials,dotnet-ssl-bypass,dotnet-xxe-vulnerability,dotnet-xpath-injection" + default: "" dotnet_sast_enabled: description: "Enable .NET SAST scanning" required: false @@ -241,9 +241,9 @@ inputs: required: false default: "" elixir_enabled_rules: - description: "Comma-separated list of Elixir SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Elixir SAST rules to run (default: empty, which runs every bundled Elixir rule)" required: false - default: "elixir-code-injection,elixir-sql-injection,elixir-command-injection,elixir-unsafe-deserialization,elixir-hardcoded-secrets,elixir-phoenix-ecto-injection" + default: "" elixir_sast_enabled: description: "Enable Elixir SAST scanning" required: false @@ -257,9 +257,9 @@ inputs: required: false default: "" go_enabled_rules: - description: "Comma-separated list of Go SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Go SAST rules to run (default: empty, which runs every bundled Go rule)" required: false - default: "go-hardcoded-credentials,go-bind-all-interfaces,go-unsafe-usage,go-ssh-insecure-ignore-host-key,go-profiling-endpoint-exposed,go-decompression-bomb,go-sql-format-string" + default: "" go_sast_enabled: description: "Enable Go SAST scanning" required: false @@ -273,9 +273,9 @@ inputs: required: false default: "" java_enabled_rules: - description: "Comma-separated list of Java SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Java SAST rules to run (default: empty, which runs every bundled Java rule)" required: false - default: "java-sql-injection,java-unsafe-deserialization,java-command-injection,java-ldap-injection,java-hardcoded-credentials,java-weak-crypto-md5,java-weak-crypto-sha1,java-xxe-vulnerability,java-ssl-bypass,java-weak-ssl-version,java-jndi-injection,java-el-injection" + default: "" java_sast_enabled: description: "Enable Java SAST scanning" required: false @@ -285,9 +285,9 @@ inputs: required: false default: "" javascript_enabled_rules: - description: "Comma-separated list of JavaScript/TypeScript SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the JavaScript/TypeScript SAST rules to run (default: empty, which runs every bundled JavaScript/TypeScript rule)" required: false - default: "js-eval-usage,js-sql-injection,js-command-injection,js-prototype-pollution,js-xss-innerhtml,js-path-traversal,js-tls-reject-unauthorized-false,js-weak-crypto-md5" + default: "" javascript_sast_enabled: description: "Enable JavaScript/TypeScript SAST scanning" required: false @@ -317,9 +317,9 @@ inputs: required: false default: "" kotlin_enabled_rules: - description: "Comma-separated list of Kotlin SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Kotlin SAST rules to run (default: empty, which runs every bundled Kotlin rule)" required: false - default: "kotlin-sql-injection,kotlin-command-injection,kotlin-code-injection,kotlin-unsafe-deserialization,kotlin-hardcoded-secrets,kotlin-ssl-bypass,kotlin-xxe-vulnerability,kotlin-ldap-injection" + default: "" kotlin_sast_enabled: description: "Enable Kotlin SAST scanning" required: false @@ -357,9 +357,9 @@ inputs: required: false default: "" php_enabled_rules: - description: "Comma-separated list of PHP SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the PHP SAST rules to run (default: empty, which runs every bundled PHP rule)" required: false - default: "php-code-injection,php-sql-injection,php-command-injection,php-unsafe-deserialization,php-ldap-injection,php-file-inclusion,php-xxe-vulnerability,php-hardcoded-secrets,php-path-traversal" + default: "" php_sast_enabled: description: "Enable PHP SAST scanning" required: false @@ -373,9 +373,9 @@ inputs: required: false default: "" python_enabled_rules: - description: "Comma-separated list of Python SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Python SAST rules to run (default: empty, which runs every bundled Python rule)" required: false - default: "python-exec-used,python-bad-file-permissions,python-hardcoded-password-string,python-eval-usage,python-request-without-cert-validation,python-ssl-bad-version,python-subprocess-shell-true,python-start-process-with-shell,python-sql-injection-format,python-flask-debug-true,python-jinja2-autoescape-false" + default: "" python_sast_enabled: description: "Enable Python SAST scanning" required: false @@ -385,9 +385,9 @@ inputs: required: false default: "" ruby_enabled_rules: - description: "Comma-separated list of Ruby SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Ruby SAST rules to run (default: empty, which runs every bundled Ruby rule)" required: false - default: "ruby-eval-injection,ruby-command-injection,ruby-sql-injection,ruby-unsafe-deserialization,ruby-hardcoded-secrets,ruby-ssl-verification-bypass,ruby-rails-find-by-sql-injection,ruby-rails-unsafe-constantize,ruby-xxe-vulnerability" + default: "" ruby_sast_enabled: description: "Enable Ruby SAST scanning" required: false @@ -397,9 +397,9 @@ inputs: required: false default: "" rust_enabled_rules: - description: "Comma-separated list of Rust SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Rust SAST rules to run (default: empty, which runs every bundled Rust rule)" required: false - default: "rust-command-injection,rust-unsafe-deserialization,rust-sql-injection,rust-hardcoded-secrets,rust-insecure-tls,rust-buffer-overflow-potential" + default: "" rust_sast_enabled: description: "Enable Rust SAST scanning" required: false @@ -409,9 +409,9 @@ inputs: required: false default: "" scala_enabled_rules: - description: "Comma-separated list of Scala SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Scala SAST rules to run (default: empty, which runs every bundled Scala rule)" required: false - default: "scala-sql-injection,scala-code-injection,scala-command-injection,scala-unsafe-deserialization,scala-hardcoded-secrets,scala-ssl-bypass,scala-slick-sql-injection" + default: "" scala_sast_enabled: description: "Enable Scala SAST scanning" required: false @@ -445,9 +445,9 @@ inputs: required: false default: "" swift_enabled_rules: - description: "Comma-separated list of Swift SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Swift SAST rules to run (default: empty, which runs every bundled Swift rule)" required: false - default: "swift-sql-injection,swift-command-injection,swift-unsafe-deserialization,swift-hardcoded-secrets,swift-ssl-bypass,swift-insecure-urlsession,swift-certificate-pinning-bypass" + default: "" swift_sast_enabled: description: "Enable Swift SAST scanning" required: false diff --git a/docs/github-action.md b/docs/github-action.md index 64f7bc5..6a31c9b 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -511,7 +511,8 @@ Dockerfiles. The exceptions: - A blank `_enabled_rules` or `_disabled_rules` field is - skipped, so the workflow's value or the action's default list applies. + skipped, so the workflow's value applies. If the workflow sets none, every + bundled rule for the language runs. - A blank notification field (Slack, Jira, Microsoft Teams, generic webhook, Microsoft Sentinel, Sumo Logic) falls back to the workflow's value, and so does a blank GitHub token. A filled-in dashboard field still wins. @@ -556,17 +557,11 @@ directly as a step: ``` Only the flags you pass take precedence. Every other setting still comes from -the dashboard. Running the image directly differs from the action in two ways: - -- The action's default rule lists (`_enabled_rules` in - [`action.yml`](../action.yml)) don't apply. Where the dashboard's rule-list - fields are blank, SAST uses the image's own defaults from - [`connectors.yaml`](../socket_basics/connectors.yaml), which include more rules - for some languages. -- The step isn't tied to an action version, so update the image tag yourself - when you upgrade. To pin by digest, append `@sha256:` to the image. - `docker buildx imagetools inspect ghcr.io/socketdev/socket-basics:3.4.1` - prints the digest. +the dashboard. Running the image directly differs from the action in one way: +the step isn't tied to an action version, so update the image tag yourself +when you upgrade. To pin by digest, append `@sha256:` to the image. +`docker buildx imagetools inspect ghcr.io/socketdev/socket-basics:3.4.1` +prints the digest. See the [Parameters Reference](parameters.md) for every CLI flag. @@ -929,7 +924,7 @@ jobs: all_rules_enabled: 'true' # Optional: enable specific bundled or custom rule IDs. - javascript_enabled_rules: 'eval-usage,prototype-pollution' + javascript_enabled_rules: 'js-code-injection-eval,js-prototype-pollution' # Ignore one or more SAST rules globally or for exact repo-relative files sast_ignore_overrides: 'js-sql-injection:index.js' @@ -942,7 +937,9 @@ Important behavior: `use_custom_sast_rules`, `custom_sast_rule_path` and `sast_ignore_overrides` have no dashboard field and come from the workflow. See [Dashboard Settings and Workflow Inputs](#dashboard-settings-and-workflow-inputs). -- `_enabled_rules` is an allowlist and can suppress custom rule IDs. +- `_enabled_rules` is an allowlist: only the listed rule IDs run, and it + can suppress custom rule IDs. When it is empty (the default), every rule runs. + IDs that match no rule are ignored with a warning in the step log. - `all_rules_enabled: 'true'` disables allowlist filtering for enabled languages. `sast_ignore_overrides` supports: @@ -1000,7 +997,7 @@ Every input has a CLI flag and environment-variable equivalent; the - `opengrep_notification_method` — Route SAST findings to one notifier (e.g. `console`, `slack`) **Rule Configuration (per language):** -- `_enabled_rules` — Comma-separated rules to enable +- `_enabled_rules` — Comma-separated rule IDs to run (default: empty, which runs every bundled rule) - `_disabled_rules` — Comma-separated rules to disable - `use_custom_sast_rules` — Enable custom SAST rule discovery from repo files - `custom_sast_rule_path` — Relative path to custom SAST rule directory diff --git a/docs/parameters.md b/docs/parameters.md index 86b4794..0052534 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -82,7 +82,7 @@ GitHub Action input is delivered to the container as the environment variable | `--use-custom-sast-rules` | `use_custom_sast_rules` | `INPUT_USE_CUSTOM_SAST_RULES` | `use_custom_sast_rules` | Use custom SAST rules instead of bundled rules (falls back to bundled rules for languages without custom rules) | | `--custom-sast-rule-path` | `custom_sast_rule_path` | `INPUT_CUSTOM_SAST_RULE_PATH` | `custom_sast_rule_path` | Relative path to custom SAST rules directory (relative to workspace if set, otherwise cwd) | | `--sast-ignore-overrides` | `sast_ignore_overrides` | `INPUT_SAST_IGNORE_OVERRIDES` | `sast_ignore_overrides` | Comma-separated list of SAST ignore overrides in rule_id or rule_id:path format | -| `---enabled-rules` | `_enabled_rules` | `INPUT__ENABLED_RULES` | `_enabled_rules` | Comma-separated allowlist of rules for one language (defaults to the high-confidence set). | +| `---enabled-rules` | `_enabled_rules` | `INPUT__ENABLED_RULES` | `_enabled_rules` | Comma-separated allowlist of rule IDs for one language. Empty (the default) runs every bundled rule for the language. | | `---disabled-rules` | `_disabled_rules` | `INPUT__DISABLED_RULES` | `_disabled_rules` | Comma-separated rules to disable for one language. | `` is one of: `c`, `cpp`, `csharp`, `dotnet`, `elixir`, `go`, `java`, `javascript`, `kotlin`, `php`, `python`, `ruby`, `rust`, `scala`, `swift`. @@ -500,20 +500,24 @@ line carrying one does not pass it through. ### Language-Specific Rule Configuration -For each language, you can enable or disable specific rules: +For each language, you can enable or disable specific rules. By default every +bundled rule for an enabled language runs. An enabled-rules list narrows the scan +to exactly those rule IDs; IDs that match no bundled rule are ignored with a +warning in the log. Rule IDs are the `id` values in +[`socket_basics/rules/`](../socket_basics/rules/). **Pattern:** `---enabled-rules` or `---disabled-rules` **Examples:** ```bash # Enable specific Python rules -socket-basics --python --python-enabled-rules "sql-injection,xss-detection" +socket-basics --python --python-enabled-rules "python-sql-injection,python-command-injection" # Disable specific JavaScript rules -socket-basics --javascript --javascript-disabled-rules "console-log,debugger-statement" +socket-basics --javascript --javascript-disabled-rules "js-debugger-statement,js-parseint-missing-radix" # Enable specific Go rules -socket-basics --go --go-enabled-rules "error-handling,sql-injection" +socket-basics --go --go-enabled-rules "go-sql-format-string,go-hardcoded-credentials" ``` **Available for:** diff --git a/socket_basics/connectors.yaml b/socket_basics/connectors.yaml index 2ddcf6d..0261809 100644 --- a/socket_basics/connectors.yaml +++ b/socket_basics/connectors.yaml @@ -167,10 +167,10 @@ connectors: # Python rule configuration - name: python_enabled_rules option: --python-enabled-rules - description: "Comma-separated list of Python SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Python SAST rules to run (default: empty, which runs every bundled Python rule)" env_variable: INPUT_PYTHON_ENABLED_RULES type: str - default: "python-path-traversal-open,python-open-redirect,python-weak-hash-md5,python-weak-hash-sha1,python-weak-cipher-des,python-ssl-verify-disabled,python-insecure-tls-version,python-code-injection-eval,python-sql-injection,python-nosql-injection,python-command-injection,python-ldap-injection,python-xss-template,python-template-injection,python-yaml-load-unsafe,python-debug-mode-enabled,python-weak-session-config,python-insecure-file-permissions,python-jwt-no-verify,python-weak-jwt-algorithm,python-plain-text-password,python-unsafe-deserialization,python-ssrf-vulnerability,python-bare-except,python-dangerous-eval-exec,python-sql-format-string,python-insecure-temp-file" + default: "" - name: python_disabled_rules option: --python-disabled-rules description: "Comma-separated list of Python SAST rules to disable" @@ -181,10 +181,10 @@ connectors: # JavaScript/TypeScript rule configuration - name: javascript_enabled_rules option: --javascript-enabled-rules - description: "Comma-separated list of JavaScript/TypeScript SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the JavaScript/TypeScript SAST rules to run (default: empty, which runs every bundled JavaScript/TypeScript rule)" env_variable: INPUT_JAVASCRIPT_ENABLED_RULES type: str - default: "js-code-injection-eval,js-sql-injection,js-nosql-injection,js-command-injection,js-ldap-injection,js-prototype-pollution,js-xss-innerhtml,js-dom-xss,js-path-traversal,js-open-redirect,js-cors-allow-all,js-weak-hash-md5,js-weak-hash-sha1,js-hardcoded-secret,js-weak-cipher,js-tls-reject-unauthorized-false,js-insecure-tls-version,js-jwt-no-verify,js-weak-jwt-secret,js-weak-session-cookie,js-default-credentials,js-error-stack-exposed,js-unsafe-deserialization,js-ssrf-vulnerability,js-react-dangerous-html,js-unsafe-buffer,js-unsafe-file-operations,js-unsafe-process-spawn,js-debugger-statement,js-parseint-missing-radix" + default: "" - name: javascript_disabled_rules option: --javascript-disabled-rules description: "Comma-separated list of JavaScript/TypeScript SAST rules to disable" @@ -201,10 +201,10 @@ connectors: # Go rule configuration - name: go_enabled_rules option: --go-enabled-rules - description: "Comma-separated list of Go SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Go SAST rules to run (default: empty, which runs every bundled Go rule)" env_variable: INPUT_GO_ENABLED_RULES type: str - default: "go-hardcoded-credentials,go-ssh-insecure-ignore-host-key,go-sql-format-string,go-sql-string-concat,go-command-execution,go-file-path-traversal,go-zip-traversal,go-bad-tls-connection,go-import-des,go-import-rc4,go-empty-password,go-sql-direct-concat" + default: "" - name: go_disabled_rules option: --go-disabled-rules description: "Comma-separated list of Go SAST rules to disable" @@ -215,10 +215,10 @@ connectors: # Java rule configuration - name: java_enabled_rules option: --java-enabled-rules - description: "Comma-separated list of Java SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Java SAST rules to run (default: empty, which runs every bundled Java rule)" env_variable: INPUT_JAVA_ENABLED_RULES type: str - default: "java-reflection-injection,java-sql-injection,java-unsafe-deserialization,java-command-injection,java-ldap-injection,java-hardcoded-credentials,java-weak-crypto-md5,java-weak-crypto-sha1,java-insecure-random,java-xxe-vulnerability,java-path-traversal,java-ssl-bypass,java-spring-security-bypass,java-jpa-sql-injection,java-struts-ognl-injection,java-jndi-injection,java-el-injection,java-template-injection,java-file-upload-no-validation" + default: "" - name: java_disabled_rules option: --java-disabled-rules description: "Comma-separated list of Java SAST rules to disable" @@ -229,10 +229,10 @@ connectors: # PHP rule configuration - name: php_enabled_rules option: --php-enabled-rules - description: "Comma-separated list of PHP SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the PHP SAST rules to run (default: empty, which runs every bundled PHP rule)" env_variable: INPUT_PHP_ENABLED_RULES type: str - default: "php-code-injection,php-sql-injection,php-command-injection,php-unsafe-deserialization,php-ldap-injection,php-file-inclusion,php-xss,php-hardcoded-secrets,php-path-traversal,php-open-redirect,php-xxe-vulnerability,php-laravel-security,php-wordpress-security,php-codeigniter-security,php-file-upload-vulnerability,php-insecure-direct-object-reference,php-prepared-statement-injection" + default: "" - name: php_disabled_rules option: --php-disabled-rules description: "Comma-separated list of PHP SAST rules to disable" @@ -243,10 +243,10 @@ connectors: # Ruby rule configuration - name: ruby_enabled_rules option: --ruby-enabled-rules - description: "Comma-separated list of Ruby SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Ruby SAST rules to run (default: empty, which runs every bundled Ruby rule)" env_variable: INPUT_RUBY_ENABLED_RULES type: str - default: "ruby-eval-injection,ruby-command-injection,ruby-sql-injection,ruby-unsafe-deserialization,ruby-dynamic-method-definition,ruby-hardcoded-secrets,ruby-path-traversal,ruby-open-redirect,ruby-xss-raw-output,ruby-ssl-verification-bypass,ruby-weak-crypto,ruby-rails-find-by-sql-injection,ruby-rails-where-injection,ruby-template-injection" + default: "" - name: ruby_disabled_rules option: --ruby-disabled-rules description: "Comma-separated list of Ruby SAST rules to disable" @@ -257,10 +257,10 @@ connectors: # C# rule configuration - name: csharp_enabled_rules option: --csharp-enabled-rules - description: "Comma-separated list of C# SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C# SAST rules to run (default: empty, which runs every bundled C# rule)" env_variable: INPUT_CSHARP_ENABLED_RULES type: str - default: "dotnet-sql-injection,dotnet-command-injection,dotnet-code-injection,dotnet-unsafe-deserialization,dotnet-ldap-injection,dotnet-hardcoded-credentials,dotnet-weak-crypto-md5,dotnet-weak-crypto-sha1,dotnet-ssl-bypass,dotnet-xss-response-write,dotnet-path-traversal,dotnet-insecure-random,dotnet-xxe-vulnerability,dotnet-aspnet-validaterequest-false,dotnet-aspnet-viewstate-mac-disabled,dotnet-aspnetcore-auth-bypass,dotnet-ef-sql-injection,dotnet-broken-access-control,dotnet-crypto-failures,dotnet-xpath-injection,dotnet-auth-failures,dotnet-integrity-failures,dotnet-ssrf,dotnet-unsafe-code" + default: "" - name: csharp_disabled_rules option: --csharp-disabled-rules description: "Comma-separated list of C# SAST rules to disable" @@ -271,10 +271,10 @@ connectors: # .NET rule configuration - name: dotnet_enabled_rules option: --dotnet-enabled-rules - description: "Comma-separated list of .NET SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the .NET SAST rules to run (default: empty, which runs every bundled .NET rule)" env_variable: INPUT_DOTNET_ENABLED_RULES type: str - default: "dotnet-sql-injection,dotnet-command-injection,dotnet-code-injection,dotnet-unsafe-deserialization,dotnet-ldap-injection,dotnet-hardcoded-credentials,dotnet-weak-crypto-md5,dotnet-weak-crypto-sha1,dotnet-ssl-bypass,dotnet-xss-response-write,dotnet-path-traversal,dotnet-insecure-random,dotnet-xxe-vulnerability,dotnet-aspnet-validaterequest-false,dotnet-aspnet-viewstate-mac-disabled,dotnet-aspnetcore-auth-bypass,dotnet-ef-sql-injection,dotnet-broken-access-control,dotnet-crypto-failures,dotnet-xpath-injection,dotnet-auth-failures,dotnet-integrity-failures,dotnet-ssrf,dotnet-unsafe-code" + default: "" - name: dotnet_disabled_rules option: --dotnet-disabled-rules description: "Comma-separated list of .NET SAST rules to disable" @@ -285,10 +285,10 @@ connectors: # C rule configuration - name: c_enabled_rules option: --c-enabled-rules - description: "Comma-separated list of C SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C SAST rules to run (default: empty, which runs every bundled C rule)" env_variable: INPUT_C_ENABLED_RULES type: str - default: "c-buffer-overflow,c-format-string-vulnerability,c-command-injection,c-sql-injection,c-memory-management,c-use-after-free,c-integer-overflow,c-null-pointer-dereference,c-hardcoded-secrets,c-race-condition,cpp-unsafe-cast,cpp-memory-management,c-weak-crypto,c-ssl-issues,c-input-validation" + default: "" - name: c_disabled_rules option: --c-disabled-rules description: "Comma-separated list of C SAST rules to disable" @@ -299,10 +299,10 @@ connectors: # C++ rule configuration - name: cpp_enabled_rules option: --cpp-enabled-rules - description: "Comma-separated list of C++ SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the C++ SAST rules to run (default: empty, which runs every bundled C++ rule)" env_variable: INPUT_CPP_ENABLED_RULES type: str - default: "c-buffer-overflow,c-format-string-vulnerability,c-command-injection,c-sql-injection,c-memory-management,c-use-after-free,c-integer-overflow,c-null-pointer-dereference,c-hardcoded-secrets,c-race-condition,cpp-unsafe-cast,cpp-memory-management,c-weak-crypto,c-ssl-issues,c-input-validation" + default: "" - name: cpp_disabled_rules option: --cpp-disabled-rules description: "Comma-separated list of C++ SAST rules to disable" @@ -313,10 +313,10 @@ connectors: # Kotlin rule configuration - name: kotlin_enabled_rules option: --kotlin-enabled-rules - description: "Comma-separated list of Kotlin SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Kotlin SAST rules to run (default: empty, which runs every bundled Kotlin rule)" env_variable: INPUT_KOTLIN_ENABLED_RULES type: str - default: "kotlin-sql-injection,kotlin-command-injection,kotlin-code-injection,kotlin-unsafe-deserialization,kotlin-hardcoded-secrets,kotlin-weak-crypto-md5,kotlin-weak-crypto-sha1,kotlin-ssl-bypass,kotlin-path-traversal,kotlin-xss,kotlin-android-intent-user-input,kotlin-spring-security-bypass,kotlin-jpa-query-injection,kotlin-template-injection,kotlin-ldap-injection,kotlin-file-upload-no-validation,kotlin-xxe-vulnerability" + default: "" - name: kotlin_disabled_rules option: --kotlin-disabled-rules description: "Comma-separated list of Kotlin SAST rules to disable" @@ -327,10 +327,10 @@ connectors: # Scala rule configuration - name: scala_enabled_rules option: --scala-enabled-rules - description: "Comma-separated list of Scala SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Scala SAST rules to run (default: empty, which runs every bundled Scala rule)" env_variable: INPUT_SCALA_ENABLED_RULES type: str - default: "scala-sql-injection,scala-code-injection,scala-command-injection,scala-unsafe-deserialization,scala-hardcoded-secrets,scala-weak-crypto-md5,scala-weak-crypto-sha1,scala-ssl-bypass,scala-path-traversal,scala-xss-template,scala-slick-sql-injection,scala-json-injection,scala-file-upload-no-validation" + default: "" - name: scala_disabled_rules option: --scala-disabled-rules description: "Comma-separated list of Scala SAST rules to disable" @@ -341,10 +341,10 @@ connectors: # Swift rule configuration - name: swift_enabled_rules option: --swift-enabled-rules - description: "Comma-separated list of Swift SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Swift SAST rules to run (default: empty, which runs every bundled Swift rule)" env_variable: INPUT_SWIFT_ENABLED_RULES type: str - default: "swift-sql-injection,swift-command-injection,swift-unsafe-deserialization,swift-hardcoded-secrets,swift-weak-crypto-md5,swift-weak-crypto-sha1,swift-ssl-bypass,swift-path-traversal,swift-webview-xss,swift-ios-insecure-storage,swift-macos-privilege-escalation,swift-macos-code-signing,swift-certificate-pinning-bypass,swift-core-data-injection" + default: "" - name: swift_disabled_rules option: --swift-disabled-rules description: "Comma-separated list of Swift SAST rules to disable" @@ -355,10 +355,10 @@ connectors: # Rust rule configuration - name: rust_enabled_rules option: --rust-enabled-rules - description: "Comma-separated list of Rust SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Rust SAST rules to run (default: empty, which runs every bundled Rust rule)" env_variable: INPUT_RUST_ENABLED_RULES type: str - default: "rust-unsafe-usage,rust-command-injection,rust-unsafe-deserialization,rust-sql-injection,rust-hardcoded-secrets,rust-weak-crypto-md5,rust-weak-crypto-sha1,rust-insecure-tls,rust-path-traversal,rust-buffer-overflow-potential,rust-diesel-injection,rust-serde-security,rust-memory-safety" + default: "" - name: rust_disabled_rules option: --rust-disabled-rules description: "Comma-separated list of Rust SAST rules to disable" @@ -369,10 +369,10 @@ connectors: # Elixir rule configuration - name: elixir_enabled_rules option: --elixir-enabled-rules - description: "Comma-separated list of Elixir SAST rules to enable (default: high-confidence rules)" + description: "Comma-separated IDs of the Elixir SAST rules to run (default: empty, which runs every bundled Elixir rule)" env_variable: INPUT_ELIXIR_ENABLED_RULES type: str - default: "elixir-code-injection,elixir-sql-injection,elixir-command-injection,elixir-unsafe-deserialization,elixir-hardcoded-secrets,elixir-weak-crypto,elixir-path-traversal,elixir-xss,elixir-open-redirect,elixir-phoenix-xss,elixir-phoenix-ecto-injection,elixir-phoenix-file-upload" + default: "" - name: elixir_disabled_rules option: --elixir-disabled-rules description: "Comma-separated list of Elixir SAST rules to disable" diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 6b8c0da..fcfc0fb 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -18,6 +18,10 @@ TRUE_STRINGS = ('true', '1', 'yes', 'on') FALSE_STRINGS = ('false', '0', 'no', 'off') +# Bundled rule files for languages OpenGrep cannot parse. Loading one makes +# OpenGrep report an UnknownLanguageError, so an all-languages scan skips them. +UNSUPPORTED_OPENGREP_RULE_FILES = frozenset({'erlang.yml', 'objective-c.yml'}) + def coerce_bool(value: Any, default: bool) -> bool: """Coerce a config value to a bool, tolerating the string forms. @@ -525,7 +529,7 @@ def should_run_sast(self) -> bool: def build_opengrep_rules(self) -> List[str]: """Build list of rule files based on enabled languages""" # Explicit mapping from connector boolean flags to bundled rule filenames. - # Exclude languages for which we don't ship rule files (objective-c, erlang). + # Erlang and Objective-C are left out: see UNSUPPORTED_OPENGREP_RULE_FILES. language_rules = { 'python_sast_enabled': 'python.yml', 'go_sast_enabled': 'go.yml', @@ -565,8 +569,7 @@ def build_opengrep_rules(self) -> List[str]: base_dir = Path(__file__).parent.parent rules_dir = base_dir / 'rules' found = [p.name for p in rules_dir.glob('*.yml') if p.is_file()] - excluded = {'objective-c.yml', 'erlang.yml'} - return [f for f in found if f not in excluded] + return [f for f in found if f not in UNSUPPORTED_OPENGREP_RULE_FILES] except Exception: return list({v for v in language_rules.values()}) # No languages enabled -> nothing to run diff --git a/socket_basics/core/connector/opengrep/__init__.py b/socket_basics/core/connector/opengrep/__init__.py index 80f8630..3c188b0 100644 --- a/socket_basics/core/connector/opengrep/__init__.py +++ b/socket_basics/core/connector/opengrep/__init__.py @@ -2,21 +2,24 @@ """ OpenGrep SAST Scanner Connector -This connector composes a single OpenGrep command that includes full -language rule files and appends --exclude-rule flags for any rules that -should be skipped. It intentionally does not attempt per-rule fallbacks. +This connector composes a single OpenGrep command from the enabled language +rule files. When a language has an enabled-rule allowlist, it passes a +temporary copy of that rule file holding only the enabled rules. It +intentionally does not attempt per-rule fallbacks. """ import json import logging import os +import shutil import subprocess import tempfile import yaml from pathlib import Path -from typing import Dict, Any, List +from typing import Dict, Any, List, Optional from ..base import BaseConnector +from ...config import UNSUPPORTED_OPENGREP_RULE_FILES # Opengrep produces canonical components/notifications directly # Import individual notifier modules @@ -41,6 +44,15 @@ def __init__(self, config): super().__init__(config) def scan(self) -> Dict[str, Any]: + # Temporary rule directories written during the scan, removed once it ends. + self._temp_rule_dirs: List[Path] = [] + try: + return self._scan() + finally: + for temp_dir in self._temp_rule_dirs: + shutil.rmtree(temp_dir, ignore_errors=True) + + def _scan(self) -> Dict[str, Any]: # Determine enabled rule files try: rule_files = self.config.build_opengrep_rules() or [] @@ -80,7 +92,7 @@ def scan(self) -> Dict[str, Any]: rule_files = [ p.name for p in Path(rules_dir).glob('*.yml') - if p.name != 'tests.yml' + if p.name != 'tests.yml' and p.name not in UNSUPPORTED_OPENGREP_RULE_FILES ] logger.info("Expanded all-languages scan to rule files: %s", rule_files) except Exception: @@ -101,7 +113,11 @@ def scan(self) -> Dict[str, Any]: logger.info(f"Custom SAST rules enabled, loading from: {custom_rules_path}") try: builder = CustomRulesBuilder(custom_rules_path) - custom_rule_files = builder.build_rule_files(rule_files) + try: + custom_rule_files = builder.build_rule_files(rule_files) + finally: + if builder.temp_dir is not None: + self._temp_rule_dirs.append(builder.temp_dir) except Exception as e: logger.error(f"Failed to build custom rule files: {e}", exc_info=True) custom_rule_files = {} @@ -128,12 +144,12 @@ def scan(self) -> Dict[str, Any]: filtered = {} config_args: List[str] = [] - + filtered_rules_dir: Optional[Path] = None + # Build config_args using custom rules where available, falling back to bundled rules # Process all enabled languages - use filtered rules if specified, otherwise use all rules for rf in rule_files: # Check if we have a custom rule file for this language - using_custom_rules = bool(custom_rule_files and rf in custom_rule_files) if custom_rule_files and rf in custom_rule_files: p = custom_rule_files[rf] logger.info("Using custom rules for %s from %s", rf, p) @@ -147,33 +163,43 @@ def scan(self) -> Dict[str, Any]: # Check if this language has specific rules enabled (filtered mode) if filtered and rf in filtered: - enabled_ids = filtered[rf] + enabled_ids = set(filtered[rf] or []) logger.info("Filtering rules for %s: %d enabled IDs configured", rf, len(enabled_ids)) try: with open(p, 'r') as fh: data = yaml.safe_load(fh) or {} - all_ids = [r.get('id') for r in (data.get('rules') or []) if r.get('id')] - # Custom-rule mode can coexist with legacy bundled allowlists. - # If none of the configured enabled IDs match custom IDs, keep all - # custom IDs active to avoid silently disabling user-authored rules. - if using_custom_rules: - matched_enabled_ids = [rid for rid in all_ids if rid in (enabled_ids or [])] - if enabled_ids and not matched_enabled_ids: - logger.warning( - "No configured enabled-rule IDs matched custom rules for %s; using all custom rules from %s", - rf, - p, - ) - config_args.extend(['--config', str(p)]) - continue - to_exclude = [rid for rid in all_ids if rid not in matched_enabled_ids] - else: - to_exclude = [rid for rid in all_ids if rid not in (enabled_ids or [])] - config_args.extend(['--config', str(p)]) - for ex in to_exclude: - config_args.extend(['--exclude-rule', ex]) + rules = [r for r in (data.get('rules') or []) if isinstance(r, dict)] + kept = [r for r in rules if r.get('id') in enabled_ids] + unknown_ids = sorted(enabled_ids - {r.get('id') for r in rules}) + if not kept: + # An allowlist that matches nothing is almost always stale or + # misspelled IDs. Running every rule keeps coverage instead of + # silently scanning nothing for this language. + logger.warning( + "None of the %d enabled-rule IDs configured for %s match a rule in %s; running all %d rules. Unmatched IDs: %s", + len(enabled_ids), rf, p, len(rules), ', '.join(unknown_ids), + ) + config_args.extend(['--config', str(p)]) + continue + if unknown_ids: + logger.warning( + "Enabled-rule IDs for %s that match no rule in %s and will be ignored: %s", + rf, p, ', '.join(unknown_ids), + ) + # Pass a copy of the file that holds only the enabled rules. + # OpenGrep prefixes rule IDs from a --config path with the path's + # directories, so --exclude-rule never matches a bare rule ID. + if filtered_rules_dir is None: + filtered_rules_dir = Path(tempfile.mkdtemp(prefix='socket_filtered_rules_')) + self._temp_rule_dirs.append(filtered_rules_dir) + filtered_path = filtered_rules_dir / rf + with open(filtered_path, 'w', encoding='utf-8') as fh: + yaml.safe_dump({**data, 'rules': kept}, fh, sort_keys=False, allow_unicode=True) + logger.info("Enabled %d of %d rules for %s", len(kept), len(rules), rf) + config_args.extend(['--config', str(filtered_path)]) except Exception: - logger.debug('Failed reading/parsing rule file %s', p, exc_info=True) + logger.warning('Failed applying enabled-rule filter to %s; running all rules from it', p, exc_info=True) + config_args.extend(['--config', str(p)]) else: # No specific rules configured - use all rules from this file logger.debug(f"Using all rules for {rf}") @@ -201,7 +227,7 @@ def scan(self) -> Dict[str, Any]: try: for p in Path(rules_dir).glob('*.yml'): # Skip tests.yml from community rules - if p.name == 'tests.yml': + if p.name == 'tests.yml' or p.name in UNSUPPORTED_OPENGREP_RULE_FILES: continue config_args.extend(['--config', str(p)]) except Exception: diff --git a/socket_basics/core/connector/opengrep/custom_rules.py b/socket_basics/core/connector/opengrep/custom_rules.py index a13fe3f..24f5338 100644 --- a/socket_basics/core/connector/opengrep/custom_rules.py +++ b/socket_basics/core/connector/opengrep/custom_rules.py @@ -202,13 +202,3 @@ def _write_rule_files(self, rules_by_file: Dict[str, List[Dict[str, Any]]]) -> D continue return custom_rule_paths - - def cleanup(self) -> None: - """Clean up temporary rule files (optional, as temp files are typically auto-cleaned).""" - if self.temp_dir and self.temp_dir.exists(): - try: - import shutil - shutil.rmtree(self.temp_dir) - logger.debug(f"Cleaned up temporary rule directory: {self.temp_dir}") - except Exception as e: - logger.warning(f"Failed to clean up temporary rule directory {self.temp_dir}: {e}") diff --git a/tests/test_action_inputs_consistency.py b/tests/test_action_inputs_consistency.py index b587246..ce391a3 100644 --- a/tests/test_action_inputs_consistency.py +++ b/tests/test_action_inputs_consistency.py @@ -67,3 +67,31 @@ def test_input_names_used_in_the_docs_are_declared() -> None: "scan_files", } assert documented <= inputs, f"undeclared: {sorted(documented - inputs)}" + + +def _bundled_rule_ids() -> set[str]: + ids: set[str] = set() + for path in (ROOT / "socket_basics" / "rules").glob("*.yml"): + for rule in (yaml.safe_load(path.read_text()) or {}).get("rules") or []: + ids.add(rule["id"]) + return ids + + +def test_default_rule_lists_name_only_existing_rules() -> None: + """A renamed rule must not leave a dead ID in a default allowlist.""" + known = _bundled_rule_ids() + defaults = { + f"action.yml:{name}": spec.get("default") or "" + for name, spec in _action()["inputs"].items() + if name.endswith(("_enabled_rules", "_disabled_rules")) + } + connectors = yaml.safe_load((ROOT / "socket_basics" / "connectors.yaml").read_text()) + for cfg in connectors["connectors"].values(): + for param in cfg.get("parameters") or []: + if param["name"].endswith(("_enabled_rules", "_disabled_rules")): + defaults[f"connectors.yaml:{param['name']}"] = param.get("default") or "" + unknown = { + where: sorted({i.strip() for i in value.split(",") if i.strip()} - known) + for where, value in defaults.items() + } + assert {k: v for k, v in unknown.items() if v} == {} diff --git a/tests/test_opengrep_custom_rules.py b/tests/test_opengrep_custom_rules.py index 3a85fad..5e9f02a 100644 --- a/tests/test_opengrep_custom_rules.py +++ b/tests/test_opengrep_custom_rules.py @@ -2,6 +2,8 @@ from pathlib import Path from types import SimpleNamespace +import yaml + from socket_basics.core.config import Config from socket_basics.core.connector.opengrep import OpenGrepScanner @@ -28,9 +30,18 @@ def _write_custom_rules_file(path: Path, rule_ids: list[str]) -> None: path.write_text("\n".join(lines), encoding="utf-8") -def _mock_subprocess_run(monkeypatch, captured_cmd: list[str]): +def _mock_subprocess_run(monkeypatch, captured_cmd: list[str], configs: dict | None = None): + """Record the command. When ``configs`` is given, also record the rule IDs + in each ``--config`` file, keyed by path, while the files still exist.""" + def _runner(cmd, capture_output, text): captured_cmd.extend(cmd) + if configs is not None: + for i, arg in enumerate(cmd): + if arg == "--config": + path = Path(cmd[i + 1]) + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + configs[str(path)] = [r["id"] for r in data.get("rules") or []] out_file = cmd[cmd.index("--output") + 1] Path(out_file).write_text(json.dumps({"results": []}), encoding="utf-8") return SimpleNamespace(stdout="", stderr="", returncode=0) @@ -176,13 +187,15 @@ def test_custom_rules_ignore_nonmatching_bundled_allowlist_ids(tmp_path, monkeyp scanner.generate_notifications = lambda _: {} captured_cmd: list[str] = [] - _mock_subprocess_run(monkeypatch, captured_cmd) + configs: dict = {} + _mock_subprocess_run(monkeypatch, captured_cmd, configs) scanner.scan() - cmd_str = " ".join(captured_cmd) - assert "socket_custom_rules_" in cmd_str - assert "--exclude-rule org.no-eval" not in cmd_str - assert "--exclude-rule org.no-innerhtml" not in cmd_str + # No allowlist ID names a custom rule, so every custom rule runs. + assert "--exclude-rule" not in captured_cmd + [(path, ids)] = configs.items() + assert "socket_custom_rules_" in path + assert sorted(ids) == ["org.no-eval", "org.no-innerhtml"] def test_custom_rules_apply_allowlist_when_custom_ids_match(tmp_path, monkeypatch): @@ -214,9 +227,155 @@ def test_custom_rules_apply_allowlist_when_custom_ids_match(tmp_path, monkeypatc scanner.generate_notifications = lambda _: {} captured_cmd: list[str] = [] - _mock_subprocess_run(monkeypatch, captured_cmd) + configs: dict = {} + _mock_subprocess_run(monkeypatch, captured_cmd, configs) scanner.scan() - cmd_str = " ".join(captured_cmd) - assert "--exclude-rule org.no-innerhtml" in cmd_str - assert "--exclude-rule org.no-eval" not in cmd_str + assert "--exclude-rule" not in captured_cmd + [(path, ids)] = configs.items() + assert "socket_filtered_rules_" in path + assert ids == ["org.no-eval"] + + +def _bundled_js_scanner(tmp_path, rule_ids: list[str], enabled: str) -> tuple[OpenGrepScanner, Path]: + workspace = tmp_path / "workspace" + workspace.mkdir(parents=True, exist_ok=True) + bundled_rules_dir = tmp_path / "bundled-rules" + bundled_file = bundled_rules_dir / "javascript_typescript.yml" + _write_rule_file(bundled_file, rule_ids) + config = Config( + { + "workspace": str(workspace), + "output_dir": str(workspace), + "javascript_sast_enabled": True, + "javascript_enabled_rules": enabled, + "opengrep_rules_dir": str(bundled_rules_dir), + "all_languages_enabled": False, + "all_rules_enabled": False, + "verbose": False, + } + ) + scanner = OpenGrepScanner(config) + scanner._convert_to_socket_facts = lambda _: {"components": []} + scanner.generate_notifications = lambda _: {} + return scanner, bundled_file + + +def test_bundled_allowlist_passes_only_enabled_rules(tmp_path, monkeypatch): + scanner, bundled_file = _bundled_js_scanner(tmp_path, ["js-a", "js-b", "js-c"], "js-a,js-c") + + captured_cmd: list[str] = [] + configs: dict = {} + _mock_subprocess_run(monkeypatch, captured_cmd, configs) + scanner.scan() + + assert "--exclude-rule" not in captured_cmd + assert str(bundled_file) not in captured_cmd + [(path, ids)] = configs.items() + assert ids == ["js-a", "js-c"] + # The filtered copy is removed once OpenGrep has run. + assert not Path(path).parent.exists() + + +def test_bundled_allowlist_warns_about_unknown_ids(tmp_path, monkeypatch, caplog): + scanner, _ = _bundled_js_scanner(tmp_path, ["js-a", "js-b"], "js-a,js-renamed-long-ago") + + configs: dict = {} + _mock_subprocess_run(monkeypatch, [], configs) + with caplog.at_level("WARNING"): + scanner.scan() + + [ids] = configs.values() + assert ids == ["js-a"] + assert "js-renamed-long-ago" in caplog.text + + +def test_bundled_allowlist_matching_nothing_runs_every_rule(tmp_path, monkeypatch, caplog): + scanner, bundled_file = _bundled_js_scanner(tmp_path, ["js-a", "js-b"], "js-typo") + + captured_cmd: list[str] = [] + configs: dict = {} + _mock_subprocess_run(monkeypatch, captured_cmd, configs) + with caplog.at_level("WARNING"): + scanner.scan() + + assert list(configs) == [str(bundled_file)] + assert "js-typo" in caplog.text + + +def test_all_rules_enabled_ignores_allowlist(tmp_path, monkeypatch): + scanner, bundled_file = _bundled_js_scanner(tmp_path, ["js-a", "js-b"], "js-a") + scanner.config.set("all_rules_enabled", True) + + configs: dict = {} + _mock_subprocess_run(monkeypatch, [], configs) + scanner.scan() + + assert configs == {str(bundled_file): ["js-a", "js-b"]} + + +def test_custom_rule_files_are_removed_after_the_scan(tmp_path, monkeypatch): + workspace = tmp_path / "workspace" + workspace.mkdir(parents=True, exist_ok=True) + _write_custom_rules_file(workspace / ".socket" / "rules" / "org-rules.yml", ["org.no-eval"]) + bundled_rules_dir = tmp_path / "bundled-rules" + _write_rule_file(bundled_rules_dir / "javascript_typescript.yml", ["js-default-rule"]) + + config = Config( + { + "workspace": str(workspace), + "output_dir": str(workspace), + "javascript_sast_enabled": True, + "use_custom_sast_rules": True, + "custom_sast_rule_path": ".socket/rules", + "opengrep_rules_dir": str(bundled_rules_dir), + "all_languages_enabled": False, + "all_rules_enabled": False, + "verbose": False, + } + ) + scanner = OpenGrepScanner(config) + scanner._convert_to_socket_facts = lambda _: {"components": []} + scanner.generate_notifications = lambda _: {} + + configs: dict = {} + _mock_subprocess_run(monkeypatch, [], configs) + scanner.scan() + + [path] = configs + assert "socket_custom_rules_" in path + assert not Path(path).parent.exists() + + +def test_all_languages_scan_skips_rule_files_opengrep_cannot_load(tmp_path, monkeypatch): + workspace = tmp_path / "workspace" + workspace.mkdir(parents=True, exist_ok=True) + bundled_rules_dir = tmp_path / "bundled-rules" + for name in ("python.yml", "erlang.yml", "objective-c.yml"): + _write_rule_file(bundled_rules_dir / name, [f"{name}-rule"]) + + config = Config( + { + "workspace": str(workspace), + "output_dir": str(workspace), + "opengrep_rules_dir": str(bundled_rules_dir), + "all_languages_enabled": True, + "all_rules_enabled": False, + "verbose": False, + } + ) + scanner = OpenGrepScanner(config) + scanner._convert_to_socket_facts = lambda _: {"components": []} + scanner.generate_notifications = lambda _: {} + + configs: dict = {} + _mock_subprocess_run(monkeypatch, [], configs) + scanner.scan() + + assert [Path(p).name for p in configs] == ["python.yml"] + + +def test_all_rules_and_languages_skip_rule_files_opengrep_cannot_load(): + rule_files = Config({"all_rules_enabled": True, "all_languages_enabled": True}).build_opengrep_rules() + assert "python.yml" in rule_files + assert not {"erlang.yml", "objective-c.yml"} & set(rule_files) diff --git a/tests/test_opengrep_rule_filter_e2e.py b/tests/test_opengrep_rule_filter_e2e.py new file mode 100644 index 0000000..dd3495e --- /dev/null +++ b/tests/test_opengrep_rule_filter_e2e.py @@ -0,0 +1,109 @@ +"""End-to-end check that ``_enabled_rules`` narrows a real OpenGrep run. + +Asserting on the command line is not enough here: OpenGrep prefixes the ID of +every rule it loads from a ``--config`` path with that path's directories, so a +command that looks right can still run every rule. These tests run the real +binary on the bundled JavaScript rules and check which rules report findings. + +Skipped when ``opengrep`` is not on PATH, unless CI requires it. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +from socket_basics.core.config import Config +from socket_basics.core.connector.opengrep import OpenGrepScanner + +REQUIRE_OPENGREP_ENV = "SOCKET_BASICS_REQUIRE_OPENGREP" + +_HAVE_OPENGREP = shutil.which("opengrep") is not None +if not _HAVE_OPENGREP and os.environ.get(REQUIRE_OPENGREP_ENV): + pytest.fail( + f"{REQUIRE_OPENGREP_ENV} is set but opengrep is not on PATH", pytrace=False + ) + +pytestmark = pytest.mark.skipif( + not _HAVE_OPENGREP, + reason="opengrep is not installed; rule filter end-to-end tests skipped", +) + +# Reports js-code-injection-eval, js-weak-hash-md5, js-debugger-statement and +# others with the bundled javascript_typescript.yml rules. +APP_JS = """\ +const crypto = require("crypto"); +function handle(req) { + eval(req.query.code); + return crypto.createHash("md5").update(req.body).digest("hex"); +} +debugger; +""" + +_real_run = subprocess.run + + +def _scan(tmp_path: Path, monkeypatch, **overrides) -> set[str]: + """Run the OpenGrep connector over APP_JS and return the reported rule IDs.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + (workspace / "app.js").write_text(APP_JS, encoding="utf-8") + + reported: set[str] = set() + + def _run_and_record(cmd, **kwargs): + result = _real_run(cmd, **kwargs) + out_file = Path(cmd[cmd.index("--output") + 1]) + for r in json.loads(out_file.read_text() or "{}").get("results", []): + reported.add(r["check_id"].rsplit(".", 1)[-1]) + return result + + monkeypatch.setattr( + "socket_basics.core.connector.opengrep.subprocess.run", _run_and_record + ) + + config = Config( + { + "workspace": str(workspace), + "output_dir": str(workspace), + "javascript_sast_enabled": True, + "all_languages_enabled": False, + "all_rules_enabled": False, + "verbose": False, + **overrides, + } + ) + scanner = OpenGrepScanner(config) + scanner._convert_to_socket_facts = lambda _: {"components": []} + scanner.generate_notifications = lambda _: {} + scanner.scan() + return reported + + +def test_without_an_allowlist_every_matching_rule_reports(tmp_path, monkeypatch): + reported = _scan(tmp_path, monkeypatch, javascript_enabled_rules="") + assert {"js-code-injection-eval", "js-weak-hash-md5", "js-debugger-statement"} <= reported + + +def test_allowlist_limits_findings_to_enabled_rules(tmp_path, monkeypatch): + reported = _scan( + tmp_path, + monkeypatch, + javascript_enabled_rules="js-code-injection-eval,js-weak-hash-md5", + ) + assert reported == {"js-code-injection-eval", "js-weak-hash-md5"} + + +def test_all_rules_enabled_overrides_the_allowlist(tmp_path, monkeypatch): + reported = _scan( + tmp_path, + monkeypatch, + javascript_enabled_rules="js-code-injection-eval", + all_rules_enabled=True, + ) + assert "js-debugger-statement" in reported From 97d11b082c9befa722a88afea49bb7ce92c25eb2 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 13 Nov 2026 22:46:50 -0500 Subject: [PATCH 2/3] fix(scanner): Keep SAST fingerprints stable across rule file locations OpenGrep computes a finding's fingerprint from its rule ID, and by default it prefixes that ID with the directories of the rule file's --config path. Filtered and custom rule files live in a new temporary directory on every run, so unchanged findings got a new fingerprint each scan, which breaks deduplication in the JSON and Microsoft Sentinel outputs. Pass --no-rewrite-rule-ids so the fingerprint depends only on the rule and the code. Also test that temporary rule directories are removed when OpenGrep is missing, when the scan raises, and when building custom rules fails. --- CHANGELOG.md | 7 ++ .../core/connector/opengrep/__init__.py | 10 ++- tests/test_opengrep_custom_rules.py | 90 +++++++++++++++++++ tests/test_opengrep_rule_filter_e2e.py | 32 +++++-- 4 files changed, 127 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c7faa6..a02b77e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). so expect fewer SAST findings. Clear the list or set `all_rules_enabled: 'true'` to keep running every rule. The step log warns about listed IDs that match no rule. See the Fixed entry below. +- **SAST finding fingerprints change once.** OpenGrep computed each finding's + `fingerprint` from a rule ID that included the rules file's directory path, + so it depended on where the rules were installed, and custom-rule findings + got a new fingerprint on every run. Fingerprints now depend only on the rule + and the code, so every SAST finding gets a new fingerprint on the first scan + after upgrading. Tools that deduplicate on the fingerprint, such as Microsoft + Sentinel or JSON output consumers, will see each finding as new once. - **Socket Tier 1 needs outbound access to `api.socket.dev` and `storage.googleapis.com`.** Socket npm CLI 1.9.0 and later download the Coana reachability engine from Socket instead of npm, with no npm fallback. diff --git a/socket_basics/core/connector/opengrep/__init__.py b/socket_basics/core/connector/opengrep/__init__.py index 3c188b0..f350f07 100644 --- a/socket_basics/core/connector/opengrep/__init__.py +++ b/socket_basics/core/connector/opengrep/__init__.py @@ -187,8 +187,8 @@ def _scan(self) -> Dict[str, Any]: rf, p, ', '.join(unknown_ids), ) # Pass a copy of the file that holds only the enabled rules. - # OpenGrep prefixes rule IDs from a --config path with the path's - # directories, so --exclude-rule never matches a bare rule ID. + # --exclude-rule is not used: it applies to every loaded file, so + # it would also drop a same-named rule from another language. if filtered_rules_dir is None: filtered_rules_dir = Path(tempfile.mkdtemp(prefix='socket_filtered_rules_')) self._temp_rule_dirs.append(filtered_rules_dir) @@ -245,7 +245,11 @@ def _scan(self) -> Dict[str, Any]: with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as tf: out_file = tf.name - cmd = ['opengrep', '--json', '--dataflow-traces', '--output', out_file] + # By default OpenGrep prefixes each rule ID with the directories of its + # --config path, and the finding fingerprint is computed from that ID. + # Rule files can live in per-run temporary directories, so keep the IDs + # bare to give the same finding the same fingerprint on every run. + cmd = ['opengrep', '--json', '--dataflow-traces', '--no-rewrite-rule-ids', '--output', out_file] verbose = self.config.get('verbose', False) cmd.append('--verbose' if verbose else '--quiet') diff --git a/tests/test_opengrep_custom_rules.py b/tests/test_opengrep_custom_rules.py index 5e9f02a..636e6a3 100644 --- a/tests/test_opengrep_custom_rules.py +++ b/tests/test_opengrep_custom_rules.py @@ -1,7 +1,9 @@ import json +import tempfile from pathlib import Path from types import SimpleNamespace +import pytest import yaml from socket_basics.core.config import Config @@ -379,3 +381,91 @@ def test_all_rules_and_languages_skip_rule_files_opengrep_cannot_load(): rule_files = Config({"all_rules_enabled": True, "all_languages_enabled": True}).build_opengrep_rules() assert "python.yml" in rule_files assert not {"erlang.yml", "objective-c.yml"} & set(rule_files) + + +def _record_temp_dirs(monkeypatch) -> list[Path]: + """Record every directory ``tempfile.mkdtemp`` creates during the test.""" + created: list[Path] = [] + real_mkdtemp = tempfile.mkdtemp + + def _mkdtemp(*args, **kwargs): + path = real_mkdtemp(*args, **kwargs) + created.append(Path(path)) + return path + + monkeypatch.setattr(tempfile, "mkdtemp", _mkdtemp) + return created + + +def _custom_and_filtered_scanner(tmp_path) -> OpenGrepScanner: + """Scanner that writes both a custom-rule and a filtered-rule directory.""" + workspace = tmp_path / "workspace" + workspace.mkdir(parents=True, exist_ok=True) + _write_custom_rules_file(workspace / ".socket" / "rules" / "org-rules.yml", ["org.no-eval", "org.no-innerhtml"]) + bundled_rules_dir = tmp_path / "bundled-rules" + _write_rule_file(bundled_rules_dir / "javascript_typescript.yml", ["js-default-rule"]) + config = Config( + { + "workspace": str(workspace), + "output_dir": str(workspace), + "javascript_sast_enabled": True, + "javascript_enabled_rules": "org.no-eval", + "use_custom_sast_rules": True, + "custom_sast_rule_path": ".socket/rules", + "opengrep_rules_dir": str(bundled_rules_dir), + "all_languages_enabled": False, + "all_rules_enabled": False, + "verbose": False, + } + ) + scanner = OpenGrepScanner(config) + scanner._convert_to_socket_facts = lambda _: {"components": []} + scanner.generate_notifications = lambda _: {} + return scanner + + +def test_temp_rule_dirs_are_removed_when_opengrep_is_missing(tmp_path, monkeypatch): + created = _record_temp_dirs(monkeypatch) + scanner = _custom_and_filtered_scanner(tmp_path) + + def _missing(cmd, capture_output, text): + raise FileNotFoundError("opengrep") + + monkeypatch.setattr("socket_basics.core.connector.opengrep.subprocess.run", _missing) + assert scanner.scan() == {} + + names = sorted(p.name.split("_rules_")[0] for p in created) + assert names == ["socket_custom", "socket_filtered"] + assert not any(p.exists() for p in created) + + +def test_temp_rule_dirs_are_removed_when_the_scan_raises(tmp_path, monkeypatch): + created = _record_temp_dirs(monkeypatch) + scanner = _custom_and_filtered_scanner(tmp_path) + + def _crash(cmd, capture_output, text): + raise RuntimeError("opengrep crashed") + + monkeypatch.setattr("socket_basics.core.connector.opengrep.subprocess.run", _crash) + with pytest.raises(RuntimeError, match="opengrep crashed"): + scanner.scan() + + assert len(created) == 2 + assert not any(p.exists() for p in created) + + +def test_custom_rule_dir_is_removed_when_building_custom_rules_fails(tmp_path, monkeypatch): + created = _record_temp_dirs(monkeypatch) + scanner = _custom_and_filtered_scanner(tmp_path) + + def _fail(self, rules_by_file): + raise OSError("disk full") + + monkeypatch.setattr( + "socket_basics.core.connector.opengrep.custom_rules.CustomRulesBuilder._write_rule_files", _fail + ) + _mock_subprocess_run(monkeypatch, []) + scanner.scan() + + assert [p.name.split("_rules_")[0] for p in created][:1] == ["socket_custom"] + assert not any(p.exists() for p in created) diff --git a/tests/test_opengrep_rule_filter_e2e.py b/tests/test_opengrep_rule_filter_e2e.py index dd3495e..9acf007 100644 --- a/tests/test_opengrep_rule_filter_e2e.py +++ b/tests/test_opengrep_rule_filter_e2e.py @@ -1,9 +1,9 @@ """End-to-end check that ``_enabled_rules`` narrows a real OpenGrep run. -Asserting on the command line is not enough here: OpenGrep prefixes the ID of -every rule it loads from a ``--config`` path with that path's directories, so a -command that looks right can still run every rule. These tests run the real -binary on the bundled JavaScript rules and check which rules report findings. +Asserting on the command line is not enough here: how OpenGrep matches and +reports rule IDs depends on how it loads the rule files, so a command that looks +right can still run every rule. These tests run the real binary on the bundled +JavaScript rules and check which rules report findings and their fingerprints. Skipped when ``opengrep`` is not on PATH, unless CI requires it. """ @@ -48,19 +48,19 @@ _real_run = subprocess.run -def _scan(tmp_path: Path, monkeypatch, **overrides) -> set[str]: - """Run the OpenGrep connector over APP_JS and return the reported rule IDs.""" +def _scan_findings(tmp_path: Path, monkeypatch, **overrides) -> set[tuple[str, str]]: + """Run the OpenGrep connector over APP_JS; return (rule ID, fingerprint) pairs.""" workspace = tmp_path / "workspace" - workspace.mkdir() + workspace.mkdir(exist_ok=True) (workspace / "app.js").write_text(APP_JS, encoding="utf-8") - reported: set[str] = set() + reported: set[tuple[str, str]] = set() def _run_and_record(cmd, **kwargs): result = _real_run(cmd, **kwargs) out_file = Path(cmd[cmd.index("--output") + 1]) for r in json.loads(out_file.read_text() or "{}").get("results", []): - reported.add(r["check_id"].rsplit(".", 1)[-1]) + reported.add((r["check_id"].rsplit(".", 1)[-1], r["extra"]["fingerprint"])) return result monkeypatch.setattr( @@ -85,6 +85,11 @@ def _run_and_record(cmd, **kwargs): return reported +def _scan(tmp_path: Path, monkeypatch, **overrides) -> set[str]: + """Run the OpenGrep connector over APP_JS and return the reported rule IDs.""" + return {rule for rule, _ in _scan_findings(tmp_path, monkeypatch, **overrides)} + + def test_without_an_allowlist_every_matching_rule_reports(tmp_path, monkeypatch): reported = _scan(tmp_path, monkeypatch, javascript_enabled_rules="") assert {"js-code-injection-eval", "js-weak-hash-md5", "js-debugger-statement"} <= reported @@ -107,3 +112,12 @@ def test_all_rules_enabled_overrides_the_allowlist(tmp_path, monkeypatch): all_rules_enabled=True, ) assert "js-debugger-statement" in reported + + +def test_filtered_scans_give_stable_fingerprints(tmp_path, monkeypatch): + """Each filtered scan writes its rules to a new temporary directory; the + fingerprints of unchanged findings must not depend on that directory.""" + enabled = "js-code-injection-eval,js-weak-hash-md5" + first = _scan_findings(tmp_path, monkeypatch, javascript_enabled_rules=enabled) + second = _scan_findings(tmp_path, monkeypatch, javascript_enabled_rules=enabled) + assert first and first == second From 3a11c653b20fb5b7f16334edf24d35cdc797d5c9 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Sat, 14 Nov 2026 00:37:58 -0500 Subject: [PATCH 3/3] fix(scanner): Keep SAST alert keys stable and honor shared-file lists Socket keys an injected SAST alert on its full props, including the OpenGrep fingerprint, and stores triage against that key. The fingerprint comes from a rule ID that OpenGrep prefixes with the rules file's directories, not its file name. Passing --no-rewrite-rule-ids would have given every SAST alert a new key once, dropping its triage. Drop that flag and write each filtered copy next to the file it was made from, under a hidden .yaml name that the *.yml rule globs skip. Kept findings report the same fingerprint as an unfiltered scan. When the rules directory is not writable, the copy falls back to a temporary directory and the log warns that fingerprints will differ. C and C++ share c_cpp.yml, and C# and .NET share dotnet.yml. When both languages in a pair are enabled and either has an empty list, the shared file now runs every rule instead of applying the other language's list. --- CHANGELOG.md | 15 +- socket_basics/core/config.py | 15 +- .../core/connector/opengrep/__init__.py | 60 +++++--- tests/test_opengrep_custom_rules.py | 130 ++++++++++++++---- tests/test_opengrep_rule_filter_e2e.py | 11 +- 5 files changed, 167 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a02b77e..09cc0ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,13 +15,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). so expect fewer SAST findings. Clear the list or set `all_rules_enabled: 'true'` to keep running every rule. The step log warns about listed IDs that match no rule. See the Fixed entry below. -- **SAST finding fingerprints change once.** OpenGrep computed each finding's - `fingerprint` from a rule ID that included the rules file's directory path, - so it depended on where the rules were installed, and custom-rule findings - got a new fingerprint on every run. Fingerprints now depend only on the rule - and the code, so every SAST finding gets a new fingerprint on the first scan - after upgrading. Tools that deduplicate on the fingerprint, such as Microsoft - Sentinel or JSON output consumers, will see each finding as new once. - **Socket Tier 1 needs outbound access to `api.socket.dev` and `storage.googleapis.com`.** Socket npm CLI 1.9.0 and later download the Coana reachability engine from Socket instead of npm, with no npm fallback. @@ -61,8 +54,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ID of every rule it loads from a rules file with the file's directories, so the `--exclude-rule` flags built from bare rule IDs never matched and every bundled rule ran. The scanner now passes OpenGrep a copy of the rules file - that holds only the enabled rules. Enabled IDs that match no rule are logged - as a warning; if none match, every rule for that language runs (#135). + that holds only the enabled rules, written next to the original so findings + keep the same fingerprint as an unfiltered scan. Enabled IDs that match no + rule are logged as a warning; if none match, every rule for that language + runs (#135). C and C++ share one rules file, as do C# and .NET; when both + languages in a pair are enabled and either has an empty list, every rule in + the shared file runs. - **Behavior change:** the `_enabled_rules` defaults are now empty in both `action.yml` and `connectors.yaml`, so every bundled rule for an enabled language runs. That is what scans already did, because the filter above never diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index fcfc0fb..eafe2ef 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -620,23 +620,24 @@ def build_filtered_opengrep_rules(self) -> Dict[str, List[str]]: } rule_file_filters: Dict[str, set] = {} + # C and C++ share c_cpp.yml, and C# and .NET share dotnet.yml. An enabled + # language with an empty list runs every rule, so the shared file must + # stay unfiltered even when the other language sets a list. + unfiltered_rule_files: set = set() for flag, (rule_file, rules_param) in language_rule_mapping.items(): if not self.get(flag, False): continue # rules_param is the exact parameter name defined in connectors.yaml - enabled_rules = self.get(rules_param, "") + enabled_rules = [s.strip() for s in (self.get(rules_param, "") or "").split(',') if s.strip()] if not enabled_rules: + unfiltered_rule_files.add(rule_file) continue - for r in [s.strip() for s in enabled_rules.split(',') if s.strip()]: - if rule_file not in rule_file_filters: - rule_file_filters[rule_file] = set() - rule_file_filters[rule_file].add(r) + rule_file_filters.setdefault(rule_file, set()).update(enabled_rules) - # Convert sets to lists for output - return {k: list(v) for k, v in rule_file_filters.items()} + return {k: list(v) for k, v in rule_file_filters.items() if k not in unfiltered_rule_files} def get_custom_rules_path(self) -> Optional[Path]: """Get the absolute path to custom SAST rules directory. diff --git a/socket_basics/core/connector/opengrep/__init__.py b/socket_basics/core/connector/opengrep/__init__.py index f350f07..233f267 100644 --- a/socket_basics/core/connector/opengrep/__init__.py +++ b/socket_basics/core/connector/opengrep/__init__.py @@ -16,7 +16,7 @@ import tempfile import yaml from pathlib import Path -from typing import Dict, Any, List, Optional +from typing import Dict, Any, List from ..base import BaseConnector from ...config import UNSUPPORTED_OPENGREP_RULE_FILES @@ -44,13 +44,43 @@ def __init__(self, config): super().__init__(config) def scan(self) -> Dict[str, Any]: - # Temporary rule directories written during the scan, removed once it ends. - self._temp_rule_dirs: List[Path] = [] + # Temporary rule files and directories written during the scan, removed + # once it ends. + self._temp_rule_paths: List[Path] = [] try: return self._scan() finally: - for temp_dir in self._temp_rule_dirs: - shutil.rmtree(temp_dir, ignore_errors=True) + for path in self._temp_rule_paths: + if path.is_dir(): + shutil.rmtree(path, ignore_errors=True) + else: + path.unlink(missing_ok=True) + + def _write_filtered_rule_file(self, source: Path, data: Dict[str, Any], kept: List[Dict[str, Any]]) -> Path: + """Write a copy of ``source`` that holds only the ``kept`` rules. + + The copy goes in the same directory as ``source``. OpenGrep prefixes each + rule ID with the directories of its rules file, not the file name, and + computes finding fingerprints from that ID. Socket includes the + fingerprint in the alert key that triage is stored against, so a copy in + another directory would give every kept finding a new key. The hidden + ``.yaml`` name keeps the copy out of the ``*.yml`` rule-file globs. + """ + try: + fd, name = tempfile.mkstemp(dir=source.parent, prefix='.socket-filtered-', suffix='.yaml') + except OSError: + logger.warning( + 'Cannot write next to %s; using a temporary directory, so fingerprints of its findings will differ from an unfiltered scan', + source, + ) + temp_dir = Path(tempfile.mkdtemp(prefix='socket_filtered_rules_')) + self._temp_rule_paths.append(temp_dir) + fd, name = tempfile.mkstemp(dir=temp_dir, prefix='.socket-filtered-', suffix='.yaml') + path = Path(name) + self._temp_rule_paths.append(path) + with os.fdopen(fd, 'w', encoding='utf-8') as fh: + yaml.safe_dump({**data, 'rules': kept}, fh, sort_keys=False, allow_unicode=True) + return path def _scan(self) -> Dict[str, Any]: # Determine enabled rule files @@ -117,7 +147,7 @@ def _scan(self) -> Dict[str, Any]: custom_rule_files = builder.build_rule_files(rule_files) finally: if builder.temp_dir is not None: - self._temp_rule_dirs.append(builder.temp_dir) + self._temp_rule_paths.append(builder.temp_dir) except Exception as e: logger.error(f"Failed to build custom rule files: {e}", exc_info=True) custom_rule_files = {} @@ -144,7 +174,6 @@ def _scan(self) -> Dict[str, Any]: filtered = {} config_args: List[str] = [] - filtered_rules_dir: Optional[Path] = None # Build config_args using custom rules where available, falling back to bundled rules # Process all enabled languages - use filtered rules if specified, otherwise use all rules @@ -187,14 +216,9 @@ def _scan(self) -> Dict[str, Any]: rf, p, ', '.join(unknown_ids), ) # Pass a copy of the file that holds only the enabled rules. - # --exclude-rule is not used: it applies to every loaded file, so - # it would also drop a same-named rule from another language. - if filtered_rules_dir is None: - filtered_rules_dir = Path(tempfile.mkdtemp(prefix='socket_filtered_rules_')) - self._temp_rule_dirs.append(filtered_rules_dir) - filtered_path = filtered_rules_dir / rf - with open(filtered_path, 'w', encoding='utf-8') as fh: - yaml.safe_dump({**data, 'rules': kept}, fh, sort_keys=False, allow_unicode=True) + # --exclude-rule does not work here: OpenGrep matches it against + # rule IDs prefixed with the rules file's directories. + filtered_path = self._write_filtered_rule_file(p, data, kept) logger.info("Enabled %d of %d rules for %s", len(kept), len(rules), rf) config_args.extend(['--config', str(filtered_path)]) except Exception: @@ -245,11 +269,7 @@ def _scan(self) -> Dict[str, Any]: with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as tf: out_file = tf.name - # By default OpenGrep prefixes each rule ID with the directories of its - # --config path, and the finding fingerprint is computed from that ID. - # Rule files can live in per-run temporary directories, so keep the IDs - # bare to give the same finding the same fingerprint on every run. - cmd = ['opengrep', '--json', '--dataflow-traces', '--no-rewrite-rule-ids', '--output', out_file] + cmd = ['opengrep', '--json', '--dataflow-traces', '--output', out_file] verbose = self.config.get('verbose', False) cmd.append('--verbose' if verbose else '--quiet') diff --git a/tests/test_opengrep_custom_rules.py b/tests/test_opengrep_custom_rules.py index 636e6a3..23d9c37 100644 --- a/tests/test_opengrep_custom_rules.py +++ b/tests/test_opengrep_custom_rules.py @@ -235,7 +235,9 @@ def test_custom_rules_apply_allowlist_when_custom_ids_match(tmp_path, monkeypatc assert "--exclude-rule" not in captured_cmd [(path, ids)] = configs.items() - assert "socket_filtered_rules_" in path + # The filtered copy sits next to the custom rules file it was made from. + assert "socket_custom_rules_" in path + assert Path(path).name.startswith(".socket-filtered-") assert ids == ["org.no-eval"] @@ -275,8 +277,11 @@ def test_bundled_allowlist_passes_only_enabled_rules(tmp_path, monkeypatch): assert str(bundled_file) not in captured_cmd [(path, ids)] = configs.items() assert ids == ["js-a", "js-c"] - # The filtered copy is removed once OpenGrep has run. - assert not Path(path).parent.exists() + # The copy sits next to the original, so OpenGrep gives its rules the same + # ID prefix, and is removed once OpenGrep has run. + assert Path(path).parent == bundled_file.parent + assert Path(path).name.startswith(".socket-filtered-") and path.endswith(".yaml") + assert [p.name for p in bundled_file.parent.iterdir()] == [bundled_file.name] def test_bundled_allowlist_warns_about_unknown_ids(tmp_path, monkeypatch, caplog): @@ -383,33 +388,43 @@ def test_all_rules_and_languages_skip_rule_files_opengrep_cannot_load(): assert not {"erlang.yml", "objective-c.yml"} & set(rule_files) -def _record_temp_dirs(monkeypatch) -> list[Path]: - """Record every directory ``tempfile.mkdtemp`` creates during the test.""" +def _record_temp_paths(monkeypatch) -> list[Path]: + """Record every directory and file ``tempfile`` creates during the test.""" created: list[Path] = [] - real_mkdtemp = tempfile.mkdtemp + real_mkdtemp, real_mkstemp = tempfile.mkdtemp, tempfile.mkstemp def _mkdtemp(*args, **kwargs): path = real_mkdtemp(*args, **kwargs) created.append(Path(path)) return path + def _mkstemp(*args, **kwargs): + fd, path = real_mkstemp(*args, **kwargs) + created.append(Path(path)) + return fd, path + monkeypatch.setattr(tempfile, "mkdtemp", _mkdtemp) + monkeypatch.setattr(tempfile, "mkstemp", _mkstemp) return created -def _custom_and_filtered_scanner(tmp_path) -> OpenGrepScanner: - """Scanner that writes both a custom-rule and a filtered-rule directory.""" +def _custom_and_filtered_scanner(tmp_path) -> tuple[OpenGrepScanner, Path]: + """Scanner that writes a custom-rule directory, a filtered copy inside it + (JavaScript) and a filtered copy next to a bundled rules file (Python).""" workspace = tmp_path / "workspace" workspace.mkdir(parents=True, exist_ok=True) _write_custom_rules_file(workspace / ".socket" / "rules" / "org-rules.yml", ["org.no-eval", "org.no-innerhtml"]) bundled_rules_dir = tmp_path / "bundled-rules" _write_rule_file(bundled_rules_dir / "javascript_typescript.yml", ["js-default-rule"]) + _write_rule_file(bundled_rules_dir / "python.yml", ["py-a", "py-b"]) config = Config( { "workspace": str(workspace), "output_dir": str(workspace), "javascript_sast_enabled": True, "javascript_enabled_rules": "org.no-eval", + "python_sast_enabled": True, + "python_enabled_rules": "py-a", "use_custom_sast_rules": True, "custom_sast_rule_path": ".socket/rules", "opengrep_rules_dir": str(bundled_rules_dir), @@ -421,12 +436,18 @@ def _custom_and_filtered_scanner(tmp_path) -> OpenGrepScanner: scanner = OpenGrepScanner(config) scanner._convert_to_socket_facts = lambda _: {"components": []} scanner.generate_notifications = lambda _: {} - return scanner + return scanner, bundled_rules_dir -def test_temp_rule_dirs_are_removed_when_opengrep_is_missing(tmp_path, monkeypatch): - created = _record_temp_dirs(monkeypatch) - scanner = _custom_and_filtered_scanner(tmp_path) +def _assert_all_removed(created: list[Path], bundled_rules_dir: Path) -> None: + assert created + assert not any(p.exists() for p in created) + assert sorted(p.name for p in bundled_rules_dir.iterdir()) == ["javascript_typescript.yml", "python.yml"] + + +def test_temp_rule_paths_are_removed_when_opengrep_is_missing(tmp_path, monkeypatch): + created = _record_temp_paths(monkeypatch) + scanner, bundled_rules_dir = _custom_and_filtered_scanner(tmp_path) def _missing(cmd, capture_output, text): raise FileNotFoundError("opengrep") @@ -434,14 +455,16 @@ def _missing(cmd, capture_output, text): monkeypatch.setattr("socket_basics.core.connector.opengrep.subprocess.run", _missing) assert scanner.scan() == {} - names = sorted(p.name.split("_rules_")[0] for p in created) - assert names == ["socket_custom", "socket_filtered"] - assert not any(p.exists() for p in created) + # The custom-rule directory, plus one filtered copy inside it and one + # next to the bundled python.yml. + assert sum(p.name.startswith("socket_custom_rules_") for p in created) == 1 + assert sorted(p.parent == bundled_rules_dir for p in created if p.name.startswith(".socket-filtered-")) == [False, True] + _assert_all_removed(created, bundled_rules_dir) -def test_temp_rule_dirs_are_removed_when_the_scan_raises(tmp_path, monkeypatch): - created = _record_temp_dirs(monkeypatch) - scanner = _custom_and_filtered_scanner(tmp_path) +def test_temp_rule_paths_are_removed_when_the_scan_raises(tmp_path, monkeypatch): + created = _record_temp_paths(monkeypatch) + scanner, bundled_rules_dir = _custom_and_filtered_scanner(tmp_path) def _crash(cmd, capture_output, text): raise RuntimeError("opengrep crashed") @@ -450,13 +473,12 @@ def _crash(cmd, capture_output, text): with pytest.raises(RuntimeError, match="opengrep crashed"): scanner.scan() - assert len(created) == 2 - assert not any(p.exists() for p in created) + _assert_all_removed(created, bundled_rules_dir) def test_custom_rule_dir_is_removed_when_building_custom_rules_fails(tmp_path, monkeypatch): - created = _record_temp_dirs(monkeypatch) - scanner = _custom_and_filtered_scanner(tmp_path) + created = _record_temp_paths(monkeypatch) + scanner, bundled_rules_dir = _custom_and_filtered_scanner(tmp_path) def _fail(self, rules_by_file): raise OSError("disk full") @@ -467,5 +489,65 @@ def _fail(self, rules_by_file): _mock_subprocess_run(monkeypatch, []) scanner.scan() - assert [p.name.split("_rules_")[0] for p in created][:1] == ["socket_custom"] - assert not any(p.exists() for p in created) + assert any(p.name.startswith("socket_custom_rules_") for p in created) + _assert_all_removed(created, bundled_rules_dir) + + +def test_filtered_copy_falls_back_to_a_temp_dir_when_rules_dir_is_read_only(tmp_path, monkeypatch, caplog): + scanner, bundled_file = _bundled_js_scanner(tmp_path, ["js-a", "js-b"], "js-a") + created = _record_temp_paths(monkeypatch) + record_mkstemp = tempfile.mkstemp + + def _mkstemp(*args, **kwargs): + if Path(kwargs.get("dir", "")) == bundled_file.parent: + raise PermissionError("read-only") + return record_mkstemp(*args, **kwargs) + + monkeypatch.setattr(tempfile, "mkstemp", _mkstemp) + configs: dict = {} + _mock_subprocess_run(monkeypatch, [], configs) + with caplog.at_level("WARNING"): + scanner.scan() + + [(path, ids)] = configs.items() + assert ids == ["js-a"] + assert "socket_filtered_rules_" in path + assert "fingerprints" in caplog.text + assert created and not any(p.exists() for p in created) + + +@pytest.mark.parametrize( + ("first", "second", "rule_file"), + [("c", "cpp", "c_cpp.yml"), ("csharp", "dotnet", "dotnet.yml")], +) +def test_shared_rule_file_stays_unfiltered_when_one_language_has_no_list(first, second, rule_file): + config = Config( + { + f"{first}_sast_enabled": True, + f"{second}_sast_enabled": True, + f"{first}_enabled_rules": "rule-x", + f"{second}_enabled_rules": " ", + } + ) + assert rule_file not in config.build_filtered_opengrep_rules() + + +@pytest.mark.parametrize( + ("first", "second", "rule_file"), + [("c", "cpp", "c_cpp.yml"), ("csharp", "dotnet", "dotnet.yml")], +) +def test_shared_rule_file_combines_both_lists(first, second, rule_file): + config = Config( + { + f"{first}_sast_enabled": True, + f"{second}_sast_enabled": True, + f"{first}_enabled_rules": "rule-x", + f"{second}_enabled_rules": "rule-y,rule-x", + } + ) + assert sorted(config.build_filtered_opengrep_rules()[rule_file]) == ["rule-x", "rule-y"] + + +def test_shared_rule_file_ignores_a_disabled_languages_empty_list(): + config = Config({"c_sast_enabled": True, "c_enabled_rules": "rule-x", "cpp_enabled_rules": ""}) + assert config.build_filtered_opengrep_rules() == {"c_cpp.yml": ["rule-x"]} diff --git a/tests/test_opengrep_rule_filter_e2e.py b/tests/test_opengrep_rule_filter_e2e.py index 9acf007..dd8f98a 100644 --- a/tests/test_opengrep_rule_filter_e2e.py +++ b/tests/test_opengrep_rule_filter_e2e.py @@ -114,10 +114,13 @@ def test_all_rules_enabled_overrides_the_allowlist(tmp_path, monkeypatch): assert "js-debugger-statement" in reported -def test_filtered_scans_give_stable_fingerprints(tmp_path, monkeypatch): - """Each filtered scan writes its rules to a new temporary directory; the - fingerprints of unchanged findings must not depend on that directory.""" +def test_filtered_findings_keep_their_unfiltered_fingerprints(tmp_path, monkeypatch): + """Socket keys a SAST alert on its props, fingerprint included, and stores + triage against that key. A finding the allowlist keeps must report the same + fingerprint as an unfiltered scan, and on every run, or its triage is lost.""" enabled = "js-code-injection-eval,js-weak-hash-md5" + unfiltered = _scan_findings(tmp_path, monkeypatch, javascript_enabled_rules="") first = _scan_findings(tmp_path, monkeypatch, javascript_enabled_rules=enabled) second = _scan_findings(tmp_path, monkeypatch, javascript_enabled_rules=enabled) - assert first and first == second + kept = {(rule, fp) for rule, fp in unfiltered if rule in enabled.split(",")} + assert kept and first == kept and second == kept