Operability: readiness probe, correlated failures, a loud proxy check, and real pagination - #89
Merged
Conversation
…heck Three operability gaps. The application was correct and effectively mute: it could not report that it was unwell, and a failure left nothing to trace. The health check could never report unhealthy. /health closed over a variable captured at startup - whether migrations succeeded when the process began, and nothing after. Once an instance was up it answered ok forever: database gone, pool exhausted, credentials rotated, still ok. The scheduled keep-warm ping made that worse rather than better, holding an instance in rotation on the strength of an answer that could not change. The obvious fix - query the database in /health - would have been wrong here, and the reason is written down in keep-warm.yml: that endpoint is pinged every few minutes to hold a free-tier instance loaded, and waking a serverless database on that cadence costs roughly 180 CU-hrs against a 100 CU-hr monthly budget. The shallow probe is deliberate. So this adds an endpoint rather than changing one. /health keeps its exact contract, including the 503 on a failed migration that the provisioning script watches for on first boot. /health/ready opens a connection and runs select 1, and is what a platform probe and alerting should use. Its failure body names the exception type and never its message, because a connection error can carry a host or a user and the endpoint is anonymous. Failures had nothing to trace them by. No exception handler meant a bare 500 with an empty body - safe, since the developer exception page is Development only, but unsupportable: nothing connected what the customer saw to what the logs recorded. Every response now carries X-Correlation-Id, a 500 repeats it in the body, and the same id is on the log line. An id supplied upstream is kept so a trace survives across services, but sanitised first: it reaches log messages, and text carrying newlines could forge whole entries (CWE-117) - the same class of defect already fixed once here, on the order-status path. The proxy setting was load-bearing and silent. Throttling partitions by caller. Behind a proxy with TrustForwardedFor left false, every caller collapses into one partition and the limits apply to all traffic combined - an outage caused by a configuration value, with nothing in the logs to say so. Trusting the header with no proxy in front is the inverse mistake and the security one, since a caller can then forge it and mint unlimited partitions. A check now watches real traffic and warns once for whichever it sees, using Interlocked so a burst produces one line rather than a page of identical ones. Eleven tests: both probes and their separation, correlation ids echoed and preserved across a hop, a forged newline stripped, an over-long id truncated, and both directions of the proxy mistake warned exactly once. 495 backend tests pass against PostgreSQL 16; dotnet format clean; build clean under -warnaserror. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
The last piece of the browser-side narrowing removed earlier. Filtering and ordering already moved into the query, but the grid still showed whatever came back from a single request, so PAGE_SIZE was a ceiling on the whole catalogue rather than a page size. It had been raised to 100 to keep 75 products visible - a number that works until someone adds the hundred-and-first, at which point products stop appearing with no error anywhere. Silent is the part that made it worth fixing rather than tuning. The grid now pages. The page is a URL parameter like every other part of the query, so a shelf can be linked, bookmarked and reached with the back button, and the pager only renders when there is more than one page - an ordinary shelf gains no furniture it does not need. Changing the query resets to the first page. Keeping the old page number across a filter change strands the reader on page 4 of a result that now has two, and the grid comes back empty for no visible reason. PAGE_SIZE drops from 100 to 24, which is a screenful rather than "as much as the API will allow". The API's own cap is untouched and no longer load-bearing. The count line reads honestly across pages: "24 products of 75, page 1 of 4" rather than a number that quietly means "on this page". Five tests: no pager when everything fits, stepping forward, stepping back from further in, the last page refusing to advance, and a query change dropping the page. That third one exists because the Previous handler is unreachable from page one, and without it the frontend coverage gate failed at 99.47% of functions - the floor caught the gap before CI did. Verified in the running app against a paging API: 24 of 75 shown, page 1 of 4, Next advancing and the URL following. 239 frontend tests and 506 backend tests pass; frontend coverage back to 100% lines and functions. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged the exception handler this pull request added: it logged Request.Path.Value straight from the request. That is the decoded path, so a URL containing %0A arrives as a real newline, ends the log entry, and begins one the caller wrote - the caller choosing what the log appears to say about them (CWE-117). Caught in review by the scanner, and the miss is worth naming: the correlation id in the same handler was sanitised for exactly this reason, with a comment citing exactly this weakness, while the value beside it went through raw. Knowing the rule is not the same as applying it everywhere it holds. LogSafe.Text strips control characters and truncates. Printable oddities are kept deliberately - a path full of strange characters is what an operator needs to see - and only the characters that can restructure the log itself are removed. A value that sanitises away to nothing becomes a marker rather than a blank field, so the log never quietly loses a column. Applied to the path and the method. The correlation id was already clean by construction. Six tests: ordinary text survives, newlines and control characters go, printable oddities are kept, a long value truncates, and nothing usable becomes a marker. 512 backend tests pass; dotnet format clean. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
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
3 participants
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.
Closes every remaining item across the Weaknesses, Opportunities and Threats quadrants. The application was correct but effectively mute — it could not report being unwell, and a failure left nothing to trace.
Readiness probe — and why the obvious fix was wrong
/healthclosed over a variable captured at startup. Once an instance was up it answeredokforever: database gone, pool exhausted, credentials rotated, stillok. The keep-warm ping made it worse, holding an instance in rotation on the strength of an answer that could not change.The natural fix — query the database in
/health— would have been a mistake, and the reason is written down inkeep-warm.yml: that endpoint is pinged every few minutes to keep a free-tier instance loaded, and waking a serverless database on that cadence costs ~180 CU-hrs against a 100 CU-hr monthly budget. The shallow probe is deliberate.So this adds an endpoint rather than changing one:
/health— unchanged contract, including the 503 on failed migration thatProvision.ps1watches on first boot. Still what keep-warm pings./health/ready— opens a connection, runsselect 1. For platform probes and alerting. Its failure body names the exception type, never its message — a connection error can carry a host or a user, and this endpoint is anonymous.Correlated failures
No exception handler meant a bare 500 with an empty body — safe (the developer page is Development-only) but unsupportable. Every response now carries
X-Correlation-Id, a 500 repeats it in the body, and the same id is on the log line.An id supplied upstream is kept so a trace survives across services — but sanitised first. It reaches log messages, and text carrying newlines could forge whole entries (CWE-117), the same class of defect already fixed once here on the order-status path.
The proxy threat, made loud
TrustForwardedForis load-bearing and silent. Left false behind a proxy, every caller collapses into one partition and the limits apply to all traffic combined — an outage caused by a config value. Trusting it with no proxy in front is the inverse and the security half: a caller can forge the header and mint unlimited partitions.A check now watches real traffic and warns once for whichever mistake it sees, using
Interlockedso a burst produces one line rather than a page of identical ones.Real pagination
PAGE_SIZEhad been raised to 100 to keep 75 products visible — fine until someone adds the 101st, at which point products stop appearing with no error anywhere.The grid pages now. Page is a URL parameter, so shelves are linkable and the back button works. The pager renders only when there's more than one page. Changing the query resets to page one — keeping it strands the reader on page 4 of a two-page result.
PAGE_SIZEdrops to 24: a screenful, not "as much as the API allows".Verified in the running app: 24 of 75, page 1 of 4, Next advancing and the URL following.
Tests
Sixteen new. Notably, the "step back a page" test exists because the Previous handler is unreachable from page one — without it the coverage gate failed at 99.47% of functions. The floor caught the gap before CI did.
-warnaserror;dotnet formatcleanGenerated by Claude Code