From a0aa2b73fd282022d5c2b94f83bec80a61bc2971 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 18 Aug 2026 21:05:21 -0700 Subject: [PATCH 1/2] test(reviewer): reject unknown manifest evidence fields --- reviewer/tests/test_manifest.py | 59 ++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 4 deletions(-) diff --git a/reviewer/tests/test_manifest.py b/reviewer/tests/test_manifest.py index a5f102ce4..88c84c292 100644 --- a/reviewer/tests/test_manifest.py +++ b/reviewer/tests/test_manifest.py @@ -2,7 +2,17 @@ from __future__ import annotations -from noema_reviewer.manifest import DependencyFinding, ReviewManifest +import pytest +from pydantic import BaseModel, ValidationError + +from noema_reviewer.manifest import ( + ChangedFile, + CheckConclusion, + DependencyFinding, + ReviewComment, + ReviewManifest, + SecurityFinding, +) from noema_reviewer.models import BLOCKING_SEVERITIES, Severity @@ -17,15 +27,56 @@ def test_unresolved_blocking_findings_filtered_by_severity_and_state() -> None: [ DependencyFinding(tool="osv", package_name="a", severity=Severity.HIGH), DependencyFinding(tool="osv", package_name="b", severity=Severity.LOW), - DependencyFinding(tool="trivy", package_name="c", severity=Severity.CRITICAL, resolved=True), + DependencyFinding( + tool="trivy", + package_name="c", + severity=Severity.CRITICAL, + resolved=True, + ), DependencyFinding(tool="trivy", package_name="d", severity=Severity.MEDIUM), ] ) - names = {finding.package_name for finding in manifest.unresolved_dependency_findings(BLOCKING_SEVERITIES)} + names = { + finding.package_name + for finding in manifest.unresolved_dependency_findings(BLOCKING_SEVERITIES) + } assert names == {"a", "d"} def test_no_blocking_findings_returns_empty() -> None: """A manifest with only low findings returns nothing blocking.""" - manifest = _manifest_with([DependencyFinding(tool="osv", package_name="x", severity=Severity.INFO)]) + manifest = _manifest_with( + [DependencyFinding(tool="osv", package_name="x", severity=Severity.INFO)] + ) assert manifest.unresolved_dependency_findings(BLOCKING_SEVERITIES) == [] + + +@pytest.mark.parametrize( + ("model", "payload"), + ( + ( + DependencyFinding, + {"tool": "osv", "package_name": "pkg", "severity": Severity.HIGH}, + ), + ( + SecurityFinding, + { + "tool": "semgrep", + "identifier": "rule-id", + "severity": Severity.MEDIUM, + "message": "finding", + }, + ), + (ReviewComment, {"author": "reviewer", "body": "comment"}), + (CheckConclusion, {"name": "ci", "conclusion": "success"}), + (ChangedFile, {"path": "src/index.ts"}), + (ReviewManifest, {"repo": "o/r", "pr_number": 1}), + ), +) +def test_manifest_wire_models_reject_unknown_evidence_fields( + model: type[BaseModel], + payload: dict[str, object], +) -> None: + """Untrusted manifest evidence fails closed instead of discarding fields.""" + with pytest.raises(ValidationError): + model.model_validate({**payload, "unexpected_evidence": "must-not-disappear"}) From 78e798b7c8d28844d44621d1129b96e4f3c80072 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 18 Aug 2026 21:06:14 -0700 Subject: [PATCH 2/2] fix(reviewer): fail closed on unknown manifest evidence --- reviewer/noema_reviewer/manifest.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/reviewer/noema_reviewer/manifest.py b/reviewer/noema_reviewer/manifest.py index 0c4fead22..6b5f630ed 100644 --- a/reviewer/noema_reviewer/manifest.py +++ b/reviewer/noema_reviewer/manifest.py @@ -9,12 +9,18 @@ from __future__ import annotations -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field from .models import Severity -class DependencyFinding(BaseModel): +class _StrictManifestModel(BaseModel): + """Fail closed when untrusted manifest evidence contains unknown fields.""" + + model_config = ConfigDict(extra="forbid") + + +class DependencyFinding(_StrictManifestModel): """A dependency vulnerability surfaced by OSV, Trivy, or dependency-review.""" tool: str = Field(description="Scanner that reported the finding (osv, trivy, dependency-review).") @@ -29,7 +35,7 @@ class DependencyFinding(BaseModel): ) -class SecurityFinding(BaseModel): +class SecurityFinding(_StrictManifestModel): """A current-head code-scanning or SARIF finding.""" tool: str = Field(description="Scanner that produced the finding.") @@ -41,7 +47,7 @@ class SecurityFinding(BaseModel): url: str = Field(default="", description="GitHub alert URL, when present.") -class ReviewComment(BaseModel): +class ReviewComment(_StrictManifestModel): """A prior review comment preserved so the reviewer never loses context.""" author: str = Field(description="Comment author login.") @@ -55,21 +61,21 @@ class ReviewComment(BaseModel): ) -class CheckConclusion(BaseModel): +class CheckConclusion(_StrictManifestModel): """A current GitHub check conclusion used in the verdict.""" name: str = Field(description="Check or status context name.") conclusion: str = Field(description="Conclusion such as success, failure, or neutral.") -class ChangedFile(BaseModel): +class ChangedFile(_StrictManifestModel): """A changed file's path plus bounded current-head content for context.""" path: str = Field(description="Repository-relative path.") content: str = Field(default="", description="Bounded current-head text content.") -class ReviewManifest(BaseModel): +class ReviewManifest(_StrictManifestModel): """Everything a review driver is allowed to see for one pull request.""" repo: str = Field(description="owner/name of the target repository.")