From e8367aeb3d6fa7194d1bbca04e058806af55264d Mon Sep 17 00:00:00 2001 From: bgagent Date: Wed, 10 Jun 2026 15:59:11 -0500 Subject: [PATCH] feat(security): custom semgrep rules for silent-success masking (AI004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare-except and empty-catch are gated, but the more dangerous AI004 variant — swallowing a failure and returning a plausible-but-empty default ([], {}, null/None, "") — had no detector. Such fallbacks hide failures from callers and let plausible-but-wrong code through green. Add custom semgrep rules for TS (catch { return []|null|{}|undefined }) and Python (except: return None|[]|{}|""), with semgrep-test fixtures covering rethrow/re-raise, finally, multi-except, and return-in-try cases. Findings anchor on the offending return line and carry agent-targeted remediation text. The new `security:sast:masking` task validates the rules, scans the repo, and writes SARIF to test-reports/ so findings stay agent-routable (pairs with CA-06). It runs inside `mise run security` but is ADVISORY (no --error) until the 40-finding baseline in .semgrep/BASELINE.md is triaged; intentional degraded-mode fallbacks use inline justified nosemgrep comments (existing repo convention). Closes #257 --- .semgrep/BASELINE.md | 87 +++++++++++++++++++ .semgrep/silent-success-masking.py | 122 ++++++++++++++++++++++++++ .semgrep/silent-success-masking.ts | 117 +++++++++++++++++++++++++ .semgrep/silent-success-masking.yaml | 124 +++++++++++++++++++++++++++ AGENTS.md | 3 +- mise.toml | 12 +++ 6 files changed, 464 insertions(+), 1 deletion(-) create mode 100644 .semgrep/BASELINE.md create mode 100644 .semgrep/silent-success-masking.py create mode 100644 .semgrep/silent-success-masking.ts create mode 100644 .semgrep/silent-success-masking.yaml diff --git a/.semgrep/BASELINE.md b/.semgrep/BASELINE.md new file mode 100644 index 00000000..92f284f7 --- /dev/null +++ b/.semgrep/BASELINE.md @@ -0,0 +1,87 @@ +# Silent-success masking — existing-code baseline + +Baseline scan for the custom rules in `silent-success-masking.yaml` +(AI004 / CA-09, [issue #257]). Captured at the rules' introduction; +**40 findings** (15 Python, 25 TypeScript). + +[issue #257]: https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/257 + +The `security:sast:masking` task is **advisory** (it prints findings and +emits SARIF but never fails) until this baseline is triaged. Each finding +must either be fixed (surface the error) or allowlisted with an inline +justified `nosemgrep` comment on the return line: + +``` +// nosemgrep: ts-silent-success-masking -- +# nosemgrep: py-silent-success-masking -- +``` + +Once every finding below is resolved, flip the task to blocking by adding +`--error` to the scan command in the root `mise.toml` +(`tasks."security:sast:masking"`) and delete this file. + +Regenerate this list with: + +``` +semgrep scan --config .semgrep/silent-success-masking.yaml --exclude '.semgrep/*' --quiet . +``` + +## Baseline findings (2026-06-10, branch point `d912ad2`) + +### Python (`py-silent-success-masking`) + +| Location | Returned default | +|---|---| +| `agent/src/config.py:108` | `""` | +| `agent/src/config.py:130` | `None` | +| `agent/src/config.py:273` | `None` | +| `agent/src/config.py:312` | `""` | +| `agent/src/hooks.py:1064` | `[]` | +| `agent/src/hooks.py:1078` | `[]` | +| `agent/src/hooks.py:1139` | `[]` | +| `agent/src/hooks.py:1193` | `[]` | +| `agent/src/linear_reactions.py:157` | `None` | +| `agent/src/nudge_reader.py:87` | `None` | +| `agent/src/nudge_reader.py:139` | `[]` | +| `agent/src/pipeline.py:123` | `None` | +| `agent/src/post_hooks.py:375` | `None` | +| `agent/src/telemetry.py:471` | `None` | +| `agent/src/telemetry.py:484` | `None` | + +### TypeScript (`ts-silent-success-masking`) + +| Location | Returned default | +|---|---| +| `cdk/src/handlers/github-webhook-processor.ts:409` | `null` | +| `cdk/src/handlers/github-webhook-processor.ts:435` | `null` | +| `cdk/src/handlers/shared/context-hydration.ts:427` | `null` | +| `cdk/src/handlers/shared/context-hydration.ts:562` | `[]` | +| `cdk/src/handlers/shared/context-hydration.ts:688` | `null` | +| `cdk/src/handlers/shared/github-comment.ts:323` | `null` | +| `cdk/src/handlers/shared/github-webhook-verify.ts:73` | `null` | +| `cdk/src/handlers/shared/linear-feedback.ts:119` | `null` | +| `cdk/src/handlers/shared/linear-issue-lookup.ts:109` | `null` | +| `cdk/src/handlers/shared/linear-issue-lookup.ts:162` | `null` | +| `cdk/src/handlers/shared/linear-oauth-resolver.ts:269` | `null` | +| `cdk/src/handlers/shared/linear-oauth-resolver.ts:347` | `null` | +| `cdk/src/handlers/shared/linear-oauth-resolver.ts:380` | `null` | +| `cdk/src/handlers/shared/linear-verify.ts:68` | `null` | +| `cdk/src/handlers/shared/memory.ts:289` | `undefined` | +| `cdk/src/handlers/shared/preflight.ts:108` | `undefined` | +| `cdk/src/handlers/shared/slack-verify.ts:64` | `null` | +| `cdk/src/handlers/shared/validation.ts:62` | `null` | +| `cdk/src/handlers/shared/validation.ts:166` | `undefined` | +| `cdk/src/handlers/webhook-create-task.ts:56` | `null` | +| `cli/src/commands/linear.ts:1672` | `null` | +| `cli/src/commands/linear.ts:1797` | `null` | +| `cli/src/commands/linear.ts:1905` | `null` | +| `cli/src/commands/slack.ts:361` | `null` | +| `cli/src/config.ts:74` | `null` | + +## Triage notes + +Some of these are likely *intentional* degraded-mode fallbacks (e.g. the +webhook signature verifiers return `null` for "not verified", and several +Linear/Slack lookups treat upstream outages as "no data"). Those should get +justified `nosemgrep` allowlist comments rather than rewrites — but each one +needs an explicit decision, which is the point of this gate. diff --git a/.semgrep/silent-success-masking.py b/.semgrep/silent-success-masking.py new file mode 100644 index 00000000..8c6eb80c --- /dev/null +++ b/.semgrep/silent-success-masking.py @@ -0,0 +1,122 @@ +# Test fixtures for py-silent-success-masking (run: semgrep test .semgrep/). + + +def fetch_items() -> list: + raise NotImplementedError + + +def masked_empty_list() -> list: + try: + return fetch_items() + except Exception: + # ruleid: py-silent-success-masking + return [] + + +def masked_none(): + try: + return fetch_items() + except ValueError as exc: + print(exc) + # ruleid: py-silent-success-masking + return None + + +def masked_empty_dict() -> dict: + try: + return {"items": fetch_items()} + except Exception: + # ruleid: py-silent-success-masking + return {} + + +def masked_empty_string() -> str: + try: + return str(fetch_items()) + except Exception: + # ruleid: py-silent-success-masking + return "" + + +def masked_dict_constructor() -> dict: + try: + return {"items": fetch_items()} + except Exception: + # ruleid: py-silent-success-masking + return dict() + + +def masked_bare_except() -> list: + try: + return fetch_items() + except: # noqa: E722 + # ruleid: py-silent-success-masking + return [] + + +def ok_reraise() -> list: + try: + return fetch_items() + except Exception as exc: + print(exc) + # ok: py-silent-success-masking + raise + + +def ok_typed_raise() -> list: + try: + return fetch_items() + except Exception as exc: + # ok: py-silent-success-masking + raise RuntimeError("fetch failed") from exc + + +def ok_meaningful_fallback() -> dict: + try: + return {"items": fetch_items()} + except Exception: + # ok: py-silent-success-masking + return {"error": True} + + +def masked_with_finally() -> list: + try: + return fetch_items() + except Exception: + # ruleid: py-silent-success-masking + return [] + finally: + print("done") + + +def masked_multi_except(): + try: + return fetch_items() + except ValueError: + # ruleid: py-silent-success-masking + return [] + except KeyError: + # ruleid: py-silent-success-masking + return None + + +# A conditional re-raise does not clear the fallthrough default: callers that +# hit the non-fatal path still cannot distinguish failure from empty success. +def masked_conditional_reraise(fatal: bool) -> list: + try: + return fetch_items() + except Exception: + if fatal: + raise + # ruleid: py-silent-success-masking + return [] + + +def ok_return_in_try_body(items: list) -> list: + try: + if not items: + # ok: py-silent-success-masking + return [] + return [i.strip() for i in items] + except Exception as exc: + raise RuntimeError("strip failed") from exc diff --git a/.semgrep/silent-success-masking.ts b/.semgrep/silent-success-masking.ts new file mode 100644 index 00000000..26194b56 --- /dev/null +++ b/.semgrep/silent-success-masking.ts @@ -0,0 +1,117 @@ +// Test fixtures for ts-silent-success-masking (run: semgrep test .semgrep/). +/* eslint-disable */ + +declare function fetchItems(): Promise; +declare function parse(s: string): Record; +declare function log(msg: string): void; + +async function maskedEmptyArray(): Promise { + try { + return await fetchItems(); + } catch (err) { + // ruleid: ts-silent-success-masking + return []; + } +} + +function maskedNull(s: string): Record | null { + try { + return parse(s); + } catch { + // ruleid: ts-silent-success-masking + return null; + } +} + +function maskedEmptyObject(s: string): Record { + try { + return parse(s); + } catch (err) { + log(String(err)); + // ruleid: ts-silent-success-masking + return {}; + } +} + +function maskedUndefined(s: string): Record | undefined { + try { + return parse(s); + } catch { + // ruleid: ts-silent-success-masking + return undefined; + } +} + +function maskedEmptyString(s: string): string { + try { + return JSON.stringify(parse(s)); + } catch { + // ruleid: ts-silent-success-masking + return ""; + } +} + +function okRethrow(s: string): Record { + try { + return parse(s); + } catch (err) { + log(String(err)); + // ok: ts-silent-success-masking + throw err; + } +} + +function okTypedThrow(s: string): Record { + try { + return parse(s); + } catch (err) { + // ok: ts-silent-success-masking + throw new Error(`parse failed: ${String(err)}`); + } +} + +function okMeaningfulFallback(s: string): Record { + try { + return parse(s); + } catch { + // ok: ts-silent-success-masking + return { error: true }; + } +} + +function maskedWithFinally(s: string): string[] { + try { + return [s]; + } catch { + // ruleid: ts-silent-success-masking + return []; + } finally { + log("done"); + } +} + +// A conditional rethrow does not clear the fallthrough default: callers that +// hit the non-fatal path still cannot distinguish failure from empty success. +function maskedConditionalRethrow(s: string): Record | null { + try { + return parse(s); + } catch (err) { + if (s.length > 0) { + throw err; + } + // ruleid: ts-silent-success-masking + return null; + } +} + +function okReturnInTryBody(items: string[]): string[] { + try { + if (items.length === 0) { + // ok: ts-silent-success-masking + return []; + } + return items.map((i) => i.trim()); + } catch (err) { + throw new Error(`trim failed: ${String(err)}`); + } +} diff --git a/.semgrep/silent-success-masking.yaml b/.semgrep/silent-success-masking.yaml new file mode 100644 index 00000000..0345e2df --- /dev/null +++ b/.semgrep/silent-success-masking.yaml @@ -0,0 +1,124 @@ +# Custom semgrep rules: silent-success masking (AI004 / CA-09, issue #257). +# +# Detects catch/except blocks that swallow a failure and return a +# plausible-but-empty default ([] / {} / null / undefined / None / "" ...) +# instead of surfacing the error. Bare-except and empty-catch are covered by +# existing gates (ruff E722/B, eslint); this targets the more dangerous +# variant where the caller sees a "successful" empty result and cannot tell +# it apart from a real one. +# +# Allowlist mechanism: intentional degraded-mode fallbacks get an inline +# // nosemgrep: -- (TS) +# # nosemgrep: -- (Py) +# on the flagged return line (repo convention; precedent in +# scripts/check-types-sync.ts and agent/src/config.py). +# +# Known limitation: a `return null`/`return []` inside a callback *defined +# within* a catch block (e.g. `.map(i => ... return null ...)`) is flagged +# even though it does not mask the outer failure. Rare in practice; use the +# nosemgrep allowlist if it occurs. +# +# Run: mise run security:sast:masking (advisory; emits SARIF) +# Test: semgrep test .semgrep/ +rules: + - id: ts-silent-success-masking + languages: [typescript, javascript] + severity: WARNING + metadata: + category: correctness + confidence: MEDIUM + abca-finding: AI004 + references: + - https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/257 + message: >- + This catch block swallows the error and returns an empty default, so + the caller cannot distinguish failure from a genuinely empty result + (silent-success masking, AI004). Fix: re-throw (`throw err;`), throw a + typed error that adds context, or return a result shape that encodes + the failure. Logging alone is not enough — the failure must reach the + caller. If this fallback is intentional degraded-mode behavior, keep it + and add on the return line + "// nosemgrep: ts-silent-success-masking -- ". + patterns: + - pattern-either: + - pattern: | + try { ... } catch ($E) { ... return $RET; ... } + - pattern: | + try { ... } catch { ... return $RET; ... } + - pattern: | + try { ... } catch ($E) { ... return $RET; ... } finally { ... } + - pattern: | + try { ... } catch { ... return $RET; ... } finally { ... } + - metavariable-regex: + metavariable: $RET + regex: ^(null|undefined|\[\s*\]|\{\s*\}|""|''|``)$ + - focus-metavariable: $RET + + - id: py-silent-success-masking + languages: [python] + severity: WARNING + metadata: + category: correctness + confidence: MEDIUM + abca-finding: AI004 + references: + - https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/257 + message: >- + This except block swallows the error and returns an empty default, so + the caller cannot distinguish failure from a genuinely empty result + (silent-success masking, AI004). Fix: re-raise (`raise`), raise a typed + error that adds context (`raise XError(...) from exc`), or return a + result shape that encodes the failure. Logging alone is not enough — + the failure must reach the caller. If this fallback is intentional + degraded-mode behavior, keep it and add on the return line + "# nosemgrep: py-silent-success-masking -- ". + patterns: + - pattern-either: + - pattern: | + try: + ... + except $EXC: + ... + return $RET + - pattern: | + try: + ... + except $EXC as $E: + ... + return $RET + - pattern: | + try: + ... + except: + ... + return $RET + - pattern: | + try: + ... + except $EXC: + ... + return $RET + finally: + ... + - pattern: | + try: + ... + except $EXC as $E: + ... + return $RET + finally: + ... + - pattern: | + try: + ... + except: + ... + return $RET + finally: + ... + - metavariable-regex: + metavariable: $RET + regex: ^(None|\[\s*\]|\{\s*\}|""|''|\(\s*\)|set\(\s*\)|dict\(\s*\)|list\(\s*\)|tuple\(\s*\))$ + - focus-metavariable: $RET diff --git a/AGENTS.md b/AGENTS.md index a328e248..85e4b2f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -111,8 +111,9 @@ Run `mise tasks --all` (with `MISE_EXPERIMENTAL=1`) for the full list. Common co - **`mise //docs:build`** — Sync and build docs site. - **`mise run security:secrets`** — Gitleaks at repo root. - **`mise run security:sast`** — Semgrep on the repo (root; includes **`agent/`** Python among paths). +- **`mise run security:sast:masking`** — Custom semgrep rules for silent-success masking (`catch`/`except` returning empty defaults, AI004). Advisory while the baseline in **`.semgrep/BASELINE.md`** is open; emits SARIF to `test-reports/`. Allowlist intentional fallbacks with an inline justified `nosemgrep: -- ` comment. - **`mise run security:deps`** — OSV Scanner on **`yarn.lock`** (all JS workspaces) and **`agent/uv.lock`**. -- **`mise run security`** — Runs **`security:secrets`**, **`security:deps`**, **`security:sast`**, **`security:grype`**, **`security:retire`**, **`security:gh-actions`**, and **`//agent:security`**. +- **`mise run security`** — Runs **`security:secrets`**, **`security:deps`**, **`security:sast`**, **`security:sast:masking`**, **`security:grype`**, **`security:retire`**, **`security:gh-actions`**, and **`//agent:security`**. - **`mise run security:retire`** — Retire.js on CDK, CLI, and docs packages. - **`mise run security:gh-actions`** — Static analysis of GitHub Actions under **`.github/`** ([zizmor](https://github.com/zizmorcore/zizmor)). - **`mise run hooks:install`** — Re-install **[prek](https://github.com/j178/prek)** git hooks (also run automatically at the end of **`mise run install`** inside a Git checkout). See [CONTRIBUTING.md](./CONTRIBUTING.md) if `core.hooksPath` blocks install. diff --git a/mise.toml b/mise.toml index 6f5f351b..99884d0c 100644 --- a/mise.toml +++ b/mise.toml @@ -95,6 +95,17 @@ run = "gitleaks protect --staged --no-banner" description = "SAST scan with semgrep (auto + OWASP top 10)" run = "semgrep scan --config auto --config p/python --config p/typescript --config p/owasp-top-ten --config p/security-audit --error --quiet ." +[tasks."security:sast:masking"] +description = "Custom semgrep rules: silent-success masking (AI004, #257). ADVISORY until the .semgrep/BASELINE.md findings are triaged, then flip to blocking by adding --error" +# `semgrep test` validates the rules against their .semgrep/ fixtures first. +# The scan excludes those fixtures (they trigger by design) and writes SARIF +# to test-reports/ (gitignored) so findings stay agent-routable (CA-06). +run = [ + "semgrep test .semgrep/", + "mkdir -p test-reports", + "semgrep scan --config .semgrep/silent-success-masking.yaml --exclude '.semgrep/*' --sarif-output=test-reports/semgrep-silent-success-masking.sarif --quiet .", +] + [tasks."security:deps"] description = "Audit dependencies for known vulnerabilities (osv-scanner)" # Yarn workspaces use the repo-root lockfile only; do not scan stale cli/docs yarn.lock copies. @@ -123,6 +134,7 @@ run = [ { task = "security:secrets" }, { task = "security:deps" }, { task = "security:sast" }, + { task = "security:sast:masking" }, { task = "security:grype" }, { task = "security:retire" }, { task = "security:gh-actions" },