Skip to content

Cover the API layer, close three gates that could not fail, and fix a feed bug that 500s /internal/refresh - #117

Merged
bgard68 merged 23 commits into
mainfrom
claude/lotteryapp-prs-security-quality-frze77
Aug 25, 2026
Merged

Cover the API layer, close three gates that could not fail, and fix a feed bug that 500s /internal/refresh#117
bgard68 merged 23 commits into
mainfrom
claude/lotteryapp-prs-security-quality-frze77

Conversation

@bgard68

@bgard68 bgard68 commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Coverage

The API layer had no tests at allProgram.cs, LotteryEndpoints.cs, DrawRefreshService.cs and DatabaseHealthCheck.cs were exercised only by the post-build smoke test. Coverage was also not measurable: no test project referenced a collector.

Layer Before After
Lottery.Api 0% (no project) 100% (167/167)
Lottery.Application 98.1% 100% (87/87)
Lottery.Infrastructure 63.1% 100% (177/177)
Lottery.Domain 92.6% 98.8% (164/166)

109 tests → 316. The two uncovered Domain lines are DrawSchedule's "unreachable" throws: the loop scans eight consecutive days and both games draw on two or more, so reaching them would mean bending production code to suit a test.

coverlet.runsettings excludes the OpenAPI source generator's output — several thousand generated lines in Lottery.Api that nothing we write can cover, and which alone cost that layer ~40 points.

New tests/Lottery.Api.Tests boots the real host over a throwaway SQLite file (real migrations, real snapshot seed, real routing and middleware); only the live feeds and the timer loop are faked, so runs are offline and need no clock.

Three gates could report success while proving nothing

This is the failure mode gate-probes.yml exists to prevent, and two of its neighbours had it.

  • era-check's live-feed validation had no set -e. With the API unreachable, RESPONSE was empty, jq produced nothing, and [ "" -gt 0 ] returned 2 — making the if false, so the step exited 0. Reproduced: exit 0 before, exit 7 after. This is the only monitor for an undocumented lottery rule change, so it has been silently green whenever it could not see anything.
  • gate-probes' third probe did not guard its dotnet new setup and had no inconclusive check, unlike probes 1 and 2. A failed scaffold fell through to the success echo.
  • The "Start API" wait loops exit 0 whether or not the API came up, because the trailing sleep succeeds.

Workflow permissions and hardening

  • cleanup-runs.yml granted actions: write workflow-wide, so any job added later would inherit the ability to delete runs. Now scoped to the one job that needs it (GHA002).
  • codeql.yml's security-events: write was also flagged by GHA002 — that one is a false positive. Code scanning has no read-only ingest path, so it is the floor for uploading SARIF, and it was already job-scoped with the workflow default read-only. Documented rather than "fixed" into a broken upload.
  • deploy-api.yml's id-token: write moved to the job that calls azure/login; API_BASE_URL now goes through env: instead of being interpolated into a run: line.
  • dependency-review's comment-summary-in-pr needed pull-requests: write and had none, so it silently never posted.
  • cleanup-runs.yml validates its workflow_dispatch inputs before they reach $(( )), which evaluates array subscripts and so can execute a command substitution.
  • Concurrency groups added to the four PR-triggered workflows.
  • .gitleaks.toml no longer allowlists package-lock.json wholesale — npm lock files can carry inline registry credentials in resolved URLs. Only the integrity hashes are excluded now.

A real bug: feed parse failures 500 /internal/refresh

MegaMillionsJackpotFeed's own summary promises "any shape change degrades to null rather than throwing", and both sibling jackpot feeds guard HttpRequestException and JsonException. This one guarded nothingXDocument.Parse throws XmlException, Deserialize throws JsonException.

Neither type matches RefreshGame's catch filter, and CompositeJackpotFeed passes them straight through, so they escape RefreshGame.ExecuteAsync. /internal/refresh — the endpoint the keep-alive workflow calls — has no try/catch, so the request 500s and any game after the failing one is skipped.

The trigger is the one this repo has already been bitten by: a bot-challenge HTML page served with a 200, which is what retired powerball.com's API.

SocrataWinningNumbersFeed had the same escape by a different route. It still refuses a bad batch — a silently short batch would let gap-repair skip real draws — but now as InvalidOperationException with the cause kept, the type the caller already handles.

Dependabot

#116, #111 and #109 each bump a package a peer constraint caps, so they fail before their tests run and are re-proposed weekly. Added semver-major ignores with the condition for lifting each, and commented the PRs. Also added the missing nuget entry for the frontend branch — that branch carries src/Lottery.Api and its CI builds it, but the existing entry has no target-branch, so those packages were never bumped or scanned.

Architecture — reviewed, unchanged

Domain has zero dependencies, Application owns the ports, Infrastructure implements them, only DependencyInjection.cs constructs concrete types, and all eight IDrawRepository methods have real consumers. Clean/Onion/DIP/SRP all hold, so nothing was changed — manufacturing findings would have been box-checking.

Verification

Against a clean worktree of the pushed HEAD: dotnet restore --locked-mode clean, dotnet build -warnaserror with zero warnings, 316/316 tests, all 13 endpoints returning expected status codes, all five security headers present and Server absent. main was merged in after #113 and the new project's lock file regenerated with --force-evaluate.

🤖 Generated with Claude Code

bgard68 and others added 23 commits August 24, 2026 13:01
Dependabot bumps init and analyze in separate PRs (#110, #114), but the two
steps must run the same codeql-action version - a mismatched pair ends the
Analyze job in a configuration error. Same consolidation #105 did for 4.37.7.
Supersedes #110 and #114.
…ests

Coverage was not measurable at all - no collector was referenced by any test
project. Adds coverlet.collector to all four, Microsoft.AspNetCore.Mvc.Testing
for the new API integration tests, and the new project to the solution.
Boots the real API over a throwaway SQLite file so migrations, seeding,
routing and middleware are all exercised for real; only the live feeds and the
timer-driven refresh loop are replaced, which keeps a test run offline.
…efresh loop

66 tests over the four API source files that previously had none.
…rant

GHA002 flagged both as excessive token permissions. cleanup-runs genuinely was
- actions:write applied workflow-wide, so any job added later would inherit the
ability to delete runs; it now sits on the one job that needs it.

CodeQL's security-events:write is the floor rather than a convenience: code
scanning has no read-only ingest path, so analyze cannot upload its SARIF
without it. It was already job-scoped with the workflow default left read-only;
the comment records why so the finding is not re-raised.
CI restores with --locked-mode, so the lock file has to carry the new
reference or the restore fails NU1004.
CI restores with --locked-mode, so the lock files have to carry the new
reference or the restore fails NU1004. Lottery.Api.Tests is a new project,
so this is its initial lock file.
Three checks could report success while proving nothing: era-check's live-feed
validation exited 0 when the API was unreachable (no set -e, and [ "" -gt 0 ]
returns 2, making the if false); gate-probes' third probe did not guard its
setup or check that the failure was the planted one; and the Start API wait
loops exit 0 whether or not the API came up.

Also scopes deploy-api's id-token:write to its job, grants dependency-review
the pull-requests:write its comment-summary-in-pr silently needed, passes
API_BASE_URL through env instead of interpolating it into a run line, adds
concurrency groups to the PR-triggered workflows, and stops gitleaks
allowlisting package-lock.json wholesale (npm lock files can carry inline
registry credentials in resolved URLs; only integrity hashes are excluded now).

Adds coverlet.runsettings so coverage measures our code rather than the
thousands of generated OpenAPI lines nothing we write can cover.
Three checks could report success while proving nothing: era-check's live-feed
validation exited 0 when the API was unreachable (no set -e, and [ "" -gt 0 ]
returns 2, making the if false); gate-probes' third probe did not guard its
setup or check that the failure was the planted one; and the Start API wait
loops exit 0 whether or not the API came up.

Also scopes deploy-api's id-token:write to its job, grants dependency-review
the pull-requests:write its comment-summary-in-pr silently needed, passes
API_BASE_URL through env instead of interpolating it into a run line, adds
concurrency groups to the PR-triggered workflows, and stops gitleaks
allowlisting package-lock.json wholesale (npm lock files can carry inline
registry credentials in resolved URLs; only integrity hashes are excluded now).

Adds coverlet.runsettings so coverage measures our code rather than the
thousands of generated OpenAPI lines nothing we write can cover.
#116, #111 and #109 each bump a package that a peer or framework constraint
caps, so every one fails before its tests run and is re-proposed the next week:

* Microsoft.OpenApi 3.x - AspNetCore.OpenApi 10.0.x requires >= 2.7.5 && < 3.0.0,
  so the restore trips NU1608, which Directory.Build.props promotes to an error.
  This is the constraint #107 pinned 2.x for.
* typescript 7.x - @angular/build 22.x has peer typescript >= 6.0 < 6.1.
* jasmine-core 7.x - karma-jasmine-html-reporter 2.2 peers ^4 || ^5 || ^6.

Each ignore is scoped to semver-major and carries the condition for lifting it,
so minor and patch updates - security ones included - keep flowing.

Also adds the missing nuget entry for the frontend branch: that branch carries
src/Lottery.Api and its CI builds it, but the existing nuget entry has no
target-branch, so those packages were never bumped or scanned.
The README still said "109 tests across three projects". Adds the API layer's
66 tests and a note on coverlet.runsettings, which excludes the OpenAPI
generator's output so the coverage figure measures our own code.
MegaMillionsJackpotFeed's own summary promises "any shape change degrades to
null rather than throwing", and both sibling jackpot feeds guard
HttpRequestException and JsonException. This one guarded nothing:
XDocument.Parse throws XmlException on a non-XML body and Deserialize throws
JsonException on a truncated one.

Neither type is matched by RefreshGame's catch filter, and CompositeJackpotFeed
passes them straight through, so they escape RefreshGame.ExecuteAsync.
/internal/refresh - the endpoint the keep-alive workflow calls - has no
try/catch, so the request 500s and any game after the failing one is skipped.

The trigger is the one this repo has already been bitten by: a bot-challenge
HTML page served with a 200, which is what retired powerball.com's API.

SocrataWinningNumbersFeed had the same escape by a different route. It still
refuses the whole batch - a silently short batch would let gap-repair skip real
draws - but now as InvalidOperationException with the cause kept, which is the
type the caller already handles.

The ten tests that pinned the previous behaviour now assert the corrected
contract. 316 tests pass; coverage unchanged.
The previous commit rewrote these DTOs while adding the exception guard. That
was not part of the fix and it was wrong: the JSON property is Winners, not
JackpotWinners, so deserialization would have silently produced a null winner
count and lost the rollover flag. Restores the file to the tested version -
the guard, and nothing else, changed.
A malformed payload, or a row whose winning_numbers is short or non-numeric,
escaped as JsonException / FormatException / ArgumentOutOfRangeException. None
of those match RefreshGame's catch filter, so one bad row 500s
/internal/refresh instead of being reported.

The batch is still refused - delivering a silently short batch would let
gap-repair skip real draws - but as InvalidOperationException with the cause
kept, which is the type the caller already handles.
These ten tests previously pinned the pre-fix behaviour, deliberately written
so a fix would fail them loudly rather than pass silently. MegaMillions now
degrades to null on a non-XML body, a truncated payload and a 5xx, matching its
class comment and both sibling feeds; Socrata still refuses a batch containing
a bad row but reports it as InvalidOperationException with the cause kept.
#113 bumped Microsoft.OpenApi 2.12.0 -> 2.12.2 and regenerated the lock files
that existed at the time. Lottery.Api.Tests is new on this branch, so its lock
file still pinned 2.12.0 and locked-mode restore failed NU1004.

Regenerated with dotnet restore --force-evaluate; no other lock file changed.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AWkVh7cyAz1gWapBH1CY8n
@bgard68
bgard68 merged commit 6fc02ca into main Aug 25, 2026
7 checks passed
@bgard68
bgard68 deleted the claude/lotteryapp-prs-security-quality-frze77 branch August 25, 2026 03:01
bgard68 added a commit that referenced this pull request Aug 25, 2026
…main (#128)

This branch carries src/Lottery.Api and its CI builds and tests it, but until
the dependabot.yml entry added in #117 it was never scanned - so its packages
drifted behind main's:

  dbup-sqlserver              6.0.16  -> 7.2.0
  Microsoft.NET.Test.Sdk      17.14.1 -> 18.9.0
  xunit.runner.visualstudio   3.1.5   -> 4.0.0

Dependabot opened #122, #123 and #124 for these and all three failed the same
way: each package is CentralTransitive, so it updated Directory.Packages.props
without regenerating packages.lock.json, and CI restores with --locked-mode,
which fails NU1004. It then closed them itself as 'no longer updatable'.

Doing them together with the lock files regenerated (--force-evaluate) is what
main needed in #85 for the same dbup-sqlserver bump.

Microsoft.Data.SqlClient is deliberately left at 6.1.6: #123 proposed 7.0.2,
a major, and main is still on 6.1.6 - matching main is the target here, not
getting ahead of it.


Claude-Session: https://claude.ai/code/session_01AWkVh7cyAz1gWapBH1CY8n

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants