fix(rules): reduce false positives in SQL injection, hardcoded secret, sensitive log, and ReDoS rules#1
Conversation
…, sensitive log, and ReDoS rules
- SQL_INJECTION: require + followed by string literal to avoid flagging safe parameter composition
- TS_HARDCODED_SECRET: add negative lookahead to exclude ${...} template variable placeholders
- SENSITIVE_LOG: add word boundary \b before password/token/secret to avoid matching 'key_id', 'job_id', etc.
- TS_REGEX_DOS: update alternation comment to clarify pattern matches || operators, not regex alternation
Reduces CoRag false positives from 12 to 3 on first scan.
f76d456 to
1718780
Compare
Before: template literal with any ${...+...} was flagged
After: require SELECT/INSERT/UPDATE/DELETE/etc. before the template
Fixes false positives on path concatenation like:
`${params.dir}${params.id ? '/' + params.id : ''}`
Fixes 35 false SQL injection warnings in opencode project.
There was a problem hiding this comment.
Pull request overview
This PR updates multiple regex-based security rules to reduce false positives when scanning repositories (notably CoRag), by tightening pattern matching for SQL injection, hardcoded secrets, sensitive logging, and regex DoS.
Changes:
- Tighten SQL injection regexes (TypeScript + general security) to reduce concatenation-related false positives.
- Adjust hardcoded-secret regexes to avoid matching
${VAR}-style placeholders. - Refine sensitive-log and regex-DoS rules to reduce spurious matches and clarify intent.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rules/typescript/sql_injection.yaml | Narrows template-literal SQL concatenation detection with keyword gating. |
| rules/typescript/regex_dos.yaml | Tightens pipe-count regex and updates rule messaging. |
| rules/typescript/hardcoded_secret.yaml | Attempts to exclude ${...} placeholders from hardcoded-secret detection. |
| rules/security/sql_injection.yaml | Narrows SQL concatenation detection to + "..." patterns. |
| rules/security/sensitive_log.yaml | Adds word-boundary matching and removes key from sensitive terms. |
| rules/security/hardcoded_secret.yaml | Attempts to exclude ${...} placeholders from hardcoded-secret detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| comment: String concatenation in SQL query | ||
| - type: regex | ||
| pattern: '(?i)`[^`]*\$\{[^}]*\+[^}]*\}[^`]*`' | ||
| pattern: '(?i)(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE|ORDER|GROUP|HAVING|LIMIT|JOIN)\s*`[^`]*\$\{[^}]*\+[^}]*\}[^`]*`' |
There was a problem hiding this comment.
This regex requires a SQL keyword before the opening backtick (i.e., outside the template literal). In typical TS/JS code the keyword appears inside the template literal (e.g., ``const q = `SELECT ... ${a + b}```), so this pattern likely won't match at all. Consider matching the keyword inside the backticks instead to avoid disabling template-literal SQL injection detection.
| pattern: '(?i)(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE|ORDER|GROUP|HAVING|LIMIT|JOIN)\s*`[^`]*\$\{[^}]*\+[^}]*\}[^`]*`' | |
| pattern: '(?i)`[^`]*(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE|ORDER|GROUP|HAVING|LIMIT|JOIN)[^`]*\$\{[^}]*\+[^}]*\}[^`]*`' |
| pattern: '(log\.|console\.|fmt\.Print|print\()(.)*\b(password|token|secret)\b' | ||
| comment: Possible sensitive data in log |
There was a problem hiding this comment.
This update removes key from the sensitive-term list (previously matched password|token|secret|key), which is more than adding a word boundary as described in the PR summary. If the intent is only to avoid false positives like key_id, consider keeping key but constraining it (e.g., \bkey\b) or matching more specific phrases like api[_-]?key.
| pattern: '\|[^| ]+\|[^| ]+\|[^| ]+' | ||
| comment: Alternation with 3+ pipes inside regex literals - use atomic groups or simplify to prevent ReDoS |
There was a problem hiding this comment.
The PR description mentions this rule change as a comment clarification, but the regex pattern itself was also tightened (from \|.*\|.*\| to \|[^| ]+\|[^| ]+\|[^| ]+). If the intention is comment-only, please revert the pattern change; otherwise update the PR description to reflect that detection behavior changed.
| pattern: '\|.*\|.*\|' | ||
| comment: Alternation with overlapping patterns - potential backtracking | ||
| pattern: '\|[^| ]+\|[^| ]+\|[^| ]+' | ||
| comment: Alternation with 3+ pipes inside regex literals - use atomic groups or simplify to prevent ReDoS |
There was a problem hiding this comment.
This comment suggests using atomic groups to mitigate ReDoS, but JavaScript/TypeScript regex does not support atomic groups (and also lacks possessive quantifiers). Consider adjusting the guidance to options that are actually available in JS (e.g., reducing nested quantifiers, limiting repetition ranges, or refactoring the pattern).
| patterns: | ||
| - type: regex | ||
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*(:?=)\s*["\x27][^"\x27]{8,}["\x27]' | ||
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*(:?=)\s*(?!.*\$\{)["\x27][^"\x27]{8,}["\x27]' |
There was a problem hiding this comment.
This pattern uses a negative lookahead (?!...), but the scanner compiles rules with Go's regexp (RE2) which does not support lookaheads. The regex will fail to compile and will be silently skipped, effectively disabling HARDCODED_SECRET detections. Consider rewriting the pattern to avoid lookarounds (e.g., match quoted strings that do not start with ${ / do not contain $ followed by {).
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*(:?=)\s*(?!.*\$\{)["\x27][^"\x27]{8,}["\x27]' | |
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*(:?=)\s*("[^"$]{8,}"|\x27[^\x27$]{8,}\x27)' |
| patterns: | ||
| - type: regex | ||
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*[=:]\s*["\x27`][^"\x27`]{8,}["\x27`]' | ||
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*[=:]\s*(?!.*\$\{)["\x27`][^"\x27`]{8,}["\x27`]' |
There was a problem hiding this comment.
This pattern uses a negative lookahead (?!...), but the scanner uses Go regexp (RE2) which does not support lookaheads. As a result the regex won't compile and will be skipped, so this branch of TS_HARDCODED_SECRET won't run. Please rewrite without lookarounds (e.g., ensure the captured string doesn't start with ${ / doesn't contain $ followed by { using RE2-compatible alternation).
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*[=:]\s*(?!.*\$\{)["\x27`][^"\x27`]{8,}["\x27`]' | |
| pattern: '(?i)(password|token|api_key|secret|apiKey|secretKey)\s*[=:]\s*["\x27`](?:[^"\x27`$]|\$[^\{]){8,}\$?["\x27`]' |
| - type: regex | ||
| pattern: '(SELECT|INSERT|UPDATE|DELETE).*\+' | ||
| comment: String concatenation in SQL query | ||
| pattern: '(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE).*\+\s*["\x27]' |
There was a problem hiding this comment.
This change makes the rule only match concatenations where + is followed by a string literal. That will no longer match common vulnerable patterns like "...WHERE id=" + user_id and will break the existing golden test that expects 2 SQL_INJECTION findings. If the intent is reducing false positives, consider tightening the SQL keyword-in-string portion instead, without excluding + <identifier> concatenations entirely.
| pattern: '(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE).*\+\s*["\x27]' | |
| pattern: '["\x27][^"\x27\n]*(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE)[^"\x27\n]*["\x27]\s*\+' |
Java: - Replace generic 20+ char pattern with hex (32+) and base64 with padding (40+, requires 2+ padding chars) - Avoids false positives on long config key names like 'vectorstore_default_collection' TypeScript: - Replace generic 20+ char pattern with UUID, hex (32+), base64 with padding (40+) - Avoids false positives on localStorage keys like '__aibot_msg_sessionid' - Avoids false positives on alphanumeric constants like version strings
Engine: exclude target/, _deps/, .venv/, venv/ directories (build artifacts, dependencies) Rust: add word boundary to sensitive_log patterns to avoid matching embedded substrings like 'max_tokens' containing 'token'
Added: .vitepress (VitePress generated files), target/ (Rust), _deps/, .venv/, venv/ These directories contain generated/build artifacts that create noise and false positives.
…s all rules - Add AST-based SQL_INJECTION checker for Go (langs/golang/parser.go): - Handle chained selectors (s.pool.QueryRow, s.db.QueryContext) - Distinguish parameterized queries (string literal) from unsafe concatenation - Prioritize AST findings over regex findings to avoid false positive dedup - Hardcoded Secret: fix pattern to work without Go's unsupported lookahead - Sensitive Log: remove generic 'key|auth' that matched job_id/correlation_key - Regex DoS: require regex literal context (/.../) to exclude array indexing - SQL Injection: add string concatenation operators to pattern detection - All 20 test packages pass
Summary
Reduces false positives when scanning CoRag from 12 issues to 3.
Changes
query := "SELECT ..." + subWhereapi_key: "${OPENAI_API_KEY}"Test results on CoRag
The remaining 3 issues are fundamental regex-only engine limitations that require AST analysis to resolve.