chore(security): masking rule precision — annotate correct fail-closed sites + fix nosemgrep placement (#790)

.semgrep/silent-success-masking.py

@@ -112,6 +112,20 @@ def masked_conditional_reraise(fatal: bool) -> list:
         return []
 
 
+# Classify-then-reraise: a "not found" is a legitimate empty success, and
+# every other error reraises on the fallthrough — so the caller can still tell
+# failure from empty. The reraise comes AFTER the classified return, which is
+# what distinguishes this from masked_conditional_reraise (raise guarded first).
+def ok_classify_then_reraise(fetch, is_not_found):
+    try:
+        return fetch()
+    except Exception as exc:
+        # ok: py-silent-success-masking
+        if is_not_found(exc):
+            return None  # genuine empty success
+        raise  # everything else fails closed
+
+
 def ok_return_in_try_body(items: list) -> list:
     try:
         if not items:

.semgrep/silent-success-masking.ts

@@ -104,6 +104,23 @@ function maskedConditionalRethrow(s: string): Record<string, unknown> | null {
   }
 }
 
+// Classify-then-rethrow: a "not found" is a legitimate empty success, and
+// every other error rethrows on the fallthrough — so the caller can still tell
+// failure from empty. The rethrow comes AFTER the classified return, which is
+// what distinguishes this from maskedConditionalRethrow (throw guarded first).
+function okClassifyThenRethrow(
+  fetch: () => string,
+  isNotFound: (e: unknown) => boolean,
+): string | undefined {
+  try {
+    return fetch();
+  } catch (err) {
+    // ok: ts-silent-success-masking
+    if (isNotFound(err)) return undefined; // genuine empty success
+    throw err; // everything else fails closed
+  }
+}
+
 function okReturnInTryBody(items: string[]): string[] {
   try {
     if (items.length === 0) {

.semgrep/silent-success-masking.yaml

@@ -13,6 +13,28 @@
 # on the flagged return line (repo convention; precedent in
 # scripts/check-types-sync.ts and agent/src/config.py).
 #
+# PLACEMENT FOOTGUN: `focus-metavariable: $RET` anchors the finding on the
+# `return` statement, so the `nosemgrep` TOKEN must sit on that return line or
+# the line IMMEDIATELY above it — that is the only span semgrep scans for a
+# suppression. A multi-line justification whose FIRST line carries the token
+# and whose continuation line then abuts the return does NOT bind (the abutting
+# line lacks the token). Put the prose above and the `# nosemgrep: ...` line
+# last, directly over the return.
+#
+# CLASSIFY-THEN-RETHROW EXEMPTION (the `pattern-not` on each rule): a catch that
+# returns an empty default and THEN rethrows on the fallthrough
+# (`catch (e) { if (isNotFound(e)) return undefined; throw e; }`) is not masking —
+# the classified empty is a genuine success and every other error still reaches
+# the caller. The return-before-throw ORDER is what distinguishes it from the
+# `maskedConditionalRethrow` / `masked_conditional_reraise` fixtures (throw
+# guarded FIRST, then an unconditional masking return), which stay flagged.
+# Trade-off (deliberate): this also exempts the genuinely-ambiguous
+# `catch (e) { if (benign(e)) return null; throw e; }`, where the benign branch
+# does mask. We accept that — a per-site `nosemgrep` cannot express ordering and
+# would train contributors to annotate correctness away (#730), and the shape is
+# rare; flag such a site in review instead. Callback returns (below) are the
+# other known gap.
+#
 # 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
@@ -50,6 +72,14 @@ rules:
               try { ... } catch ($E) { ... return $RET; ... } finally { ... }
           - pattern: |
               try { ... } catch { ... return $RET; ... } finally { ... }
+      # Classify-then-rethrow is NOT masking: a classified return that is
+      # followed by a throw on the catch's fallthrough surfaces every other
+      # error, so the caller can still distinguish failure from empty success.
+      # The order is load-bearing — it is exactly what separates this shape
+      # from the `maskedConditionalRethrow` fixture (throw guarded FIRST, then
+      # an unconditional masking return), which stays flagged.
+      - pattern-not: |
+          try { ... } catch ($E) { ... return $RET; ... throw $ERR; }
       - metavariable-regex:
           metavariable: $RET
           regex: ^(null|undefined|\[\s*\]|\{\s*\}|""|''|``)$
@@ -118,6 +148,26 @@ rules:
                 return $RET
               finally:
                 ...
+      # Classify-then-reraise is NOT masking (see the TS twin above): a
+      # classified return followed by a `raise` on the fallthrough surfaces
+      # every other error. The order distinguishes it from
+      # masked_conditional_reraise (raise guarded FIRST, then a masking return).
+      - pattern-not: |
+          try:
+            ...
+          except $EXC as $E:
+            ...
+            return $RET
+            ...
+            raise
+      - pattern-not: |
+          try:
+            ...
+          except $EXC as $E:
+            ...
+            return $RET
+            ...
+            raise $ERR
       - metavariable-regex:
           metavariable: $RET
           regex: ^(None|\[\s*\]|\{\s*\}|""|''|\(\s*\)|set\(\s*\)|dict\(\s*\)|list\(\s*\)|tuple\(\s*\))$

AGENTS.md

@@ -20,7 +20,7 @@ mise run hooks:install    # prek git hooks (also runs at end of install)
 mise run hooks:run        # pre-commit + pre-push locally
 ```
 
-Security subtasks: `mise run security:secrets`, `security:sast`, `security:sast:masking`, `security:deps`, `security:retire`, `security:gh-actions`. For `security:sast:masking` allowlist intentional fallbacks with an inline `nosemgrep: <rule-id> -- <reason>` comment.
+Security subtasks: `mise run security:secrets`, `security:sast`, `security:sast:masking`, `security:deps`, `security:retire`, `security:gh-actions`. For `security:sast:masking` allowlist intentional fallbacks with an inline `nosemgrep: <rule-id> -- <reason>` comment on the flagged `return` line (or the line immediately above) — the rule anchors on the `return`, so a token placed higher does not bind.
 
 Package commands: [cdk/AGENTS.md](./cdk/AGENTS.md), [cli/AGENTS.md](./cli/AGENTS.md), [agent/AGENTS.md](./agent/AGENTS.md), [docs/AGENTS.md](./docs/AGENTS.md).
 

agent/src/observability.py

@@ -81,8 +81,7 @@ def current_otel_trace_id() -> str | None:
         # up there, transform to that form (the timestamp is the first 8 hex chars).
         return trace.format_trace_id(ctx.trace_id)
     except Exception:
-        # nosemgrep: py-silent-success-masking -- trace id is a graceful-missing
-        # correlation field; a tracer fault must not fail the caller's write path.
+        # nosemgrep: py-silent-success-masking -- tracer fault must not fail the write path
         return None
 
 

cdk/src/handlers/registry-publish.ts

@@ -113,6 +113,7 @@ function parseBody(raw: string | null): RegistryPublishRequest | null {
   try {
     return JSON.parse(raw) as RegistryPublishRequest;
   } catch {
+    // nosemgrep: ts-silent-success-masking -- malformed JSON is an expected client-input class, not a swallowed fault; null IS the failure encoding and the caller turns it into a 400 VALIDATION_ERROR.
     return null;
   }
 }

cdk/test/handlers/registry-handlers.test.ts

@@ -109,6 +109,18 @@ describe('registry-publish handler', () => {
     expect(res.statusCode).toBe(400);
   });
 
+  test('400 VALIDATION_ERROR on malformed JSON body', async () => {
+    // parseBody returns null on a JSON.parse failure and the handler maps that
+    // to a 400 — the fail-closed path the nosemgrep suppression on parseBody claims.
+    const res = await publishHandler(makeEvent({
+      requestContext: { ...makeEvent().requestContext, authorizer: withGroups(['RegistryPublisher']) },
+      body: '{not json',
+    }));
+    expect(res.statusCode).toBe(400);
+    expect(JSON.parse(res.body).error.code).toBe('VALIDATION_ERROR');
+    expect(mockPublish).not.toHaveBeenCalled();
+  });
+
   test('400 on non-exact asset_version', async () => {
     const res = await publishHandler(makeEvent({
       requestContext: { ...makeEvent().requestContext, authorizer: withGroups(['RegistryPublisher']) },

cli/src/commands/jira.ts

@@ -231,9 +231,10 @@ export async function upsertOauthSecret(
             ...(typeof value.app_actor_configured_at === 'string'
               && { app_actor_configured_at: value.app_actor_configured_at }),
           };
-        } catch { // nosemgrep: ts-silent-success-masking -- OAuth setup intentionally replaces malformed secret JSON
+        } catch {
           // OAuth setup is the recovery path for malformed secret JSON. It
-          // deliberately replaces the bad value instead of preserving it.
+          // deliberately replaces the bad value instead of preserving it (the
+          // catch is empty: no masked return, so no masking suppression needed).
         }
       }
       const put = await client.send(new PutSecretValueCommand({

cli/src/linear-oauth.ts

@@ -493,7 +493,6 @@ export async function readExistingOauthTokens(
   try {
     raw = await fetchSecretString();
   } catch (err) {
-    // nosemgrep: ts-silent-success-masking -- "no such secret" IS the empty success here: a first install has no bundle, so there is no token to carry forward. Every other error rethrows below, which is the fail-closed half of the contract this function exists for.
     if (isNotFound(err)) return undefined; // genuine first install
     throw err;
   }

mise.toml

@@ -172,6 +172,7 @@ description = "Custom semgrep rules: silent-success masking (AI004, #257). Block
 # to test-reports/ (gitignored) so findings stay agent-routable (CA-06).
 run = [
   "semgrep test .semgrep/",
+  "node scripts/check-masking-suppression-placement.mjs",
   "mkdir -p test-reports",
   "semgrep scan --config .semgrep/silent-success-masking.yaml --exclude '.semgrep/*' --sarif-output=test-reports/semgrep-silent-success-masking.sarif --error --quiet .",
 ]
@@ -191,6 +192,7 @@ description = "Masking scan ratcheted to newly-introduced findings only (per-PR
 # run anywhere; the per-PR CI job sets SEMGREP_MASKING_BASELINE to the PR base SHA.
 run = [
   "semgrep test .semgrep/",
+  "node scripts/check-masking-suppression-placement.mjs",
   "semgrep scan --config .semgrep/silent-success-masking.yaml --exclude '.semgrep/*' --baseline-commit \"${SEMGREP_MASKING_BASELINE:-origin/main}\" --error --quiet .",
 ]
 

scripts/check-masking-suppression-placement.mjs

@@ -0,0 +1,71 @@
+#!/usr/bin/env node
+// Guards the silent-success-masking suppression footgun (#790).
+//
+// `focus-metavariable: $RET` in .semgrep/silent-success-masking.yaml anchors each
+// finding on the `return` statement, so a `nosemgrep: <rule> -- ...` token only
+// binds when it sits ON that return line or the line IMMEDIATELY above it. A
+// multi-line justification whose token drifts higher silently STOPS suppressing
+// (or, worse, looks suppressed while the finding is live). This check asserts
+// every masking suppression token is placed where semgrep will honour it, so the
+// drift that mis-bound agent/src/observability.py cannot recur unnoticed.
+//
+// Run: node scripts/check-masking-suppression-placement.mjs  (wired into
+// `security:sast:masking` and `:masking:range`).
+
+import { execFileSync } from 'node:child_process';
+import { readFileSync } from 'node:fs';
+
+const TOKEN = /nosemgrep:[^\n]*\bsilent-success-masking\b/;
+// The rule anchors on a return; re-throwing (`raise`/`throw`) is the ok path but
+// is accepted here too so the guard never fights a legitimate placement.
+//
+// KNOWN LIMITATION (deliberate): this matches the bare word anywhere on the line,
+// including inside a comment or a string literal. So a token whose next line only
+// *mentions* `return`/`raise`/`throw` in prose — `// we return early below` — is
+// accepted even though the real anchor sits further down and the suppression does
+// not bind. That is a false negative for THIS guard, never a false positive: it
+// can wave through a mis-placed token, but it cannot flag a correct one.
+//
+// Left as-is on purpose. This check is belt-and-suspenders over `semgrep test
+// .semgrep/`, which is the actual correctness gate — the scan itself reports the
+// finding when a suppression fails to bind, so the failure mode here is "the
+// friendly early warning stayed quiet", not "a mask shipped undetected".
+// Tightening it (strip comments/strings before testing) trades that simplicity
+// for a language-aware parser in a defensive lint. Revisit if a real mis-binding
+// ever slips past.
+const ANCHOR = /\b(return|raise|throw)\b/;
+const SOURCE_EXT = /\.(ts|tsx|js|mjs|cjs|py)$/;
+
+// Tracked source files only; the .semgrep/ fixtures use `ok:`/`ruleid:` markers,
+// not `nosemgrep:`, so excluding them is belt-and-suspenders.
+const files = execFileSync('git', ['ls-files', '-z'], { encoding: 'utf8' })
+  .split('\0')
+  .filter((f) => f && SOURCE_EXT.test(f) && !f.startsWith('.semgrep/'));
+
+const violations = [];
+for (const file of files) {
+  const lines = readFileSync(file, 'utf8').split('\n');
+  lines.forEach((line, i) => {
+    if (!TOKEN.test(line)) return;
+    const onTokenLine = ANCHOR.test(line); // inline `return x; // nosemgrep: ...`
+    const onNextLine = i + 1 < lines.length && ANCHOR.test(lines[i + 1]);
+    if (!onTokenLine && !onNextLine) {
+      violations.push({ file, line: i + 1, text: line.trim() });
+    }
+  });
+}
+
+if (violations.length > 0) {
+  console.error(
+    `\n✖ ${violations.length} silent-success-masking suppression(s) mis-placed — the token must sit on the flagged \`return\` line or the line directly above it, or it does not bind:\n`,
+  );
+  for (const v of violations) {
+    console.error(`  ${v.file}:${v.line}\n      ${v.text}`);
+  }
+  console.error(
+    '\nMove the `# nosemgrep:`/`// nosemgrep:` line directly above the `return`, with any prose above that. See .semgrep/silent-success-masking.yaml (PLACEMENT FOOTGUN).\n',
+  );
+  process.exit(1);
+}
+
+console.log('✓ all silent-success-masking suppressions are correctly placed');