fix(security): scope the working-hours export and grid to the caller - #1722
Merged
Merged
Conversation
Both the single-worker Excel export and the working-hours grid took a site id straight from the request and never checked whether the caller was entitled to it. Any signed-in user could read a colleague's hours by guessing a site id: the grid returned 200 with their rows, and the export handed back their workbook. A test pins both. Both now resolve the caller through SiteScopeResolver, the helper the all-workers export already uses, and refuse anything outside that scope: an admin reaches every site, a manager their own plus the sites in their managed tags, a worker only their own. Refusals are deliberately indistinguishable. An unknown site id is in nobody's allow-set, an admin's included, so it answers exactly as an out-of-scope one does — same message, same status, and the narrowing happens in memory so the work does not vary with the id either. Without that, the two answers differed and could be used to enumerate which site ids exist. Index is now a gate over a private IndexUnscoped. The all-workers export calls the unscoped body directly because it has already narrowed its site ids, and gating the shared body would re-resolve the caller's scope once per site in the workbook. Two admin-visible changes, both on requests that produce no data today: a bogus site id now answers "Worker not found." instead of the crash-path message, as does a site that has plan registrations but no live assignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR scopes the working-hours grid and single-worker Excel export to the caller’s permitted sites, matching existing planning-board authorization rules.
Changes:
- Added caller-scope checks for both endpoints.
- Added an unscoped internal path for already-authorized export calls.
- Updated fixtures and added coverage for admins, managers, workers, and indistinguishable refusals.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelShiftColumnOrderTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelHolidayColumnTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportTagsColumnTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportE2ETests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursDisplayParityTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/MobileFlexRecomputeAndCascadeTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ExportTagFilterAndSiteTagsTests.cs | Updated as part of this pull request. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs | Updated as part of this pull request. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return scope.ErrorKey; | ||
| } | ||
|
|
||
| return scope.Narrow([siteId]).Count == 0 ? "SiteNotFound" : null; |
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.
Follow-up to #1721, which scoped the all-workers export. This closes the same hole on the two routes that were left.
Problem
The single-worker Excel export (
GET working-hours/reports/file) and the working-hours grid (POST working-hours/index) both tookSiteIdstraight from the request and never checked whether the caller was entitled to it. Any signed-in user could read a colleague's hours by guessing or enumerating a site id — the grid answered 200 with their rows, the export handed back their workbook. Both endpoints are[Authorize]only.The new tests fail against
stablefor exactly that reason: with no gate, the out-of-scope call simply succeeds.Fix
Both routes resolve the caller through
SiteScopeResolver— the helper #1721 introduced — and refuse anything outside it:Refusals are deliberately indistinguishable. An unknown site id is in nobody's allow-set (an admin's included), so it answers exactly as an out-of-scope one: same
SiteNotFoundmessage, same 400. The narrowing runs in memory over a list the query does not vary by id, so there is no timing signal either. Before this, an unknown id fell into a catch and produced a different message from an out-of-scope one — enough to enumerate which site ids are real. Two tests assert the two cases answer identically.Indexis now a thin gate over a privateIndexUnscoped. The all-workers export calls the unscoped body directly, because it has already narrowed its site ids; gating the shared body would re-resolve the caller's scope once per site in the workbook.Behaviour changes
Admin-visible, both on requests that produce no data today:
Every request that produces a workbook or a grid today is unaffected — the gate demands a non-removed
AssignedSitefor the requested id, which the code already required a few lines later.Non-admins lose access to sites outside their scope. That is the fix.
Tests
Ten cases in
ExportTagFilterAndSiteTagsTests(already in both shard allowlists), mirroring each other across the two routes: admin any site; manager in-tags allowed and out-of-tags refused; plain worker own allowed and another refused; and per route, unknown and out-of-scope answering identically.Both refusal tests first read or export the target site as an admin and assert success, so a refusal cannot pass because the site was never readable in the fixture.
Three existing fixtures gained a real admin caller because they passed
baseDbContext: null!; no assertion in any of them changed.MobileFlexRecomputeAndCascadeTestsneeded no change and now documents why: its non-admin user resolves through the plain-worker branch and asks for the very site it is linked to.Verified locally:
dotnet build. Tests run in CI.Follow-ups, not in this PR
Indexnow callsGetCore()and opens an SDK context twice per request, plus a fullAssignedSitesload, on the hottest endpoint. Threading the open context and resolved scope intoIndexUnscopedwould halve that.Read,ReadSimple,ReadFullByCurrentUser,CalculateHoursSummary,UpdateWorkingHour) is[AllowAnonymous]and takes a caller-suppliedsdkSiteIdalongside a token. Whether those validate the token against the requested site is unverified — if they don't, it is this same class of hole on an anonymous surface. Worth its own investigation.🤖 Generated with Claude Code