security: AI agent hardening — rate limiter keys, trusted proxy config, captcha cleanup#1102
Merged
Merged
Conversation
…isleading 429 captcha signal F3: Replace Identity.Name (mutable display name) with ClaimTypes.NameIdentifier (stable user ID) as the ChatEndpoint rate limiter partition key, matching the partition strategy used by the global and MCP rate limiters. F4: Replace unconditional KnownProxies/KnownIPNetworks.Clear() with a config-driven trusted-proxy allowlist (ForwardedHeaders:TrustedProxyCidrs). When no CIDRs are configured the behaviour is unchanged (all forwarded headers trusted), but production deployments can now lock this down to their Azure Container Apps ingress CIDR to prevent X-Forwarded-For spoofing and IP-based rate-limit bypass. F7: Remove requiresCaptcha=true from the 429 rate-limit response body. The hCaptcha infrastructure exists (ICaptchaService, CaptchaOptions) but chat requests do not yet validate the CaptchaResponse token server-side, so advertising the field as a gate control was misleading. Updated the CaptchaResponse comment to clarify its pending integration status.
…nown-user keys - Replace raw config-length check with IOptions<ForwardedHeadersOptions> post-Build DI resolution so warning fires for both 'not configured' and 'all CIDRs failed to parse' - Normalize ContentRateLimiterPolicy fallback to 'unknown-user' for consistency
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
EnsureSitemapHealthy only throws InvalidOperationException; using a generic catch was overly broad. Addresses GitHub code quality feedback.
Member
Author
|
Fixed in 653055f — narrowed |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens AI chat and MCP rate-limiting behavior by moving rate-limit partitions toward stable user identifiers, cleaning up unused captcha signals, and adding trusted proxy CIDR configuration for forwarded headers.
Changes:
- Swaps mutable identity-name rate-limit keys for
ClaimTypes.NameIdentifier. - Adds
ForwardedHeaders:TrustedProxyCidrsconfiguration and startup warnings. - Removes unused/misleading chat captcha request and 429 response fields.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
EssentialCSharp.Web/Program.cs |
Updates forwarded-header configuration, rate-limit partition keys, 429 response body, and startup logging. |
EssentialCSharp.Web/Services/ContentRateLimiterPolicy.cs |
Uses stable user ID fallback for authenticated content rate-limit partitions. |
EssentialCSharp.Web/Services/McpRateLimiterPolicy.cs |
Removes mutable identity-name fallback from MCP authenticated partitions. |
EssentialCSharp.Web/Controllers/ChatMessageRequest.cs |
Removes unused CaptchaResponse field from chat requests. |
EssentialCSharp.Web/appsettings.json |
Adds ForwardedHeaders:TrustedProxyCidrs configuration skeleton. |
Comments suppressed due to low confidence (3)
EssentialCSharp.Web/Program.cs:132
- This warning bypasses the repository's established source-generated logging pattern and configured logging providers (for example, Program.cs:624-640 and Services/McpRateLimiterPolicy.cs:70-71 use [LoggerMessage]). Because malformed CIDRs are security-relevant startup configuration, logging them via
Console.Errormay keep them out of Application Insights/OpenTelemetry or structured log processing.
else
Console.Error.WriteLine($"[WARN] ForwardedHeaders:TrustedProxyCidrs: could not parse '{cidr}' as an IP network — entry skipped. Check your configuration.");
EssentialCSharp.Web/Program.cs:381
- The chat 429 response contract is being changed here, but there are no ChatController/chat rate-limiting tests in EssentialCSharp.Web.Tests, while similar rate-limiter behavior is tested for content and MCP endpoints. Add an integration test that exercises
/api/chatrate limiting and asserts the response shape, including thatrequiresCaptchais absent andretryAfteris still emitted when available.
Dictionary<string, object> errorResponse = new()
{
["error"] = "Rate limit exceeded. Please wait before sending another message.",
["statusCode"] = 429
};
EssentialCSharp.Web/Program.cs:127
- This security-critical forwarded-header parsing path is not covered by tests. Existing integration tests exercise rate limiting, but none verify that a configured trusted CIDR accepts
X-Forwarded-For, that untrusted sources are ignored, or that invalid/empty configuration does not silently re-enable spoofing; adding those cases would prevent regressions in the hardening this PR is targeting.
var trustedCidrs = builder.Configuration
.GetSection("ForwardedHeaders:TrustedProxyCidrs")
.Get<string[]>() ?? [];
foreach (var cidr in trustedCidrs)
Replaces the dead CaptchaResponse comment with a working implementation:
Server-side (ChatController):
- Inject ICaptchaService + IOptions<CaptchaOptions>
- Validate hCaptcha token before processing each message
- Graceful degradation: skipped when SecretKey is not configured (dev)
- Fail-open on hCaptcha outages to avoid blocking legitimate users
- Returns 403 + { errorCode: 'captcha_failed' } on invalid tokens
Client-side (chat-module.js / ChatWidget.vue):
- Invisible hCaptcha widget rendered outside v-if dialog so it persists
across open/close cycles and only initializes once
- getCaptchaToken() wraps execute() in a Promise; resolves instantly for
non-suspicious users (invisible mode)
- Token included as captchaResponse in every request body
- Widget reset after each send (success or failure) for a fresh token
- 403 responses mapped to captcha-error type in the UI
- hCaptcha legal disclosure shown in footer when captcha is configured
Layout:
- Expose window.HCAPTCHA_SITE_KEY from CaptchaOptions for the JS layer
- Reject null/empty token server-side before making outbound hCaptcha call - Add LogCaptchaValidationFailed [LoggerMessage] for observability on rejections - Document fail-open policy with explicit rationale comment - JS: add captchaPending flag to block concurrent getCaptchaToken() calls - JS: wrap getCaptchaToken() in 15-second timeout race to prevent UI freeze - JS: clearTimeout(timeoutId) in resolve/reject wrappers to prevent stale timer from corrupting state of subsequent calls
getCaptchaToken() was returning null immediately when captchaWidgetId === null (hCaptcha script still loading), causing the server to reject with 403 for any user who submitted a message before the widget finished rendering. Fix: introduce captchaWidgetReady Promise that resolves once hcaptcha.render() completes. getCaptchaToken() now chains onto it so early sends wait (up to the 15s timeout) rather than sending a null token.
- Fix misleading log message: 'failing open' -> 'failing closed (503)' in LogCaptchaServiceUnavailable (hCaptcha unavailable returns 503, not open) - Remove dead ArgumentException/FormatException catch blocks from sitemap validation; EnsureSitemapHealthy only throws InvalidOperationException - Remove duplicate top-level ForwardedHeaders section from appsettings.json (two keys at the same path; keep the one with both TrustedProxyCidrs and TrustedProxies) - Fix captcha challenge timeout: start 90s timer after widget is ready, not before — previously script-load time ate into the challenge window; also increases from 15s to 90s to accommodate interactive challenges
hCaptcha already fires error-callback and expired-callback in all failure cases, so the manual setTimeout was redundant complexity. Removing it simplifies getCaptchaToken() and avoids the arbitrary timeout value debate.
- Add bounded hCaptcha widget-ready wait (15s) to avoid indefinite pending chat requests when the third-party script is blocked or never loads - Pass the request cancellation token into stream endpoint captcha error writes to stop writing to aborted responses on client disconnect - Emit chat hCaptcha site key to the client only when both SecretKey and SiteKey are configured, keeping client/server captcha enablement aligned
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds invisible hCaptcha support to the chat widget and strengthens security around trusted proxy handling and rate limiting. The most important changes include server- and client-side hCaptcha integration, improved trusted proxy CIDR configuration for forwarded headers, and several security and code quality improvements.
hCaptcha Integration:
ChatController, including a newIsCaptchaValidAsyncmethod, dependency injection ofICaptchaServiceand options, and logging for captcha failures and service outages. The endpoints now require a valid hCaptcha token when configured, failing open only if the hCaptcha service is unavailable. [1] [2] [3] [4] [5]ChatMessageRequestto require and document theCaptchaResponsefield, with validation and a larger maximum length._Layout.cshtml, and rendered the invisible widget container and legal disclosure in the chat UI only when captcha is enabled. [1] [2] [3] [4] [5]chat-module.js, including widget initialization, token retrieval with timeout and error handling, and reset logic.Trusted Proxy and Rate Limiting Security:
Program.cs, including parsing from configuration, warnings for missing/invalid configuration, and detailed comments about the security implications of not setting this value. [1] [2] [3] [4]ClaimTypes.NameIdentifierfor user partition keys, improving stability and reducing risk of key conflation. [1] [2] [3] [4]Other Improvements:
requiresCaptchafield from the rate limit error response.InvalidOperationException, making error handling more precise.These changes collectively add robust human verification to the chat feature, improve rate limiting and IP spoofing protections, and enhance code maintainability.