#61 — Bug: `GetCurrentUserIdQuery` missing from the internal-only endpoint exclusion list #61

New issue

Closed

opened 2026-08-18 13:13:23 +02:00 by lena · 0 comments

lena commented

2026-08-18 13:13:23 +02:00

(Migrated from git.butzei.de)

Feature `#61` — Bug: `GetCurrentUserIdQuery` missing from the internal-only endpoint exclusion list

Requested by: Backend Engineer, spotted during #57's audit of internal-plumbing request types,
2026-07-21
Priority: Could
Blocked by: none

Problem

EndpointRouteBuilderExtensions.MapRequests auto-maps every public IRequest type to a POST
endpoint unless explicitly excluded. Five internal-only session/plumbing commands are correctly
excluded: SetCurrentUserIdCommand, DestroyCurrentSessionCommand,
RevokeAllSessionsForCurrentUserCommand, GetSessionsRevokedBeforeUtcQuery,
SendEmailVerificationCommand. GetCurrentUserIdQuery — a sibling in the exact same
CqsTodo.Features.User namespace, also public, also only ever dispatched as a constructor-injected
dependency from inside other handlers (confirmed during #57's audit: no direct caller anywhere
outside handler bodies) — is the one that was never added, and is currently live at
POST /api/GetCurrentUserIdQuery.

Severity is low: the handler only returns the caller's own session-derived UserId? (null if
unauthenticated), so there's no cross-user data exposure or IDOR — unlike the #59 review's
GetSessionsRevokedBeforeUtcQuery finding, which did leak other users' data. Still the same category
of bug (an internal-only type missing from the exclusion list becomes an unintended live endpoint with
no compile error or test failure to flag it), so it's worth closing for consistency and because the
next thing added near it could be worse.

Scope

Add GetCurrentUserIdQuery to the exclusion list in
CqsTodo.WebApi/EndpointRouteBuilderExtensions.cs.
Extend MapRequestsTests.MapRequests_excludes_internal_only_session_types_from_real_assembly
(added in #59's review) to also assert this type produces no route.

Out of scope

Any broader redesign of the exclusion-list mechanism itself (e.g. a marker interface/attribute
instead of a hardcoded list) — already logged as accepted debt from the #59 review
(docs/SECURITY_NOTES.md), not re-litigated here.

Agents involved

Backend Engineer — one-line fix + test extension
Security Agent — confirm no other public, non-excluded internal-only types exist (re-run the
same audit #57/#59 did)

Resolution — Closed, not a bug (2026-07-21)

This story's premise was factually wrong. The fix was implemented, tested (new regression test
green, full solution green), and committed — then /code-review's cross-file tracer angle checked
the frontend for callers and found one: ReactUi/src/App.tsx:23 calls
callApi('GetCurrentUserIdQuery', {}) over real HTTP inside a useEffect on every app mount, to
determine login status. It's also a live path in the checked-in generated OpenAPI schema
(ReactUi/src/api/generated/openapi-schema.d.ts).

The "confirmed during #57's audit: no direct caller anywhere outside handler bodies" claim above
only checked C# call sites (constructor-injected handler dependencies), not the frontend's HTTP
callers — the exact blind spot that made the five sibling types (all actually internal) look like
a pattern this type also fit. It doesn't: GetCurrentUserIdQuery is the load-bearing "am I logged
in" check for the entire app, not internal plumbing, despite being marked IInternalRequest (that
marker was added in #57 for a different purpose — scoping metrics — and was never meant to imply
"has no external caller"; conflating the two is the root confusion here).

Impact avoided: the fix was reverted before push. Had it shipped, POST /api/GetCurrentUserIdQuery would have 404'd, and every user's login-status check on every page
load would have failed — likely presenting as an app-wide stuck "Loading…" screen. Neither test
suite would have caught it: the new backend test only asserted the route's absence (that was the
regression, framed as the fix), and ReactUi/src/api/api.test.ts mocks fetch directly rather than
exercising the real MapRequests pipeline end-to-end.

No further action required. GetCurrentUserIdQuery stays off the exclusion list and continues
to be served at POST /api/GetCurrentUserIdQuery, matching current (correct) behavior. Closing
without code changes.

See the backend-engineer and security-agent team memory for the reusable lesson (verify "no
caller" claims against frontend/generated-schema call sites too, not just C# grep, before trusting
an audit's exclusion-list recommendation).

# Feature `#61` — Bug: `GetCurrentUserIdQuery` missing from the internal-only endpoint exclusion list **Requested by:** Backend Engineer, spotted during `#57`'s audit of internal-plumbing request types, 2026-07-21 **Priority:** Could **Blocked by:** none --- ## Problem `EndpointRouteBuilderExtensions.MapRequests` auto-maps every public `IRequest` type to a `POST` endpoint unless explicitly excluded. Five internal-only session/plumbing commands are correctly excluded: `SetCurrentUserIdCommand`, `DestroyCurrentSessionCommand`, `RevokeAllSessionsForCurrentUserCommand`, `GetSessionsRevokedBeforeUtcQuery`, `SendEmailVerificationCommand`. `GetCurrentUserIdQuery` — a sibling in the exact same `CqsTodo.Features.User` namespace, also `public`, also only ever dispatched as a constructor-injected dependency from inside other handlers (confirmed during `#57`'s audit: no direct caller anywhere outside handler bodies) — is the one that was never added, and is currently live at `POST /api/GetCurrentUserIdQuery`. Severity is low: the handler only returns the *caller's own* session-derived `UserId?` (`null` if unauthenticated), so there's no cross-user data exposure or IDOR — unlike the `#59` review's `GetSessionsRevokedBeforeUtcQuery` finding, which did leak other users' data. Still the same category of bug (an internal-only type missing from the exclusion list becomes an unintended live endpoint with no compile error or test failure to flag it), so it's worth closing for consistency and because the next thing added near it could be worse. ## Scope - Add `GetCurrentUserIdQuery` to the exclusion list in `CqsTodo.WebApi/EndpointRouteBuilderExtensions.cs`. - Extend `MapRequestsTests.MapRequests_excludes_internal_only_session_types_from_real_assembly` (added in `#59`'s review) to also assert this type produces no route. ## Out of scope - Any broader redesign of the exclusion-list mechanism itself (e.g. a marker interface/attribute instead of a hardcoded list) — already logged as accepted debt from the `#59` review (`docs/SECURITY_NOTES.md`), not re-litigated here. ## Agents involved - **Backend Engineer** — one-line fix + test extension - **Security Agent** — confirm no other public, non-excluded internal-only types exist (re-run the same audit `#57`/`#59` did) --- ## Resolution — Closed, not a bug (2026-07-21) **This story's premise was factually wrong.** The fix was implemented, tested (new regression test green, full solution green), and committed — then `/code-review`'s cross-file tracer angle checked the frontend for callers and found one: `ReactUi/src/App.tsx:23` calls `callApi('GetCurrentUserIdQuery', {})` over real HTTP inside a `useEffect` on every app mount, to determine login status. It's also a live path in the checked-in generated OpenAPI schema (`ReactUi/src/api/generated/openapi-schema.d.ts`). The "confirmed during `#57`'s audit: no direct caller anywhere outside handler bodies" claim above only checked C# call sites (constructor-injected handler dependencies), not the frontend's HTTP callers — the exact blind spot that made the five sibling types (all *actually* internal) look like a pattern this type also fit. It doesn't: `GetCurrentUserIdQuery` is the load-bearing "am I logged in" check for the entire app, not internal plumbing, despite being marked `IInternalRequest` (that marker was added in `#57` for a different purpose — scoping metrics — and was never meant to imply "has no external caller"; conflating the two is the root confusion here). **Impact avoided:** the fix was reverted before push. Had it shipped, `POST /api/GetCurrentUserIdQuery` would have 404'd, and every user's login-status check on every page load would have failed — likely presenting as an app-wide stuck "Loading…" screen. Neither test suite would have caught it: the new backend test only asserted the route's absence (that was the regression, framed as the fix), and `ReactUi/src/api/api.test.ts` mocks `fetch` directly rather than exercising the real `MapRequests` pipeline end-to-end. **No further action required.** `GetCurrentUserIdQuery` stays off the exclusion list and continues to be served at `POST /api/GetCurrentUserIdQuery`, matching current (correct) behavior. Closing without code changes. See the backend-engineer and security-agent team memory for the reusable lesson (verify "no caller" claims against frontend/generated-schema call sites too, not just C# grep, before trusting an audit's exclusion-list recommendation).