Skip to content

fix(api): ignore forwarded headers when no proxy is trusted, and close MCP audit gaps - #1260

Merged
mforce merged 4 commits into
mainfrom
fix/mcp-audit2
Oct 12, 2026
Merged

mforce merged 4 commits into
mainfrom
fix/mcp-audit2

Conversation

@mforce

@mforce mforce commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Part of #789

Fixes four findings from the second MCP audit, plus a forwarded-header spoofing defect that was already on main.

What changes for a deployment

Forwarded headers are ignored when no proxy is trusted. ASP.NET's ForwardedHeadersMiddleware trusts every peer when its trust lists are empty (checkKnownIps in aspnetcore v10.0.0). Under the documented direct-TLS opt-out (RateLimiting:AllowNoTrustedProxies=true, empty RateLimiting:TrustedProxies), any client could pick its own address for the per-IP login limiter (#143) and the /mcp failure budget and in-flight cap (#1225). It could also pick its own scheme. With an empty list, AddCluckworkEdgeSecurity now sets ForwardedHeaders.None, and the app does not add the middleware. None also covers ASPNETCORE_FORWARDEDHEADERS_ENABLED=true. That setting makes the default host add its own forwarding middleware through a startup filter, so only skipping the app's middleware would have left that path open. Deployments with a non-empty list behave as before.

R2-01 (P2). A misconfigured proxy list is now reported. With a non-empty list, X-Forwarded-For from a peer outside it is ignored, so every client behind that peer shares one address in each per-address limit. One caller can then spend the /mcp budget for everyone. The first such request in a process logs RateLimiting.ForwardedHeadersIgnored at Warning and names the peer. The trust check copies the framework's CheckKnownAddress, covering KnownProxies, KnownIPNetworks and mapped IPv4. The check runs once for all routes, just before UseForwardedHeaders, because only there is RemoteIpAddress still the socket peer, and the login limiter collapses the same way /mcp does. The budget itself is unchanged.

R2-05 (P3). cluckwork_record_daily_entry now trims grade names, as the read tools do. " Large" used to be refused.

COD2-02 (P3). cluckwork_record_daily_entry with a null or blank flock now returns flock is required. before the idempotency claim. It used to throw NullReferenceException, which the SDK logged at Error and reported to the model as a generic error. idempotencyKey and flock are the only required text parameters in any tool, and idempotencyKey already had this check, so this is one line beside it rather than a general null check in the bad-argument filter.

R2-03 (P3), with a corrected premise. The audit said Assert.Subset(served, declared) missed a declared tool that is not served. That was wrong. xUnit checks that its second argument is a subset of its first, so the old assertion already caught a hidden tool. What it missed was an extra tool that is served but not declared. The test now uses Assert.Equal on the sorted names, and the declared set includes the test host's ProbeTools.

Where to start reading

  1. src/Cluckwork.Api/Hosting/CluckworkEdgeSecurityServiceCollectionExtensions.cs holds the None selection and UseTrustedForwardedHeaders (the warning and the trust check). Program.cs swaps one call.
  2. src/Cluckwork.Api/Mcp/DailyEntryTools.cs holds the flock check and the grade trim.
  3. Tests: ForwardedHeaderTrustKestrelTests.cs (two new no-trust hosts over real Kestrel), ForwardedHeadersIgnoredLoggingTests.cs, RecordDailyEntryToolFaultTests.cs (claim counter), McpToolErrorTests.cs, RecordDailyEntryToolTests.cs and McpEndpointTests.cs.
  4. Docs: decision records Deploy: production proxy-trust chain unsolved — silently disables #144 HSTS and the #143 per-IP login limiter #260 and MCP slice 3: map /mcp, wire RBAC and tools/list filtering #806, the event in SecurityEvents, and its row in docs/security/log-redaction-policy.md.

The branch has three fix commits and one merge of main. The second and third commits narrow the forwarded-header fix after local review rounds.

Tests

  • The targeted integration slices pass, 521 of 521, on the merge with main. They cover MCP, OAuth, forwarded headers, the no-trust hosts, rate limits, trusted proxies, security events, the login-counter probe, security headers, auth cookies, HSTS over TLS and step-up.
  • The architecture guards pass, 386 of 386.
  • The no-trust tests run over real Kestrel, so the trust decision sees a genuine socket peer. One host sets ForwardedHeaders_Enabled=true. Both keep the loopback address and http under a spoofed X-Forwarded-For: 203.0.113.10 and X-Forwarded-Proto: https, and log no warning.
  • NullFlock_IsRefusedBeforeTheKeyIsClaimed counts INSERT INTO idempotency_records. It sees 0 for a null flock and 1 for an unknown flock, which is the positive control.

Mutation checks

Each mutation was applied, the named tests were run, and the file was restored.

Mutation Result
M1. The warning is never logged Red
M2. It warns on every request Red
M3. The warning ignores KnownIPNetworks Red
M4. The helper adds both middlewares with forwarding off Red, because the warning fires
M5. One tool hidden from serving, Equal Red
M5a. The same hidden tool, with the original Subset Red, because the old test already caught it
M5b. One extra undeclared tool served, Equal Red
M5c. The same extra tool, with the original Subset Green. This is the gap Equal closes.
M6. No grade trim Red
M7. The flock check removed Red, in the bad-argument rows and the claim test
M7b. The flock check moved inside the claim callback Red, in the claim test only
M8. The warning ignores KnownProxies Red
M9. Forwarding left on with an empty list Red, in both no-trust tests
M9b. The round-1 code (no None, early return on an empty list) Red, in the ForwardedHeaders_Enabled test only: RemoteIpAddress was 203.0.113.10

M9b reproduces the ASPNETCORE_FORWARDEDHEADERS_ENABLED path. In that case, only the host's own startup filter could have applied the header.

Live run

Each run used an isolated sim stack built from this branch, with the TLS proxy from tools/verify/mcp/.

  • A TypeScript SDK client completed OAuth consent with the password step-up and listed 10 tools. The MCP Inspector CLI listed the same 10 tools over the proxy.
  • Inspector recorded a daily entry with grades: [{"grade": " Large", "quantity": 90}]. Reading it back showed a Draft with Large at 90.
  • Inspector called the same tool with flock: null and got An error occurred invoking 'cluckwork_record_daily_entry': flock is required. The API log had no Error entries for these calls; the refusals were at Information.
  • With RateLimiting__TrustedProxies__0=10.255.0.0/24, two /mcp requests through the proxy logged exactly one Warning: RateLimiting.ForwardedHeadersIgnored, Peer=::ffff:172.20.0.1. With 172.16.0.0/12, before and after, there were none.
  • Opt-out run: no TrustedProxies, RateLimiting__AllowNoTrustedProxies=true, and a login limit of 3. Five wrong-password logins went straight to the app port, each with a different spoofed X-Forwarded-For (203.0.113.1 to .5) and X-Forwarded-Proto: https. They returned 401, 401, 401, 429, 429, so all five landed in one bucket. Every Auth.LoginFailed and Auth.RateLimitRejected event named the real peer 172.20.0.1, and there was no ForwardedHeadersIgnored warning.

The consent screenshots are in a comment below.

…P audit gaps

- Log RateLimiting.ForwardedHeadersIgnored at Warning, once per process, when
  X-Forwarded-For arrives from a peer outside RateLimiting:TrustedProxies.
  Every client behind that peer shares one address in the login limiter and
  the /mcp failure budget and in-flight cap, so one caller can spend it for all.
- cluckwork_record_daily_entry refuses a null or blank flock with
  "flock is required." before the idempotency claim, instead of a
  NullReferenceException logged at Error.
- cluckwork_record_daily_entry trims grade names, as the read tools do.
- ToolSchemas_AreServedForEveryTool_WithoutNull asserts served == declared.
ASP.NET's ForwardedHeadersMiddleware trusts every peer when its trust lists
are empty, so under RateLimiting:AllowNoTrustedProxies any client could set
its own address for the per-IP limits and its own scheme. With an empty
RateLimiting:TrustedProxies the forwarding middleware is now not added.

RateLimiting.ForwardedHeadersIgnored follows the framework's trust decision:
it fires only when the list is non-empty and the peer is in neither
KnownIPNetworks nor KnownProxies.

Tests: an opt-out host over Kestrel keeps the real peer and scheme and logs no
warning; a KnownProxies peer does not warn; a null flock is refused before any
idempotency claim.
ASPNETCORE_FORWARDEDHEADERS_ENABLED=true makes the default host add its own
ForwardedHeadersMiddleware through a startup filter, so not adding the app's
middleware left that path trusting every peer under an empty
RateLimiting:TrustedProxies. An empty list now sets ForwardedHeaders.None,
which also governs the host's middleware; the app's helper adds neither
middleware when forwarding is off.

A Kestrel host with ForwardedHeaders_Enabled=true and no trusted proxy keeps
the socket address and the http scheme under spoofed X-Forwarded-For and
X-Forwarded-Proto.
@mforce

mforce commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Consent screen from the live run, captured at 1:1 from the isolated stack built at 6ef8d27. Later commits on this branch change only forwarded-header handling, so this screen is the same at the head. The TypeScript SDK client reached it through discovery, dynamic registration and PKCE, then completed the password step-up and got a token with farm:read daily-entries:write. Frames are 1280 and 390 wide, light and dark.

Consent screen, 1280 light

Consent screen, 1280 dark

Consent screen, 390 light

Consent screen, 390 dark

@mforce
mforce merged commit 8c3ba0e into main Oct 12, 2026
21 checks passed
@mforce
mforce deleted the fix/mcp-audit2 branch October 12, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant