Skip to content

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

Open
ClintEastman02 wants to merge 2 commits into
aws-samples:mainfrom
ClintEastman02:chore/790-masking-rule-precision
Open

chore(security): masking rule precision — annotate correct fail-closed sites + fix nosemgrep placement (#790)#862
ClintEastman02 wants to merge 2 commits into
aws-samples:mainfrom
ClintEastman02:chore/790-masking-rule-precision

Conversation

@ClintEastman02

Copy link
Copy Markdown
Contributor

Closes #790 (Category 0 of the #756 silent-success-masking gate).

What changed vs. what the issue proposed

The issue proposed tightening the rule with a pattern-not for a catch/except that contains a throw/raise. Investigation showed that can't be delivered as a clean precision win, so this PR takes a different route. Full reasoning below.

Why not the pattern-not

cli/src/linear-oauth.ts's correct classify-then-rethrow —

catch (err) {
  if (isNotFound(err)) return undefined; // "not found" legitimately means nothing
  throw err;                             // everything else fails closed
}

— is structurally indistinguishable from the conditional-rethrow masking the rule was built to catch, encoded in the maskedConditionalRethrow / masked_conditional_reraise fixtures shipped in the original rule PR #311:

catch (err) { if (cond) throw err; return null; } // sometimes throws, otherwise masks

Both "catch, contain a throw, and return an empty default." Semgrep's ... ellipsis matching is too loose to anchor on the throw being the terminal / fallthrough statement, so any pattern-not broad enough to clear the linear-oauth false positive also silences the #311 detection — a real, deliberately-flagged masking pattern. Weakening a security rule to remove two annotations is the wrong trade, so we annotate instead.

Changes

  • cli/src/linear-oauth.ts (readExistingWebhookSecret) — justified nosemgrep on the classified return undefined: "no such secret" is the empty success (a first install has no bundle); every other error rethrows below (the fix(cli): linear setup clobbers a second workspace's webhook signing secret with the stack-wide one → 401 Invalid signature #611 fail-closed contract). Mirrors the already-annotated twin readExistingOauthTokens.
  • cdk/src/handlers/registry-publish.ts (parseBody) — justified nosemgrep: malformed JSON is an expected client-input class, and null is the failure encoding — the caller turns it into a 400 VALIDATION_ERROR. This site is structurally identical to a real mask, so no rule change could clear it.
  • agent/src/observability.py — the existing nosemgrep never bound: it was a multi-line comment whose continuation line abutted the return None, leaving the token two lines above the finding. Restructured so the # nosemgrep: token sits directly above the return.
  • .semgrep/silent-success-masking.yaml — documented the placement footgun in the rule header (focus-metavariable: $RET anchors the finding on the return; the nosemgrep token must be on the return line or the line immediately above).

Verification

Line numbers in the issue were verified against the PR #788 branch and had drifted; all sites re-verified on current main.

…d sites + fix nosemgrep placement (aws-samples#790)

Category 0 of the aws-samples#756 silent-success-masking gate. Investigation showed the
issue's proposed rule change (a `pattern-not` for a catch/except containing a
throw/raise) cannot be delivered: linear-oauth's correct classify-then-rethrow
is structurally indistinguishable from the conditional-rethrow masking the rule
deliberately catches (the `maskedConditionalRethrow`/`masked_conditional_reraise`
fixtures from the original rule PR aws-samples#311). Any pattern-not broad enough to clear
the false positive also silences a real detection. So instead of weakening the
rule, this annotates the three mis-flagged sites and fixes a suppression that
never bound:

- cli/src/linear-oauth.ts (readExistingWebhookSecret): justified nosemgrep on the
  classified `return undefined` — "no such secret" IS the empty success; every
  other error rethrows (the aws-samples#611 fail-closed contract). Mirrors the already-
  annotated twin readExistingOauthTokens.
- cdk/src/handlers/registry-publish.ts (parseBody): justified nosemgrep — malformed
  JSON is an expected client-input class; null IS the failure encoding and the
  caller returns 400 VALIDATION_ERROR.
- agent/src/observability.py: the existing nosemgrep did not bind (multi-line
  comment whose continuation line abutted the return, token two lines up).
  Restructured so the `# nosemgrep:` token sits directly above `return None`.
- .semgrep/silent-success-masking.yaml: documented the placement footgun in the
  rule header (focus-metavariable:$RET anchors on the return; the token must be on
  the return line or the line immediately above).

Verification: `semgrep test .semgrep/` passes; the three sites drop out of the
full masking scan with no new findings; the ratcheted PR gate reports zero new
findings; ruff, eslint, tsc, and the agent observability tests are green.
@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@12c9b63). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage        ?   92.48%           
=======================================
  Files           ?      328           
  Lines           ?    95035           
  Branches        ?    10504           
=======================================
  Hits            ?    87891           
  Misses          ?     7144           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ion, malformed-JSON test

- observability.py: move nosemgrep token directly above return None (rule
  placement footgun), tighten justification
- linear-oauth.ts: fix suppression attribution to the aws-samples#612-B1 fail-closed
  contract (was mis-citing aws-samples#611)
- registry-handlers.test.ts: add 400 VALIDATION_ERROR malformed-JSON test
  proving the parseBody fail-closed path the suppression claims
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(security): masking rule precision — stop flagging correct fail-closed code + fix mis-placed suppression (#756 Cat 0)

2 participants