#26 — In-App Notification Center #26
Labels
No labels
priority/could
priority/must
priority/should
priority/wont
status/blocked
status/claimed
status/done-migrated
type/bug
type/feature
type/infra
type/tech-debt
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
robert/todo#26
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Story: In-App Notification Center
As a list member,
I want a notification bell that shows me relevant updates,
so that I know when something needs my attention without constantly checking every list.
Acceptance criteria:
#16)Out of scope for this story:
#28)#29)Blockers: None (richer content possible after
#16assign todo is delivered)Priority: Should — closes the feedback loop for assigned work and membership changes.
security (
26_in_app_notifications_security.md)Security Pre-Review: Story
#26— In-App Notification CenterReviewer: Security Agent
Date: 2026-07-09
Design document:
26_in_app_notifications_design.mdStatus: Approved with required changes
Summary
The overall design is architecturally sound. Authorization scoping for the query and bulk-mark-all paths is correct. The WebSocket gating pattern mirrors the existing publishers faithfully. Six issues are raised below; two are required changes that must be resolved before or during implementation, and four are lower-severity items with clear mitigations.
Findings
1. IDOR / Authorization —
MarkNotificationReadCommandSeverity: High
The handler correctly performs an ownership check (
entity.RecipientId != currentUserId → throw UnauthorizedAccessException). However, as the design itself flags,NotificationIdis a sequential auto-increment integer. A motivated attacker who holds one validNotificationId(from their own session) can enumerate adjacent IDs and invokeMarkNotificationReadCommandagainst them, probing for the existence of other users' notifications.The current exception mapping in
ExceptionHandler.csdistinguishes cleanly:EntityNotFoundExceptionUnauthorizedAccessExceptionThis means the handler's current design returns 403 when a user probes another user's notification that exists, which confirms the notification ID is valid for some user. An attacker can therefore distinguish "this ID exists but belongs to someone else" (403) from "this ID does not exist" (404). This is an oracle for notification existence.
Recommendation (required): When
RecipientId != currentUserId, throwEntityNotFoundExceptionrather thanUnauthorizedAccessException. The caller receives 404 in both the "does not exist" and "belongs to another user" cases, eliminating the enumeration oracle. The internal message can still be "not your notification" for logging purposes. Document this as a deliberate security decision in the handler comment.2. Sequential
NotificationIdExposure (Design Q5)Severity: Medium
Related to Finding 1. Sequential integer PKs allow an authenticated user to infer the total notification volume of the system (by comparing the highest ID they have received with neighbouring IDs). While this does not directly expose content, it is an information leak about system activity.
Recommendation: For this MVP, the 404-masking fix in Finding 1 is the mandatory control. Switching to a non-sequential ID type (e.g., ULID or UUID v7) would eliminate the enumeration vector entirely but is a scope change. It is acceptable to defer that to a follow-up story, provided the 404-masking is in place at launch.
3. Information Leakage —
NotificationDtoexposesRecipientIdover WebSocketSeverity: Medium
NotificationDtois defined as:The design document's §6.3 filter description implies the DTO carries
RecipientIdfor WS routing (change.Data!.RecipientId == userId.Value). However, theNotificationDtorecord as defined does not includeRecipientId. The design text is inconsistent: §6.3 referenceschange.Data!.RecipientIdas the filter field, but the DTO definition in §3.1 has no such field.If
RecipientIdis added toNotificationDtoto enable the WS filter, it will be serialised into the JSON payload sent to the client. The client already knows their own user ID so this is not a cross-user leak, but it does expose an internal database integer (the user's primary key) unnecessarily.There are two clean alternatives:
RecipientIdin a parallel wrapper type that is used only for in-process routing and never serialised. The WS publisher holdsnotificationChangesasIObservable<Change<NotificationId, NotificationDto>>— this type carries the DTO. A lightweight approach is to use a separateIObservable<(UserId RecipientId, Change<NotificationId, NotificationDto>)>internally so the recipient is routed without embedding it in the DTO.RecipientIdtoNotificationDtobut annotate it with[JsonIgnore](System.Text.Json) so it is used for in-process routing but not sent over the wire or returned by the REST queries. Ensure the JSON serialiser options on the WS send path respect[JsonIgnore](the existingJsonOptionsinUserScopedTodoListChangePublisherdo not configureDefaultIgnoreCondition, so[JsonIgnore]will be respected).Recommendation (required): Choose one of the two approaches above and document it in the implementation. The WS payload sent to the browser must not contain
RecipientId.4. Notification Body Injection —
TodoListTitleLacks a Length CapSeverity: Medium
The
Bodyfield is constructed server-side from persisted data:TodoTitle(inCommon/Types/TodoTitle.cs) is validated with a hard cap of 1,024 characters — safe.TodoListTitle(inCommon/Types/TodoListTitle.cs) validates only that the value is non-null. There is no length cap. A user who creates a list with a very long title (e.g., 10,000 characters) causes the persistedBodyfield to be arbitrarily large. This has two secondary consequences:Bodycolumn has noMaxLengthconfigured in the EF Core entity (builder.Property(x => x.Kind).HasConversion<string>()is the only property annotation shown;Bodygets the defaulttextcolumn type in PostgreSQL, which accepts up to 1 GB).There is no HTML or SQL injection risk here because the value is stored as plain text and never rendered as raw HTML server-side, and EF Core parameterises all values. The risk is a DB storage and wire-size concern.
Recommendation: Add a length cap to
TodoListTitle.Validate()— 255 characters is conventional for a list title and consistent with UX constraints. Additionally, addbuilder.Property(x => x.Body).HasMaxLength(2048)(or a derived maximum that accounts for the longest possible template with the longest valid title) to theNotificationEntityconfiguration so the DB enforces the cap as a second line of defence.5. WebSocket Authentication — New Endpoint Correctly Follows Established Pattern
Severity: Info
UserScopedTodoListChangePublisher.cs(the reference implementation) performs the following sequence before accepting the WebSocket:context.WebSockets.IsWebSocketRequest→ 400 if false.getUserIdHandler.Handle(new GetCurrentUserIdQuery(), ...)→ 401 if null.context.WebSockets.AcceptWebSocketAsync().This order is critical: accepting the WebSocket before verifying the user would allow an unauthenticated connection to be upgraded and then terminated abortly, wasting server resources and potentially leaking connection state.
The design for
UserScopedNotificationPublisher(§6.3) shows the same correct order (userId is null → StatusCode = 401; return). As long as the implementation follows this pattern exactly — which the existing publisher code makes clear — the new endpoint is safe.Recommendation: No change required. Add a unit test that verifies a non-WebSocket request to
/api/changes/NotificationDtogets 400, and an unauthenticated WebSocket upgrade attempt gets 401, consistent with the existing WS publisher tests (if any exist).6. Rate Limiting — No Existing Middleware; New Endpoints Are Unprotected
Severity: Medium
A search of the codebase finds no rate limiting middleware, no
AddRateLimiter/UseRateLimitercall, noAspNetCoreRateLimitpackage reference, and no per-endpoint throttle attribute anywhere in the project. This is a pre-existing gap, not introduced by this story.The new queries
GetNotificationsForCurrentUserQueryandGetUnreadNotificationCountForCurrentUserQueryare routed viaMapRequests, which maps all publicIRequesttypes toPOST /api/{TypeName}. Both queries hit the DB and are callable in a tight loop by an authenticated user.GetNotificationsForCurrentUserQueryalready capsLimitat 200 rows (design §5.1), which limits per-request DB cost. The count query is a singleCOUNT(*)over an indexed column.Recommendation: The absence of rate limiting is a pre-existing gap that is out of scope for this story to fix. However, the team should track it as a separate security backlog item. For this story specifically, ensure the
Limitparameter validation (max 200) is enforced by a guard in the handler body — e.g.,if (request.Limit > 200) throw new ArgumentException(...)— rather than relying on the frontend to send a sane value.7. Multi-Replica Duplicate WS Publish — Affected-Row Count Mitigation Is Correct
Severity: Info / Confirmed
Design Q7 asks whether checking the affected-row count from
ExecuteSqlInterpolatedAsyncis sufficient to suppress duplicate WS publishes when multiple replicas runDueNotificationJobconcurrently.Confirmed: yes, this is the correct mitigation.
ExecuteSqlInterpolatedAsyncreturns the number of rows affected. For anINSERT ... ON CONFLICT DO NOTHINGstatement, the return value is:1if the row was inserted (this replica wins; it should publish).0if the conflict fired and nothing was inserted (another replica already inserted; skip the publish).Because the unique constraint
UX_NotificationEntity_IdempotencyKeyis enforced by PostgreSQL atomically, exactly one replica will see a return value of 1 for any given(RecipientId, TodoListId, TodoNr, Kind, DATE(CreatedAt))tuple. Only that replica should callnotificationSubject.OnNext(...).Implementation note: The design describes fetching inserted IDs "with a short time window" after the bulk insert. This approach is fragile in a multi-replica scenario because both replicas may query the table and find the newly inserted rows, causing both to publish. The recommended approach is to batch the inserts and check the return value of each individual
INSERT ... ON CONFLICT DO NOTHINGcall, publishing only when the return is 1. If a true bulk insert (multiple rows in a single SQL statement) is used, the total return value must equal the expected insert count; any shortfall means some conflicts occurred and those specific rows must not be republished. The implementation must be careful here — a naive "fetch rows inserted in the last N seconds" approach will produce duplicate WS events.8. Self-Assignment Suppression (PO Decision Q2) — Security Implications
Severity: Info
The PO has decided: if
request.AssigneeId == currentUserId, no notification is created. This is implemented by comparing the current user's ID against the assignee ID before callingCreateNotificationCommand.From a security standpoint, this check is straightforward and benign. There is no TOCTOU risk: the
currentUserIdis resolved from the session inside the same request scope. The only edge case is an authorisation-level concern: theAssignTodoCommandHandleralready enforces that the assignee is a member of the list (isMembercheck), so a user cannot self-assign to a list they don't belong to, and they certainly cannot assign to someone else without that person being a member. The suppression logic introduces no new attack surface.Recommendation: No security concern. Implement as designed. Add a unit test asserting no notification row is created when
assigneeId == currentUserId.9. ON DELETE CASCADE — Both FK Constraints
Severity: Low
Both
RecipientId → UserEntity(Id)andTodoListId → TodoListEntity(Id)useON DELETE CASCADE. This means:One subtle concern: if a user is deleted while another user's notification references the deleted user as an actor (e.g., "Alex added you to 'X'"), that notification also vanishes because
RecipientIdis the joining user's ID — not the actor's ID. There is no FK on the inviter or assigner. This is correct by the schema design.However, if in a future story the inviter's name is added to the
Bodyor a newActorIdFK column is introduced pointing toUserEntity, that FK should useON DELETE SET NULLorON DELETE RESTRICT, not CASCADE, to avoid silently deleting the recipient's notification when an unrelated user (the actor) is deleted.Recommendation: No change required for this story. Add a comment in the migration and entity configuration noting that any future
ActorIdFK must not use CASCADE.Authorization Model Summary Assessment
GetNotificationsForCurrentUserQuerycurrentUserIdGetUnreadNotificationCountForCurrentUserQuerycurrentUserIdMarkNotificationReadCommandMarkAllNotificationsReadCommandcurrentUserIdCreateNotificationCommand(internal)RecipientIdfrom trusted handlers/jobCreateDueNotificationsCommand(internal)RecipientIdtaken from DB rows (AssigneeId)/api/changes/NotificationDtochange.Data.RecipientId == userIdRequired Changes Before or During Implementation
Finding 1 (High): In
MarkNotificationReadCommandHandler, changethrow new UnauthorizedAccessException(...)tothrow EntityNotFound.Create<NotificationEntity>(...)whenRecipientId != currentUserId. This eliminates the 403-vs-404 enumeration oracle.Finding 3 (Medium): Resolve the
RecipientIdfield ambiguity. Either use a wrapper observable type that carries recipient routing data outside the serialised DTO, or add[JsonIgnore]toRecipientIdif it must live onNotificationDto. The JSON payload sent to the browser must not includeRecipientId.Recommended Changes (Should Fix)
Finding 4 (Medium): Add a length cap to
TodoListTitle.Validate()(suggested: 255 characters). AddHasMaxLengthto theBodyproperty inNotificationEntityEF Core configuration.Finding 6 (Medium): Enforce the
Limit <= 200constraint in the handler body with anArgumentException, not just as a documented convention.Finding 7 (Info/Confirmed): Ensure the multi-replica WS publish logic is based on the per-row
INSERTreturn value, not a post-hoc time-window query. Document this explicitly in the handler implementation.Informational (No Action Required)
ActorIdFK must not use CASCADE.Verdict
Approved with required changes.
Two items (Findings 1 and 3) must be resolved during implementation before the branch is submitted for code review. The remaining items are improvements that should be addressed in the same story (Findings 4, 6, 7) or deferred with a recorded decision (Findings 2, 5, 8, 9).
design (
26_in_app_notifications_design.md)Design: Story
#26— In-App Notification CenterStatus: Ready for Security pre-review
Author: Software Architect
Date: 2026-07-09
1. Summary
A
NotificationEntitytable records per-user notifications that are created as side effects inside existing CQS handlers and a new background job. Notifications are delivered to connected clients in real-time over a new user-scoped WebSocket channel (/api/changes/NotificationDto) that reuses the establishedISubject<Change<TId,TDto>>pattern. The frontend polls nothing; the bell badge stays live as long as the WebSocket is open.Four events produce notifications:
Notifications persist in PostgreSQL and survive session loss. A daily background job creates due-today and overdue notifications and also purges rows older than 30 days.
2. DB Schema Changes
2.1 New table:
NotificationEntityIndexes:
The idempotency unique index covers only rows with a
TodoNr(i.e., due-today and overdue kinds) so the background job cannot insert a duplicate for the same todo on the same UTC calendar day. For list-level and assignment notifications (inserted by handlers, never by the job) no uniqueness constraint is applied because the handler path is already idempotent by the nature of those operations (invitation accepted once, assignment set once).ON DELETE CASCADE on both FKs: if a list or a user is deleted the notification rows disappear automatically without application code. This is the right behaviour — a deleted list's notifications are meaningless.
Kindenum values (stored as string, matches the C# enum):AddedToListTodoAssignedTodoDueTodayTodoOverdue2.2 EF Core entity
2.3 New enum
2.4 New Vogen value object
NotificationIdmust also be registered inCqsTodo/DbContext/VogenEfCoreConverters.cs:2.5 Migration name
AddNotificationEntity3. DTO Changes
3.1 New
NotificationDtoinCommon/Dtos/The
TodoListTitleis denormalised into the DTO so the frontend can display the list name without a second round-trip. TheTodoNris included so the frontend can deep-link to the specific item when present.4. Commands
4.1
MarkNotificationReadCommandHandler logic:
NotificationEntitybyNotificationId.EntityNotFoundException.entity.RecipientId != currentUserIdthrowUnauthorizedAccessException("Not your notification").ExecuteUpdateAsyncto setIsRead = true.Unit.Default.No WS publish needed — the badge update is a client-side optimistic decrement on click (the notification list is re-fetched or the local Zustand store is patched).
Authorization:
The ownership guard (step 3) is handled in the handler body, not as a separate
AuthorizeIsCurrentUserAuthenticateddecorator, because the authorization depends on loading the entity first.4.2
MarkAllNotificationsReadCommandHandler logic:
currentUserIdfromGetCurrentUserIdQuery.ExecuteUpdateAsyncallNotificationEntityrows whereRecipientId == currentUserId && IsRead == false, settingIsRead = true.Unit.Default.Authorization:
4.3 Internal
CreateNotificationCommand(not exposed as an API endpoint)This is an internal command used by the notification-creating handlers and the background job. It is not registered via
MapRequests(it is notpublic) and has no HTTP endpoint.Handler logic:
NotificationEntityusing the supplied parameters andCreatedAt = DateTimeOffset.UtcNow.DbContext(per established pattern) to obtain the generatedIdand joinedTodoListTitle.notificationSubject.OnNext(new Change<NotificationId, NotificationDto>(ChangeReason.Add, dto.Id, dto)).NotificationDto.Because
CreateNotificationCommandisinternal,MapRequestsskips it (it only mapspublicIRequesttypes).No
ConfigureDi: This handler is registered directly with[SetupHandler]and does not useIHaveDiConfigbecause it is not externally callable; authorization is enforced by the calling handlers.Injection into calling handlers:
5. Queries
5.1
GetNotificationsForCurrentUserQueryHandler logic:
currentUserIdfromGetCurrentUserIdQuery.NotificationEntitywhereRecipientId == currentUserId, ordered byCreatedAt DESC, limited toLimitrows.UnreadOnly == true, add.Where(x => !x.IsRead).TodoListEntityto getTitlefor the DTO.NotificationDto(inline select, no Mapperly needed for this simple projection).Return type:
IReadOnlyCollection<NotificationDto>Authorization:
Note on
Limit: The frontend uses a fixed default of 50. TheLimitparameter is validated to a maximum of 200 on the handler side to prevent DoS via large page fetches. Pagination is out of scope for this story.5.2
GetUnreadNotificationCountForCurrentUserQueryHandler logic:
COUNT(*)whereRecipientId == currentUserId && !IsRead. Used to populate the badge count on initial page load and after reconnect.Authorization:
AuthorizeIsCurrentUserAuthenticatedQuery.6. Real-Time Delivery
6.1 Chosen approach
A new user-scoped WebSocket publisher (
UserScopedNotificationPublisher) is added at/api/changes/NotificationDto. This follows exactly the same pattern asUserScopedTodoChangePublisherandUserScopedTodoListChangePublisher. The subject isISubject<Change<NotificationId, NotificationDto>>.Notifications are inherently user-scoped (each notification has a single recipient), so a global broadcast subject filtered by
RecipientIdis all that is needed. No membership set tracking is required for this subject — notifications are not filtered by list access.6.2 Subject registration in
CqsTodo/Setup.cs6.3
UserScopedNotificationPublisherHowever, since
NotificationDtocontainsRecipientIdalready, the publisher checkschange.Data?.RecipientId == userId.Valueto route the event only to the intended recipient's WebSocket connection.6.4 Endpoint registration in
Program.cs6.5 Why a new WS endpoint rather than piggybacking
The existing
/api/changes/TodoDtoand/api/changes/TodoListDtochannels are list-scoped; they carryChange<TodoId, TodoDto>andChange<TodoListId, TodoListBroadcastDto>respectively. Squeezing notification events onto one of those channels would require a discriminated union or an envelope type — a cross-cutting structural change. A new endpoint is cheaper, keeps each channel strongly typed, and the WebSocket handshake cost is negligible for a logged-in user who holds two or three connections already.7. Background Job — Due-Today and Overdue Notifications
7.1 Scope
The background job handles only
TodoDueTodayandTodoOverduenotifications.AddedToListandTodoAssignednotifications are always created synchronously inside their respective command handlers (§4.3).7.2 Implementation approach
Use
PeriodicTimerinside aBackgroundService(ASP.NET CoreIHostedServicesubclass). No external scheduler library (Hangfire, Quartz) is needed;PeriodicTimerwas introduced in .NET 6 specifically for this pattern and avoids thread pool waste.Registration in
Setup.cs(or inProgram.csviabuilder.Services):7.3
CreateDueNotificationsCommand(internal, not HTTP-exposed)Handler logic:
Because the
UX_NotificationEntity_IdempotencyKeyunique index includesDATE(CreatedAt AT TIME ZONE 'UTC'), re-running the job on the same calendar day (e.g., after a crash and restart) producesON CONFLICT ... DO NOTHINGfor already-created rows. The job therefore never double-notifies.Implementation note on upsert: EF Core 10 supports
ExecuteInsertAsyncwith conflict resolution viaUseStrategy(InsertConflictResolution.DoNothing)or a raw SQL interpolated string. The raw SQL approach is preferred here because theInsertAsyncEF Core API does not yet surface per-column conflict targets:The inserted IDs are then fetched back (query the rows just inserted with a short time window) and published.
7.4 Purge strategy (see also §8)
The
CreateDueNotificationsCommandhandler also runs the 30-day purge in the same transaction/scope:Bundling purge with the notification job keeps the codebase simple — no separate hosted service needed.
8. 30-Day Purge Strategy
As noted in §7.4, purge is executed once per day inside
CreateDueNotificationsCommand. TheExecuteDeleteAsynccall targetsCreatedAt < (UtcNow - 30 days)and uses a single bulk-delete SQL statement, which is efficient even for large tables (the index onRecipientId, CreatedAtcovers the range scan).If the application is restarted or the job fails, rows may accumulate beyond 30 days until the next successful run. This is acceptable for MVP (a cosmetic worst-case of one extra day of stale rows).
No separate purge command, endpoint, or hosted service is added.
9. Trigger Points — Handler Modifications
9.1
AcceptListInvitationCommandHandler— "Added to list" notificationAfter the membership is saved and the list DTO is fetched, inject and call:
9.2
AssignTodoCommandHandler— "Todo assigned" notificationAfter publishing the todo change event, and only when
request.AssigneeIdis non-null (a clear-assignment produces no notification):Self-assignment edge case: If the current user assigns a todo to themselves, a notification is still created. The story AC does not exclude self-assignment notifications; this is the simplest behaviour and consistent with the other notification types.
10. Authorization Summary
GetNotificationsForCurrentUserQueryHandlerAuthorizeIsCurrentUserAuthenticatedQuery)GetUnreadNotificationCountForCurrentUserQueryHandlerMarkNotificationReadCommandHandlerMarkAllNotificationsReadCommandHandlerCreateNotificationCommandHandlerCreateDueNotificationsCommandHandlerIHostedServicescope/api/changes/NotificationDtoUserScopedNotificationPublisherGetCurrentUserIdQueryreturns null (same pattern as other WS publishers)Security notes for pre-review:
MarkNotificationReadCommand: A user must not be able to mark another user's notification as read. The handler must load the entity and compareRecipientId == currentUserIdbefore updating. This check MUST NOT be skipped even though only the notification owner can know theNotificationId(IDs are sequential integers, trivially enumerable).GetNotificationsForCurrentUserQuerynever leaks cross-user data: The WHERE clause always scopes bycurrentUserId; there is noNotificationId-based lookup that could return another user's row.UserScopedNotificationPublisherfilters onchange.Data!.RecipientId == userId.Value.Datais always non-null forChangeReason.Addevents and notifications are only ever added (never updated or deleted via the WS subject). This is safe.CreateDueNotificationsCommandmust NOT useGetCurrentUserIdQuery— it operates on behalf of all users. The job injectsRecipientIddirectly from the scanned todo rows.Bodystring is constructed in C# from data already in the DB (list title, todo title). It is not constructed from user-supplied free text except through those already-validated fields. No injection risk beyond what those fields already carry.11. Files That Will Change / New Files
Backend — new files
Common/Types/NotificationId.csCommon/Dtos/NotificationDto.csCqsTodo/Entities/NotificationEntity.csIEntity<T>configurationCqsTodo/Entities/NotificationKind.csCqsTodo/Features/Notifications/CreateNotificationCommandHandler.csCqsTodo/Features/Notifications/CreateDueNotificationsCommandHandler.csCqsTodo/Features/Notifications/GetNotificationsForCurrentUserQueryHandler.csCqsTodo/Features/Notifications/GetUnreadNotificationCountForCurrentUserQueryHandler.csCqsTodo/Features/Notifications/MarkNotificationReadCommandHandler.csCqsTodo/Features/Notifications/MarkAllNotificationsReadCommandHandler.csCqsTodo/BackgroundJobs/DueNotificationJob.csBackgroundServicehosting the periodic timerCqsTodo.WebApi/WebSocket/UserScopedNotificationPublisher.csRecipientIdCqsTodo/Migrations/<timestamp>_AddNotificationEntity.csBackend — files that change
CqsTodo/DbContext/VogenEfCoreConverters.cs[EfCoreConverter<NotificationId>]CqsTodo/Setup.csISubject<Change<NotificationId,NotificationDto>>and its observableCqsTodo.WebApi/Program.csUserScopedNotificationPublisheras transient; map/api/changes/NotificationDto; registerDueNotificationJobviaAddHostedServiceCqsTodo/Features/TodoLists/AcceptListInvitationCommandHandler.csIHandler<CreateNotificationCommand, NotificationDto>; createAddedToListnotification after membership saveCqsTodo/Features/Todos/AssignTodoCommandHandler.csIHandler<CreateNotificationCommand, NotificationDto>; createTodoAssignednotification after todo updateTest files — new
CqsTodo.Tests/Features/Notifications/CreateNotificationCommandHandlerTests.csCqsTodo.Tests/Features/Notifications/GetNotificationsForCurrentUserQueryHandlerTests.csUnreadOnlyfilter; no cross-user leakageCqsTodo.Tests/Features/Notifications/GetUnreadNotificationCountForCurrentUserQueryHandlerTests.csCqsTodo.Tests/Features/Notifications/MarkNotificationReadCommandHandlerTests.csUnauthorizedAccessException)CqsTodo.Tests/Features/Notifications/MarkAllNotificationsReadCommandHandlerTests.csCqsTodo.Tests/Features/Notifications/CreateDueNotificationsCommandHandlerTests.csTodoDueTodayrows; overdue job createsTodoOverduerows; idempotent on re-run same day; done todos skipped; unassigned todos skipped; purges rows older than 30 daysCqsTodo.Tests/Features/TodoLists/AcceptListInvitationCommandHandlerTests.csCqsTodo.Tests/Features/Todos/AssignTodoCommandHandlerTests.csFrontend — out of scope for this doc; components needed
NotificationBell— header icon + unread badge; opensNotificationPanelNotificationPanel— dropdown/drawer listingNotificationDto[]; "Mark all as read" buttonNotificationItem— renders body, list name, relative timestamp; navigates on clicknotifications: NotificationDto[],unreadCount: number; WebSocket subscription on/api/changes/NotificationDto; actions:markRead(id),markAllRead(),fetchInitial()getNotificationsForCurrentUser(),getUnreadNotificationCount(),markNotificationRead(id),markAllNotificationsRead()12. Open Questions for Security Agent
PO decisions (resolved before security pre-review):
TodoListInvitationEntityand is a separate scope item.request.AssigneeId == currentUserId→ skip). You already know you assigned it to yourself.TodoAssignednotification stays in her list after reassignment — this is fine for MVP. Alice was a member and saw the todo title; no new information is leaked.For Security Agent review:
Sequential
NotificationIdenumeration: ShouldMarkNotificationReadCommandreturn 404 (not found) or 403 (forbidden) when a user probes another user's notification? Recommend 404 to avoid confirming existence of other users' notifications.Rate limiting: Is the existing API layer rate-limiting sufficient for
GetNotificationsForCurrentUserQueryandGetUnreadNotificationCountForCurrentUserQuery? These are cheap but could be called in a tight loop.Multi-instance duplicate WS publish: In a multi-replica deployment,
DueNotificationJobruns in every instance. TheON CONFLICT DO NOTHINGprevents DB duplicates, but the WS publish would fire in every instance for the same notification. The handler must check the affected-row count fromExecuteSqlInterpolatedAsync— if 0 rows inserted (conflict), skip the publish. Confirm this is the correct mitigation.handoff (
26_in_app_notifications_handoff.md)Handoff: Story
#26— In-App Notification CenterBranch:
feature/in-app-notificationsStatus: Ready for human review
Date: 2026-07-10
What was built
A full in-app notification center: bell icon with unread badge, real-time delivery
over WebSocket, persistent storage in PostgreSQL, and a daily background job for
due-today / overdue notifications.
Commits
426d9d7cdf5757115b14953b30e3Acceptance criteria status
AcceptListInvitationAssignTodo(suppressed for self-assign)Test coverage
Security
All required and should-fix items from the pre-review are addressed:
MarkNotificationReadreturns 404 for both not-found and wrong-user(UserId, Change<...>)used for routingBodycolumnHasMaxLength(2048)ArgumentExceptionin handler bodyFinal security verdict: Approved.
Known limitations / out of scope
#28)#29)security_final (
26_in_app_notifications_security_final.md)Security Final Review: Story
#26— In-App Notification CenterReviewer: Security Agent
Date: 2026-07-10
Branch:
feature/in-app-notificationsPre-review document:
26_in_app_notifications_security.mdStatus: Approved
Summary
All five pre-review findings (two required, three should-fix) have been correctly and completely implemented. No new high- or medium-severity issues were found during the final review. Two minor observations are noted below for completeness; neither blocks merge.
Pre-Review Findings — Verdict
Finding 1 (Required, High) — IDOR oracle in
MarkNotificationReadCommandVerdict: Resolved
MarkNotificationReadCommandHandler.csthrowsEntityNotFound.Create<NotificationEntity>(...)in both the "does not exist" and "belongs to another user" cases (lines 27–34). The comment in the handler explicitly documents the deliberate security decision:The test
Throws_EntityNotFoundException_not_UnauthorizedAccessException_when_notification_belongs_to_different_userinMarkNotificationReadCommandHandlerTests.csassertsEntityNotFoundException(notUnauthorizedAccessException), explicitly comments the HTTP 404 / enumeration rationale, and additionally verifies the notification remains unread after the rejected probe. Finding fully addressed.Finding 2 (Info) — Sequential
NotificationIdexposureVerdict: Resolved (deferred by design)
The 404-masking fix from Finding 1 is in place, which is the required MVP control. Switching to a non-sequential ID type remains deferred, consistent with the pre-review recommendation. No action required for this story.
Finding 3 (Required, Medium) —
RecipientIdinNotificationDtoleaking over WebSocketVerdict: Resolved
The preferred wrapper-observable approach (option 1 from the pre-review) was implemented:
NotificationDto(Common/Dtos/NotificationDto.cs) contains noRecipientIdfield. The comment documents this as intentional.Setup.csisISubject<(UserId RecipientId, Change<NotificationId, NotificationDto>)>— the recipient lives in the tuple wrapper, never in the serialised DTO.CreateNotificationCommandHandlerpublishes(request.RecipientId, new Change<...>(...))on the wrapper subject.CreateDueNotificationsCommandHandlerdoes the same inInsertAndPublishAsync.UserScopedNotificationPublisherinjectsIObservable<(UserId RecipientId, Change<NotificationId, NotificationDto> Change)>, filters.Where(x => x.RecipientId == userId.Value), then extracts.Select(x => x.Change)before serialising. The JSON payload sent to the browser isChange<NotificationId, NotificationDto>only — noRecipientIdin the wire format.Finding fully addressed.
Finding 4 (Should-fix, Medium) —
TodoListTitlelength cap andBodyHasMaxLengthVerdict: Resolved
TodoListTitle.Validate()(Common/Types/TodoListTitle.cs) now rejects values longer than 255 characters:NotificationEntity.Configure()setsbuilder.Property(x => x.Body).HasMaxLength(2048). The migration confirms the column ischaracter varying(2048).Worst-case body lengths (all safely under 2048):
"You were added to '<title>'""'<title>' was assigned to you""'<title>' is due today""'<title>' is overdue"The 2,048-char column cap is generous enough to accommodate all templates with full-length source fields, with headroom to spare. Finding fully addressed.
Finding 5 (Info) — WebSocket authentication ordering
Verdict: Resolved (pattern correctly followed)
UserScopedNotificationPublisher.Subscribe()follows the identical guard sequence as the reference implementation (UserScopedTodoListChangePublisher):IsWebSocketRequest→ 400 if false (line 33–37).getUserIdHandler.Handle(...)→ 401 if null (lines 40–45).AcceptWebSocketAsync()only after both checks pass (line 47).The comment on line 39 explicitly references "Security Finding 5 pattern". Finding confirmed correct.
Finding 6 (Should-fix, Medium) —
Limitparameter not server-enforcedVerdict: Resolved
GetNotificationsForCurrentUserQueryHandlerguards at the top ofHandle:A dedicated test
Throws_ArgumentException_when_Limit_exceeds_200asserts thatLimit: 201throwsArgumentException. Finding fully addressed.Finding 7 (Should-fix, Info) — Multi-replica duplicate WS publish
Verdict: Resolved
CreateDueNotificationsCommandHandler.InsertAndPublishAsync()uses individualINSERT ... ON CONFLICT ... DO NOTHINGstatements and checksaffected != 1before publishing (lines 102–113). Only the replica that inserted the row (return value = 1) callsnotificationSubject.OnNext(...). Replicas that lose the race (return value = 0) return early.The comment in the handler explicitly explains the multi-replica rationale. The post-insert read uses a time-window query on
CreatedAt >= todayOffsetwith anOrderByDescending(...).FirstOrDefaultAsync(...)to fetch the DTO for publishing — this is safe because only the inserting replica reaches this code path (theaffected != 1guard precedes it).One detail: the post-insert query filters by
RecipientId,TodoListId,TodoNr,Kind, andCreatedAt >= todayOffset. In the unlikely event of clock skew wheretodayOffsetslightly lags the inserted row'sCreatedAt,FirstOrDefaultAsyncreturns null and the handler silently skips the WS publish (line 137–138). The row is already in the DB; the user will see it on next poll or reconnect. This is an acceptable edge case for a daily background job. Finding fully addressed.New Findings
NF-1 (Low) —
Limitparameter has no lower-bound guardFile:
CqsTodo/Features/Notifications/GetNotificationsForCurrentUserQueryHandler.csThe handler guards
Limit > 200but does not guardLimit <= 0. PassingLimit = 0returns an empty list (harmless —Take(0)is valid). PassingLimit = -1would throwArgumentOutOfRangeExceptionfrom EF Core'sTaketranslation in .NET 10.This is not a security issue — the endpoint requires authentication, and the default is 50. However, the exception thrown for a negative
Limitwould be an unhandledArgumentOutOfRangeExceptionrather than the cleanArgumentExceptionused forLimit > 200, which could produce a different HTTP status code depending on howExceptionHandler.csmaps that exception type.Recommendation: Add
if (request.Limit <= 0) throw new ArgumentException("Limit must be positive.", nameof(request));alongside the existing upper-bound check. Low priority — no security impact.NF-2 (Info) —
CreateDueNotificationsCommandandCreateNotificationCommandconfirmed not HTTP-exposedFiles:
CqsTodo/Features/Notifications/CreateDueNotificationsCommandHandler.cs,CqsTodo/Features/Notifications/CreateNotificationCommandHandler.csBoth commands are declared
internal.EndpointRouteBuilderExtensions.MapRequestsfilters onx.IsPublic(line 26 ofEndpointRouteBuilderExtensions.cs), which means both commands are invisible to the HTTP routing layer by design. Confirmed safe — no action required.NF-3 (Info) — In-memory overdue filter is a performance consideration, not a security concern
File:
CqsTodo/Features/Notifications/CreateDueNotificationsCommandHandler.cs(lines 41–57)The overdue query fetches all incomplete, assigned todos with a non-null
DueDateand applies the< todayfilter in-memory due to aDateOnly/DateTimevalue-converter limitation. As noted in the code comment, this is acceptable for a once-daily background job. There is no security concern here; the data is read from trusted DB rows, not from user input. The comment should be retained in the code as documentation of the design decision.Authorization Model — Final Assessment
GetNotificationsForCurrentUserQueryRecipientId == currentUserIdGetUnreadNotificationCountForCurrentUserQueryRecipientId == currentUserIdMarkNotificationReadCommandMarkAllNotificationsReadCommandRecipientId == currentUserIdCreateNotificationCommandRecipientIdCreateDueNotificationsCommandRecipientIdfrom DB rows (AssigneeId)/api/changes/NotificationDtoRecipientIdin DTOAcceptListInvitationnotification!isAlreadyMemberAssignTodonotificationassigneeId != null && assigneeId != currentUserIdMigration Verification
The migration
20260709214336_AddNotificationEntity.cswas reviewed:Bodycolumn:character varying(2048)— matchesHasMaxLength(2048).UX_NotificationEntity_IdempotencyKeypartial unique index created via raw SQL inUp()withWHERE "TodoNr" IS NOT NULL— matches the design requirement.Down()drops the index explicitly before dropping the table — correct.onDelete: ReferentialAction.Cascade— correct per design.Overall Verdict
Approved.
All five pre-review findings (Findings 1, 3, 4, 6, 7) are correctly implemented. The two informational items (Findings 2, 5, 8, 9) were handled as designed. New Finding NF-1 (missing lower-bound Limit guard) is low priority with no security impact and does not block merge. The branch is ready for code review and merge.