-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add P0 semgrep security rules #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| rules: | ||
| - id: dangerous-inner-html | ||
| message: >- | ||
| dangerouslySetInnerHTML bypasses React's XSS protection. | ||
| All uses require manual review — sanitization alone does not suppress this rule. | ||
| Add a nosemgrep comment after review to suppress. | ||
| See: security.md — "Never use dangerouslySetInnerHTML with user-supplied content" | ||
| severity: WARNING | ||
| languages: [typescript, javascript] | ||
| pattern-either: | ||
| - pattern: <$EL dangerouslySetInnerHTML={...} /> | ||
| - pattern: <$EL dangerouslySetInnerHTML={...}>...</$EL> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| rules: | ||
| - id: fallback-secret-js | ||
| message: >- | ||
| Hardcoded fallback for secret-like env var. | ||
| Secret variables should fail explicitly if not set, not fall back to defaults. | ||
| See: security.md — "Never hardcode secrets as fallback values" | ||
| severity: ERROR | ||
| languages: [javascript, typescript] | ||
| pattern-either: | ||
| - patterns: | ||
| - pattern: process.env.$VAR || "..." | ||
| - metavariable-regex: | ||
| metavariable: $VAR | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
| - patterns: | ||
| - pattern: process.env.$VAR ?? "..." | ||
| - metavariable-regex: | ||
| metavariable: $VAR | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
| - patterns: | ||
| - pattern: process.env["$VAR"] || "..." | ||
| - metavariable-regex: | ||
| metavariable: $VAR | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
| - patterns: | ||
| - pattern: process.env["$VAR"] ?? "..." | ||
| - metavariable-regex: | ||
| metavariable: $VAR | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
|
|
||
| - id: fallback-secret-python | ||
| message: >- | ||
| Hardcoded fallback for secret-like env var. | ||
| Secret variables should fail explicitly if not set, not fall back to defaults. | ||
| See: security.md — "Never hardcode secrets as fallback values" | ||
| severity: ERROR | ||
| languages: [python] | ||
| pattern-either: | ||
| - patterns: | ||
| - pattern: os.environ.get($KEY, "...") | ||
| - metavariable-regex: | ||
| metavariable: $KEY | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex for identifying secret-like key names is case-sensitive. This means it would miss keys like regex: "(?i).*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" |
||
| - patterns: | ||
| - pattern: os.getenv($KEY, "...") | ||
| - metavariable-regex: | ||
| metavariable: $KEY | ||
| regex: ".*(SECRET|PASSWORD|CREDENTIAL|PRIVATE|AUTH|API_KEY|TOKEN).*" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| rules: | ||
| - id: no-eval-dynamic-exec | ||
| message: >- | ||
| Dynamic code execution detected. Use structured alternatives | ||
| (JSON.parse, ast.literal_eval, etc.) instead of executing arbitrary strings. | ||
| See: security.md — "Never use eval(), Function(), or dynamic code execution" | ||
| severity: ERROR | ||
| languages: [javascript, typescript] | ||
| pattern-either: | ||
| - pattern: eval(...) | ||
| - pattern: new Function(...) | ||
| - pattern: Function(...) | ||
|
|
||
| - id: no-eval-dynamic-exec-python | ||
| message: >- | ||
| Dynamic code execution detected. Use structured alternatives | ||
| (json.loads, ast.literal_eval, etc.) instead of executing arbitrary strings. | ||
| See: security.md — "Never use eval(), Function(), or dynamic code execution" | ||
| severity: ERROR | ||
| languages: [python] | ||
| pattern-either: | ||
| - pattern: eval(...) | ||
| - pattern: exec(...) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| rules: | ||
| - id: unsafe-yaml-load | ||
| message: >- | ||
| yaml.load() without SafeLoader can execute arbitrary code. | ||
| Use yaml.safe_load() or yaml.load(data, Loader=yaml.SafeLoader). | ||
| See: security.md — "Use yaml.safe_load not yaml.load" | ||
| severity: ERROR | ||
| languages: [python] | ||
| patterns: | ||
| - pattern: yaml.load(...) | ||
| - pattern-not: yaml.load(..., Loader=yaml.SafeLoader) | ||
| - pattern-not: yaml.load(..., Loader=yaml.BaseLoader) | ||
| - pattern-not: yaml.load(..., Loader=yaml.CSafeLoader) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // Tests for dangerous-inner-html rule | ||
| import DOMPurify from "dompurify"; | ||
|
|
||
| function Unsanitized({ content }: { content: string }) { | ||
| // ruleid: dangerous-inner-html | ||
| return <div dangerouslySetInnerHTML={{ __html: content }} />; | ||
| } | ||
|
|
||
| function Sanitized({ content }: { content: string }) { | ||
| // ruleid: dangerous-inner-html | ||
| return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }} />; | ||
| } | ||
|
|
||
| function Safe({ content }: { content: string }) { | ||
| // ok: dangerous-inner-html | ||
| return <div>{content}</div>; | ||
| } | ||
|
|
||
| function Suppressed({ content }: { content: string }) { | ||
| // ok: dangerous-inner-html | ||
| // nosemgrep: dangerous-inner-html | ||
| return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }} />; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // Tests for fallback-secret-js rule | ||
|
|
||
| // ruleid: fallback-secret-js | ||
| const apiKey = process.env.API_KEY || "sk-default-key-12345"; | ||
| // ruleid: fallback-secret-js | ||
| const token = process.env.AUTH_TOKEN ?? "default-token"; | ||
| // ruleid: fallback-secret-js | ||
| const secret = process.env.SECRET || "fallback-secret"; | ||
| // ruleid: fallback-secret-js | ||
| const password = process.env.DB_PASSWORD || "admin123"; | ||
|
|
||
| // ok: fallback-secret-js | ||
| const host = process.env.DB_HOST || "localhost"; | ||
| // ok: fallback-secret-js | ||
| const port = process.env.PORT || "3000"; | ||
| // ok: fallback-secret-js | ||
| const env = process.env.NODE_ENV || "development"; | ||
| // ok: fallback-secret-js | ||
| const logLevel = process.env.LOG_LEVEL || "info"; | ||
|
|
||
| // Bracket notation variants | ||
| // ruleid: fallback-secret-js | ||
| const key2 = process.env["API_KEY"] || "sk-bracket-key"; | ||
| // ok: fallback-secret-js | ||
| const host2 = process.env["DB_HOST"] || "127.0.0.1"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Tests for fallback-secret-python rule | ||
| import os | ||
|
|
||
| # ruleid: fallback-secret-python | ||
| api_key = os.environ.get("API_KEY", "sk-default-key-12345") | ||
| # ruleid: fallback-secret-python | ||
| token = os.getenv("AUTH_TOKEN", "default-token") | ||
| # ruleid: fallback-secret-python | ||
| secret = os.environ.get("SECRET", "fallback-secret") | ||
|
|
||
| # ok: fallback-secret-python | ||
| host = os.environ.get("DB_HOST", "localhost") | ||
| # ok: fallback-secret-python | ||
| port = int(os.environ.get("PORT", "8080")) | ||
| # ok: fallback-secret-python | ||
| env = os.getenv("NODE_ENV", "development") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Tests for no-eval-dynamic-exec rule | ||
|
|
||
| // ruleid: no-eval-dynamic-exec | ||
| const result = eval(userInput); | ||
| // ruleid: no-eval-dynamic-exec | ||
| const fn = new Function("return " + code); | ||
| // ruleid: no-eval-dynamic-exec | ||
| const fn2 = Function("alert(1)"); | ||
|
|
||
| // ok: no-eval-dynamic-exec | ||
| const data = JSON.parse(rawJson); | ||
| // ok: no-eval-dynamic-exec | ||
| const config = { eval: false }; | ||
| // ok: no-eval-dynamic-exec | ||
| const name = "eval"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Tests for no-eval-dynamic-exec-python rule | ||
| import ast | ||
| import json | ||
|
|
||
| # ruleid: no-eval-dynamic-exec-python | ||
| result = eval(user_input) | ||
| # ruleid: no-eval-dynamic-exec-python | ||
| exec(code_string) | ||
|
|
||
| # ok: no-eval-dynamic-exec-python | ||
| data = json.loads(raw_json) | ||
| # ok: no-eval-dynamic-exec-python | ||
| value = ast.literal_eval(literal_string) |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,16 @@ | ||||||||||||||
| # Tests for unsafe-yaml-load rule | ||||||||||||||
| import yaml | ||||||||||||||
|
|
||||||||||||||
| # ruleid: unsafe-yaml-load | ||||||||||||||
| data = yaml.load(raw) | ||||||||||||||
| # ruleid: unsafe-yaml-load | ||||||||||||||
| data2 = yaml.load(raw, Loader=yaml.UnsafeLoader) | ||||||||||||||
| # ruleid: unsafe-yaml-load | ||||||||||||||
| data3 = yaml.load(raw, Loader=yaml.Loader) | ||||||||||||||
|
|
||||||||||||||
| # ok: unsafe-yaml-load | ||||||||||||||
| data4 = yaml.safe_load(raw) | ||||||||||||||
| # ok: unsafe-yaml-load | ||||||||||||||
| data5 = yaml.load(raw, Loader=yaml.SafeLoader) | ||||||||||||||
| # ok: unsafe-yaml-load | ||||||||||||||
| data6 = yaml.load(raw, Loader=yaml.BaseLoader) | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The corresponding rule in
Suggested change
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| #!/usr/bin/env bash | ||
| # Validates semgrep rules against annotated test fixtures. | ||
| # | ||
| # Uses semgrep's native --test framework (# ruleid: / # ok: annotations). | ||
| # Copies rules and fixtures to a temp dir because semgrep --test | ||
| # skips hidden directories (.semgrep/). | ||
| # | ||
| # Convention: rule file stem matches test file stem. | ||
| # .semgrep/no-eval.yaml <--> tests/fixtures/semgrep/no-eval.{js,py,...} | ||
| set -euo pipefail | ||
|
|
||
| RULES_DIR=".semgrep" | ||
| FIXTURES_DIR="tests/fixtures/semgrep" | ||
|
|
||
| if [[ ! -d "$RULES_DIR" ]]; then | ||
| echo "ERROR: Rules directory not found: $RULES_DIR" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| if [[ ! -d "$FIXTURES_DIR" ]]; then | ||
| echo "ERROR: Fixtures directory not found: $FIXTURES_DIR" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| WORK_DIR=$(mktemp -d) | ||
| trap 'rm -rf "$WORK_DIR"' EXIT | ||
|
|
||
| cp "$RULES_DIR"/*.yaml "$WORK_DIR/" | ||
| cp "$FIXTURES_DIR"/* "$WORK_DIR/" | ||
|
|
||
| semgrep --test "$WORK_DIR/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex for identifying secret-like variable names is case-sensitive. This means it would miss variables like
process.env.api_keyorprocess.env.Auth_Token. To make the rule more robust, consider making the regex case-insensitive using(?i). This change should also be applied to the regex on line 19.