fix(mcptransport): aggregate OriginValidation bypass variants into one finding (LAB-5584) - #288
Conversation
…ding (LAB-5584) mcptransport.OriginValidation emitted one scored attempt per bypass variant. A server that validates no Origin fails every variant, so a single flaw surfaced as ten identical 1.0 rows — 84% of everything the MCP probes produced across our target corpus, and 10 of 12 rows on a benign reference server. The ten variants are ten ways of asking one question: does this endpoint enforce the Origin/Host allowlist the MCP spec requires. Fold them into a single scored attempt for the endpoint, class origin-validation-sweep, carrying every crafted value as evidence. The per-variant detail is preserved, because which variants pass is what a remediator acts on: a case-variant-only bypass means the allowlist exists but compares case-sensitively, a different fix from an endpoint that accepts any origin. Evidence renders ACCEPTED / REJECTED / NOT TESTED buckets plus a validator verdict, and the metadata carries the structured variant list, accepted classes, and counts. Credentialed CORS reflection found by the preflight escalates the aggregated finding's stated impact from "a page can drive the tool surface blind" to "a page can also read the responses". Baseline and preflight attempts are unchanged. Host-class severity tiering is unchanged — the sweep carries the same target-class stamp, so the detector needs no change. When every variant dies in transit the sweep reports as errored rather than safe: collapsing to one row is only sound if that row is trustworthy, and a dead endpoint reporting SAFE is worse than the noise this removes. Measured against DVMCP challenges 1-10 over the legacy SSE transport, before/after the change: findings 90 -> 10, attempts 110 -> 30, with the set of accepted variant classes identical in both (no information lost). The hardened DVMCP streamable-HTTP servers stay clean in both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review
Critical Issues
internal/probes/mcptransport/originvalidation.go:517— If some variants fail in transit while at least one is rejected, the aggregate is marked complete withaccepted=false, producing a SAFE score despite most checks being untested. Partial transport failures should make a non-vulnerable result inconclusive/error.internal/probes/mcptransport/originvalidation.go:418— A failed CORS preflight is reduced tocredentialedRead=false;internal/probes/mcptransport/originvalidation.go:554then reports reflection as “absent.” Preserve preflight failure state and report “not tested” rather than asserting absence.
Security
No security concerns flagged.
Suggestions
No suggestions.
Reviewed by Codex (gpt-5.6-sol)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Set on the aggregated sweep attempt because it escalates that finding's | ||
| // impact: without it a rebound page drives the tool surface blind; with it | ||
| // the page can also read the responses. | ||
| MetadataKeyOriginValidationCredentialedRead = "mcptool.originvalidation_credentialed_read" |
WalkthroughThe origin-validation probe now aggregates crafted Origin and Host results into one endpoint-level sweep finding. The finding records accepted, rejected, and transport-failed variants with evidence and representative transcripts. Baseline and preflight attempts remain separate. The probe handles request-construction and transport failures as variant results, preserves SSE timeout handling, and escalates credentialed CORS reflection. Metadata keys, detector scoring, and tests now use the aggregated sweep model. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/probes/mcptransport/originvalidation_test.go (1)
161-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail loudly if the metadata type changes.
Both helpers discard the type-assertion result. If the probe ever stores a different shape, the helpers return empty values and the callers report misleading errors instead of the real contract break.
♻️ Proposed change
- classes, _ := raw.([]string) + classes, ok := raw.([]string) + if !ok { + t.Fatalf("accepted-classes metadata is %T, want []string", raw) + }Apply the same pattern to
sweepVariants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/probes/mcptransport/originvalidation_test.go` around lines 161 - 193, Update acceptedVariantClasses and sweepVariants to validate the metadata type assertions instead of discarding their boolean results; fail immediately with t.Fatal when the stored value is not the expected []string or []map[string]any shape, then continue building and returning the parsed values for valid metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/probes/mcptransport/originvalidation.go`:
- Around line 569-580: Update validatorVerdict so the enforced verdict is
returned only when accepted == 0, rejected > 0, and errored == 0; when errored
variants exist, report the result as incomplete or inconclusive while preserving
the existing verdicts for fully rejected, entirely unresolved, and fully
accepted cases.
---
Nitpick comments:
In `@internal/probes/mcptransport/originvalidation_test.go`:
- Around line 161-193: Update acceptedVariantClasses and sweepVariants to
validate the metadata type assertions instead of discarding their boolean
results; fail immediately with t.Fatal when the stored value is not the expected
[]string or []map[string]any shape, then continue building and returning the
parsed values for valid metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5ca41d4-b384-449b-819b-3d6cc95d2e67
📒 Files selected for processing (5)
internal/detectors/mcptransport/originvalidation.gointernal/detectors/mcptransport/originvalidation_test.gointernal/probes/mcptransport/originvalidation.gointernal/probes/mcptransport/originvalidation_test.gopkg/attempt/metadata_keys.go
| func validatorVerdict(accepted, rejected, errored int) string { | ||
| switch { | ||
| case accepted == 0 && rejected > 0: | ||
| return "every crafted value was refused — Origin/Host validation is enforced" | ||
| case accepted == 0: | ||
| return "no variant was accepted, but none completed cleanly either — inconclusive" | ||
| case rejected == 0 && errored == 0: | ||
| return "NO Origin/Host validation is enforced — every crafted value was accepted" | ||
| default: | ||
| return "validation is PARTIAL — the accepted classes below are the checks that are missing" | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the "enforced" verdict when variants failed in transit.
validatorVerdict returns "validation is enforced" whenever accepted == 0 && rejected > 0, even if some variants never reached the server. The verdict line is the summary a remediator reads, so it should state the incomplete coverage.
🔧 Proposed fix
switch {
+ case accepted == 0 && rejected > 0 && errored > 0:
+ return fmt.Sprintf("every crafted value that landed was refused, but %d never completed — validation appears enforced, coverage is partial", errored)
case accepted == 0 && rejected > 0:
return "every crafted value was refused — Origin/Host validation is enforced"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validatorVerdict(accepted, rejected, errored int) string { | |
| switch { | |
| case accepted == 0 && rejected > 0: | |
| return "every crafted value was refused — Origin/Host validation is enforced" | |
| case accepted == 0: | |
| return "no variant was accepted, but none completed cleanly either — inconclusive" | |
| case rejected == 0 && errored == 0: | |
| return "NO Origin/Host validation is enforced — every crafted value was accepted" | |
| default: | |
| return "validation is PARTIAL — the accepted classes below are the checks that are missing" | |
| } | |
| } | |
| func validatorVerdict(accepted, rejected, errored int) string { | |
| switch { | |
| case accepted == 0 && rejected > 0 && errored > 0: | |
| return fmt.Sprintf("every crafted value that landed was refused, but %d never completed — validation appears enforced, coverage is partial", errored) | |
| case accepted == 0 && rejected > 0: | |
| return "every crafted value was refused — Origin/Host validation is enforced" | |
| case accepted == 0: | |
| return "no variant was accepted, but none completed cleanly either — inconclusive" | |
| case rejected == 0 && errored == 0: | |
| return "NO Origin/Host validation is enforced — every crafted value was accepted" | |
| default: | |
| return "validation is PARTIAL — the accepted classes below are the checks that are missing" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/probes/mcptransport/originvalidation.go` around lines 569 - 580,
Update validatorVerdict so the enforced verdict is returned only when accepted
== 0, rejected > 0, and errored == 0; when errored variants exist, report the
result as incomplete or inconclusive while preserving the existing verdicts for
fully rejected, entirely unresolved, and fully accepted cases.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e7d6088a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case accepted == 0 && rejected > 0: | ||
| return "every crafted value was refused — Origin/Host validation is enforced" |
There was a problem hiding this comment.
When one crafted variant times out or is dropped but at least one other variant returns a clean rejection, this branch reports that every value was refused; aggregateSweep then completes the attempt with accepted=false, so the detector scores the whole sweep SAFE even though part of the validator was never tested. Treat errored > 0 && accepted == 0 as inconclusive/error instead of enforced so a partial transport failure cannot become a green Origin/Host result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude Review
Critical issues
- Partial transport failure can under-report (aggregateSweep / validatorVerdict). When some variants error in transit but at least one completes-and-rejects and none are accepted, the attempt completes with accepted=false and the detector scores it 0.0 SAFE. Meanwhile validatorVerdict takes its first branch (accepted==0 and rejected present, which ignores the errored count) and prints "every crafted value was refused — validation is enforced," even though the errored variants were never tested — one could be the value the server would accept. Only the all-errored case is promoted to StatusError; the mixed-error case reads as a clean pass, where the old per-variant code surfaced that errored variant as its own error row. Consider marking the sweep inconclusive (MetadataKeyInconclusive) or fixing the verdict text when there are errored variants and none accepted, so a transient error on the decisive variant cannot hide behind a green row — the same reasoning the PR already applies to the all-errored case.
Security
No security concerns flagged. No new external network surface — the probe already sent these crafted Origin/Host values; this only changes how results are folded into attempts. No secrets or auth handling touched.
Test coverage
Excellent — new tests cover the one-finding invariant, evidence completeness, partial validation, all-variants-failed to errored, conditional CORS escalation, and the borrowed-client request count. The mixed-error scoring path noted above is the one case not exercised.
Generated with Claude Code
2 participants
Fixes LAB-5584
Problem
mcptransport.OriginValidationemitted one scored attempt per bypass variant. A server that validates no Origin fails every variant, so a single flaw surfaced as ten identical 1.0 rows.Per the ticket's corpus measurement: 260 of 308 findings across thirteen HTTP MCP targets — 84% — were this one check. On one lab it was 100% of the output. On the benign
server-everythingreference server it was 10 of 12 rows, so a server with no vulnerabilities still produced twelve red rows.The ten variants (two external origins, null, two extension origins, two localhost-lookalikes, a case variant, two unexpected Hosts) are ten ways of asking one question — does this endpoint enforce the Origin/Host allowlist the spec requires — and a server that validates nothing answers all ten identically.
The check itself is sound and stays: the finding is real, the loopback/LAN vs public severity tiering is well reasoned, and the CORS half is already separated. The defect was purely that ten proofs of one property were presented as ten vulnerabilities.
Guard was checked first, per the ticket
emitPerProbeRisksin Guard (backend/pkg/tasks/capabilities/augustus/augustus.go) already buckets findings by probe name and emits one risk per probe. The ten attempts have always collapsed to onellm-mcptransport-originvalidationrisk, so the customer-facing risk count was never wrong.What was wrong Guard-side is the proof attachment on that single risk: ten near-identical
ScanFindingsblobs. This change makes it one legible finding. No Guard change is needed —evidenceFromMetadatacopies attempt metadata verbatim, so the new keys flow through untouched, and nothing in the Guard backend or frontend is coupled to the class values (grepped).Change
One scored attempt for the endpoint (class
origin-validation-sweep) carrying every crafted value as evidence.Per-variant detail is preserved deliberately — which variants pass is what a remediator acts on. A case-variant-only bypass means the allowlist exists but compares case-sensitively; that's a different fix from an endpoint that accepts any origin.
credentialed_readmetadata key...._variants(structured per-variant list),..._accepted_classes,..._variants_sent,..._variants_accepted,..._credentialed_read.One judgement call worth reviewing
When every variant dies in transit, the sweep now reports as errored rather than safe. Collapsing to one row is only sound if that row is trustworthy, and a dead endpoint reporting SAFE would be worse than the noise this removes. Test locks it in.
Verification
Built the binary before and after the change and ran both against the live DVMCP corpus (challenges 1-10, legacy SSE transport):
Every challenge went 9 findings → 1 (nine not ten because these bind
127.0.0.1:900X, an all-numeric host where the case variant is correctly skipped as a would-be FP).No information lost — machine-checked that the set of variant classes in the nine BEFORE findings is identical to the set in the one AFTER finding's evidence, on all ten challenges.
Negative control: the DVMCP streamable-HTTP servers (mcp 1.28.1, hardened because FastMCP auto-enables DNS-rebinding protection when constructed with the default loopback host) score clean both before and after, and the aggregated evidence states it positively — "every crafted value was refused — Origin/Host validation is enforced." So the collapse doesn't manufacture a finding where there isn't one, and a hardened server reads as one green row instead of eleven.
Note the corpus here is the SSE half of DVMCP only, not the ticket's full thirteen targets, so this doesn't reproduce its exact 230-finding projection.
go build ./..., fullgo test ./...,gofmt, andgolangci-lint(0 issues) all pass.🤖 Generated with Claude Code