Close what a leak-and-security sweep found - #32
Merged
Conversation
ivanvyd
force-pushed
the
fix/leaks-and-encoding
branch
from
July 29, 2026 19:20
fdd79cc to
e2150e8
Compare
Six defects, none of them large, all of them reachable. The dashboard's own page is the one place in the library that builds HTML as a string instead of letting Razor build it, and its title went in unencoded. The title is a host's setting rather than a visitor's, so this only bites when a host builds it from something it did not write itself -- a per-tenant label, a value out of a database -- but that is an ordinary thing to do and nothing stopped such a value closing the title element and opening a script one. The page is now built by a method that can be asserted on without a host, which is what makes the test a test rather than a restatement of the encoder. A dashboard component left its handler behind when it was disposed. The service is scoped to a circuit, so it was assumed that letting the circuit end released everything, and that holds exactly while the dashboard is mounted once per circuit. A host that routes to it inside its own layout builds a new component every time the user navigates back to it, on the same circuit, and each of those left a handler that went on being called after its component was torn down. UnsubscribeFromStateChangesAsync is the missing half. Two callers scheduling one checker at once could leave a timer that nothing could stop. Installing a schedule was "cancel the old one, then start the new one" -- two steps, so both callers could install one, and only the last stored was reachable. The other kept triggering, could not be unscheduled because nothing pointed at it any more, and held a linked CancellationTokenSource that was never disposed. A lock is the whole fix; scheduling happens at startup and when somebody changes an interval, so it is nowhere near a hot path. MaxHistoryPageSize was documented in the README and read by nobody -- get_check_history clamped to a hard-coded 200 instead, so a host that lowered it to keep responses small got no such thing. It is taken as an optional constructor parameter so that reading it does not change how the type is built. The relational table-name guard admitted a name with a trailing newline, because in .NET `$` matches immediately before one as well as at the end of the string. Nothing could be smuggled through it -- a lone trailing newline is whitespace to every engine here, and a second one fails -- but a guard whose own error message promises "letters, digits and underscores" should not accept one, so it ends at `\z` now. A checker name from a REST route reached the log with its control characters intact. The not-found branch logs that name precisely when it matches nothing, percent-encoded CR and LF arrive decoded, and a sink writing plain text writes them as line breaks -- which is a caller forging log entries. Only the logging call sites are sanitised; the lookup and the response body still see the name as it arrived. Two things the sweep raised are deliberately not changed here. Healthie.Api requires no authorization unless the host asks for it, and the dashboard's AllowMutations defaults to true: both are documented decisions with real reasons, and changing either is a behaviour break and the maintainer's call, not a cleanup. A checker's exception message is likewise still reported verbatim -- that message is the product.
The release workflow set contents: write at the top level, so every job in it inherited the ability to push to the repository. There is one job today and it does need write -- it creates the GitHub release at the end -- so this changes nothing about what runs. It changes what a job added later starts with: read, and it has to ask. OpenSSF Scorecard rates a workflow-wide contents: write a high finding for that reason, and it is one of the alerts this repository's own scorecard run reports.
NewGroupOption is a sentinel that a real group name cannot collide with, and it was written by putting an actual NUL byte in the source rather than the escape for one. A single NUL is all it takes for git to classify a file as binary, so every diff of the dashboard's largest code-behind has been showing as "Bin 27758 -> 28046 bytes" instead of the lines that changed -- including in the pull request this commit belongs to, where a reviewer could not see the unsubscribe being added. "\u0000new" is the same string to the compiler. Nothing about the sentinel or the comparisons against it changes; the file is simply diffable now.
Review found four things, two of which reintroduced the leaks this branch set out to close. Dispose was a third, unguarded writer of the timer table. Taking the lock in ScheduleAsync and UnscheduleAsync and not in Dispose leaves the same race by another door: clearing the dictionary between an in-flight schedule's stop and its install drops a live timer without cancelling it. Dispose takes the lock now, with a bounded wait so a stuck caller cannot hang shutdown. It also no longer disposes the semaphore -- SemaphoreSlim needs that only once its AvailableWaitHandle has been asked for, which nothing here does, and disposing it under a live waiter leaves that waiter blocked for ever instead of throwing. A schedule that lands after disposal is now a no-op rather than installing a timer nothing is left to stop. The component unsubscribed as the first, unguarded line of DisposeAsync. The circuit is allowed to dispose the scoped service first, and then that call throws on a disposed lock and every line below it is skipped -- the clock loop and its token source among them, which is exactly the leak class this branch is about. Two XML doc blocks were pasted after the previous member's documentation instead of before their own member. SubscribeToStateChangesAsync and the public SetCheckerInterval action both lost their docs entirely, and the new members inherited malformed ones. The build said so in twenty-eight warnings I did not read, because I grepped its output for "error". The solution now builds with none of any kind. Also: NotifyAsync snapshotted the handler list without the lock the two mutators take. Pre-existing, but this branch adds the second mutator.
ivanvyd
force-pushed
the
fix/leaks-and-encoding
branch
from
July 29, 2026 21:19
e2150e8 to
e554e02
Compare
This was referenced Jul 29, 2026
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.
Three read-only audit lenses swept the whole of
src/for memory leaks, injection and data exposure, and authorization and DoS. Every finding below was re-verified atfile:linebefore anything was changed; several the audits raised are deliberately not changed, and why is at the bottom.Fixed
The dashboard's own page did not encode its title.
MapHealthieUIbuilds that one page as an interpolated string rather than through Razor, which encodes every interpolation for you — it is the single place in the library that hand-builds HTML.HealthieUIOptions.DashboardTitleis a host's setting, not a visitor's, so this only bites when a host builds it from something it did not write itself (a per-tenant label, a value out of a database). Nothing stopped such a value closing the<title>element and opening a<script>one.A dashboard component left its handler behind when it was disposed. The service is scoped to a circuit, so the code assumed the circuit ending released everything. That holds exactly while the dashboard is mounted once per circuit — and a host that routes to
HealthieDashboardinside its own layout builds a new component every time the user navigates back to it, on the same circuit. Each one left a handler that went on being called after its component was torn down, rooting the component and everything it held.IHealthieDashboardService.UnsubscribeFromStateChangesAsyncis the missing half; the component calls it first thing inDisposeAsync.Two callers scheduling one checker at once could leave a timer nothing could stop. Installing a schedule was "cancel the old one, then start the new one" — two steps, so both callers could install one and only the last stored was reachable. The other kept triggering the checker, could not be unscheduled because nothing pointed at it any more, and held a linked
CancellationTokenSourcethat was never disposed (which leaks a registration on the parent token). A lock is the whole fix; scheduling happens at startup and when somebody changes an interval, never on a hot path.HealthieMcpOptions.MaxHistoryPageSizewas documented in the README and read by nobody.get_check_historyclamped to a hard-coded 200, so a host that lowered the option to keep responses small got no such thing. Taken as an optional constructor parameter, so reading it does not change howHealthieToolsis constructed.The relational table-name guard admitted a name with a trailing newline. In .NET
$matches immediately before one as well as at end-of-string, so"state\n"passed a check whose own exception message promises "letters, digits and underscores". Verified by running the regex:Not exploitable — a lone trailing newline is whitespace to all three engines and a second one fails, so no statement can be smuggled through. Fixed anyway, because a guard that admits what it documents as impossible is a bad thing to build on later.
A checker name from a REST route reached the log with its control characters intact. The not-found branch logs that name precisely when it matches no checker, percent-encoded CR/LF arrive here already decoded, and a sink writing plain text writes them as line breaks — an unauthenticated caller forging log entries. Only the 12 logging call sites are sanitised; the lookup and the response body still see the name exactly as it arrived.
Deliberately not changed
Healthie.Apirequires no authorization unless the host asks for it, andHealthieUIOptions.AllowMutationsdefaults totrue. Both were confirmed exactly as the audit described — six mutating endpoints reachable with no credentials if a host maps the controller and does nothing else. Both are documented decisions with stated reasons, and flipping either is a behaviour break for every existing consumer. That is the maintainer's call, not a cleanup to slip into a fix PR.A checker's exception message is still reported verbatim through the REST, MCP and dashboard read surfaces. That message is the product — it is how a human finds out why a check failed. Worth knowing that a custom checker letting a driver exception through publishes whatever that driver's
Messagecontains, but redacting it would break the feature.Verification
389 tests green on both net8.0 and net10.0.
Mutation-tested — a passing test proves nothing until it has been seen to fail:
$againThe scheduler race gets a real regression test rather than a mutation: 5/5 runs fail against the unfixed scheduler, 8/8 pass against the fixed one. Its first version was vacuous and I caught it the same way — nothing in the unfixed
ScheduleAsyncyields, soTask.WhenAllover 32 calls ran them strictly sequentially and no two ever overlapped. It needs aBarrierand real threads to race at all.Healthie.Apihad no test project reference at all, so it had no unit coverage; the reference is added here.Not covered
The component's
DisposeAsynccalling unsubscribe is not itself tested — that needs a Blazor component harness the repo does not have (the E2E suite drives a browser, not components). The service-level unsubscribe it calls is tested and mutation-checked.No release — version stays at 3.1.4.