Skip to content

chore(security): ratchet silent-success-masking gate + clear 5 Category 3 findings (#756) - #788

Merged
isadeks merged 6 commits into
aws-samples:mainfrom
ClintEastman02:chore/756-masking-gate-cat3
Aug 31, 2026
Merged

isadeks merged 6 commits into
aws-samples:mainfrom
ClintEastman02:chore/756-masking-gate-cat3

Conversation

@ClintEastman02

@ClintEastman02 ClintEastman02 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Scopes issue #756 to the two pieces from @scottschreckengaust's triage that weren't claimed for follow-up issues: the missing PR-level gate (the actual cause of the drift) and the 5 genuinely-defensible Category 3 suppressions. Categories 0–2 (rule false positives, registry parse failures, best-effort lookups) are left for the separate issues proposed in the triage.

Area

  • agent — Python runtime / Docker image
  • cdk — infrastructure, handlers, constructs
  • tooling — root mise.toml, scripts, CI workflows

Related

Changes

The gate — a ratchet, not a full scan (fixes the drift). The blocking full-repo masking scan (security:sast:masking) only runs in security.yml (workflow_dispatch + weekly Monday cron only — not push-to-main) and the pre-push hook. security-pr.yml never ran it, so new findings land on main unchecked while every contributor is blocked on untouched debt on git push. That asymmetry is why the count grew 21 → 25 (the #664 registry stack) and why #755 needed --no-verify.

  • Add security:sast:masking:range to mise.tomlsemgrep --baseline-commit, so it reports only findings a branch ADDS relative to the base. Mirrors the existing security:secrets:range. Defaults baseline to origin/main so it's safe to run anywhere.
  • Wire it into security-pr.yml: the existing range-resolution step now also emits a baseline SHA (PR base / merge-group base / HEAD on dispatch), and a new step runs the ratchet. A PR that adds a masking finding now fails at PR time; the 25 pre-existing findings on main are not re-flagged.

The 5 Category 3 suppressions — genuinely-contract fail-open / feature-detect only, justified inline on the return line (repo convention; a comment above doesn't bind under the rule's focus-metavariable: $RET). Not blanket-annotated, per #730.

Site Why fail-open is the contract
cdk/.../jira-webhook-processor.ts (comments) Third-party comments are advisory enrichment; a screening outage drops them (logged), the reporter's task proceeds.
cdk/.../linear-webhook-processor.ts (project docs + comments) Same posture, mirrors the Jira processor.
agent/src/clarification_tool.py Optional-dependency feature detect for claude_agent_sdk; None is the documented "server not registered" signal, marker fallback covers it.
agent/src/hooks.py The stuck guard is advisory-only (see docstring); max_turns is the real backstop, so [] is "no steer this turn", not a masked failure.

Masking scan drops 25 → 20 active findings (semgrep test .semgrep/ still 2/2).

Scope note — this does not green main. The gate stops new masking findings at PR time; it does not clear the pre-existing debt. The full security:sast:masking scan (weekly security.yml + the pre-push hook) still reports the remaining findings, so main's weekly security suite stays red and the pre-push hook keeps forcing --no-verify until the follow-ups land. #790 (Cat 0), #791 (Cat 1 — registry attribution-erasure, security), and #792 (Cat 2) carry that remainder.

Verification

semgrep (scan + fixtures), ruff check/format, eslint (cdk), zizmor, and python -m py_compile all pass on the changed files. The security:sast:masking:range task was confirmed to exit non-zero on a newly-introduced finding and zero when only pre-existing debt remains.

⚠️ Committed/pushed with --no-verify: the agent-quality pre-push suite has an environment-specific test-isolation bug — test_registry_loader.py::TestMcpJsonNotCommittable::test_untracked_mcp_json_cannot_be_staged corrupts the real repo HEAD (commits a tree-deleting "init" commit and rewrites user.name/user.email) when run from the repo root, though it passes in isolation. Flagging separately; not touched by this PR.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

…tes (aws-samples#756)

Adds a per-PR gate for the py/ts-silent-success-masking rule (AI004, aws-samples#257)
and clears the genuinely-defensible fail-open findings, per the triage on
aws-samples#756.

Gate (the actual fix for the drift): the blocking full-repo masking scan
only runs in security.yml (push-to-main/weekly) and the pre-push hook, so
new findings land on main unchecked while every contributor is blocked on
untouched debt (the aws-samples#664 drift, and why aws-samples#755 needed --no-verify). Add
`security:sast:masking:range` (semgrep --baseline-commit, mirrors
`security:secrets:range`) and wire it into security-pr.yml so a PR that
ADDS a masking finding fails at PR time without re-flagging the 25
pre-existing findings on main.

Suppressions (Category 3 only -- genuinely-contract fail-open/feature-detect;
justified inline on the return line per repo convention, not blanket-
annotated):
- jira/linear webhook processors: guardrail screening fail-open -- third-party
  comments/docs are advisory enrichment; a screening outage drops them
  (logged) and the reporter's task still proceeds.
- clarification_tool.py: optional-dependency feature detect for
  claude_agent_sdk; None is the documented "not registered" signal.
- hooks.py: the stuck guard is advisory-only; max_turns is the real backstop.

Masking scan drops 25 -> 20 active findings. The remaining 20 (rule false
positives, registry parse failures, best-effort lookups) are Categories 0-2
and tracked separately.

Note: committed with --no-verify because the agent-quality pre-push suite has
an environment-specific test-isolation bug (test_untracked_mcp_json_cannot_be_
staged corrupts repo HEAD when run from the root); all relevant checks
(semgrep, ruff, eslint, yaml, zizmor) were verified manually.
@ClintEastman02
ClintEastman02 requested review from a team as code owners August 25, 2026 14:23
@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

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

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@780161b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
cdk/src/handlers/linear-webhook-processor.ts 0.00% 2 Missing ⚠️
agent/src/hooks.py 0.00% 1 Missing ⚠️
cdk/src/handlers/jira-webhook-processor.ts 0.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #788   +/-   ##
=======================================
  Coverage        ?   92.34%           
=======================================
  Files           ?      320           
  Lines           ?    90433           
  Branches        ?     8994           
=======================================
  Hits            ?    83508           
  Misses          ?     6925           
  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.

bgagent and others added 2 commits August 26, 2026 11:39
…e + pin required-check binding (aws-samples#756)

Follow-up hardening from the PR aws-samples#788 review (silent-failure-hunter / security-review INFO findings).

- security:sast:masking:range now runs `semgrep test .semgrep/` first, like the
  full scan does. Previously the per-PR ratchet skipped fixture validation, so a
  PR that WEAKENED the rule or broke a fixture would only be caught weekly / at
  pre-push (currently --no-verify bypassable), not at PR time.

- Document in security-pr.yml that the "Secrets, deps, and workflow scan" job is
  a required status check (main ruleset) re-run in the merge queue with trunk's
  workflow, so a fork PR cannot gut the masking step to slip a regression past
  merge. Guards against accidentally removing the step or renaming the job.

- Drop the hardcoded "25 pre-existing findings" count from the range-task comment
  (was already inconsistent with aws-samples#756's title); point at the issue instead.
@isadeks

isadeks commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Clean ratchet, and the five nosemgrep justifications each carry a concrete "why this is not a bug" rationale — that's exactly the right pattern for Category 3. One tiny nit:

  • echo "Resolved SEMGREP_MASKING_BASELINE='$(grep '^baseline=' "$GITHUB_OUTPUT" | ...)'" re-reads $GITHUB_OUTPUT after writing to it. Works, but if a future case block writes another baseline= line the tail -n1 masks that. Consider capturing the value in a shell variable before the echo … >> "$GITHUB_OUTPUT" and echoing the variable. Same for the gitleaks range= echo above.

Otherwise LGTM — glad to see the required-status-check binding called out in the comment above the step.

isadeks and others added 2 commits August 31, 2026 14:58
…UTPUT (aws-samples#788)

Address isadeks review nit on aws-samples#788: the Resolve step re-read $GITHUB_OUTPUT
via grep|tail -n1 after writing to it, so a future case block adding another
range=/baseline= line would silently mask the intended value in the log echo.
Capture range and baseline in shell variables, write those to $GITHUB_OUTPUT
once, and echo the variables directly.
@ClintEastman02

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in 586a7a0. The Resolve commit range step now captures range/baseline in shell variables, writes each to $GITHUB_OUTPUT once, and echoes the variables directly, so the log lines no longer depend on grep … | tail -n1 re-reading the file (which a future case block writing another baseline=/range= line could have masked). Applied to both the gitleaks range= and the semgrep baseline= echoes.

@isadeks
isadeks added this pull request to the merge queue Aug 31, 2026
Merged via the queue into aws-samples:main with commit c33befa Aug 31, 2026
4 checks passed
@scottschreckengaust scottschreckengaust added the v1 Version 1 label Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1 Version 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants