[erikdarlingdata]

Item 5 of #2479.

The gap

All five refusal sites across the two listeners were a bare StatusCode = …; return;:

Host	Gate	Status	Logged before
MCP	Host header allowlist (DNS-rebinding guard)	400	nothing
MCP	bearer token	401	nothing
MCP	source CIDR (mcp.network.allowFrom)	403	nothing
Web	Host header allowlist	400	nothing
Web	source CIDR (web.network.allowFrom)	403	nothing

So "is my token wrong, or my CIDR wrong?" was answerable only from the client — which sees one opaque status code and cannot tell a Host-allowlist 400 from a malformed request, or a CIDR 403 from anything else. The person who can fix either is on the server, and the server said nothing.

Each gate now emits a WARN naming itself and the config key that governs it.

The web host's ShowLogin path is included too, even though it answers 200 rather than 401 — but only when a token was actually presented and did not match. That is precisely the state an operator asks about when a ?token= they pasted did not work, and the status code hides it. A first visit with no token is the normal route to the login page, so logging that would make every bookmark a warning.

Out of scope on purpose: DarlingWebEndpoints' validation 400s and the CSRF 415 already return a body that explains itself to the client. This is about the gates that refuse silently, before any handler.

The rate limit, and why it is shaped this way

This is the load-bearing half, so it is measured rather than asserted about.

• Key is (gate, source address), and a first sighting always logs immediately. A limiter that makes the tester wait out a window before their own answer appears has not shipped anything. Only repeats fold — 10 minutes — which costs nothing, because a second identical refusal from the same address through the same gate carries no information the first did not. Fix the token and now fail the CIDR check, and that is a different key, which logs at once.
• Per source, not per gate alone. Per-gate is a smaller key set and it is wrong: one scanner's line would win the window and hide the tester's refusal completely — this defect reintroduced by the fix for it.
• Which makes the map unbounded, so there is a cap. Past 16 distinct sources in a window, further sources fold into one per-gate aggregate that says so ("speaking for several sources — the per-source log budget is full, which usually means this port is being scanned"). Bounds memory and volume to cap + gate count entries and one line per gate per window.
• The accepted cost, stated rather than discovered: under an active scan the per-source slots may be taken and a tester arriving mid-scan lands in the aggregate. That is visible rather than silent, and if the port is being scanned, that is the more urgent thing for the operator to know.
• Eviction is keyed on last SEEN, not last logged. Keyed on last logged, a source still being refused every second would be dropped at the two-window horizon and re-log as a fresh first sighting — turning the rate limit into a rate multiplier, which is worse than no limit because it looks like one is working.
• The budget belongs to the started server, not the process, so a rebind starts clean rather than inheriting the previous listener's scan and swallowing the new one's first refusals.

Measured ceiling: 108,000 refused requests over an hour (30/s across three gates from a few hundred sources) produce 100 log lines and 19 tracked entries.

What is never logged

The token, any prefix of it, and its length. A line may say whether a credential was presented — an absent Authorization header is a client that was never configured; a wrong one is a token that does not match mcp.network.encryptedToken, and each points at a different next step — and nothing about its value. Pinned per call site rather than trusted.

The Host header is the only request-derived value echoed, and it goes through a sanitizer first. A log file is a text file: a header carrying \r\n would otherwise forge whole log entries.

Verification

• Ran the shipped DarlingHttpRefusalLog.cs — <Compile Include>'d into a throwaway net10.0 console harness, since Darling.Tests is net10.0-windows and cannot run on macOS. 26/26 assertions pass, including the 108,000-request scan ceiling, the first-sighting-is-immediate guarantee, the noisy-neighbour case, the cap announcement, the last-seen eviction invariant, and the log-injection cases.
• Simulated both structural pins against the real host files: MCP has 3 of 3 4xx sites reported, Web 2 of 2, 6 Report calls total, no forbidden token identifier in any argument list.
• Re-checked the existing HostHeaderGuardTests assertions against the edited files — FirstMiddlewareAfterBuild still finds IsAllowedHost(context.Request.Host.Host and Status400BadRequest in the first middleware, and the guard still precedes if (networkMode) and MapMcp. Adding lines inside that lambda was the one thing that could have broken them.
• Service and Darling.Tests both build clean, 0 errors.
• Against dev the whole test class fails to compile — DarlingHttpRefusalLog does not exist there.
• Not verified: no request has been pushed through a live Kestrel host, so the exact rendered line and the interaction with the file/Event Log sinks are unexercised. Manual check on a box: with mcp.network exposed, curl -H "Authorization: Bearer wrong" http://<ip>:5152/ should produce one WARN naming the access-token gate, and a second identical curl within ten minutes should produce none.

Onboarding

Makes docs/uat-onboarding.md §4.4 "When it says enabled and still will not connect" substantially unnecessary — the section exists because the server-side answer had to be reconstructed from the client's status code. The tell it teaches (401 = token, 403 = CIDR, 400 = Host) is now stated in the service log at the moment it happens, naming the config key. §5's "the things that will actually bite you" entry on the same subject can go the same way. The gate list is still worth keeping as a reference table; the "you can only tell from the client" framing is what stops being true.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

[claude]

Minor/non-blocking: ExtractBearerToken is now called twice on this refusal path — once inside IsBearerTokenAuthorized above, once here to decide the "presented vs. mismatched" wording. Only reached on an already-refused request, so no real perf concern, but if IsBearerTokenAuthorized were changed to return the extracted token (or a small result type) this duplicate parse would go away for free.

[claude]

Review summary

This is a well-scoped, well-tested change — no correctness bugs found in DarlingHttpRefusalLog or its call sites. The rate-limiting policy (first sighting always logs, repeats fold with the suppressed count riding the next line, per-(gate, source) keying with a capped aggregate fallback, eviction on last-seen not last-logged) is exactly what it needs to be to avoid becoming a log-filling DoS vector itself, and the test suite (DarlingHttpRefusalLogTests.cs) actually measures the claims (108k-request ceiling, no-hidden-tester-under-a-scanner, no-token-value-ever-logged, CRLF-can't-forge-a-line) rather than just asserting they're true.

Specifically checked and found solid:

• Log injection: Sanitize strips control chars (< 0x20 and 0x7F, covering CR/LF) and caps length before the Host header is ever echoed — the only attacker-controlled value that reaches a log line.
• Secret handling: no refusal site logs the token, a prefix, or its length — only "presented" vs. "did not match," pinned by a source-text test (NoRefusalLine_CanCarryTheToken).
• Concurrency: the per-source/per-gate maps are Dictionary (not ConcurrentDictionary) under a single lock — correct, since a decision has to read+write last-seen/last-logged/suppressed and check Count against the cap atomically, and this path is only hit on an already-refused request so contention is a non-issue.
• Scoping: DarlingHttpRefusalLog is a local per-ExecuteAsync instance (not static), so a listener rebind starts with a clean budget — matches the TheBudget_IsPerStartedServer_NotProcessWide pin.
• All 5 original silent StatusCode = ...; return; sites (3 on MCP, 2 on Web, plus the Web ShowLogin 200 case) now report, confirmed by grepping both host files directly.

Lite/Darling parity note (non-blocking)

Lite's McpHostService.cs has the identical DNS-rebinding Host-allowlist gate (context.Response.StatusCode = StatusCodes.Status400BadRequest; return; at line 131) with no logging — the same silent-refusal shape this PR fixes on both Darling hosts. Lite has no bearer-token/CIDR gates, so the full rate-limited DarlingHttpRefusalLog machinery would be overkill there, and the existing comment in that file already rates the severity as informational (loopback-only, single-user desktop app). Still, it's the same class of gap this PR was written to close, so it may be worth a small follow-up on the Lite side (even a simple non-rate-limited log line, since Lite is loopback-only and can't be scanned the way the LAN-exposed Darling ports can) so the two apps don't drift on this specific point.

Minor nit (left inline)

DarlingMcpHostService.cs calls ExtractBearerToken twice on the token-refusal path (once inside IsBearerTokenAuthorized, once to decide the "presented vs. mismatched" wording). Only reached on an already-refused request, so no real perf impact — just flagging in case IsBearerTokenAuthorized later grows a variant that returns the extracted token directly.

No T-SQL in this PR, so the collector/style conventions (OPTION(RECOMPILE), AS aliasing, etc.) don't apply here.

[erikdarlingdata]

Good catch — fixed in 2a857dc by hoisting the header to a local, so StringValues.ToString() runs once and both readers share it.

I did not take the fuller version (having IsBearerTokenAuthorized return what it parsed): its signature is pinned by DarlingMcpHostTests.IsBearerTokenAuthorized_MatchesOnlyExactBearerToken and DarlingHostBindingTests, and threading a result type through it to save one parse on an already-refused request is not a trade worth making. The comment now says that, so the next reader does not re-open the question.

[claude]

DescribeSuppression always renders "N further refusal(s) from this source were not logged...", but this text is also used for aggregate entries (decision.Aggregated == true), where the folded refusals came from multiple different source addresses once the per-source cap (16) was exceeded, not from one source.

Concretely: with the per-source budget full, gate Token keeps getting hit by many different overflow IPs within the same 10-minute window. The first overflow logs with Aggregated: true and creates the _perGate[Token] entry; every subsequent overflow (from any other IP) folds into it via Fold(aggregate, ..., aggregated: true), incrementing Suppressed. When the window elapses and the next overflow logs, the line will combine the "(speaking for several sources …)" scope suffix with "N further refusal(s) from this source were not logged" — actively misleading, since those N refusals were spread across many different addresses, not repeats from one. That directly undercuts the PR's stated goal of giving the operator an accurate answer, and isn't covered by PastTheCap_TheLineSaysItSpeaksForSeveralSources (which only exercises a single overflow, before any folding/suppression happens on the aggregate).

Suggest branching the wording on decision.Aggregated (e.g. "…were not logged from other sources sharing this gate's aggregate…") or otherwise not implying a single source when the aggregate bucket is speaking for the cap overflow.

[claude]

Reviewed the diff (DarlingHttpRefusalLog.cs, its tests, and the wiring into DarlingMcpHostService.cs / DarlingWebHostService.cs). This is pure C#/ASP.NET pipeline code — no T-SQL, so the style-guide items in CONTRIBUTING.md (OPTION(RECOMPILE), AS aliasing, etc.) don't apply here.

Overall: the rate-limiting design is sound and well-tested (first-sighting-always-logs, per-(gate,source) folding, eviction on last-seen not last-logged, capped memory with an announced aggregate fallback). Left one inline finding on a wording bug in the suppression message when it applies to an aggregated (multi-source) bucket rather than a single source — see the inline comment on DarlingHttpRefusalLog.cs.

Lite/Darling parity check: this only touches the Darling service host. I checked Lite/Mcp/McpHostService.cs — it shares the same Host-header allowlist gate (HostHeaderGuard/DarlingHostBinding.IsAllowedHost, still a bare StatusCode = 400; return; with no logging) but has no bearer-token or CIDR gates at all (loopback-only, tokenless by design). So there's no Lite counterpart for the Token/CIDR logging added here, and I don't think there should be — but the Host-allowlist 400 in Lite is now the one refusal site in the codebase that still refuses silently. Given Lite's own comment calls that gate "informational severity (single-user desktop app)," this is probably fine to leave as-is, but flagging in case parity was intended to extend that far.

Security: the token-value redaction (present/absent or match/mismatch only, never the value/prefix/length) and the CR/LF-stripping Sanitize() on the only attacker-controlled field echoed (Host header) both look correct and are exercised by tests (NoRefusalLine_CanCarryTheToken, AHostHeader_CannotForgeLogLines).

Performance: Report/Observe only run on an already-refused request and do a bounded (≤ ~19-entry) dictionary scan under a lock; no concern there.

No missing-index DMV comments applicable — no T-SQL in this PR.

[erikdarlingdata]

Right, and the analysis is exact — the aggregate bucket folds many different addresses through one gate, so "from this source" is false there, and false in the direction that costs most: it sends the operator looking for one busy client when what is happening is a broad scan, which is the very thing the aggregate bucket exists to tell them.

Fixed in 84086e4. The clause now branches on Decision.Aggregated:

• aggregate: 399 further refusal(s) through this gate, from other sources, were not logged in the last 10 minute(s).
• per-source: 7 further refusal(s) from this source were not logged in the last 10 minute(s).

Your point about the coverage gap was the useful half — PastTheCap_TheLineSaysItSpeaksForSeveralSources only exercises the first overflow, before any folding, so the clause that ships with the folded count was never rendered by a test. The new TheAggregateSuppressionClause_DoesNotClaimTheyCameFromOneSource fills the budget, pushes 400 distinct overflow addresses through one gate, crosses the window, and asserts both wordings — including that a per-source entry still says "from this source", so the fix cannot flatten both into one hedge. Rendered output verified against the shipped class.

[claude]

ExtractBearerToken(authorization) is null collapses two different client states into one message. It returns null not only when the Authorization header is absent, but also when it's present with the wrong scheme (e.g. Authorization: Basic xxx) or Bearer with a blank/whitespace-only token (see ExtractBearerToken below, lines 921-937).

In those malformed-but-present cases, the operator sees "no 'Authorization: Bearer ' header was presented" even though a header was sent — just not in the expected shape. Given this whole feature exists so operators can tell "client never configured" from "client configured wrong" at a glance, that's exactly the ambiguity it's meant to resolve. Consider distinguishing "header absent" (authorizationHeaderValue is null) from "header present but not a valid Bearer token" (ExtractBearerToken(...) is null but header non-null) so a malformed scheme doesn't get reported as "not presented".

[claude]

Reviewed. The core DarlingHttpRefusalLog implementation is careful and well-tested (per-gate/per-source keying, last-seen eviction, aggregate overflow, CRLF sanitization, token-value exclusion) — the design rationale in the class doc and the test suite hold up against the code. Left one inline note on a message-accuracy nit in the bearer-token refusal path.

Lite/Darling parity note (not a blocker, flagging per review scope): Lite/Mcp/McpHostService.cs:129-133 has the same shape of silent refusal this PR fixes on Darling — the Host-header allowlist gate does context.Response.StatusCode = StatusCodes.Status400BadRequest; return; with zero logging. This PR's new DarlingHttpRefusalLog lives in Darling/PerformanceMonitor.Darling.Service/Hosting/ rather than PerformanceMonitor.Common, so Lite has no path to reuse it even if a future PR wants to close that gap (unlike HostHeaderGuard, which is already shared and is what both hosts call into). Given the PR explicitly frames this as closing "the gap" of silent refusals, worth a conscious call on whether Lite's tokenless/CIDR-less, loopback-only host warrants the same treatment (even if lower severity, per the existing comment in that file) — or whether this is intentionally Darling-only scope for now.

No correctness bugs found beyond the inline note. No T-SQL touched by this PR, so the collector-style conventions in CONTRIBUTING.md don't apply here.

[erikdarlingdata]

Correct, and it is the same defect this PR exists to fix, sitting inside the fix. ExtractBearerToken returns null for both "no header" and "a header that is not a well-formed Bearer", so the line told an operator nothing was presented while their client was sending Authorization: Basic … every second.

Fixed in 4bae26d. Three states now, because each is a different next step:

header	message
absent / blank	no 'Authorization: Bearer <token>' header was presented
Basic …, a bare token, Bearer with an empty or blank token	an Authorization header WAS presented but is not a 'Bearer <token>' (wrong scheme, or an empty token after 'Bearer')
well-formed Bearer that misses	the presented bearer token does not match mcp.network.encryptedToken

Pulled out into a pure DescribeBearerRefusal so all three are pinned by a table rather than by reading a lambda — and so the "never the value" property becomes structural: it reads the header's shape and has nothing else to leak. The table covers the wrong scheme, a bare token with no scheme, Bearer with empty and blank tokens, and the case-insensitive scheme forms that must still reach the third state. Verified against the shipped method, 14/14.

[claude]

Minor wording nit: DarlingHttpRefusalLog.Report's message template is fixed as "{Surface} refused a request from {Source} with {Status}: the {Gate} gate rejected it — {Reason}...". Passing Status200OK here produces a line like:

Web dashboard refused a request from 10.0.0.5 with 200: the access token gate rejected it — the presented ?token= does not match web.network.encryptedToken, so the login page was served instead.

"refused ... with 200" reads as self-contradictory (200 = success), which could trip up anyone grepping the log for actual denials (e.g. filtering on status >= 400) or just reading it cold. The PR description explains why this 200 case is intentionally included, but that reasoning doesn't reach the log line itself. Worth either rewording the shared template to not always say "refused", or giving this call site a distinct phrasing (e.g. "declined a stale ?token= and served the login page instead" rather than "refused ... with 200").

[claude]

Reviewed the diff (DarlingHttpRefusalLog.cs + the two host wiring changes + the new test file). This is a tightly-scoped, well-tested change — the rate-limiter design (per-source-first-sighting-always-logs, per-gate aggregate overflow announced, eviction on last-seen not last-logged) holds up, and the token-redaction / CRLF-sanitization boundaries are correctly enforced (only the Host header is echoed, and only through Sanitize; no call site passes the token's value, prefix, or length into Report).

Two things worth a look, both left as inline suggestions/notes where I could anchor them:

1. Wording nit (inline comment on DarlingWebHostService.cs): the ShowLogin refusal is reported with StatusCodes.Status200OK through the shared template "{Surface} refused a request ... with {Status}: ...", producing a log line like "Web dashboard refused a request from X with 200: ..." — self-contradictory since 200 means the request succeeded (it got the login page). Not a functional bug, just something that'll read oddly to an operator or a log-scraping tool that filters on status >= 400.

2. Lite/Darling parity note (couldn't anchor inline since the file isn't in this PR's diff — see Lite/Mcp/McpHostService.cs:129): Lite's MCP host has the same Host-header allowlist gate Darling has, and it still refuses silently after this PR. The bearer-token and CIDR gates this PR covers have no Lite equivalent at all (Lite is loopback-only/tokenless, per its own comment), so most of this genuinely doesn't apply — but the Host-allowlist 400 is a gate the two editions do share, so it's worth confirming that leaving it silent in Lite is a deliberate call (Lite's lower threat model) rather than an oversight, so it doesn't read as unintentional drift later.

No correctness bugs found in the rate-limiter logic itself, and the test file's structural pins (every 4xx site reports, no forbidden token identifiers, budget is per-started-server) look sound.

[erikdarlingdata]

Agreed — and it is worse than a nit, because a log line that says "refused … with 200" argues with itself and would mislead anyone filtering for denials on status. The reasoning for including the 200 case was in the PR description, which is not where the person reading the log is standing.

Fixed in df057b6. The verb is derived from the status and the code is stated as HTTP N:

MCP refused a request from 10.0.0.5 (HTTP 401): the access token gate rejected it — ...
Web dashboard did not authorize a request from 10.0.0.5 (HTTP 200): the access token gate rejected it — the presented ?token= does not match web.network.encryptedToken, so the login page was served instead.

Kept as a pure DescribeOutcome so the boundary is pinned rather than asserted about, and so the structured template keeps its named placeholders instead of being concatenated into one opaque string.

[claude]

DescribeOutcome exists specifically so a 200/302 refusal isn't logged as "refused" (see the docstring above it, and TheOutcomeVerb_AgreesWithTheStatusCode). But this format string hardcodes the {Gate} gate rejected it regardless of outcome. For the web host's ShowLogin path (200, wrong ?token=), the rendered line is:

Web dashboard did not authorize a request from 10.0.0.5 (HTTP 200): the access token gate rejected it — the presented ?token= does not match web.network.encryptedToken, so the login page was served instead.

"did not authorize ... HTTP 200 ... gate rejected it" reintroduces the exact self-contradiction DescribeOutcome was added to eliminate — no test currently checks the fully-rendered message, only DescribeOutcome in isolation. Worth making the middle clause outcome-aware too (or rephrasing it to something outcome-neutral, e.g. "because of the {Gate} gate").

[claude]

Minor: this is documented (here and in the class-level doc above) as bounding "distinct sources," but the dictionary key built at Observe (gate.ToString() + "|" + source) is actually keyed on (gate, source), not source alone. A single address that trips more than one gate over time (fix the Host header, then get refused on the token, then on the CIDR — exactly the iterating-tester scenario this feature is designed around) consumes one slot per gate it trips, not one slot total. With 3 gates and a cap of 16, worst case only ~5 truly distinct addresses get individual treatment before others start folding into the per-gate aggregate, despite the docs implying room for 16 sources. Not a safety issue (the aggregate path still bounds memory/log volume correctly), but the doc/constant name overstates the effective per-address budget.

[claude]

Reviewed. This is C# only (no T-SQL), so the CONTRIBUTING.md style checklist mostly doesn't apply here.

Lite/Darling parity: no drift. Lite's McpHostService is loopback-only and tokenless — it has just the shared HostHeaderGuard 400 gate (untouched by this PR) and no bearer-token or CIDR gate to log about, so there's no Lite counterpart that needs the same treatment. DarlingHttpRefusalLog is correctly Darling-only.

Design: the rate-limit shape (first-sighting-always-logs, per-(gate,source) folding, capped-then-aggregated overflow, last-seen eviction) is sound and the test suite exercises the tricky parts (noisy-neighbour, overflow wording, eviction-vs-multiplier) well.

Security: token value/prefix/length are never logged, confirmed by both the redaction tests and the Report call sites (only header shape is ever described). The Host header is the one attacker-controlled value that gets echoed, and it's sanitized (CRLF stripped, length-capped) before interpolation — good.

Left two inline notes:

1. A real wording bug: Report()'s log format string hardcodes "the {Gate} gate rejected it" regardless of DescribeOutcome, so the web host's 200 (ShowLogin, bad ?token=) case still renders "...HTTP 200... gate rejected it" — reintroducing the self-contradiction DescribeOutcome was specifically added to fix. No test covers the fully-rendered message, only DescribeOutcome in isolation.
2. A minor doc/behavior mismatch: the tracked-sources cap is documented as bounding "distinct sources" but is actually keyed on (gate, source), so a source that trips multiple gates consumes multiple slots — not a safety issue, just an overstated effective budget.

Nothing else stood out as a correctness, security, or performance regression.

[erikdarlingdata]

Both fixed in 3041b73, and both were narrower statements than they sounded.

The sentence. You are right that DescribeOutcome was only half the fix — the middle clause still said the gate "rejected it", so the line contradicted itself two clauses later. The gate is now stated as the reason rather than as a second verdict, which is accurate at every status:

Web dashboard did not authorize a request from 10.0.0.5 (HTTP 200), because of the
access token gate — the presented ?token= does not match web.network.encryptedToken,
so the login page was served instead.

Your observation that nothing tested the full message is the part that mattered — that is exactly how the contradiction survived the first fix. TheRenderedLine_NeverContradictsItsOwnStatusCode now renders through a real ILogger and asserts no clause anywhere in a sub-400 line calls it a rejection or a refusal.

The budget. Also right, and the doc overstated it by ~3x. Renamed to DefaultMaxTrackedKeys and sized at 16 × gate count, so the budget an operator actually cares about — distinct addresses — is the 16 the design intends rather than the ~5 a per-key cap of 16 delivered. SixteenDistinctAddresses_AllGetTheirOwnLine_EvenTrippingEveryGate drives 16 addresses through all three gates and asserts not one falls into the aggregate.

Re-measured the ceiling with the larger cap: 108,000 refused requests over an hour now produce 258 lines and 51 tracked keys, against 100 and 19 before. Still a hard ceiling.