fix(mcp): honor explicit transport security overrides - #2033
Conversation
|
Fresh exact-head official review requested for be03cd5. Please re-evaluate every explicit override and wire-normalization blocker from review 4895106143, including actual SDK middleware behavior. Merge stays frozen until all same-head checks are green and the detailed Merge Recommendation explicitly says Approve/Merge. |
Q00
added
the
no-issue
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2033 |
| HEAD checked | be03cd57cdbd0774ed5740df1bc14def6b50b7ab |
| Request ID | req_1786361110_1530-retry-1-retry-2 |
| Review record | 4b0934da-f889-4ff8-936a-d8a91c16dc41 |
What Improved
- Explicit Host and Origin overrides now reach the pinned SDK middleware for loopback binds instead of silently inheriting SDK defaults.
- Empty Origin allowlists are explicitly fail-closed, while requests without an Origin remain compatible with non-browser MCP clients.
- Host inference now correctly handles case normalization, ordinary IDNA labels, compressed IPv4, and bracketed/compressed IPv6.
- Security policy remains centralized in
auth.pyand is passed consistently to both SSE and streamable HTTP adapters.
Issue Requirements
| Requirement | Status |
|---|---|
| Explicit Host policy overrides SDK loopback autoprotection | Met |
| Explicit Origin policy overrides SDK loopback defaults | Met |
| Explicit empty Origin allowlist rejects loopback browser origins | Met |
| Preserve SDK-compatible loopback Host defaults when no Host override is supplied | Met |
| Normalize inferred case, IPv4, and bracketed IPv6 authorities to wire form | Met |
| Normalize inferred trailing-dot DNS authorities exactly as sent on the wire | Not met |
| Normalize all inferred IDNA authorities exactly as sent on the wire | Not met |
| Keep socket bind input unchanged | Met |
| Add real middleware regressions for Host, Origin, overrides, and non-browser requests | Partially met |
| Independent verifier and same-head CI claims | N/A — external CI/verifier artifacts were not available in the supplied snapshot |
Prior Findings Status
No prior ouroboros-agent[bot] review rounds were provided. In the requested fresh re-evaluation, the explicit Host override, explicit Origin override, empty-Origin fail-closed behavior, and adapter propagation concerns are resolved. The claimed exact wire normalization remains blocking for trailing-dot and modern-IDNA authorities.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/server/auth.py:119 | BLOCKING | as_url_authority() strips the trailing root dot, but real HTTP clients preserve it in the Host header. With --host Example.COM., the inferred allowlist is example.com:*, while HTTPX and WHATWG URL serialization send example.com.:8080; the pinned SDK middleware therefore rejects the request with 421. The helper is also used by build_auth_settings(), so stripping the dot can change an absolute FQDN into a search-list-relative name in advertised authentication URLs. Preserve the trailing dot for wire and URL authority construction, using a separately canonicalized value only for identity checks such as loopback classification. The new tests at tests/unit/mcp/server/test_network_security.py:446 currently fabricate a dotless wire value and therefore encode the incorrect expectation. |
| 2 | src/ouroboros/mcp/server/auth.py:121 | BLOCKING | Python's built-in idna codec implements legacy IDNA behavior that does not match modern HTTP URL serialization for all labels. For example, this code converts faß.de to fass.de, while HTTPX and browser/WHATWG serialization produce xn--fa-hia.de; the inferred Host allowlist consequently rejects the real wire header, and authentication metadata can advertise a different DNS authority. Use the same UTS-46/IDNA processing as the supported HTTP client/runtime boundary and add a regression using a label whose IDNA 2003 and modern wire forms differ—the current BÜCHER test cannot detect this class of mismatch. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/mcp/server/adapter.py:1083 | Low | High | Update the serving documentation: Ouroboros now supplies explicit transport-security settings on loopback rather than relying on the SDK to auto-enable them. |
Non-blocking Suggestions
| 1 | src/ouroboros/mcp/server/adapter.py:1083 | Documentation | The comment still attributes loopback DNS-rebinding protection to SDK auto-configuration, which no longer describes the effective path after this PR. |
Test Coverage Notes
- Ran
tests/unit/mcp/server/test_network_security.pyandtests/unit/mcp/server/test_adapter.py: 331 passed. - Ran related CLI propagation/startup suites: 93 passed.
- Ruff passed for all three changed files.
- Direct probes against pinned MCP 2.0.0 middleware confirmed that inferred settings reject
example.com.:8080,xn--bcher-kva.example.:8080, and modern-IDNAxn--fa-hia.de:8080. - Coverage is strong for explicit Host/Origin overrides, empty-Origin rejection, IPv4, IPv6, case normalization, and ordinary IDNA. The trailing-dot test uses an inaccurate wire header, and no test exercises an IDNA 2003/modern-IDNA divergence.
Design Notes
Centralizing transport policy in auth.py and sharing it across adapters is the correct architecture. The remaining flaw is conceptual: one helper conflates DNS identity canonicalization with exact HTTP wire and URL serialization, even though those transformations have different trailing-dot and IDNA requirements.
Design / Roadmap Gate
Both SSE and streamable HTTP receive the same explicit security object, operator overrides are passed through unchanged, and there is no persistence or replay impact. Compatibility is not yet safe for supported DNS-shaped bind input: absolute FQDNs and modern-IDNA labels can be rejected by the middleware or transformed into a different advertised authentication authority. The socket bind value itself remains unchanged, but the derived public contract does not consistently represent that authority.
Directional Notes
Maintainer memory directed attention to consistent explicit-override propagation and a single policy authority across adapter paths; current source confirms that part is implemented correctly. It also prompted verification against actual pinned middleware rather than self-authored expected values, which exposed the trailing-dot and IDNA mismatches. The findings rely on current source and runtime probes, not memory.
Test Coverage
- Ran
tests/unit/mcp/server/test_network_security.pyandtests/unit/mcp/server/test_adapter.py: 331 passed. - Ran related CLI propagation/startup suites: 93 passed.
- Ruff passed for all three changed files.
- Direct probes against pinned MCP 2.0.0 middleware confirmed that inferred settings reject
example.com.:8080,xn--bcher-kva.example.:8080, and modern-IDNAxn--fa-hia.de:8080. - Coverage is strong for explicit Host/Origin overrides, empty-Origin rejection, IPv4, IPv6, case normalization, and ordinary IDNA. The trailing-dot test uses an inaccurate wire header, and no test exercises an IDNA 2003/modern-IDNA divergence.
Merge Recommendation
Request changes. Preserve absolute-FQDN wire spelling, adopt HTTP-compatible modern IDNA serialization, and replace the synthetic expectations with regressions derived from real client Host serialization through MCP 2.0.0 middleware. The explicit override and Origin fixes otherwise appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: be03cd5
request_id: req_1786361110_1530-retry-1-retry-2
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
be03cd5 to
0a0f56d
Compare
|
Fresh exact-head official review requested for 0a0f56d on base f34ca39. Independent verifier: PASS, blocker count 0. Please re-run review 4896332616 against preserved absolute-FQDN wire spelling, modern UTS-46 IDNA, and real HTTPX 2 to MCP 2 middleware serialization. Draft merge freeze remains until all same-head CI is green and the detailed Merge Recommendation explicitly says Approve/Merge. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2033 |
| HEAD checked | 0a0f56d8e9eaedc383b37eab1155437926430d1c |
| Request ID | req_1786364257_1552 |
| Review record | a296bf11-d15d-4efe-af13-f051d1fa7400 |
What Improved
- Explicit Host and Origin overrides now reach both SSE and streamable-HTTP SDK paths, including loopback binds.
- Empty Origin policies now fail closed instead of inheriting the SDK’s browser-loopback defaults.
- DNS authorities preserve trailing dots and use HTTPX-compatible UTS-46 IDNA serialization.
- Tests exercise the pinned MCP middleware rather than only inspecting generated settings.
Issue Requirements
| Requirement | Status |
|---|---|
| Correct transport-security gaps left by PR #2019 | Partially met |
| Explicit Host policy overrides SDK loopback defaults | Met |
| Explicit Origin policy overrides SDK loopback defaults | Met |
| Explicit empty Origin allowlist rejects browser origins | Met |
| Preserve SDK-compatible loopback Host defaults when no override is supplied | Met |
| Normalize inferred DNS authorities as HTTPX 2 emits them | Met |
| Normalize inferred IP authorities as HTTPX 2 emits them | Partially met — non-canonical IPv6 spellings are rejected |
| Preserve absolute-FQDN trailing-dot wire spelling | Met |
| Use modern UTS-46 IDNA serialization | Met |
| Leave socket bind input unchanged | Met |
| Add actual middleware regressions for Origin, Host, overrides, and non-browser clients | Partially met — the IPv6 regression substitutes a canonical URL instead of exercising the original bind spelling |
| Same-head CI and official review explicitly recommend Approve/Merge | Not met — this review identifies a blocking runtime mismatch |
Prior Findings Status
The prior absolute-FQDN and modern UTS-46 IDNA concerns are withdrawn based on current source and real middleware verification. The broader requirement that inferred authorities match actual HTTPX 2 wire serialization remains modified and narrowed to non-canonical IPv6 spellings, with fresh current-snapshot runtime evidence.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/server/auth.py:130 | BLOCKING | as_url_authority() compresses and lowercases IPv6 literals before constructing the inferred Host allowlist, but pinned HTTPX 2 preserves the IPv6 spelling supplied in the client URL. For a supported bind such as 2001:0DB8:0:0:0:0:0:1, the server allows only [2001:db8::1]:*, while an HTTPX request to the operator-provided address sends [2001:0DB8:0:0:0:0:0:1]:8080; the real MCP middleware rejects it with HTTP 421. The new regression at tests/unit/mcp/server/test_network_security.py:464 masks this by pairing the expanded bind with a separately hard-coded compressed client URL instead of constructing the request from host. Preserve/include the actual HTTPX wire spelling for the bind input, or otherwise accept equivalent IPv6 spellings, and add a middleware test using the same expanded input on both sides. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/mcp/server/adapter.py:1087 | Low | High | Fix the documentation typo “and the Ouroboros supplies” to “and Ouroboros supplies.” |
Non-blocking Suggestions
| 1 | src/ouroboros/mcp/server/adapter.py:1087 | Documentation | The updated serve documentation contains a minor grammatical typo. |
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --group mcp-test python -m pytest tests/unit/mcp/server/test_network_security.py tests/unit/mcp/server/test_adapter.py -q: 342 passed. - Independently inspected the pinned MCP 2
TransportSecurityMiddlewareand HTTPX 2.9.1 serialization. - A focused production-path probe confirmed expanded or differently compressed IPv6 client spellings receive HTTP 421, while only the canonical compressed spelling passes.
- PR-body claims of multi-version CI, lint, MyPy, and broader 326-test verification were reported by the contributor but were not independently rerun in this tarball review.
Design Notes
Security policy remains correctly centralized in auth.py, with the adapter acting as a thin transport envelope. The remaining defect is in the canonical-authority assumption: the SDK performs textual Host matching, so semantic IPv6 equivalence cannot substitute for the client’s actual wire spelling.
Design / Roadmap Gate
The change has no persistence or replay impact and preserves explicit operator allowlists. Loopback Origin behavior, DNS normalization, and adapter compatibility are positively verified. The affected public network boundary is not yet safe for supported expanded or non-canonically compressed IPv6 bind input because normal clients using that same address are rejected by DNS-rebinding middleware.
Directional Notes
Maintainer memory directed inspection toward truthful runtime capability enforcement and uniform policy propagation. Current evidence confirms uniform propagation across both transports, but direct pinned-runtime probing shows the inferred IPv6 contract still differs from executable HTTPX/MCP behavior.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --group mcp-test python -m pytest tests/unit/mcp/server/test_network_security.py tests/unit/mcp/server/test_adapter.py -q: 342 passed. - Independently inspected the pinned MCP 2
TransportSecurityMiddlewareand HTTPX 2.9.1 serialization. - A focused production-path probe confirmed expanded or differently compressed IPv6 client spellings receive HTTP 421, while only the canonical compressed spelling passes.
- PR-body claims of multi-version CI, lint, MyPy, and broader 326-test verification were reported by the contributor but were not independently rerun in this tarball review.
Merge Recommendation
Request changes. Fix inferred IPv6 Host handling so an HTTPX 2 client connecting with the operator-provided IPv6 spelling passes the pinned MCP middleware, and replace the hard-coded canonical test URL with a same-input end-to-end regression. The remaining explicit override, Origin, trailing-dot, and IDNA changes are otherwise ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 0a0f56d
request_id: req_1786364257_1552
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
0a0f56d to
1c3d346
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2033 |
| HEAD checked | 1c3d34682088568db4c74709b714cda8396c6044 |
| Request ID | req_1786370338_1591 |
| Review record | 42a15835-9a2c-4fa6-9aa7-67f857c6500e |
What Improved
- Explicit Host and Origin allowlists now reach both SSE and streamable HTTP, including SDK-default loopback binds.
- Empty Origin policy now rejects browser origins instead of inheriting permissive SDK loopback defaults.
- DNS trailing dots, modern IDNA authorities, exact ports, and operator-provided IPv6 spellings receive substantially stronger wire-level coverage.
- Non-default ephemeral-port inference fails closed, while explicit operator allowlists remain available.
Issue Requirements
| Requirement | Status |
|---|---|
| Preserve SDK-compatible loopback Host defaults only when no explicit override is supplied | Met |
| Honor explicit Host and Origin policies across SDK-default loopback binds | Met |
| Treat an empty Origin allowlist as fail-closed for browser origins | Met |
| Normalize inferred DNS and IP authorities without broadening authority or security classification | Not met — UTS-46 normalization broadens loopback classification beyond the socket resolver’s identity |
| Preserve DNS case, trailing-dot, IPv4, and bracketed-IPv6 wire compatibility | Met |
| Accept the operator-provided IPv6 spelling at the configured exact port | Met |
| Avoid inferred wildcard-port matching for non-default authorities | Met |
Fail closed for non-default or distinct IPv6 spelling with port 0 |
Met |
| Apply one transport-security policy across SSE and streamable HTTP | Met |
Prior Findings Status
The prior absolute-FQDN, modern-IDNA, and same-input IPv6 wire-spelling concerns are addressed in the current snapshot and are not maintained as blockers. This round identifies a distinct current-head regression introduced by applying the new wire normalization to the credential-exposure classifier.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/server/auth.py:78 | BLOCKING | Loopback classification now reuses UTS-46 wire normalization, but the socket resolver does not necessarily resolve the original hostname to that normalized identity. For example, local\u115fhost becomes localhost through as_url_authority(..., uts46=True), so is_loopback_host() returns true and resolve_network_security() permits credential-free serving with the SDK loopback Host defaults. Python’s socket-resolution IDNA path instead encodes the original bind name as the distinct xn--localhost-fj9a. If that DNS name resolves to a machine’s routable interface, both the CLI gate and adapter gate allow an unauthenticated network MCP server, and a reachable caller can supply Host: localhost:<port> to pass middleware. Keep wire-authority normalization separate from the fail-closed exposure classifier: only exact ASCII loopback hostname spellings after case/root-dot normalization, or actual loopback IP literals, should bypass authentication. Add a regression proving UTS-46 compatibility mappings cannot turn a distinct resolver hostname into a loopback classification. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran the changed focused suites:
360 passed. - Ran the broader MCP unit suite:
2058 passed, 1 skipped. - Ruff lint and formatting checks passed for all four changed files.
- Inspected the pinned MCP 2.0.0 middleware and HTTPX 2 host serializer directly.
- Manual hostile normalization probe reproduced the blocking divergence: UTS-46 authority
localhost, resolver labelxn--localhost-fj9a, loopback classification true, and authentication disabled.
Design Notes
Centralizing transport policy in auth.py and passing one resolved security object through both network transports is the correct architecture. The remaining defect is a boundary mix-up: HTTP wire canonicalization must not determine whether a bind is trusted as machine-local.
Design / Roadmap Gate
The changed boundary controls whether network clients need credentials before reaching seed execution. There is no persistence or replay impact, and explicit policy overrides are now propagated consistently. Compatibility and security are not merge-safe because caller-controlled bind input can be classified using a different normalization algorithm from the socket resolver, permitting credential-free service on a distinct DNS authority.
Directional Notes
Maintainer guidance focused review on uniform fail-closed authority and truthful adapter behavior. Current source confirms uniform transport propagation, but explicit hostile normalization checks show the loopback authentication decision no longer matches the socket bind identity. Memory was used only to select this boundary for inspection; the blocker is grounded in current code and runtime probes.
Test Coverage
- Ran the changed focused suites:
360 passed. - Ran the broader MCP unit suite:
2058 passed, 1 skipped. - Ruff lint and formatting checks passed for all four changed files.
- Inspected the pinned MCP 2.0.0 middleware and HTTPX 2 host serializer directly.
- Manual hostile normalization probe reproduced the blocking divergence: UTS-46 authority
localhost, resolver labelxn--localhost-fj9a, loopback classification true, and authentication disabled.
Merge Recommendation
Request changes. Separate strict loopback classification from UTS-46 wire serialization and add an adversarial regression for compatibility-mapped Unicode hostnames. The explicit override, Origin, trailing-dot, and IPv6 corrections otherwise appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 1c3d346
request_id: req_1786370338_1591
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1c3d346 to
9a59513
Compare
Separate exact wire and advertised URL authority serialization from loopback identity canonicalization. Absolute DNS root dots now remain intact, and modern UTS-46 IDNA maps faß.de to xn--fa-hia.de in line with the pinned MCP 2 HTTP client boundary. Exercise inferred Host policy through real httpx2 serialization and MCP TransportSecurityMiddleware for dotted ASCII, Unicode, IDNA-divergent, loopback, IPv4, and IPv6 authorities. Refresh the adapter contract to describe Ouroboros-owned explicit transport security. Affected files: - src/ouroboros/mcp/server/auth.py - src/ouroboros/mcp/server/adapter.py - tests/unit/mcp/server/test_network_security.py
9a59513 to
f5660ad
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2033 |
| HEAD checked | f5660ad00874160e73c08470785629ef3dbbf670 |
| Request ID | req_1786375866_1618 |
| Review record | d69e28b3-fea4-4488-ae6d-3de94a574ffe |
What Improved
- Explicit Host and Origin policies now consistently reach both SSE and streamable HTTP transports.
- Empty Origin policy remains fail-closed on loopback instead of inheriting SDK browser-origin defaults.
- DNS, IDNA, trailing-dot, strict loopback-classification, exact-port, and ordinary IPv6 spelling behavior are substantially improved.
Issue Requirements
| Requirement | Status |
|---|---|
| Explicit Host policy overrides SDK loopback defaults | Met |
| Explicit Origin policy overrides SDK loopback defaults | Met |
| Explicit empty Origin allowlist rejects loopback browser origins | Met |
| Normalize inferred DNS and IP authorities to client wire spelling without changing socket bind input | Partially met — covered DNS and unscoped IP forms work, but scoped IPv6 cannot complete authenticated startup |
| Admit canonical and operator-provided IPv6 spellings only at the configured exact port | Partially met — verified for unscoped IPv6; scoped IPv6 fails before serving |
| Do not infer unsafe wildcard-port patterns | Partially met — non-default inference is exact, while SDK-compatible loopback defaults deliberately retain :* |
| Fail closed for non-default or distinct IPv6 spelling with port zero | Met |
| Preserve uniform policy propagation across SSE and streamable HTTP | Met |
Prior Findings Status
The prior trailing-dot and modern-IDNA concern is addressed, the same-input unscoped IPv6 spelling concern is addressed, and the compatibility-mapped Unicode loopback-classification concern is addressed. Those prior blockers are withdrawn based on current source and runtime evidence. The scoped-IPv6 startup failure is a new blocker found in the current snapshot.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/server/auth.py:139 | BLOCKING | Scoped IPv6 bind addresses remain unusable on the authenticated network path. ipaddress accepts a valid bind such as fe80::1%lo, and socket resolution succeeds, but as_url_authority() returns [fe80::1%lo]; build_auth_settings() then constructs http://[fe80::1%lo]:8080, which MCP's Pydantic AuthSettings rejects as an invalid IPv6 URL. Because every non-loopback bind requires authentication, a link-local IPv6 address requiring a zone identifier cannot start the server. Either support scoped authority serialization throughout the SDK contract or reject scoped binds early with a clear documented error, and add focused coverage. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
| 1 | docs/cli-reference.md:1187 | Documentation | The network-exposure documentation still says the SDK automatically enables loopback DNS-rebinding protection. Current code always supplies explicit settings so that empty Origin policy remains fail-closed; update this explanation to reflect the new authority and behavior. |
| 2 | src/ouroboros/mcp/server/auth.py:350 | Contract clarity | The PR body says inferred :* patterns are eliminated, but SDK-compatible loopback defaults still infer three wildcard-port entries and therefore accept malformed suffixes such as localhost:evil. This appears low-risk on a loopback-only, credential-free bind, but the PR description should explicitly identify this deliberate exception. |
Test Coverage Notes
- Reviewed the changed network-security and adapter tests plus the pinned MCP 2.0.0 middleware implementation.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --group mcp-test python -m pytest tests/unit/mcp/server/test_network_security.py tests/unit/mcp/server/test_adapter.py -q: 376 passed.- Focused network-security suite: 138 passed.
- Ruff passed for all four changed files.
- Direct runtime probes verified SDK matching behavior, malformed loopback wildcard behavior, socket resolution of
fe80::1%lo, and the resultingAuthSettingsvalidation failure.
Design Notes
The shared resolver remains the correct single authority for transport policy, and both adapters consume the same wiring. The remaining gap is at the socket-address-to-URL-metadata boundary for scoped IPv6.
Design / Roadmap Gate
There is no persistence or replay impact. Explicit overrides, empty-Origin semantics, strict credential authority, DNS normalization, and ordinary IPv6 compatibility are sound. Compatibility is not yet merge-safe for link-local IPv6 binds requiring a scope identifier: the socket layer accepts the authority, but authenticated metadata construction crashes before either network transport starts.
Directional Notes
Maintainer guidance focused inspection on truthful runtime capability enforcement, explicit hostile gates, and avoiding divergent adapter policies. Current evidence confirms uniform propagation and fail-closed override behavior; the blocker comes from independently probing a valid IPv6 socket authority through the changed serialization path.
Test Coverage
- Reviewed the changed network-security and adapter tests plus the pinned MCP 2.0.0 middleware implementation.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --group mcp-test python -m pytest tests/unit/mcp/server/test_network_security.py tests/unit/mcp/server/test_adapter.py -q: 376 passed.- Focused network-security suite: 138 passed.
- Ruff passed for all four changed files.
- Direct runtime probes verified SDK matching behavior, malformed loopback wildcard behavior, socket resolution of
fe80::1%lo, and the resultingAuthSettingsvalidation failure.
Merge Recommendation
Request changes. Handle scoped IPv6 authorities explicitly and add a regression proving the selected behavior on the authenticated serving path. The previously reported trailing-dot, IDNA, same-input IPv6, and Unicode loopback-classification blockers are otherwise resolved.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: f5660ad
request_id: req_1786375866_1618
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1 participant
Summary
Changes
127.0.0.1, and::1browser origins.:*: the pinned middleware treats it as a raw prefix and would otherwise admit malformed ports such as:eviland:8080.evil. Wildcards remain available only through an explicit operator allowlist.0, where no actual bound port is available to authorize.Verification
360 passedeach.2192 passed, 1 skipped, 1 xfailed.1963 passed, 7 skipped.h11 → Starlette → MCP 2probes: canonical and operator IPv6 spelling at the configured port return 2xx; different, missing, empty, dotted, evil, and doubled port suffixes return 421.Corrective follow-up to #2019 and the official review rounds on this PR. The
no-issuelabel records that this is a security-hardening follow-up to an already merged PR rather than a separately tracked issue.Merge freeze remains until same-head CI is green and the official exact-head review body explicitly recommends Approve/Merge.