Fail closed on separator-only AllowedHosts in standalone MCP host security

[Chris0Jeky]

Closes #1367

Defect

Program.ApplyStandaloneMcpHostSecurity (backend/src/Taskdeck.Api/Program.cs) rewrote
AllowedHosts to the loopback allowlist only when the raw value was blank or contained an
any-host token (*, 0.0.0.0, [::]). A separator-only value like ";", ";;", or " ; "
slipped through both guards:

• string.IsNullOrWhiteSpace(";") is false.
• Splitting with RemoveEmptyEntries | TrimEntries yields an empty array, so .Any(any-host) is
  false.

ASP.NET Core HostFilteringMiddleware then parses ";" as zero configured hosts and falls back
to allowing all hosts — the exact fail-open this method exists to prevent.

Fix

Compute the parsed host set once and treat "parses to zero non-empty hosts" as the failure
condition. Fail closed to StandaloneMcpLoopbackAllowedHosts whenever the post-split host set is
empty (this subsumes the old blank check) or contains an any-host token. Explicit exact
allowlists are unchanged.

var configuredHosts = configuration["AllowedHosts"]?
    .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)
    ?? Array.Empty<string>();
var containsAnyHost = configuredHosts.Any(host => host is "*" or "0.0.0.0" or "[::]");
if (configuredHosts.Length == 0 || containsAnyHost)
{
    configuration["AllowedHosts"] = StandaloneMcpLoopbackAllowedHosts;
}

Tests

Extended the existing #1364 host-security tests in
backend/tests/Taskdeck.Api.Tests/McpHttpTransportApiKeyTests.cs:

• StandaloneMcpHostSecurity_ReplacesPermissiveAllowedHosts gains regression cases ";", ";;",
  " ; " — each proven to be rewritten to the loopback allowlist.
• StandaloneMcpHostSecurity_PreservesExplicitAllowedHosts becomes a theory adding
  "mcp.example.com" (legitimate explicit allowlist preserved) and "good; ;" (a real host mixed
  with separator noise is preserved, not failed closed).

Verification

• dotnet build backend/Taskdeck.sln -c Release -m:1 — 0 errors.
• dotnet test ... --filter "FullyQualifiedName~McpHttpTransportApiKeyTests" — 43 passed, 0
  failed (15 host-security cases, including the 5 new ones, all green).

No EF/model changes; no docs gates touched.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gemini-code-assist]

Code Review

This pull request improves the security of the standalone MCP host configuration in Program.cs by ensuring that empty or separator-only values (such as ";", ";;", or " ; ") in AllowedHosts are treated as invalid and fail closed to the loopback allowlist. This prevents HostFilteringMiddleware from bypassing host filtering. Corresponding unit tests in McpHttpTransportApiKeyTests.cs have been added and updated to verify this behavior. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Chris0Jeky]

Adversarial self-review — no findings

Reviewed the diff against the failure conditions in #1367 at head fa54099c.

CRITICAL / HIGH / MEDIUM / LOW: none.

Verification of the three adversarial questions:

1. Does the fix cover every zero-host parse? Yes. configuration["AllowedHosts"]?.Split(...) ?? Array.Empty<string>() yields length 0 for null, "", whitespace-only, and every separator-only form (";", ";;", " ; ", " ;; ; "). Length == 0 is a strict superset of the old IsNullOrWhiteSpace guard, so nothing the old code failed closed on is now allowed through, and the separator-only gap is closed.
2. Does it change behavior for valid configs? No. The rewrite condition is a strict superset of the previous one (identical any-host predicate over the identical split, plus the empty-set case). Explicit allowlists (mcp.example.test, mcp.example.com) and a real host with trailing separator noise (good; ; → parses to ["good"]) are preserved unchanged — proven by the preservation theory.
3. Whitespace-entry edge cases (good; ;)? Covered by a regression case; preserved, not failed closed.

Also checked: the method is idempotent (re-running on localhost;127.0.0.1;[::1] parses to 3 non-wildcard hosts → no rewrite); Array.Empty<string>() / .Any compile under the existing usings (build is 0 errors).

Out of scope (noted, not changed): the any-host token set (* / 0.0.0.0 / [::]) is pre-existing and unchanged; alternate wildcard spellings (bare ::, expanded IPv6) aren't matched. That is orthogonal to #1367's zero-host fail-open and would be scope creep to alter here.

Bot comments: the Copilot reviewer returned "unable to review … quota limit" (no findings); no other comments present.

Test evidence: dotnet test --filter FullyQualifiedName~McpHttpTransportApiKeyTests → 43 passed / 0 failed, including the 5 new cases (;, ;;, ; rewritten to loopback; mcp.example.com, good; ; preserved). Build -c Release -m:1 → 0 errors.

[Chris0Jeky]

Consolidated adversarial review findings (two independent reviews)

Two independent adversarial reviews (security lens + test lens) of this PR were adjudicated by the batch coordinator. Four findings survive; all will be fixed in one batched push per the zero-skip policy. Bot status: Gemini Code Assist reviewed with "no feedback to provide"; Copilot was quota-blocked. No unaddressed bot threads.

HIGH

F2 — The call site is untested and there is no end-to-end proof (test lens, CONFIRMED). ApplyStandaloneMcpHostSecurity is invoked exactly once (the standalone --mcp --transport http branch, Program.cs:75). Deleting that call leaves every test green: all existing assertions are config-string-only. Nothing proves the rewritten allowlist actually causes HostFilteringMiddleware to reject a hostile Host header, that the post-CreateBuilder config mutation propagates into HostFilteringOptions, or that loopback Hosts are admitted. Fix: one integration test that boots the real standalone MCP HTTP host in-process on an ephemeral port and asserts (a) Host: evil.example → 400 and (b) a loopback Host → not 400 (401 missing-API-key is the pass signal).

MEDIUM

F1 — Port-suffixed any-host wildcards bypass the guard but disable HostFilteringMiddleware (security lens, CONFIRMED). The guard tests raw split tokens with exact ordinal match (host is "*" or "0.0.0.0" or "[::]"), but the middleware normalizes each configured entry via new HostString(entry).Host — stripping :port — before its own top-level-wildcard test. So AllowedHosts=0.0.0.0:5001 (or *:5000, [::]:80) is preserved by the guard as an "explicit allowlist" yet parsed by the middleware as a top-level wildcard → _allowAnyNonEmptyHost = true → host filtering disabled. Fix: normalize each split token the same way the middleware does (new HostString(token).Host) before the any-host comparison; add theory rows "0.0.0.0:5001", "*:5000", "[::]:80", "good.example;0.0.0.0:5001"; update docs/platform/CONFIGURATION_REFERENCE.md, which mirrors the too-narrow wildcard set.

F3 — New comment overclaims (test lens). The comment says all separator-only inputs mean "allow every host", but the middleware splits with RemoveEmptyEntries ONLY (no TrimEntries): ";"/";;" → zero entries → allow-all, while " ; " → two whitespace entries → an ACTIVE filter that rejects everything. Both are misconfigurations and failing closed to loopback is right — but the comment must state both modes accurately. Fix: correct the comment.

LOW

F4 — Preservation theory pins byte-identical raw strings without stating intent (test lens, adjudicated partial-accept). Raw preservation IS the deliberate contract (valid operator configs are not normalized) — behavior stays, but the theory gets a short comment documenting that intent so a future normalization change is a conscious contract change, not an accident.

Fixes, verification evidence, and a finding → commit map will follow in this thread.

[Chris0Jeky]

Fix evidence — all four findings addressed (zero-skip)

All findings from the consolidated adversarial review above are fixed and pushed. Head: 6dfccf3c.

ADDENDUM 2026-07-17 (new head fa5c857d): the F1 direction below was REVERSED by a subsequent Codex review round — see the addendum section at the bottom. The F1 row is kept unedited for the audit trail, but its port-strip normalization was wrong and has been replaced by an exact mirror of the middleware's ToUriComponent() semantics.

Finding	Fix commit(s)	What changed	Verification
F1 (MEDIUM) port-suffixed wildcards bypass the guard	6ef60af8, 084216fe, 6dfccf3c	Guard now normalizes each split token exactly like HostFilteringMiddleware — new HostString(host).Host (strips :port) before the "*" / "0.0.0.0" / "[::]" comparison (Program.cs). Theory rows added: "0.0.0.0:5001", "*:5000", "[::]:80", "good.example;0.0.0.0:5001" — all now rewritten to the loopback allowlist. docs/platform/CONFIGURATION_REFERENCE.md AllowedHosts section updated to state the with-or-without-:port normalization and the zero-host fail-closed rule.	All 4 new rows pass; full class green (counts below).
F2 (HIGH) call site untested, no end-to-end proof	6ef60af8 (test seam), f480c9ec (test)	New StandaloneMcpHostFilteringTests boots the REAL --mcp --transport http entry point in-process (ephemeral port, throwaway Connectors__EncryptionKey + temp SQLite via env, AllowedHosts=";" — the exact #1367 trigger) and asserts: (a) Host: evil.example → 400 from HostFilteringMiddleware; (b) loopback Host → 401 missing-API-key (passes filtering, reaches ApiKeyMiddleware — explicitly NOT 400); plus the built app's config equals the loopback allowlist, and clean shutdown (exit 0). Program.cs restructuring stayed minimal: an 11-line internal test seam (OnStandaloneMcpHttpAppBuilt) — well under the ~30-line constraint, no extraction. The test runs in a DisableParallelization collection (process-wide env vars + entry-point drive).	Mutation-verified: with ApplyStandaloneMcpHostSecurity call commented out, the test FAILS (1/1 failed); with the call restored it passes.
F3 (MEDIUM) comment overclaims allow-all for " ; "	6ef60af8	Comment rewritten to state both middleware modes accurately: ";"/";;" → zero entries → filtering disabled (every host allowed); " ; " → whitespace entries → active filter rejecting every host. Both misconfigurations fail closed to loopback.	Comment-only; behavior covered by existing rows.
F4 (LOW) preservation contract undocumented	084216fe	Contract comment added on StandaloneMcpHostSecurity_PreservesExplicitAllowedHosts: valid operator allowlists are preserved byte-for-byte, never normalized; changing that is a deliberate contract change.	Comment-only; theory unchanged and green.

Test evidence (Release, --no-build after clean build):

• dotnet build backend/Taskdeck.sln -c Release -m:1 → 0 errors (12 pre-existing warnings).
• --filter "FullyQualifiedName~McpHttpTransportApiKeyTests|FullyQualifiedName~StandaloneMcpHostFilteringTests" → 48 passed / 0 failed / 0 skipped (was 43: +4 F1 theory rows, +1 integration test).
• Mutation run (guard call deleted): --filter "FullyQualifiedName~StandaloneMcpHostFilteringTests" → 1 failed / 0 passed, restored → green.

Bot threads: Gemini Code Assist reviewed with "no feedback to provide"; Copilot was quota-blocked. No inline threads existed at the time of the original comment.

---

ADDENDUM — Codex round 2 (head fa5c857d): F1 direction reversed + two test hardenings

A Codex review on 6dfccf3c posted 3 inline threads; all three are fixed, replied to with direct evidence, and resolved.

F1 REVERSAL (Codex P2, Program.cs — CORRECT; overrules the original F1 fix). The real HostFilteringMiddleware normalizes each configured entry as new HostString(entry).ToUriComponent() — which retains the :port suffix — before its exact-ordinal IsTopLevelWildcard test. So AllowedHosts=0.0.0.0:5001 is NOT allow-all in the middleware: it is a literal pattern that no real Host header can match (request hosts are compared portless), i.e. already fail-closed (deny-all). The original F1 fix's port-strip (HostString.Host) rewrote that fail-closed misconfiguration to the loopback allowlist — strictly weaker: on a non-loopback bind it would admit spoofed loopback Host headers that the literal pattern rejects. Both directions were adversarially reviewed; the middleware source semantics (ToUriComponent, not Host) are the deciding evidence.

• ecdff6d7 — guard now mirrors the middleware exactly (new HostString(host).ToUriComponent() is "*" or "0.0.0.0" or "[::]"), with a code comment citing the ToUriComponent+IsTopLevelWildcard behavior so this does not flip-flop again. Zero-host fail-closed and bare-wildcard rewrite are unchanged.
• 6ef7ecb9 — the four port-suffixed theory rows ("0.0.0.0:5001", "*:5000", "[::]:80", "good.example;0.0.0.0:5001") flipped from "rewritten" to "preserved", with the middleware rationale documented on the preservation theory.
• fa5c857d — docs/platform/CONFIGURATION_REFERENCE.md corrected: port-suffixed entries are literal host patterns (effectively deny-all for real traffic — an operator misconfiguration that fails safe), not wildcards.

Entry-point result hardening (Codex P1, StandaloneMcpHostFilteringTests.cs — adjudicated likely invalid; hardened anyway). For C# async top-level statements the assembly entry point is the compiler's synchronous bridge returning int — proven by execution: the pre-hardening (int) cast passed every local Release run, including the awaited clean-shutdown exit-code assertion, which would have faulted with InvalidCastException if Invoke returned Task<int>. 89fa8acd nonetheless handles all result shapes (int / Task<int> / Task) explicitly, removing the dependence on the compiler detail.

Startup-race fix (Codex P2, StandaloneMcpHostFilteringTests.cs — confirmed real latent flake). 89fa8acd moves the ApplicationStarted subscription inside the OnStandaloneMcpHttpAppBuilt seam, which runs synchronously on the entry-point thread between Build() and RunAsync() — the subscription now provably precedes host start, so a fast startup can no longer be missed.

Verification at fa5c857d: build 0 errors; McpHttpTransportApiKeyTests + StandaloneMcpHostFilteringTests → 48 passed / 0 failed / 0 skipped; StandaloneMcpHostFilteringTests run 3x consecutively → 3/3 green (derisking the startup-race fix).

---

ADDENDUM 2 — Codex round 3 (head 0c39fe8e): final convergence to exact middleware mirroring

A Codex P2 on fa5c857d (Program.cs) correctly extended the port-suffix argument to whitespace: the middleware splits with RemoveEmptyEntries but does NOT trim, so " ; " parses to whitespace-only literal entries — an ACTIVE deny-all filter — while the guard's TrimEntries classified it as zero hosts and rewrote it to the loopback allowlist (weaker: Host: 127.0.0.1 spoofable on a non-loopback bind). The earlier keep-the-trim adjudication was superseded for consistency.

The final rule as implemented (7ee42eb3) — the guard mirrors the middleware's parse EXACTLY: Split(';', RemoveEmptyEntries) with no trimming, wildcard test on new HostString(entry).ToUriComponent(). Rewrite to localhost;127.0.0.1;[::1] exactly when the middleware itself would disable filtering: zero parsed entries (null / blank / ";" / ";;" — the true #1367 fail-open) or a top-level wildcard (* / 0.0.0.0 / [::]). Every other value — whitespace-bearing (" ; ", " * ", "good; ;"), port-suffixed pseudo-wildcards — is preserved because the middleware fails closed on it (deny-all or partial).

• 7134f704 — " ; " and " * " moved to the Preserves theory with the deny-all rationale; ";"/";;"/blank remain in the Replaces theory.
• 0c39fe8e — docs/platform/CONFIGURATION_REFERENCE.md restated as the single rule.
• Integration test unchanged (uses ";", still in the rewrite set).

This was the third and final convergence step (zero-host → port-suffix → whitespace); the guard's decision function is now equivalent to the middleware's own configuration semantics by construction.

Verification at 0c39fe8e: build 0 errors; McpHttpTransportApiKeyTests + StandaloneMcpHostFilteringTests → 48 passed / 0 failed / 0 skipped; integration test re-run → 1/1 green. Thread replied to with evidence and resolved; zero unresolved threads on the PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6dfccf3cbd

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa5c857de7

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".