diff --git a/socket_basics/core/connector/opengrep/__init__.py b/socket_basics/core/connector/opengrep/__init__.py index ff4aebb..c557022 100644 --- a/socket_basics/core/connector/opengrep/__init__.py +++ b/socket_basics/core/connector/opengrep/__init__.py @@ -727,9 +727,17 @@ def generate_notifications(self, components: List[Dict[str, Any]]) -> Dict[str, groups: Dict[str, List[Dict[str, Any]]] = {} for c in comps_map.values(): for a in c.get('alerts', []): - alert_action = (a.get('action') or '').strip().lower() - if alert_action == 'ignore': + # Skip suppressed alerts. A rule disabled via *_disabled_rules or + # matched by a local SAST ignore override is forced to action + # 'ignore' and tagged with an actionReason by the normalizer. Gate + # on that explicit reason rather than action == 'ignore', because + # 'ignore' is also the default action the normalizer derives for + # low-severity findings -- those should still notify when a user + # opts in to low severities. Suppressed alerts still ship in the + # uploaded facts; this only gates notifications. + if (a.get('actionReason') or '') in ('disabled_rule', 'sast_ignore_override'): continue + # Filter by severity - only include alerts that match allowed severities alert_severity = (a.get('severity') or '').strip().lower() if alert_severity and hasattr(self, 'allowed_severities') and alert_severity not in self.allowed_severities: diff --git a/tests/test_notification_action_filter.py b/tests/test_notification_action_filter.py new file mode 100644 index 0000000..6b25f2d --- /dev/null +++ b/tests/test_notification_action_filter.py @@ -0,0 +1,149 @@ +"""Suppressed findings must be excluded from notifications. + +A rule disabled via *_disabled_rules (or matched by a local SAST ignore +override) is forced to action 'ignore' and tagged with an ``actionReason`` by +the normalizer. The dashboard honors that, but notification generation +previously keyed off severity alone, so suppressed critical/high findings still +posted to the PR comment, Slack, etc. + +generate_notifications() now drops suppressed alerts before building any notifier +output. It gates on the explicit ``actionReason`` rather than +``action == 'ignore'``, because 'ignore' is also the default action the +normalizer derives for low-severity findings -- those must still notify when a +user opts in to low severities. Suppressed alerts still ship in the uploaded +facts; only notifications are gated. +""" + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +from scripts.preview_pr_comments import make_mock_config + +from socket_basics.core.connector.normalizer import _normalize_alert +from socket_basics.core.connector.opengrep import OpenGrepScanner + + +def _make_scanner(config, allowed_severities=("critical", "high")): + # generate_notifications() only needs .config and .allowed_severities; + # bypass __init__, which shells out to build the opengrep rule set. + scanner = OpenGrepScanner.__new__(OpenGrepScanner) + scanner.config = config + scanner.allowed_severities = set(allowed_severities) + return scanner + + +def _alert(rule_id, severity, snippet, line): + # A raw alert as the connector emits it, before normalization. No action or + # actionReason -- the normalizer derives those from config + severity. + return { + "title": rule_id, + "severity": severity, + "subType": "sast-generic", + "location": {"path": "slik/domain/GetActionablePlanStatusesUseCase.kt"}, + "props": { + "ruleId": rule_id, + "filePath": "slik/domain/GetActionablePlanStatusesUseCase.kt", + "startLine": line, + "endLine": line, + "codeSnippet": snippet, + }, + } + + +def _normalize(scanner, components): + # Run every alert through the real normalizer with the scanner's config, so + # actionReason and default actions are set exactly as in the pipeline. + for c in components: + c["alerts"] = [_normalize_alert(a, connector=scanner) for a in c["alerts"]] + return components + + +@pytest.fixture +def config(): + cfg = make_mock_config() + # Suppress a SAST rule the way org/repo config lands in config: the dashboard + # API's kotlinDisabledRules maps to kotlin_disabled_rules, as does a repo-level + # *_disabled_rules action input. + cfg["kotlin_disabled_rules"] = "kotlin-sql-injection" + return cfg + + +def test_suppressed_alert_excluded_from_pr_notifications(config): + scanner = _make_scanner(config) + components = _normalize(scanner, [ + { + "id": "GetActionablePlanStatusesUseCase.kt", + "name": "GetActionablePlanStatusesUseCase.kt", + "type": "generic", + "alerts": [ + _alert("kotlin-sql-injection", "critical", "SUPPRESSED_SNIPPET_XYZ", 66), + _alert("kotlin-weak-hash", "critical", "ACTIVE_SNIPPET_ABC", 70), + ], + } + ]) + + # The normalizer flagged only the disabled rule as suppressed. + alerts = components[0]["alerts"] + assert alerts[0]["actionReason"] == "disabled_rule" + assert "actionReason" not in alerts[1] + + result = scanner.generate_notifications(components) + content = "\n".join(item["content"] for item in result.get("github_pr", [])) + + # The active finding survives. + assert "kotlin-weak-hash" in content + assert "ACTIVE_SNIPPET_ABC" in content + + # The suppressed finding is gone, even though it is critical. + assert "kotlin-sql-injection" not in content + assert "SUPPRESSED_SNIPPET_XYZ" not in content + + # Summary counts only the non-suppressed critical. + assert "Critical: 1" in content + assert "Critical: 2" not in content + + +def test_all_suppressed_yields_no_notifications(config): + scanner = _make_scanner(config) + components = _normalize(scanner, [ + { + "id": "f.kt", + "name": "f.kt", + "type": "generic", + "alerts": [_alert("kotlin-sql-injection", "critical", "X", 66)], + } + ]) + + # Every alert suppressed -> no groups -> empty per-notifier mapping. + assert scanner.generate_notifications(components) == {} + + +def test_low_severity_not_suppressed_still_notifies(config): + # The normalizer maps low severity to the default action 'ignore'. That must + # NOT be treated as suppression: a user who opts in to low severities should + # still see those findings in the PR comment. Regression guard against gating + # notifications on action == 'ignore' instead of the explicit actionReason. + scanner = _make_scanner(config, allowed_severities=("critical", "high", "low")) + components = _normalize(scanner, [ + { + "id": "f.kt", + "name": "f.kt", + "type": "generic", + "alerts": [_alert("kotlin-style-nit", "low", "LOW_SNIPPET_LMN", 12)], + } + ]) + + # Low maps to action 'ignore' but carries no actionReason (not suppressed). + low = components[0]["alerts"][0] + assert low["action"] == "ignore" + assert "actionReason" not in low + + result = scanner.generate_notifications(components) + content = "\n".join(item["content"] for item in result.get("github_pr", [])) + + # The opted-in low-severity finding still notifies. + assert "kotlin-style-nit" in content + assert "LOW_SNIPPET_LMN" in content diff --git a/tests/test_sast_ignore_overrides.py b/tests/test_sast_ignore_overrides.py index de36943..04cccc1 100644 --- a/tests/test_sast_ignore_overrides.py +++ b/tests/test_sast_ignore_overrides.py @@ -171,10 +171,19 @@ def test_count_blocking_alerts_skips_ignored_findings(): def test_opengrep_notifications_skip_ignored_findings(): scanner = OpenGrepScanner(Config({'workspace': '.'})) + # A genuinely-suppressed finding carries an actionReason (the normalizer + # attaches one for disabled rules and SAST ignore overrides). Notifications + # gate on that reason rather than action == 'ignore', because 'ignore' is + # also the default action for low-severity findings. component = { 'id': 'index.js', 'qualifiers': {'scanner': 'opengrep', 'type': 'javascript'}, - 'alerts': [{**_build_alert(), 'action': 'ignore', 'subType': 'sast-javascript'}], + 'alerts': [{ + **_build_alert(), + 'action': 'ignore', + 'actionReason': 'disabled_rule', + 'subType': 'sast-javascript', + }], } notifications = scanner.generate_notifications([component])