feat: add SAST scanning to CI pipeline (CI-01)#915
Conversation
Add Taskdeck-specific Semgrep rules for C# and TypeScript: - Missing [Authorize] on controllers (CWE-862) - Raw SQL string interpolation (CWE-89) - Logging sensitive data (CWE-532) - Domain referencing Infrastructure (ADR-0001) - Hardcoded connection strings (CWE-798) - innerHTML/document.write usage (CWE-79) - Raw localStorage token storage (ADR-0009) - Disabled security ESLint rules (CWE-95) Closes part of #870
Reads Semgrep JSON output and produces Markdown + JSON summaries for GitHub CI step summaries. Supports enforcement mode that exits non-zero on ERROR-level findings. Part of #870
Reusable workflow that installs Semgrep via pip, runs scans with both registry rules (p/csharp, p/typescript, p/jwt) and custom Taskdeck rules, then summarizes findings as CI step summaries with artifact upload. Part of #870
ci-extended: triggers on security label or workflow_dispatch (advisory). ci-nightly: runs unconditionally every night (advisory). Both non-blocking initially with path to enforcement via enforce-findings input. Closes #870
Documents the decision to adopt Semgrep for SAST scanning, including rationale, alternatives considered, rule strategy, and CI integration topology. Part of #870
- Pin Semgrep version (1.113.0) for reproducible builds - Remove stdout/stderr redirect that swallowed error messages - Remove redundant --json flag (--json-output suffices) - Fix ADR-0031 text to match actual pip-based installation
Adversarial Self-ReviewIssues found and fixed (commit 8b521b2)
Remaining considerations (not blocking)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43e21f7220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasHighCritical = severityCounts.ERROR > 0; | ||
| const hasEnforcementFailures = enforce && hasHighCritical; |
There was a problem hiding this comment.
Fail enforcement when Semgrep scan did not complete
hasEnforcementFailures is computed only from ERROR findings, so an enforcement run still exits successfully when Semgrep itself fails (for example, non-zero scan exit, missing JSON output, or parse failure). In that scenario the security gate can report success without a valid scan result. Include scan/parse failure signals (e.g., non-zero scanExitCode or parse error state) in enforcement failure logic so enforcement cannot be bypassed by scanner failures.
Useful? React with 👍 / 👎.
| // If the file doesn't exist or is invalid, create an empty result | ||
| semgrepResults = { results: [], errors: [] }; |
There was a problem hiding this comment.
Preserve Semgrep parse failures in summary output
When reading Semgrep JSON fails, the script replaces the result with an empty { results: [], errors: [] }, which later triggers the “No SAST findings...passed all configured rules” message. This produces a misleading pass state whenever Semgrep output is missing or malformed. Keep a parse-failure marker in errors (or equivalent state) so the generated summary explicitly reports the scan as failed/incomplete instead of clean.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces SAST scanning using Semgrep, featuring custom rules for C# and TypeScript, a dedicated ignore file, and a Node.js script for summarizing scan results in CI. Feedback focuses on correcting logic errors in the Semgrep rules—specifically regarding controller exclusion regex and unhandled fetch detection—and resolving documentation discrepancies in the new ADR. Additionally, improvements were suggested for the summary script to prevent Markdown table breakage and ensure status logs do not pollute the CI step summary output.
| pattern: | | ||
| fetch(...).then(...) | ||
| fix: | | ||
| fetch(...).then(...).catch(...) |
There was a problem hiding this comment.
The unhandled-fetch rule has two significant issues:
- The
patternis too broad; it will matchfetch(...).then(...)even if it is followed by a.catch(), because the pattern matches the prefix of the chain. This will lead to many false positives. - The
fixuses...which is treated as a literal string in Semgrep fixes, resulting in broken code (e.g.,fetch(...).then(...).catch(...)literally).
To fix this, use patterns with a pattern-not to exclude handled fetches, and use metavariables in the fix to preserve the original code.
patterns:
- pattern: fetch($URL).then($CB)
- pattern-not: fetch($URL).then($CB).catch(...)
fix: fetch($URL).then($CB).catch(err => console.error(err))| } | ||
| - metavariable-regex: | ||
| metavariable: $CONTROLLER | ||
| regex: ^(?!Health|Auth$).*Controller$ |
There was a problem hiding this comment.
The regex ^(?!Health|Auth$).*Controller$ does not correctly exclude AuthController. The negative lookahead (?!Health|Auth$) only fails if the string starts with "Health" or is exactly "Auth". Since "AuthController" starts with "A" and is not exactly "Auth", it will be incorrectly flagged by this rule. To specifically exclude HealthController and AuthController, the regex should be more precise.
regex: ^(?!(Health|Auth)Controller$).*Controller$| - Runs in the **ci-extended** lane (triggered by `security` label) and **ci-nightly** (unconditionally). | ||
| - **Non-blocking initially** (advisory mode): findings are reported as CI step summaries and artifacts but do not fail the build. | ||
| - **Path to enforcement**: The `enforce-findings` input can be set to `true` to gate on ERROR-level findings. Plan to enable after baseline triage. | ||
| - Installs Semgrep via pip (version-pinned) alongside Node.js for the summary script. |
There was a problem hiding this comment.
There is a discrepancy between this ADR and the implementation described in the PR summary. The ADR states that the semgrep/semgrep:latest container image is used, but the PR summary mentions switching to pip install semgrep to allow using setup-node in the same job. Please update the ADR to reflect the actual implementation.
| - Installs Semgrep via pip (version-pinned) alongside Node.js for the summary script. | |
| - Uses setup-python and pip install semgrep for scanning to allow Node.js-based summary scripts in the same job. |
| for (const f of displayFindings) { | ||
| const file = f.path || "unknown"; | ||
| const line = f.start?.line || "?"; | ||
| const msg = (f.extra?.message || "").replace(/\n/g, " ").slice(0, 200); |
There was a problem hiding this comment.
The finding message is inserted directly into a Markdown table row. If the message contains a pipe character (|), it will break the table formatting. You should escape pipe characters to ensure the summary renders correctly.
| const msg = (f.extra?.message || "").replace(/\n/g, " ").slice(0, 200); | |
| const msg = (f.extra?.message || "").replace(/\n/g, " ").replace(/\|/g, "\\|").slice(0, 200); |
| // --------------------------------------------------------------------------- | ||
| if (outputMarkdown) { | ||
| writeFileSync(resolve(outputMarkdown), markdown, "utf8"); | ||
| console.log(`Markdown summary written to ${outputMarkdown}`); |
There was a problem hiding this comment.
Informational logs are printed to stdout. When this script is used to generate the GITHUB_STEP_SUMMARY (e.g., by redirecting output), these logs will pollute the Markdown summary. Status messages should be sent to stderr instead.
| console.log(`Markdown summary written to ${outputMarkdown}`); | |
| console.error(`Markdown summary written to ${outputMarkdown}`); |
|
|
||
| if (outputJson) { | ||
| writeFileSync(resolve(outputJson), JSON.stringify(jsonSummary, null, 2), "utf8"); | ||
| console.log(`JSON summary written to ${outputJson}`); |
There was a problem hiding this comment.
Informational logs are printed to stdout. When this script is used to generate the GITHUB_STEP_SUMMARY (e.g., by redirecting output), these logs will pollute the Markdown summary. Status messages should be sent to stderr instead.
| console.log(`JSON summary written to ${outputJson}`); | |
| console.error(`JSON summary written to ${outputJson}`); |
- Fix controller exclusion regex to properly exclude AuthController - Fix unhandled-fetch rule: remove broken fix template, add pattern-not to exclude fetch chains that already have .catch() - Harden enforcement: fail when scan is incomplete (non-zero exit code or JSON parse failure) instead of silently reporting clean - Inject parse-failure error marker so summary never shows false pass - Escape pipe characters in markdown table messages - Use stderr for status messages to keep stdout clean for markdown - Fix ADR-0031: s/container image/pip install/ in Consequences
Adversarial Review -- PR #915 (SAST Scanning)I performed a thorough adversarial review of all changed files, evaluated all 8 bot review comments, and pushed fixes for the legitimate findings. Summary below. Issues Found and Fixed (commit 0c0c00a)1. Controller exclusion regex is wrong (severity: medium) The regex Note: This is currently moot because Gemini raised this -- legitimate, fixed. 2. Unhandled-fetch rule produces false positives and has broken Two problems:
Fixed by switching to Gemini raised this -- legitimate, fixed. 3. Enforcement bypass on scanner failure (severity: critical) When enforcement is enabled and Semgrep crashes (exit code >= 2) or produces no/invalid JSON output, the script silently creates an empty result set and reports "No SAST findings. The codebase passed all configured rules." This means enforcement mode can be trivially bypassed by any condition that causes Semgrep to fail. Fixed by tracking Codex P1 raised this -- legitimate, fixed. 4. Misleading "passed" on parse failure (severity: medium) When Codex P2 raised this -- legitimate, fixed. 5. Pipe characters break markdown table (severity: low) Semgrep messages containing Gemini raised this -- legitimate, fixed. 6. Status messages pollute stdout (severity: low) The "Markdown summary written to" and "JSON summary written to" messages used Gemini raised this -- legitimate, fixed. 7. ADR mentions "container image" but implementation uses pip install (severity: low) The Consequences section references "Semgrep container image adds ~1 min to job startup" but the actual implementation uses Gemini raised this -- legitimate, fixed. Bot Comments Evaluated but NOT Acted OnGemini's ADR line 32 comment ("update ADR to reflect pip install"): The ADR line 32 already correctly says "Installs Semgrep via pip (version-pinned)." The stale "container image" reference was only in the Consequences section (line 55), which I fixed. The line Gemini pointed at was already correct. Additional Findings from My Own ReviewA. No B. Semgrep exit code semantics are correctly handled C. Workflow permissions are minimal (good) D. No SEMGREP_APP_TOKEN or registry auth required E. Test files are scanned (acceptable) F. The TD-CS-003 (logging sensitive data) rule may have false positives G. VerdictAll 7 bot findings were legitimate and have been fixed. The most critical fix is the enforcement bypass on scanner failure (#3), which could have allowed a security gate to silently pass without a valid scan result. The fixes are in commit 0c0c00a. The PR is now ready for merge after CI passes on the fix commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0c00a4ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const scanFailed = scanExitCode !== 0 && scanExitCode !== 1; // 1 = findings found, >1 = error | ||
| const scanIncomplete = parseError !== null || scanFailed; |
There was a problem hiding this comment.
Fail enforcement when Semgrep reports scan errors
scanIncomplete only checks JSON parse failure and exit codes other than 0/1, so enforcement can still pass when Semgrep returns a valid JSON file containing non-empty errors (for example parse/analysis warnings when --strict is not enabled in .github/workflows/reusable-sast-scanning.yml). In that case coverage is partial but hasEnforcementFailures remains false, which weakens the intended security gate; treat errors.length > 0 as incomplete (or run Semgrep with strict error behavior) when --enforce is used.
Useful? React with 👍 / 👎.
| - pattern: localStorage.setItem("token", ...) | ||
| - pattern: localStorage.setItem("accessToken", ...) | ||
| - pattern: localStorage.setItem("jwt", ...) | ||
| - pattern: localStorage.setItem("session", ...) | ||
| - pattern: localStorage.setItem("refresh_token", ...) |
There was a problem hiding this comment.
Match Taskdeck token keys in localStorage SAST rule
This rule only matches exact keys like "token" and "session", but Taskdeck’s real persisted auth keys are namespaced (taskdeck_token / taskdeck_session in frontend/taskdeck-web/src/utils/tokenStorage.ts), so a direct localStorage.setItem('taskdeck_token', ...) outside the approved storage module will not be flagged. That leaves a blind spot in the policy this rule is meant to enforce.
Useful? React with 👍 / 👎.
The string escaping for markdown table cells was escaping newlines and pipes but not backslashes. A backslash in a Semgrep finding message could combine with a subsequent character to produce unintended escape sequences. Escape backslashes first, before other replacements. Fixes CodeQL finding: incomplete string escaping at line 185.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5fea3d0a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - pattern: ExecuteSqlRaw($"...") | ||
| - pattern: FromSqlRaw("..." + ...) | ||
| - pattern: ExecuteSqlRaw("..." + ...) |
There was a problem hiding this comment.
Detect ExecuteSqlRawAsync in SQL injection rule
The raw-SQL injection rule only matches ExecuteSqlRaw(...) and misses ExecuteSqlRawAsync(...), so interpolated or concatenated SQL passed through the async API will not be reported at ERROR severity. This creates a real blind spot because the codebase already uses ExecuteSqlRawAsync in infrastructure repositories/services, so risky async callsites can bypass the new SAST policy entirely.
Useful? React with 👍 / 👎.
| sast-scanning: | ||
| name: SAST Scanning (Semgrep) | ||
| if: github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'security')) | ||
| uses: ./.github/workflows/reusable-sast-scanning.yml |
There was a problem hiding this comment.
Trigger CI Extended for Semgrep rule-only PRs
This new SAST job is added to CI Extended, but the workflow-level pull_request.paths filter (defined at the top of this file) does not include .semgrep/** or .semgrepignore. As a result, PRs that only change Semgrep rules can skip the entire workflow, so broken or invalid SAST rule updates may merge without any PR-time execution of the new scan.
Useful? React with 👍 / 👎.
* docs: update STATUS.md and AUDIT.md for 10-PR post-merge sweep Add delivery wave entry for PRs #914--#924 covering CI/hardening (SAST, migration validation, performance regression gate, circuit breaker), frontend decomposition (ReviewView, InboxView, AutomationChatView), ops (alerting rules), docs (data model ERD), and UX (session timeout warning). Mark resolved items in AUDIT.md: oversized views, session timeout, SAST, alerting rules, data model reference, performance regression tests. * docs: update IMPLEMENTATION_MASTERPLAN.md with wave 27 delivery history Add delivery wave 27 for PRs #914--#924 covering CI/hardening (SAST, migration validation, performance regression gate, circuit breaker), frontend decomposition (ReviewView, InboxView, AutomationChatView), ops (alerting rules), docs (data model ERD), and session timeout warning. Note ADR-0031 and ADR-0032. Update wave 26 to cross-reference view decomposition resolution. * docs: mark 10 issues delivered in ISSUE_EXECUTION_GUIDE.md Add Stage 7 with all 10 issues from PRs #914--#924 marked as delivered. Update Stage 6 execution note to reflect view decomposition is now resolved. * docs: add ADR-0031 and ADR-0032 to decisions index ADR-0031: SAST Scanning with Semgrep (from PR #915, CI-01) ADR-0032: Circuit Breaker for External API Calls (from PR #924, HARD-01) Note: Both PRs originally created ADR-0031. Renumbered the circuit breaker ADR to ADR-0032 to resolve the conflict. * docs: update TESTING_GUIDE.md with new CI gates and test totals Add migration-validation job to ci-required, SAST scanning and performance regression gate to ci-extended, and both to ci-nightly. Update test counts for circuit breaker (23 backend) and session timeout (19 frontend) tests. * docs: update CLAUDE.md frontend architecture description Reflect view decomposition pattern (thin shells + extracted composables/components) and add examples of view-specific composables and component directories. * docs: mark resolved items in EXPANSION_ROADMAP and HARDENING docs Mark view decomposition (ReviewView, InboxView, AutomationChatView) and monitoring/alerting setup as resolved in both roadmap files. * fix: format STATUS.md Last Updated line to pass docs governance check The governance regex requires the line to end after the date (YYYY-MM-DD) with only optional whitespace. Move the parenthetical sweep note to its own line. * fix: correct OPS-27 and PERF-12 issue numbers across docs OPS-27 (config validation) is GitHub issue #863, not #858. PERF-12 (board list pagination) is GitHub issue #848, not #859. The wrong numbers (#858/#859) belong to FE-17 and FE-18 respectively. Fixes references in STATUS.md, IMPLEMENTATION_MASTERPLAN.md, ISSUE_EXECUTION_GUIDE.md, AUDIT.md, HARDENING_AND_PERFORMANCE.md, TESTING_GUIDE.md, and CONFIGURATION_REFERENCE.md.
Summary
.semgrep/for project-specific security patterns (missing[Authorize], raw SQL injection, logging secrets, architecture violations, XSS, insecure token storage)securitylabel) and ci-nightly (unconditional) as non-blocking advisory checksscripts/ci/summarize-sast-findings.mjsto produce GitHub step summaries and machine-readable JSON outputDesign Decisions
enforce-findings: falseby default. Flip totrueafter baseline triage to gate on ERROR-level findings.reusable-sast-scanning.ymlcalled from both lane workflows.setup-python+pip install semgrepinstead ofsemgrep/semgrepcontainer to allowsetup-nodefor the summary script in the same job.p/csharp,p/typescript,p/jwtfrom Semgrep registry plus 10 custom rules in.semgrep/.Test Plan
securitylabel to confirm Semgrep runs end-to-end.semgrepignoreexcludes build artifacts, test fixtures, and generated codeCloses #870