diff --git a/src/cmcp_runtime/inspection/pipeline.py b/src/cmcp_runtime/inspection/pipeline.py index afe8d9d..c95761d 100644 --- a/src/cmcp_runtime/inspection/pipeline.py +++ b/src/cmcp_runtime/inspection/pipeline.py @@ -222,8 +222,16 @@ def _stage4_injection_detection( injection_score=score, ) return StageResult(stage="injection", decision="allow") - except Exception: # nosec B110 - pass # Fall through to regex + except Exception: + # Falling through to the local patterns is the right behaviour, but + # doing it silently means a broken AGT detector looks identical to a + # working one while the weaker starter set is what actually ran. + _log.warning( + "AGT PromptInjectionDetector.detect() failed; " + "falling back to local patterns v%s", + _PATTERNS_VERSION, + exc_info=True, + ) # Fallback: regex patterns patterns = custom_patterns or _COMPILED_PATTERNS @@ -288,7 +296,7 @@ def _classify_sensitivity( 1. catalog_entry.sensitivity_level: always applied 2. field-level x-sensitivity annotations in output_schema properties - 3. pattern matching on response content (AGT CredentialRedactor or regex fallback) + 3. pattern matching on response content (AGT CredentialRedactor and local patterns) """ tags: list[str] = [] @@ -312,22 +320,47 @@ def _classify_sensitivity( except (json.JSONDecodeError, ValueError): pass - # Source 3: content pattern matching + # Source 3: content pattern matching. + # + # AGT and the local patterns both run, rather than AGT displacing them. Two + # reasons. First, AGT's CredentialRedactor does not redact a secret at all + # when a suffix is glued to it (microsoft/agent-governance-toolkit#3494: + # `AKIA..._old` passes through whole), so treating it as sufficient leaves a + # live key untagged. Second, an either/or meant any breakage on the AGT path + # silently disabled content classification entirely instead of degrading to + # the local patterns. if response_text: + _SENSITIVE = ("pii", "confidential", "hipaa_phi", "mnpi") + if _AGT_AVAILABLE and _agt_redactor is not None: - try: - matches = _agt_redactor.find_credentials(response_text) - if matches and not any(t in tags for t in ("pii", "confidential", "hipaa_phi", "mnpi")): - tags.append("pii") - except Exception: # nosec B110 - pass - else: - # Regex fallback for when AGT is unavailable - for pattern, tag in _PII_PATTERNS: - if pattern.search(response_text) and tag not in tags: - tags.append(tag) - if len(tags) >= 4: # cap scan at 4 distinct tags - break + # find_matches covers secrets, find_pii_matches covers PII spans. + # Both are classmethods on CredentialRedactor. A previous version + # called `find_credentials`, which has never existed on that class + # in any agt-core release, so this whole branch raised AttributeError + # into the handler below and contributed nothing. + for method in ("find_matches", "find_pii_matches"): + finder = getattr(_agt_redactor, method, None) + if finder is None: + # Narrow and loud: an AGT API that moved should surface as a + # degraded stage, not as silent unconditional passing. + _log.warning( + "AGT CredentialRedactor has no %s(); " + "falling back to local patterns for content classification", + method, + ) + continue + try: + if finder(response_text) and not any(t in tags for t in _SENSITIVE): + tags.append("pii") + except Exception: # nosec B110 - never let detection break the pipeline + _log.warning("AGT CredentialRedactor.%s() failed", method, exc_info=True) + + # Local patterns always run, as a second pass rather than a fallback. + for pattern, tag in _PII_PATTERNS: + if len(tags) >= 4: # cap scan at 4 distinct tags + break + if pattern.search(response_text) and tag not in tags: + tags.append(tag) return tags @@ -339,7 +372,7 @@ class SensitivityClassificationStage: Applies three classification sources in order: 1. catalog_entry.sensitivity_level annotation 2. x-sensitivity field-level tags in output_schema properties - 3. Content pattern matching (AGT CredentialRedactor or regex fallback) + 3. Content pattern matching (AGT CredentialRedactor and local patterns) """ def run( @@ -398,13 +431,35 @@ def __init__( self._agt_redactor: Any = None self._agt_response_scanner: Any = None if _AGT_AVAILABLE: + # Constructed independently. Sharing one try block meant a failure in + # the first component skipped the other two, so a single upstream API + # change disabled three security components at once, silently. try: _cfg = DetectionConfig(sensitivity=injection_sensitivity) self._agt_injection_detector = PromptInjectionDetector(config=_cfg) + except Exception: + _log.warning( + "AGT PromptInjectionDetector unavailable; " + "stage 4 falls back to local patterns v%s", + _PATTERNS_VERSION, + exc_info=True, + ) + try: self._agt_redactor = CredentialRedactor() + except Exception: + _log.warning( + "AGT CredentialRedactor unavailable; " + "stage 3 falls back to local patterns", + exc_info=True, + ) + try: self._agt_response_scanner = AGTResponseScanner() - except Exception: # nosec B110 - pass + except Exception: + _log.warning( + "AGT MCPResponseScanner unavailable; " + "stage 4 loses MCP-specific threat detection", + exc_info=True, + ) def run( self, @@ -518,8 +573,17 @@ def run( injection_scanner = "timeout" stage_results["injection"] = "deny" agt_mcp_denied = True - except Exception: # nosec B110 - pass + except Exception: + # Unlike the timeout above this does not deny, because a scanner + # that errored has produced no verdict either way and stage 4's + # own detection still runs below. It must not be silent though: + # MCP-specific threat coverage is gone for this response. + _log.warning( + "AGT MCPResponseScanner.scan_response() failed for %s; " + "no MCP-specific threat coverage on this response", + catalog_entry.tool_name, + exc_info=True, + ) # INJECT-002: wrap AGT PromptInjectionDetector with the same timeout bound. def _run_s4() -> StageResult: diff --git a/tests/unit/test_stage3_sensitivity.py b/tests/unit/test_stage3_sensitivity.py index c383104..cb7bcff 100644 --- a/tests/unit/test_stage3_sensitivity.py +++ b/tests/unit/test_stage3_sensitivity.py @@ -160,3 +160,114 @@ def test_no_duplicate_pii_from_catalog_and_field(): with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", False): result = stage.run({"ssn": "123-45-6789"}, _make_entry("pii", output_schema=schema)) assert result.sensitivity_tags.count("pii") == 1 + + +# ── Source 3 with AGT present: local patterns must still run (#476) ─────────── +# +# The previous implementation called `_agt_redactor.find_credentials()`, a method +# that has never existed on agt-core's CredentialRedactor (verified against the +# pinned 4.1.0 wheel: the real names are find_matches / find_pii_matches). The +# call raised AttributeError into a bare `except Exception: pass`, and because +# the local patterns sat in the `else` branch of that check they never ran +# either. With AGT installed, content-based classification was dead. + +class _RedactorMissingMethod: + """An agt-core CredentialRedactor without the method the caller expects.""" + + +class _RedactorRaising: + @staticmethod + def find_matches(_value: str) -> list[str]: + raise RuntimeError("upstream blew up") + + @staticmethod + def find_pii_matches(_value: str) -> list[str]: + raise RuntimeError("upstream blew up") + + +class _RedactorSuffixBlind: + """Reproduces microsoft/agent-governance-toolkit#3494: a secret with a + suffix glued to it is not matched at all, so AGT alone calls it clean.""" + + @staticmethod + def find_matches(value: str) -> list[str]: + return ["AKIA"] if "AKIAIOSFODNN7EXAMPLE" in value and "_old" not in value else [] + + @staticmethod + def find_pii_matches(_value: str) -> list[str]: + return [] + + +def test_local_patterns_still_run_when_agt_lacks_the_method(): + stage = SensitivityClassificationStage() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + result = stage.run( + {"data": "SSN is 123-45-6789"}, _make_entry(), _RedactorMissingMethod() + ) + assert "pii" in result.sensitivity_tags + + +def test_local_patterns_still_run_when_agt_raises(): + stage = SensitivityClassificationStage() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + result = stage.run( + {"data": "SSN is 123-45-6789"}, _make_entry(), _RedactorRaising() + ) + assert "pii" in result.sensitivity_tags + + +def test_suffixed_secret_still_tagged_despite_agt_3494(): + stage = SensitivityClassificationStage() + blind = _RedactorSuffixBlind() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + clean = stage.run({"k": "AKIAIOSFODNN7EXAMPLE x@y.com"}, _make_entry(), blind) + suffixed = stage.run({"k": "AKIAIOSFODNN7EXAMPLE_old x@y.com"}, _make_entry(), blind) + # AGT catches the bare key; only the local pass catches the suffixed one. + assert "pii" in clean.sensitivity_tags + assert "pii" in suffixed.sensitivity_tags + + +# ── AGT component construction is independent (#476) ────────────────────────── +# +# All three components were built in one try block, so a failure constructing +# the first left the other two unbuilt and all three silently None. One upstream +# API change disabled three security components at once, with no log line. + +def test_one_failing_agt_component_does_not_disable_the_others(): + from cmcp_runtime.inspection.pipeline import InspectionPipeline + + def _boom(*_a: object, **_k: object) -> None: + raise RuntimeError("upstream API changed") + + with ( + patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True), + patch("cmcp_runtime.inspection.pipeline.DetectionConfig", _boom), + patch("cmcp_runtime.inspection.pipeline.CredentialRedactor", lambda: "redactor"), + patch("cmcp_runtime.inspection.pipeline.AGTResponseScanner", lambda: "scanner"), + ): + pipeline = InspectionPipeline() + + # The detector failed, but the other two must still be constructed. + assert pipeline._agt_injection_detector is None + assert pipeline._agt_redactor == "redactor" + assert pipeline._agt_response_scanner == "scanner" + + +def test_failing_agt_component_is_logged_not_swallowed(caplog: Any) -> None: + import logging + + from cmcp_runtime.inspection.pipeline import InspectionPipeline + + def _boom(*_a: object, **_k: object) -> None: + raise RuntimeError("upstream API changed") + + with ( + caplog.at_level(logging.WARNING, logger="cmcp_runtime.inspection.pipeline"), + patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True), + patch("cmcp_runtime.inspection.pipeline.DetectionConfig", _boom), + patch("cmcp_runtime.inspection.pipeline.CredentialRedactor", lambda: "redactor"), + patch("cmcp_runtime.inspection.pipeline.AGTResponseScanner", lambda: "scanner"), + ): + InspectionPipeline() + + assert any("PromptInjectionDetector unavailable" in r.message for r in caplog.records)