feat(checks): flag insecure deserialization introduced in a PR diff

CHANGELOG.md

@@ -4,7 +4,19 @@ Follows [Keep a Changelog](https://keepachangelog.com/) / [SemVer](https://semve
 
 ## [Unreleased]
 
-_Nothing yet._
+### Added — insecure-deserialization check (`deser.introduced`)
+
+- Flags a deserialization sink introduced by the diff, across Python (`pickle`,
+  `marshal`, `shelve`, `yaml.load` without a safe Loader, `yaml.unsafe_load`), Java
+  (`ObjectInputStream` / `readObject`), PHP (`unserialize`), Ruby (`Marshal.load` /
+  `YAML.load`) and .NET (`BinaryFormatter` and friends).
+- **Advisory, never blocking.** Whether the input is attacker-controlled cannot be
+  read off a diff hunk, so this adds reviewer context rather than deciding
+  mergeability — matching how `arch.dynamic_exec` treats `eval`/`exec`.
+- Added lines only, so a *removed* sink (i.e. a fix) is not a finding.
+- Whole-line comments are skipped, and `yaml.load(..., Loader=SafeLoader)` /
+  `yaml.safe_load` / `json.loads` are not flagged. Sinks in test/fixture paths are
+  reported at MEDIUM rather than HIGH.
 
 ## [0.2.0] — 2026-08-18
 

signetry_reviewer/checks.py

@@ -300,6 +300,76 @@ def scan_injection_surfaces(diff: str) -> list[Finding]:
     return findings
 
 
+# --- 3b. insecure deserialization introduced in the diff --------------------
+
+# (label, pattern). Each entry is a deserialization *sink* that executes attacker
+# data during decoding. Ordered per language; the first match on a line wins so one
+# added line yields at most one finding.
+_DESER_SINKS: tuple[tuple[str, re.Pattern[str]], ...] = (
+    ("Python pickle", re.compile(r"\b(?:c?pickle|_pickle)\.loads?\s*\(")),
+    ("Python marshal", re.compile(r"\bmarshal\.loads?\s*\(")),
+    ("Python shelve", re.compile(r"\bshelve\.open\s*\(")),
+    # yaml.load is only unsafe without a safe Loader; yaml.unsafe_load always is.
+    ("Python yaml.load", re.compile(
+        r"\byaml\.unsafe_load\s*\("
+        r"|\byaml\.load\s*\((?![^)]*(?:Safe|CSafe|Base)Loader)"
+    )),
+    ("Java native deserialization", re.compile(
+        r"new\s+(?:[\w.]*\.)?ObjectInputStream\s*\(|\.readObject\s*\(\s*\)"
+    )),
+    ("PHP unserialize", re.compile(r"\bunserialize\s*\(")),
+    ("Ruby Marshal/YAML load", re.compile(r"\bMarshal\.load\s*\(|\bYAML\.load\s*\(")),
+    (".NET BinaryFormatter", re.compile(
+        r"\bnew\s+(?:BinaryFormatter|NetDataContractSerializer|LosFormatter|ObjectStateFormatter)\s*\("
+    )),
+)
+
+# Deserializing a value is not automatically a vulnerability — a fixture loading its
+# own artifact is routine. These paths are where that is the likely reading.
+_DESER_BENIGN_PATH = re.compile(r"(?:^|/)(?:tests?|fixtures?|testdata|examples?|benchmarks?)(?:/|$)")
+
+
+def scan_deserialization(diff: str) -> list[Finding]:
+    """Flag an insecure-deserialization sink introduced by the diff.
+
+    Advisory only. Deserialization is the shape of an RCE surface, but whether the
+    input is attacker-controlled cannot be decided from a diff hunk — so this adds
+    context for a reviewer and never decides mergeability, matching how
+    ``arch.dynamic_exec`` treats ``eval``/``exec``.
+
+    Added lines only, like every other scanner here: a PR is judged on what it
+    introduces, not on deserialization that already existed.
+    """
+    findings: list[Finding] = []
+    for file, ln, text in added_lines(diff):
+        t = text.strip()
+        # Skip comment-only lines across the languages this covers, so documenting a
+        # sink does not trip the rule (the mistake ci.pull_request_target made).
+        if t.startswith(("#", "//", "*", "/*")):
+            continue
+        for label, pat in _DESER_SINKS:
+            if not pat.search(t):
+                continue
+            in_test_path = bool(_DESER_BENIGN_PATH.search(file.lower()))
+            findings.append(Finding(
+                id="deser.introduced",
+                category=Category.SECURITY,
+                severity=Severity.MEDIUM if in_test_path else Severity.HIGH,
+                title=f"Insecure deserialization introduced ({label})"
+                      + (" in a test/fixture path" if in_test_path else ""),
+                detail=f"An added line deserializes data via {label}. On attacker-controlled "
+                       "input this is remote code execution during decoding, not merely a "
+                       "parsing bug. Confirm the input is trusted, or move to a safe format.",
+                file=file, line=ln,
+                remediation="Use a data-only format (JSON) for untrusted input; for YAML use "
+                            "yaml.safe_load / Loader=SafeLoader; if a binary format is "
+                            "required, sign and verify it before decoding.",
+                blocking=False,
+            ))
+            break  # one finding per added line
+    return findings
+
+
 # --- 4. architectural smells ------------------------------------------------
 
 
@@ -348,5 +418,6 @@ def run_all_deterministic(diff: str, *, protected_globs: tuple[str, ...] = ()) -
     findings += scan_ci_permissions(diff)
     findings += scan_dependency_skew(diff)
     findings += scan_injection_surfaces(diff)
+    findings += scan_deserialization(diff)
     findings += scan_architecture(diff, protected_globs=protected_globs)
     return findings

tests/test_reviewer.py

@@ -285,3 +285,71 @@ def test_review_json_serializable():
     r = review_diff(_diff("src/util.py", ["    return 1"]), repo="a/b", required_check="success")
     json.dumps(r.to_public())
     assert r.to_public()["advisory"] is True
+
+
+# --- insecure deserialization introduced in the diff (#2) -------------------
+
+
+def test_deserialization_sinks_flagged_across_languages():
+    for path, line in [
+        ("app.py", "    obj = pickle.loads(raw)"),
+        ("app.py", "    c = marshal.loads(b)"),
+        ("app.py", "    db = shelve.open(name)"),
+        ("app.py", "    cfg = yaml.load(text)"),
+        ("app.py", "    cfg = yaml.unsafe_load(text)"),
+        ("Dao.java", "    ObjectInputStream in = new ObjectInputStream(s);"),
+        ("Dao.java", "    Object o = in.readObject();"),
+        ("index.php", "$o = unserialize($_POST['d']);"),
+        ("app.rb", "  o = Marshal.load(data)"),
+        ("Repo.cs", "    var f = new BinaryFormatter();"),
+    ]:
+        r = review_diff(_diff(path, [line]), repo="a/b", required_check="success")
+        hits = [f for f in r.findings if f.id == "deser.introduced"]
+        assert len(hits) == 1, f"expected one finding for {line!r}, got {len(hits)}"
+
+
+def test_deserialization_is_advisory_never_blocking():
+    # A deser sink alone must not decide mergeability — whether the input is
+    # attacker-controlled cannot be read off a diff hunk.
+    r = review_diff(_diff("app.py", ["    obj = pickle.loads(raw)"]), repo="a/b", required_check="success")
+    f = next(x for x in r.findings if x.id == "deser.introduced")
+    assert f.blocking is False
+    assert r.verdict != Verdict.BLOCK
+
+
+def test_deserialization_safe_forms_not_flagged():
+    for line in [
+        "    cfg = yaml.load(text, Loader=yaml.SafeLoader)",
+        "    cfg = yaml.load(text, Loader=SafeLoader)",
+        "    cfg = yaml.safe_load(text)",
+        "    cfg = json.loads(text)",
+    ]:
+        r = review_diff(_diff("app.py", [line]), repo="a/b", required_check="success")
+        assert not [f for f in r.findings if f.id == "deser.introduced"], line
+
+
+def test_deserialization_ignores_comments():
+    for path, line in [
+        ("app.py", "    # never use pickle.loads( on untrusted input"),
+        ("Dao.java", "    // ObjectInputStream is unsafe here"),
+    ]:
+        r = review_diff(_diff(path, [line]), repo="a/b", required_check="success")
+        assert not [f for f in r.findings if f.id == "deser.introduced"], line
+
+
+def test_deserialization_added_lines_only():
+    # A REMOVED deser line is a fix, not a finding.
+    removal = ("--- a/app.py\n+++ b/app.py\n@@ -1,2 +1,1 @@\n unchanged\n"
+               "-    obj = pickle.loads(raw)\n")
+    r = review_diff(removal, repo="a/b", required_check="success")
+    assert not [f for f in r.findings if f.id == "deser.introduced"]
+
+
+def test_deserialization_lower_severity_in_test_paths():
+    prod = review_diff(_diff("app.py", ["    obj = pickle.loads(raw)"]), repo="a/b", required_check="success")
+    test = review_diff(_diff("tests/test_x.py", ["    obj = pickle.loads(raw)"]), repo="a/b", required_check="success")
+    p = next(f for f in prod.findings if f.id == "deser.introduced")
+    t = next(f for f in test.findings if f.id == "deser.introduced")
+    assert p.severity == Severity.HIGH
+    assert t.severity == Severity.MEDIUM
+    assert "test/fixture" in t.title