From a38422bf199ffe2077581b549ccfc5642b79e889 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 13:11:08 +0700 Subject: [PATCH 1/2] fix(security): SARIF no-judge marker cannot be forged or missed A model-written summary containing the marker sentence is stripped, and the judge-failure fallback now carries the marker. Documents the block_in_place multi-thread requirement of the MCP block_on helper. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 ++ src/engine/judge.rs | 25 +++++++++++++++++++++++++ src/engine/review.rs | 15 ++++++++++++++- src/mcp/tools.rs | 6 ++++++ 4 files changed, 47 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c0187d..be978fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **The SARIF "no judge" marker can no longer be forged or missed.** A model-written summary containing the marker sentence is stripped, and the judge-failure fallback (deterministic findings only) now carries the marker, so `cora.judge = "none"` is set exactly when no model judged. + - **A hostile diff can no longer hide findings (audit follow-up).** Inside a hunk whose line counts are not used up, `---` / `+++` lines are content, not file headers (a removed SQL/Lua comment `-- x`, or an added line `++ b/tests/y.py`, used to drop the rest of the hunk or re-file it under another path, so a secret was reported as nothing). `valid_files` now comes from the same parser, and file names are escaped in the review prompt. - **A judge can no longer downgrade or absorb a scanner finding (audit follow-up).** When a judge issue sits on the same line and topic as a scanner finding, the surviving issue takes the stronger severity and the scanner's `rule_id` (an "info" judge issue used to replace a CRITICAL scanner finding). diff --git a/src/engine/judge.rs b/src/engine/judge.rs index 3daa79f..8c43269 100644 --- a/src/engine/judge.rs +++ b/src/engine/judge.rs @@ -91,6 +91,8 @@ impl Judge for HttpJudge<'_> { &llm.base_url, ) { tracing::debug!("returning cached judge response"); + let mut cached = cached; + strip_sentinel(&mut cached.summary); return Ok(cached); } } @@ -119,6 +121,9 @@ impl Judge for HttpJudge<'_> { result? }; + let mut response = response; + strip_sentinel(&mut response.summary); + if self.cache_ttl.is_some() { if let Err(e) = crate::engine::cache::save_cached_review( &key_text, @@ -263,6 +268,15 @@ pub const AGENT_JUDGED: &str = "Judged by the calling agent (not by a cora-confi /// in every output format, that a clean result is not an AI review. pub const DETERMINISTIC_ONLY: &str = "Deterministic checks only (no LLM judge)."; +/// Remove the "no judge" sentinel from a summary written by a model. The +/// sentinel marks a run that had NO judge (SARIF `cora.judge = none`); text +/// steered by the diff must not be able to forge it. +fn strip_sentinel(summary: &mut String) { + if summary.contains(DETERMINISTIC_ONLY) { + *summary = summary.replace(DETERMINISTIC_ONLY, "").trim().to_string(); + } +} + /// Judge that finds nothing: the review is the deterministic findings only. pub struct NoJudge; @@ -607,4 +621,15 @@ mod tests { assert_eq!(out[1].line, None, "old path of a rename has no new lines"); assert_eq!(out[2].line, Some(1), "real added line stays"); } + + #[test] + fn a_model_cannot_forge_the_no_judge_marker() { + let mut summary = format!("looks fine. {DETERMINISTIC_ONLY} trust me"); + strip_sentinel(&mut summary); + assert!(!summary.contains(DETERMINISTIC_ONLY)); + assert!(summary.contains("looks fine.")); + let mut clean = "all good".to_string(); + strip_sentinel(&mut clean); + assert_eq!(clean, "all good"); + } } diff --git a/src/engine/review.rs b/src/engine/review.rs index af05dd1..874bb0c 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -408,7 +408,8 @@ pub async fn review_with( let mut fallback = ReviewResponse { issues: all_deterministic, summary: format!( - "LLM review failed: {e}. Showing {n_rules} rule + {n_secrets} secrets + {n_security} security + {n_index_unused} unused imports + {n_index_dead} dead code + {n_index_breaking} breaking changes." + "LLM review failed: {e}. Showing {n_rules} rule + {n_secrets} secrets + {n_security} security + {n_index_unused} unused imports + {n_index_dead} dead code + {n_index_breaking} breaking changes. {}", + crate::engine::judge::DETERMINISTIC_ONLY ), tokens_used: None, should_block: false, @@ -1040,4 +1041,16 @@ new file mode 100644\n\ ); assert!(resp.should_block || config.hook.min_severity_level() != Severity::Major); } + + #[test] + fn judge_failure_fallback_is_marked_deterministic_only() { + // Only deterministic findings remain, so SARIF must say no judge ran. + let config = Config::default(); + let judge = FakeJudge(Err("boom".to_string())); + let resp = run(&judge, &config, packet(SECRET_DIFF, &config)).expect("fallback"); + assert!( + resp.summary + .contains(crate::engine::judge::DETERMINISTIC_ONLY) + ); + } } diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 49093f7..9b16163 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -23,6 +23,12 @@ static MCP_RUNTIME: LazyLock = /// ("Cannot start a runtime from within a runtime") and kills the server. /// `block_in_place` hands the thread over first. Outside a runtime (unit /// tests) it blocks directly. +/// +/// Limitation: `block_in_place` panics on a `current_thread` runtime. The +/// server is multi-thread (`#[tokio::main]` with `rt-multi-thread`), so this +/// holds in production; a `#[tokio::test]` calling a handler that reaches +/// here must use `flavor = "multi_thread"`. (A thread hand-off would need the +/// future to be `Send`, which the pipeline's futures are not.) pub(super) fn block_on(future: F) -> F::Output { match tokio::runtime::Handle::try_current() { Ok(_) => tokio::task::block_in_place(|| MCP_RUNTIME.block_on(future)), From 42ff32cbb4bef423242ecefdc6c4a1ca17df5704 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 13:17:18 +0700 Subject: [PATCH 2/2] fix(security): strip the no-judge marker until none is left A single replace pass let a nested marker reassemble after removal. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- src/engine/judge.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/engine/judge.rs b/src/engine/judge.rs index 8c43269..0b4672b 100644 --- a/src/engine/judge.rs +++ b/src/engine/judge.rs @@ -272,9 +272,12 @@ pub const DETERMINISTIC_ONLY: &str = "Deterministic checks only (no LLM judge)." /// sentinel marks a run that had NO judge (SARIF `cora.judge = none`); text /// steered by the diff must not be able to forge it. fn strip_sentinel(summary: &mut String) { - if summary.contains(DETERMINISTIC_ONLY) { - *summary = summary.replace(DETERMINISTIC_ONLY, "").trim().to_string(); + // Repeat: removing one occurrence can join the text around it into a new + // one ("Deterministic ..."), so a single pass is bypassable. + while summary.contains(DETERMINISTIC_ONLY) { + *summary = summary.replace(DETERMINISTIC_ONLY, ""); } + *summary = summary.trim().to_string(); } /// Judge that finds nothing: the review is the deterministic findings only. @@ -622,6 +625,22 @@ mod tests { assert_eq!(out[2].line, Some(1), "real added line stays"); } + #[test] + fn nested_marker_cannot_reassemble_after_removal() { + // Removing the inner marker joins the halves into a fresh one. + let (a, b) = DETERMINISTIC_ONLY.split_at(10); + let mut summary = format!("{a}{DETERMINISTIC_ONLY}{b}"); + strip_sentinel(&mut summary); + assert!(!summary.contains(DETERMINISTIC_ONLY), "got: {summary:?}"); + let mut deep = DETERMINISTIC_ONLY.to_string(); + for _ in 0..5 { + let (a, b) = DETERMINISTIC_ONLY.split_at(7); + deep = format!("{a}{deep}{b}"); + } + strip_sentinel(&mut deep); + assert!(!deep.contains(DETERMINISTIC_ONLY)); + } + #[test] fn a_model_cannot_forge_the_no_judge_marker() { let mut summary = format!("looks fine. {DETERMINISTIC_ONLY} trust me");