feat: notifications, @mentions, and comment deep-links (#71 #72 #73) #127

🚨 BLOCKER — User enumeration endpoint has no permission check

GET /api/users/search?q= exposes full user list (name + UUID) to any authenticated session with zero permission check. An attacker with any valid cookie can enumerate all users by iterating single-character queries (?q=a, ?q=b, …). For a family archive this is a real privacy violation.

Fix: Add @RequirePermission(Permission.READ_ALL) at the controller class level, consistent with every other controller in this codebase. The UserSearchControllerTest must then also test the 403 path for a user without permissions.

🚨 **BLOCKER — User enumeration endpoint has no permission check** `GET /api/users/search?q=` exposes full user list (name + UUID) to any authenticated session with zero permission check. An attacker with any valid cookie can enumerate all users by iterating single-character queries (`?q=a`, `?q=b`, …). For a family archive this is a real privacy violation. **Fix:** Add `@RequirePermission(Permission.READ_ALL)` at the controller class level, consistent with every other controller in this codebase. The `UserSearchControllerTest` must then also test the 403 path for a user without permissions.

backend/src/main/java/org/raddatz/familienarchiv/model/DocumentComment.java

						
				@@ -1,10 +1,12 @@

				package org.raddatz.familienarchiv.model;

marcel commented

⚠️ MAJOR — @Transient field + FetchType.LAZY = potential LazyInitializationException at runtime

mentionDTOs is a @Transient field that is populated by the service before returning the entity. mentions is mapped as FetchType.LAZY. Read methods in CommentService are intentionally non-@Transactional (per architecture rules) — which means calling comment.getMentions() from withMentionDTOs() outside a transaction will trigger a LazyInitializationException.

This will not show up in unit tests (Mockito) but will explode in the integration test or production when the entity is fetched fresh from the repo.

Fix: Either change mentions to FetchType.EAGER (acceptable for a small bounded collection like mentions), or — better — introduce a proper CommentResponseDTO so you're not mutating a JPA entity with transient display state.

⚠️ **MAJOR — `@Transient` field + `FetchType.LAZY` = potential `LazyInitializationException` at runtime** `mentionDTOs` is a `@Transient` field that is populated by the service before returning the entity. `mentions` is mapped as `FetchType.LAZY`. Read methods in `CommentService` are intentionally non-`@Transactional` (per architecture rules) — which means calling `comment.getMentions()` from `withMentionDTOs()` outside a transaction will trigger a `LazyInitializationException`. This will not show up in unit tests (Mockito) but will explode in the integration test or production when the entity is fetched fresh from the repo. **Fix:** Either change `mentions` to `FetchType.EAGER` (acceptable for a small bounded collection like mentions), or — better — introduce a proper `CommentResponseDTO` so you're not mutating a JPA entity with transient display state.

backend/src/main/java/org/raddatz/familienarchiv/repository/NotificationRepository.java

						
				@@ -0,0 +1,25 @@

				package org.raddatz.familienarchiv.repository;

marcel commented

2026-03-27 23:34:54 +01:00

ℹ️ INFO — Unused method: findByRecipientIdOrderByCreatedAtDesc

The non-paginated List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID) is declared but never called — the paginated findByRecipientId(UUID, Pageable) is used exclusively. Dead code in repositories adds noise and risks someone accidentally calling the unbounded query on a user with thousands of notifications.

Fix: Remove the method.

ℹ️ **INFO — Unused method: `findByRecipientIdOrderByCreatedAtDesc`** The non-paginated `List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID)` is declared but never called — the paginated `findByRecipientId(UUID, Pageable)` is used exclusively. Dead code in repositories adds noise and risks someone accidentally calling the unbounded query on a user with thousands of notifications. **Fix:** Remove the method.

backend/src/main/java/org/raddatz/familienarchiv/service/CommentService.java

						
				@@ -1,10 +1,12 @@

				package org.raddatz.familienarchiv.service;

marcel commented

🚨 BLOCKER — Architecture violation: direct repository access across domain boundary

CommentService directly injects AppUserRepository in saveMentions(). CLAUDE.md is explicit: services must never reach into another domain's repository. Cross-domain data access must go through the owning service.

This breaks the layering contract and untestable in isolation through the intended interface — any CommentServiceTest now has to mock a repository it shouldn't even know about.

Fix: Add UserService.findAllById(Collection<UUID>), inject UserService into CommentService, and call that instead.

🚨 **BLOCKER — Architecture violation: direct repository access across domain boundary** `CommentService` directly injects `AppUserRepository` in `saveMentions()`. CLAUDE.md is explicit: services must never reach into another domain's repository. Cross-domain data access must go through the owning service. This breaks the layering contract and untestable in isolation through the intended interface — any `CommentServiceTest` now has to mock a repository it shouldn't even know about. **Fix:** Add `UserService.findAllById(Collection<UUID>)`, inject `UserService` into `CommentService`, and call that instead.

backend/src/main/java/org/raddatz/familienarchiv/service/NotificationService.java

						
				@@ -0,0 +1,187 @@

				package org.raddatz.familienarchiv.service;

marcel commented

🚨 BLOCKER — Fragile mixed injection + ReflectionTestUtils hack

The class uses @RequiredArgsConstructor but mailSender is injected via @Autowired(required = false) — a mixed injection approach. The test is forced to use ReflectionTestUtils.setField() to inject the mock. This is a maintenance trap: if mailSender is ever renamed, the reflection call silently breaks at runtime with no compile-time error — the email path stops being tested without any CI failure.

Fix: Inject Optional<JavaMailSender> as a constructor parameter (works with Lombok @RequiredArgsConstructor) or use @ConditionalOnBean(JavaMailSender.class) on a separate email-sending component. Either approach is injection-safe and doesn't require reflection in tests.

🚨 **BLOCKER — Fragile mixed injection + ReflectionTestUtils hack** The class uses `@RequiredArgsConstructor` but `mailSender` is injected via `@Autowired(required = false)` — a mixed injection approach. The test is forced to use `ReflectionTestUtils.setField()` to inject the mock. This is a maintenance trap: if `mailSender` is ever renamed, the reflection call silently breaks at runtime with no compile-time error — the email path stops being tested without any CI failure. **Fix:** Inject `Optional<JavaMailSender>` as a constructor parameter (works with Lombok `@RequiredArgsConstructor`) or use `@ConditionalOnBean(JavaMailSender.class)` on a separate email-sending component. Either approach is injection-safe and doesn't require reflection in tests.

backend/src/main/java/org/raddatz/familienarchiv/service/NotificationService.java

						
				@@ -0,0 +1,187 @@

				package org.raddatz.familienarchiv.service;

marcel commented

⚠️ MAJOR — Notification failure can silently roll back the parent comment

notifyReply() and notifyMentions() are @Transactional and are called from within the already-@Transactional replyToComment() / postComment() in CommentService. They join the outer transaction. If a notification save fails (DB constraint, unexpected null), it rolls back the entire outer transaction — the comment itself is lost.

A user's comment should never disappear because a notification couldn't be persisted.

Fix: Annotate both methods with @Transactional(propagation = Propagation.REQUIRES_NEW) to isolate notification failures, or use @Async to fire them outside the comment transaction entirely.

⚠️ **MAJOR — Notification failure can silently roll back the parent comment** `notifyReply()` and `notifyMentions()` are `@Transactional` and are called from within the already-`@Transactional` `replyToComment()` / `postComment()` in `CommentService`. They join the outer transaction. If a notification save fails (DB constraint, unexpected null), it rolls back the entire outer transaction — the comment itself is lost. A user's comment should never disappear because a notification couldn't be persisted. **Fix:** Annotate both methods with `@Transactional(propagation = Propagation.REQUIRES_NEW)` to isolate notification failures, or use `@Async` to fire them outside the comment transaction entirely.

backend/src/test/java/org/raddatz/familienarchiv/controller/NotificationControllerTest.java

						
				@@ -0,0 +1,162 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

⚠️ MAJOR — PATCH /api/notifications/{id}/read missing 401 test

Every other endpoint in this controller has an unauthenticated (401) test. The markOneRead endpoint is the only one missing it. This inconsistency means a security regression specifically on this endpoint would not be caught by CI.

Fix: Add markOneRead_returns401_whenUnauthenticated() mirroring the pattern already used for getNotifications_returns401_whenUnauthenticated.

⚠️ **MAJOR — `PATCH /api/notifications/{id}/read` missing 401 test** Every other endpoint in this controller has an unauthenticated (401) test. The `markOneRead` endpoint is the only one missing it. This inconsistency means a security regression specifically on this endpoint would not be caught by CI. **Fix:** Add `markOneRead_returns401_whenUnauthenticated()` mirroring the pattern already used for `getNotifications_returns401_whenUnauthenticated`.

backend/src/test/java/org/raddatz/familienarchiv/controller/UserSearchControllerTest.java

						
				@@ -0,0 +1,71 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

🔵 MINOR — search_returnsAtMostTenResults does not assert the count

The test name promises a cap of 10 results but the assertion only checks isOk(). If the LIMIT 10 in the repository query is accidentally removed, this test still passes.

Fix:

.andExpect(jsonPath("$.length()").value(lessThanOrEqualTo(10)));

🔵 **MINOR — `search_returnsAtMostTenResults` does not assert the count** The test name promises a cap of 10 results but the assertion only checks `isOk()`. If the `LIMIT 10` in the repository query is accidentally removed, this test still passes. **Fix:** ```java .andExpect(jsonPath("$.length()").value(lessThanOrEqualTo(10))); ```

backend/src/test/java/org/raddatz/familienarchiv/service/CommentServiceTest.java

marcel commented

⚠️ MAJOR — notifyMentions() call path has no test coverage

The new replyToComment_triggersNotification_afterSave test verifies reply notifications, but there is no equivalent test that notificationService.notifyMentions() is called when mentionedUserIds is non-empty — for either postComment or replyToComment. If that call is accidentally removed or the condition changes, nothing in CI will catch it.

Fix: Add:

postComment_triggersNotifyMentions_whenMentionedUserIdsProvided
replyToComment_triggersNotifyMentions_whenMentionedUserIdsProvided

⚠️ **MAJOR — `notifyMentions()` call path has no test coverage** The new `replyToComment_triggersNotification_afterSave` test verifies reply notifications, but there is no equivalent test that `notificationService.notifyMentions()` is called when `mentionedUserIds` is non-empty — for either `postComment` or `replyToComment`. If that call is accidentally removed or the condition changes, nothing in CI will catch it. **Fix:** Add: - `postComment_triggersNotifyMentions_whenMentionedUserIdsProvided` - `replyToComment_triggersNotifyMentions_whenMentionedUserIdsProvided`

backend/src/test/java/org/raddatz/familienarchiv/service/NotificationServiceTest.java

						
				@@ -0,0 +1,201 @@

				package org.raddatz.familienarchiv.service;

marcel commented

⚠️ MAJOR — Several service method paths are untested

Missing test cases:

markRead_throwsNotFound_whenNotificationDoesNotExist — the NOTIFICATION_NOT_FOUND code path is real and user-facing (stale ID from frontend). No test covers it.
markAllRead_delegatesToRepository — markAllRead() is tested end-to-end via the controller but never in isolation at the service layer.
countUnread_delegatesToRepository — same gap.

These are not exotic edge cases — they are production paths that the frontend already exercises.

⚠️ **MAJOR — Several service method paths are untested** Missing test cases: - `markRead_throwsNotFound_whenNotificationDoesNotExist` — the `NOTIFICATION_NOT_FOUND` code path is real and user-facing (stale ID from frontend). No test covers it. - `markAllRead_delegatesToRepository` — `markAllRead()` is tested end-to-end via the controller but never in isolation at the service layer. - `countUnread_delegatesToRepository` — same gap. These are not exotic edge cases — they are production paths that the frontend already exercises.

frontend/src/lib/components/CommentThread.svelte

marcel commented

🔵 MINOR — setTimeout(100) magic delay for deep-link scroll is flaky

setTimeout(() => {
  const el = document.querySelector(`[data-comment-id="${targetCommentId}"]`);
  el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
}, 100);

On slow devices or slow network responses, 100ms may not be enough for the panel to render and the data-comment-id element to be in the DOM. The scrollIntoView call then silently no-ops — the deep link appears broken with no error.

Fix: Use await tick() (Svelte's DOM-flush promise) followed by a requestAnimationFrame, or a MutationObserver that waits for the element to appear rather than assuming a fixed delay.

🔵 **MINOR — `setTimeout(100)` magic delay for deep-link scroll is flaky** ```typescript setTimeout(() => { const el = document.querySelector(`[data-comment-id="${targetCommentId}"]`); el?.scrollIntoView({ behavior: 'smooth', block: 'center' }); }, 100); ``` On slow devices or slow network responses, 100ms may not be enough for the panel to render and the `data-comment-id` element to be in the DOM. The `scrollIntoView` call then silently no-ops — the deep link appears broken with no error. **Fix:** Use `await tick()` (Svelte's DOM-flush promise) followed by a `requestAnimationFrame`, or a `MutationObserver` that waits for the element to appear rather than assuming a fixed delay.

frontend/src/lib/components/MentionEditor.svelte

						
				@@ -0,0 +1,235 @@

				<script lang="ts">

marcel commented

🔵 MINOR — debounceTimer not cleared on component destroy

The debounceTimer is cleared inside closePopup(), but there is no onDestroy hook. If the component unmounts while a debounced search is pending (user navigates away mid-typing), the timeout fires on an unmounted component and causes a stale state update.

Fix:

import { onDestroy } from 'svelte';
onDestroy(() => clearTimeout(debounceTimer));

🔵 **MINOR — `debounceTimer` not cleared on component destroy** The `debounceTimer` is cleared inside `closePopup()`, but there is no `onDestroy` hook. If the component unmounts while a debounced search is pending (user navigates away mid-typing), the timeout fires on an unmounted component and causes a stale state update. **Fix:** ```typescript import { onDestroy } from 'svelte'; onDestroy(() => clearTimeout(debounceTimer)); ```

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

marcel commented

🔵 MINOR — relativeTime() returns hard-coded German strings

"gerade eben", "vor {n} Min.", "vor {n} Std.", "vor {n} Tag(en)" are hard-coded German regardless of the active Paraglide locale. The project supports de/en/es and notification messages use m.* keys — but timestamps fall back to German for all locales.

Fix: Use Intl.RelativeTimeFormat with the current locale, or add Paraglide keys (e.g. m.notification_time_just_now(), m.notification_time_minutes_ago({ n })).

🔵 **MINOR — `relativeTime()` returns hard-coded German strings** `"gerade eben"`, `"vor {n} Min."`, `"vor {n} Std."`, `"vor {n} Tag(en)"` are hard-coded German regardless of the active Paraglide locale. The project supports de/en/es and notification messages use `m.*` keys — but timestamps fall back to German for all locales. **Fix:** Use `Intl.RelativeTimeFormat` with the current locale, or add Paraglide keys (e.g. `m.notification_time_just_now()`, `m.notification_time_minutes_ago({ n })`).

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

				import { onMount, onDestroy } from 'svelte';

marcel commented

🔵 MINOR — aria-label="ungelesen" is hard-coded German

The unread indicator dot has aria-label="ungelesen". Screen readers in English or Spanish will read "ungelesen" — a German word. This is an a11y regression.

Fix: Use a Paraglide key: aria-label={m.notification_unread()}.

🔵 **MINOR — `aria-label="ungelesen"` is hard-coded German** The unread indicator dot has `aria-label="ungelesen"`. Screen readers in English or Spanish will read "ungelesen" — a German word. This is an a11y regression. **Fix:** Use a Paraglide key: `aria-label={m.notification_unread()}`.

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

				import { onMount, onDestroy } from 'svelte';

				import { goto } from '$app/navigation';

marcel commented

🔵 MINOR — <div role="button"> does not handle the Space key

Each notification item is a <div role="button" tabindex="0"> with a manual onkeydown handler. The handler only handles Enter — but ARIA authoring practices specify that both Enter and Space activate buttons. Keyboard-only users pressing Space will have no response.

Fix: Replace <div role="button"> with a real <button> element. Native <button> handles both Enter and Space for free, is focusable by default, and requires no tabindex or manual keyboard wiring.

🔵 **MINOR — `<div role="button">` does not handle the Space key** Each notification item is a `<div role="button" tabindex="0">` with a manual `onkeydown` handler. The handler only handles `Enter` — but ARIA authoring practices specify that both `Enter` *and* `Space` activate buttons. Keyboard-only users pressing Space will have no response. **Fix:** Replace `<div role="button">` with a real `<button>` element. Native `<button>` handles both Enter and Space for free, is focusable by default, and requires no `tabindex` or manual keyboard wiring.

frontend/src/lib/utils/mention.spec.ts

						
				@@ -0,0 +1,120 @@

				import { describe, it, expect } from 'vitest';

marcel commented

2026-03-27 23:34:59 +01:00

🔵 MINOR — XSS coverage missing in renderBody test suite

The spec covers HTML escaping of the comment body content, but has no test where a mention.firstName or mention.lastName contains HTML-special characters. This means the XSS vector identified in renderBody (Finding #9 above) would not be caught by CI even after the fix is applied — leaving the regression unguarded.

Fix: Add a test case:

it('should escape HTML in mention display names', () => {
  const body = '@<script>';
  const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }];
  const html = renderBody(body, mentions);
  expect(html).not.toContain('<script>');
  expect(html).toContain('&lt;script&gt;');
});

🔵 **MINOR — XSS coverage missing in `renderBody` test suite** The spec covers HTML escaping of the comment body content, but has no test where a `mention.firstName` or `mention.lastName` contains HTML-special characters. This means the XSS vector identified in `renderBody` (Finding #9 above) would not be caught by CI even after the fix is applied — leaving the regression unguarded. **Fix:** Add a test case: ```typescript it('should escape HTML in mention display names', () => { const body = '@<script>'; const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }]; const html = renderBody(body, mentions); expect(html).not.toContain('<script>'); expect(html).toContain('<script>'); }); ```

frontend/src/lib/utils/mention.ts

						
				@@ -0,0 +1,67 @@

				import type { MentionDTO } from '$lib/types';

marcel commented

⚠️ MAJOR — Stored XSS vector in renderBody: mention display names are not escaped

const link = `<a class="mention" data-user-id="${mention.id}" href="#">@${displayName}</a>`;

The comment body text is correctly escape-first-then-process, but mention.firstName and mention.lastName are injected into the link string without any HTML escaping. An admin who registers a user with lastName: '"\><img src=x onerror=alert(1)>' produces a working XSS payload rendered in every comment that mentions that user.

Fix: Apply the same HTML escape treatment to displayName before injecting it. Also add a test in mention.spec.ts:

it('escapes HTML special chars in mention display name', () => {
  const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }];
  const result = renderBody('@<script> alert(1)', mentions);
  expect(result).not.toContain('<script>');
});

⚠️ **MAJOR — Stored XSS vector in `renderBody`: mention display names are not escaped** ```typescript const link = `<a class="mention" data-user-id="${mention.id}" href="#">@${displayName}</a>`; ``` The comment body text is correctly escape-first-then-process, but `mention.firstName` and `mention.lastName` are injected into the link string without any HTML escaping. An admin who registers a user with `lastName: '"\><img src=x onerror=alert(1)>'` produces a working XSS payload rendered in every comment that mentions that user. **Fix:** Apply the same HTML escape treatment to `displayName` before injecting it. Also add a test in `mention.spec.ts`: ```typescript it('escapes HTML special chars in mention display name', () => { const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }]; const result = renderBody('@<script> alert(1)', mentions); expect(result).not.toContain('<script>'); }); ```

frontend/src/routes/profile/+page.server.ts

						
				@@ -1,10 +1,15 @@

				import { fail } from '@sveltejs/kit';

marcel commented

2026-03-27 23:34:56 +01:00

⚠️ MAJOR — Checkbox preference values are unreliable without JS

The updateNotificationPrefs action reads:

notifyOnReply: formData.get('notifyOnReply') === 'true',

…from a <input type="hidden"> that is synced to a Svelte $state boolean via JS. When JS is disabled or use:enhance fails, unchecked checkboxes are not submitted in standard HTML forms — the hidden input will always carry its last JS-bound value, not the actual checkbox state. Preferences can be silently saved incorrectly.

Fix: Either read the checkbox directly using formData.has('notifyOnReply') (standard HTML checkbox presence pattern), or explicitly document and guard that this form requires JavaScript to function correctly.

⚠️ **MAJOR — Checkbox preference values are unreliable without JS** The `updateNotificationPrefs` action reads: ```typescript notifyOnReply: formData.get('notifyOnReply') === 'true', ``` …from a `<input type="hidden">` that is synced to a Svelte `$state` boolean via JS. When JS is disabled or `use:enhance` fails, unchecked checkboxes are not submitted in standard HTML forms — the hidden input will always carry its last JS-bound value, not the actual checkbox state. Preferences can be silently saved incorrectly. **Fix:** Either read the checkbox directly using `formData.has('notifyOnReply')` (standard HTML checkbox presence pattern), or explicitly document and guard that this form requires JavaScript to function correctly.

marcel reviewed 2026-03-27 23:35:43 +01:00

marcel left a comment

QA Review — Sara Holt, Senior QA Engineer

Verdict: REQUEST_CHANGES (posted as COMMENT — Gitea blocks self-review REQUEST_CHANGES)

3 BLOCKERs, 7 MAJORs, 8 MINORs, 3 INFOs. All inline comments are filed on the affected files.

Priority summary

BLOCKERs (must fix before merge):

CommentService directly accesses AppUserRepository — violates the layering contract in CLAUDE.md
@Autowired(required = false) + ReflectionTestUtils hack for mailSender — fragile, breaks silently on rename
GET /api/users/search has no @RequirePermission — any authenticated session can enumerate all users

MAJORs (strong recommendation to fix before merge):

NotificationController missing @RequirePermission — AOP gate never fires
mentionDTOs @Transient + FetchType.LAZY = LazyInitializationException outside a transaction
notifyReply/notifyMentions join the outer transaction — notification failure silently rolls back the user's comment
Stored XSS vector in renderBody: mention.firstName/lastName not HTML-escaped before injection into {@html ...}
Notification prefs form uses hidden inputs synced by JS — breaks silently without JS
Missing test cases: markRead 404 path, markAllRead, countUnread, PATCH 401, notifyMentions call verification

MINORs / INFOs: filed as inline comments — most are straightforward one-liners (onDestroy, aria-label Paraglide key, <button> instead of <div role=button>, tick() instead of setTimeout(100)).

The XSS finding is the one I'd push hardest on — the escape-first pattern is correctly applied to the comment body text, but the mention display names bypass it entirely. Given that {@html renderBody(...)} is already in use, one unescaped name field is enough for a stored XSS. This needs a fix and a regression test before merge.

## QA Review — Sara Holt, Senior QA Engineer **Verdict: REQUEST_CHANGES** *(posted as COMMENT — Gitea blocks self-review REQUEST_CHANGES)* 3 BLOCKERs, 7 MAJORs, 8 MINORs, 3 INFOs. All inline comments are filed on the affected files. --- ### Priority summary **BLOCKERs (must fix before merge):** 1. `CommentService` directly accesses `AppUserRepository` — violates the layering contract in CLAUDE.md 2. `@Autowired(required = false)` + `ReflectionTestUtils` hack for `mailSender` — fragile, breaks silently on rename 3. `GET /api/users/search` has no `@RequirePermission` — any authenticated session can enumerate all users **MAJORs (strong recommendation to fix before merge):** - `NotificationController` missing `@RequirePermission` — AOP gate never fires - `mentionDTOs` `@Transient` + `FetchType.LAZY` = `LazyInitializationException` outside a transaction - `notifyReply`/`notifyMentions` join the outer transaction — notification failure silently rolls back the user's comment - Stored XSS vector in `renderBody`: `mention.firstName`/`lastName` not HTML-escaped before injection into `{@html ...}` - Notification prefs form uses hidden inputs synced by JS — breaks silently without JS - Missing test cases: `markRead` 404 path, `markAllRead`, `countUnread`, `PATCH` 401, `notifyMentions` call verification **MINORs / INFOs:** filed as inline comments — most are straightforward one-liners (`onDestroy`, aria-label Paraglide key, `<button>` instead of `<div role=button>`, `tick()` instead of `setTimeout(100)`). --- The XSS finding is the one I'd push hardest on — the escape-first pattern is correctly applied to the comment body text, but the mention display names bypass it entirely. Given that `{@html renderBody(...)}` is already in use, one unescaped name field is enough for a stored XSS. This needs a fix *and* a regression test before merge.

backend/src/main/java/org/raddatz/familienarchiv/controller/NotificationController.java

						
				@@ -0,0 +1,71 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

⚠️ MAJOR — No @RequirePermission on notification controller

All five notification endpoints have no @RequirePermission annotation. Resolving the current user via authentication.getName() is not the same as the AOP PermissionAspect gate — a disabled or low-privilege account that passes the Spring Security filter layer can still call these endpoints.

The NotificationControllerTest tests 401 for unauthenticated, but never tests that a user with no permissions is blocked (403). This gap means a regression here would not be caught by CI.

Fix: Add @RequirePermission(Permission.READ_ALL) at the controller class level. Update tests to cover the 403 case.

⚠️ **MAJOR — No `@RequirePermission` on notification controller** All five notification endpoints have no `@RequirePermission` annotation. Resolving the current user via `authentication.getName()` is not the same as the AOP `PermissionAspect` gate — a disabled or low-privilege account that passes the Spring Security filter layer can still call these endpoints. The `NotificationControllerTest` tests 401 for unauthenticated, but never tests that a user with *no permissions* is blocked (403). This gap means a regression here would not be caught by CI. **Fix:** Add `@RequirePermission(Permission.READ_ALL)` at the controller class level. Update tests to cover the 403 case.

backend/src/main/java/org/raddatz/familienarchiv/controller/UserSearchController.java

						
				@@ -0,0 +1,29 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

🚨 BLOCKER — User enumeration endpoint has no permission check

GET /api/users/search?q= exposes full user list (name + UUID) to any authenticated session with zero permission check. An attacker with any valid cookie can enumerate all users by iterating single-character queries (?q=a, ?q=b, …). For a family archive this is a real privacy violation.

Fix: Add @RequirePermission(Permission.READ_ALL) at the controller class level, consistent with every other controller in this codebase. The UserSearchControllerTest must then also test the 403 path for a user without permissions.

🚨 **BLOCKER — User enumeration endpoint has no permission check** `GET /api/users/search?q=` exposes full user list (name + UUID) to any authenticated session with zero permission check. An attacker with any valid cookie can enumerate all users by iterating single-character queries (`?q=a`, `?q=b`, …). For a family archive this is a real privacy violation. **Fix:** Add `@RequirePermission(Permission.READ_ALL)` at the controller class level, consistent with every other controller in this codebase. The `UserSearchControllerTest` must then also test the 403 path for a user without permissions.

backend/src/main/java/org/raddatz/familienarchiv/model/DocumentComment.java

						
				@@ -1,10 +1,12 @@

				package org.raddatz.familienarchiv.model;

marcel commented

⚠️ MAJOR — @Transient field + FetchType.LAZY = potential LazyInitializationException at runtime

mentionDTOs is a @Transient field that is populated by the service before returning the entity. mentions is mapped as FetchType.LAZY. Read methods in CommentService are intentionally non-@Transactional (per architecture rules) — which means calling comment.getMentions() from withMentionDTOs() outside a transaction will trigger a LazyInitializationException.

This will not show up in unit tests (Mockito) but will explode in the integration test or production when the entity is fetched fresh from the repo.

Fix: Either change mentions to FetchType.EAGER (acceptable for a small bounded collection like mentions), or — better — introduce a proper CommentResponseDTO so you're not mutating a JPA entity with transient display state.

⚠️ **MAJOR — `@Transient` field + `FetchType.LAZY` = potential `LazyInitializationException` at runtime** `mentionDTOs` is a `@Transient` field that is populated by the service before returning the entity. `mentions` is mapped as `FetchType.LAZY`. Read methods in `CommentService` are intentionally non-`@Transactional` (per architecture rules) — which means calling `comment.getMentions()` from `withMentionDTOs()` outside a transaction will trigger a `LazyInitializationException`. This will not show up in unit tests (Mockito) but will explode in the integration test or production when the entity is fetched fresh from the repo. **Fix:** Either change `mentions` to `FetchType.EAGER` (acceptable for a small bounded collection like mentions), or — better — introduce a proper `CommentResponseDTO` so you're not mutating a JPA entity with transient display state.

backend/src/main/java/org/raddatz/familienarchiv/repository/NotificationRepository.java

						
				@@ -0,0 +1,25 @@

				package org.raddatz.familienarchiv.repository;

marcel commented

2026-03-27 23:34:54 +01:00

ℹ️ INFO — Unused method: findByRecipientIdOrderByCreatedAtDesc

The non-paginated List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID) is declared but never called — the paginated findByRecipientId(UUID, Pageable) is used exclusively. Dead code in repositories adds noise and risks someone accidentally calling the unbounded query on a user with thousands of notifications.

Fix: Remove the method.

ℹ️ **INFO — Unused method: `findByRecipientIdOrderByCreatedAtDesc`** The non-paginated `List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID)` is declared but never called — the paginated `findByRecipientId(UUID, Pageable)` is used exclusively. Dead code in repositories adds noise and risks someone accidentally calling the unbounded query on a user with thousands of notifications. **Fix:** Remove the method.

backend/src/main/java/org/raddatz/familienarchiv/service/CommentService.java

						
				@@ -1,10 +1,12 @@

				package org.raddatz.familienarchiv.service;

marcel commented

🚨 BLOCKER — Architecture violation: direct repository access across domain boundary

CommentService directly injects AppUserRepository in saveMentions(). CLAUDE.md is explicit: services must never reach into another domain's repository. Cross-domain data access must go through the owning service.

This breaks the layering contract and untestable in isolation through the intended interface — any CommentServiceTest now has to mock a repository it shouldn't even know about.

Fix: Add UserService.findAllById(Collection<UUID>), inject UserService into CommentService, and call that instead.

🚨 **BLOCKER — Architecture violation: direct repository access across domain boundary** `CommentService` directly injects `AppUserRepository` in `saveMentions()`. CLAUDE.md is explicit: services must never reach into another domain's repository. Cross-domain data access must go through the owning service. This breaks the layering contract and untestable in isolation through the intended interface — any `CommentServiceTest` now has to mock a repository it shouldn't even know about. **Fix:** Add `UserService.findAllById(Collection<UUID>)`, inject `UserService` into `CommentService`, and call that instead.

backend/src/main/java/org/raddatz/familienarchiv/service/NotificationService.java

						
				@@ -0,0 +1,187 @@

				package org.raddatz.familienarchiv.service;

marcel commented

🚨 BLOCKER — Fragile mixed injection + ReflectionTestUtils hack

The class uses @RequiredArgsConstructor but mailSender is injected via @Autowired(required = false) — a mixed injection approach. The test is forced to use ReflectionTestUtils.setField() to inject the mock. This is a maintenance trap: if mailSender is ever renamed, the reflection call silently breaks at runtime with no compile-time error — the email path stops being tested without any CI failure.

Fix: Inject Optional<JavaMailSender> as a constructor parameter (works with Lombok @RequiredArgsConstructor) or use @ConditionalOnBean(JavaMailSender.class) on a separate email-sending component. Either approach is injection-safe and doesn't require reflection in tests.

🚨 **BLOCKER — Fragile mixed injection + ReflectionTestUtils hack** The class uses `@RequiredArgsConstructor` but `mailSender` is injected via `@Autowired(required = false)` — a mixed injection approach. The test is forced to use `ReflectionTestUtils.setField()` to inject the mock. This is a maintenance trap: if `mailSender` is ever renamed, the reflection call silently breaks at runtime with no compile-time error — the email path stops being tested without any CI failure. **Fix:** Inject `Optional<JavaMailSender>` as a constructor parameter (works with Lombok `@RequiredArgsConstructor`) or use `@ConditionalOnBean(JavaMailSender.class)` on a separate email-sending component. Either approach is injection-safe and doesn't require reflection in tests.

backend/src/main/java/org/raddatz/familienarchiv/service/NotificationService.java

						
				@@ -0,0 +1,187 @@

				package org.raddatz.familienarchiv.service;

marcel commented

⚠️ MAJOR — Notification failure can silently roll back the parent comment

notifyReply() and notifyMentions() are @Transactional and are called from within the already-@Transactional replyToComment() / postComment() in CommentService. They join the outer transaction. If a notification save fails (DB constraint, unexpected null), it rolls back the entire outer transaction — the comment itself is lost.

A user's comment should never disappear because a notification couldn't be persisted.

Fix: Annotate both methods with @Transactional(propagation = Propagation.REQUIRES_NEW) to isolate notification failures, or use @Async to fire them outside the comment transaction entirely.

⚠️ **MAJOR — Notification failure can silently roll back the parent comment** `notifyReply()` and `notifyMentions()` are `@Transactional` and are called from within the already-`@Transactional` `replyToComment()` / `postComment()` in `CommentService`. They join the outer transaction. If a notification save fails (DB constraint, unexpected null), it rolls back the entire outer transaction — the comment itself is lost. A user's comment should never disappear because a notification couldn't be persisted. **Fix:** Annotate both methods with `@Transactional(propagation = Propagation.REQUIRES_NEW)` to isolate notification failures, or use `@Async` to fire them outside the comment transaction entirely.

backend/src/test/java/org/raddatz/familienarchiv/controller/NotificationControllerTest.java

						
				@@ -0,0 +1,162 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

⚠️ MAJOR — PATCH /api/notifications/{id}/read missing 401 test

Every other endpoint in this controller has an unauthenticated (401) test. The markOneRead endpoint is the only one missing it. This inconsistency means a security regression specifically on this endpoint would not be caught by CI.

Fix: Add markOneRead_returns401_whenUnauthenticated() mirroring the pattern already used for getNotifications_returns401_whenUnauthenticated.

⚠️ **MAJOR — `PATCH /api/notifications/{id}/read` missing 401 test** Every other endpoint in this controller has an unauthenticated (401) test. The `markOneRead` endpoint is the only one missing it. This inconsistency means a security regression specifically on this endpoint would not be caught by CI. **Fix:** Add `markOneRead_returns401_whenUnauthenticated()` mirroring the pattern already used for `getNotifications_returns401_whenUnauthenticated`.

backend/src/test/java/org/raddatz/familienarchiv/controller/UserSearchControllerTest.java

						
				@@ -0,0 +1,71 @@

				package org.raddatz.familienarchiv.controller;

marcel commented

🔵 MINOR — search_returnsAtMostTenResults does not assert the count

The test name promises a cap of 10 results but the assertion only checks isOk(). If the LIMIT 10 in the repository query is accidentally removed, this test still passes.

Fix:

.andExpect(jsonPath("$.length()").value(lessThanOrEqualTo(10)));

🔵 **MINOR — `search_returnsAtMostTenResults` does not assert the count** The test name promises a cap of 10 results but the assertion only checks `isOk()`. If the `LIMIT 10` in the repository query is accidentally removed, this test still passes. **Fix:** ```java .andExpect(jsonPath("$.length()").value(lessThanOrEqualTo(10))); ```

backend/src/test/java/org/raddatz/familienarchiv/service/CommentServiceTest.java

marcel commented

⚠️ MAJOR — notifyMentions() call path has no test coverage

The new replyToComment_triggersNotification_afterSave test verifies reply notifications, but there is no equivalent test that notificationService.notifyMentions() is called when mentionedUserIds is non-empty — for either postComment or replyToComment. If that call is accidentally removed or the condition changes, nothing in CI will catch it.

Fix: Add:

postComment_triggersNotifyMentions_whenMentionedUserIdsProvided
replyToComment_triggersNotifyMentions_whenMentionedUserIdsProvided

⚠️ **MAJOR — `notifyMentions()` call path has no test coverage** The new `replyToComment_triggersNotification_afterSave` test verifies reply notifications, but there is no equivalent test that `notificationService.notifyMentions()` is called when `mentionedUserIds` is non-empty — for either `postComment` or `replyToComment`. If that call is accidentally removed or the condition changes, nothing in CI will catch it. **Fix:** Add: - `postComment_triggersNotifyMentions_whenMentionedUserIdsProvided` - `replyToComment_triggersNotifyMentions_whenMentionedUserIdsProvided`

backend/src/test/java/org/raddatz/familienarchiv/service/NotificationServiceTest.java

						
				@@ -0,0 +1,201 @@

				package org.raddatz.familienarchiv.service;

marcel commented

⚠️ MAJOR — Several service method paths are untested

Missing test cases:

markRead_throwsNotFound_whenNotificationDoesNotExist — the NOTIFICATION_NOT_FOUND code path is real and user-facing (stale ID from frontend). No test covers it.
markAllRead_delegatesToRepository — markAllRead() is tested end-to-end via the controller but never in isolation at the service layer.
countUnread_delegatesToRepository — same gap.

These are not exotic edge cases — they are production paths that the frontend already exercises.

⚠️ **MAJOR — Several service method paths are untested** Missing test cases: - `markRead_throwsNotFound_whenNotificationDoesNotExist` — the `NOTIFICATION_NOT_FOUND` code path is real and user-facing (stale ID from frontend). No test covers it. - `markAllRead_delegatesToRepository` — `markAllRead()` is tested end-to-end via the controller but never in isolation at the service layer. - `countUnread_delegatesToRepository` — same gap. These are not exotic edge cases — they are production paths that the frontend already exercises.

frontend/src/lib/components/CommentThread.svelte

marcel commented

🔵 MINOR — setTimeout(100) magic delay for deep-link scroll is flaky

setTimeout(() => {
  const el = document.querySelector(`[data-comment-id="${targetCommentId}"]`);
  el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
}, 100);

On slow devices or slow network responses, 100ms may not be enough for the panel to render and the data-comment-id element to be in the DOM. The scrollIntoView call then silently no-ops — the deep link appears broken with no error.

Fix: Use await tick() (Svelte's DOM-flush promise) followed by a requestAnimationFrame, or a MutationObserver that waits for the element to appear rather than assuming a fixed delay.

🔵 **MINOR — `setTimeout(100)` magic delay for deep-link scroll is flaky** ```typescript setTimeout(() => { const el = document.querySelector(`[data-comment-id="${targetCommentId}"]`); el?.scrollIntoView({ behavior: 'smooth', block: 'center' }); }, 100); ``` On slow devices or slow network responses, 100ms may not be enough for the panel to render and the `data-comment-id` element to be in the DOM. The `scrollIntoView` call then silently no-ops — the deep link appears broken with no error. **Fix:** Use `await tick()` (Svelte's DOM-flush promise) followed by a `requestAnimationFrame`, or a `MutationObserver` that waits for the element to appear rather than assuming a fixed delay.

frontend/src/lib/components/MentionEditor.svelte

						
				@@ -0,0 +1,235 @@

				<script lang="ts">

marcel commented

🔵 MINOR — debounceTimer not cleared on component destroy

The debounceTimer is cleared inside closePopup(), but there is no onDestroy hook. If the component unmounts while a debounced search is pending (user navigates away mid-typing), the timeout fires on an unmounted component and causes a stale state update.

Fix:

import { onDestroy } from 'svelte';
onDestroy(() => clearTimeout(debounceTimer));

🔵 **MINOR — `debounceTimer` not cleared on component destroy** The `debounceTimer` is cleared inside `closePopup()`, but there is no `onDestroy` hook. If the component unmounts while a debounced search is pending (user navigates away mid-typing), the timeout fires on an unmounted component and causes a stale state update. **Fix:** ```typescript import { onDestroy } from 'svelte'; onDestroy(() => clearTimeout(debounceTimer)); ```

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

marcel commented

🔵 MINOR — relativeTime() returns hard-coded German strings

"gerade eben", "vor {n} Min.", "vor {n} Std.", "vor {n} Tag(en)" are hard-coded German regardless of the active Paraglide locale. The project supports de/en/es and notification messages use m.* keys — but timestamps fall back to German for all locales.

Fix: Use Intl.RelativeTimeFormat with the current locale, or add Paraglide keys (e.g. m.notification_time_just_now(), m.notification_time_minutes_ago({ n })).

🔵 **MINOR — `relativeTime()` returns hard-coded German strings** `"gerade eben"`, `"vor {n} Min."`, `"vor {n} Std."`, `"vor {n} Tag(en)"` are hard-coded German regardless of the active Paraglide locale. The project supports de/en/es and notification messages use `m.*` keys — but timestamps fall back to German for all locales. **Fix:** Use `Intl.RelativeTimeFormat` with the current locale, or add Paraglide keys (e.g. `m.notification_time_just_now()`, `m.notification_time_minutes_ago({ n })`).

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

				import { onMount, onDestroy } from 'svelte';

marcel commented

🔵 MINOR — aria-label="ungelesen" is hard-coded German

The unread indicator dot has aria-label="ungelesen". Screen readers in English or Spanish will read "ungelesen" — a German word. This is an a11y regression.

Fix: Use a Paraglide key: aria-label={m.notification_unread()}.

🔵 **MINOR — `aria-label="ungelesen"` is hard-coded German** The unread indicator dot has `aria-label="ungelesen"`. Screen readers in English or Spanish will read "ungelesen" — a German word. This is an a11y regression. **Fix:** Use a Paraglide key: `aria-label={m.notification_unread()}`.

frontend/src/lib/components/NotificationBell.svelte

						
				@@ -0,0 +1,304 @@

				<script lang="ts">

				import { onMount, onDestroy } from 'svelte';

				import { goto } from '$app/navigation';

marcel commented

🔵 MINOR — <div role="button"> does not handle the Space key

Each notification item is a <div role="button" tabindex="0"> with a manual onkeydown handler. The handler only handles Enter — but ARIA authoring practices specify that both Enter and Space activate buttons. Keyboard-only users pressing Space will have no response.

Fix: Replace <div role="button"> with a real <button> element. Native <button> handles both Enter and Space for free, is focusable by default, and requires no tabindex or manual keyboard wiring.

🔵 **MINOR — `<div role="button">` does not handle the Space key** Each notification item is a `<div role="button" tabindex="0">` with a manual `onkeydown` handler. The handler only handles `Enter` — but ARIA authoring practices specify that both `Enter` *and* `Space` activate buttons. Keyboard-only users pressing Space will have no response. **Fix:** Replace `<div role="button">` with a real `<button>` element. Native `<button>` handles both Enter and Space for free, is focusable by default, and requires no `tabindex` or manual keyboard wiring.

frontend/src/lib/utils/mention.spec.ts

						
				@@ -0,0 +1,120 @@

				import { describe, it, expect } from 'vitest';

marcel commented

2026-03-27 23:34:59 +01:00

🔵 MINOR — XSS coverage missing in renderBody test suite

The spec covers HTML escaping of the comment body content, but has no test where a mention.firstName or mention.lastName contains HTML-special characters. This means the XSS vector identified in renderBody (Finding #9 above) would not be caught by CI even after the fix is applied — leaving the regression unguarded.

Fix: Add a test case:

it('should escape HTML in mention display names', () => {
  const body = '@<script>';
  const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }];
  const html = renderBody(body, mentions);
  expect(html).not.toContain('<script>');
  expect(html).toContain('&lt;script&gt;');
});

🔵 **MINOR — XSS coverage missing in `renderBody` test suite** The spec covers HTML escaping of the comment body content, but has no test where a `mention.firstName` or `mention.lastName` contains HTML-special characters. This means the XSS vector identified in `renderBody` (Finding #9 above) would not be caught by CI even after the fix is applied — leaving the regression unguarded. **Fix:** Add a test case: ```typescript it('should escape HTML in mention display names', () => { const body = '@<script>'; const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }]; const html = renderBody(body, mentions); expect(html).not.toContain('<script>'); expect(html).toContain('<script>'); }); ```

frontend/src/lib/utils/mention.ts

						
				@@ -0,0 +1,67 @@

				import type { MentionDTO } from '$lib/types';

marcel commented

⚠️ MAJOR — Stored XSS vector in renderBody: mention display names are not escaped

const link = `<a class="mention" data-user-id="${mention.id}" href="#">@${displayName}</a>`;

The comment body text is correctly escape-first-then-process, but mention.firstName and mention.lastName are injected into the link string without any HTML escaping. An admin who registers a user with lastName: '"\><img src=x onerror=alert(1)>' produces a working XSS payload rendered in every comment that mentions that user.

Fix: Apply the same HTML escape treatment to displayName before injecting it. Also add a test in mention.spec.ts:

it('escapes HTML special chars in mention display name', () => {
  const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }];
  const result = renderBody('@<script> alert(1)', mentions);
  expect(result).not.toContain('<script>');
});

⚠️ **MAJOR — Stored XSS vector in `renderBody`: mention display names are not escaped** ```typescript const link = `<a class="mention" data-user-id="${mention.id}" href="#">@${displayName}</a>`; ``` The comment body text is correctly escape-first-then-process, but `mention.firstName` and `mention.lastName` are injected into the link string without any HTML escaping. An admin who registers a user with `lastName: '"\><img src=x onerror=alert(1)>'` produces a working XSS payload rendered in every comment that mentions that user. **Fix:** Apply the same HTML escape treatment to `displayName` before injecting it. Also add a test in `mention.spec.ts`: ```typescript it('escapes HTML special chars in mention display name', () => { const mentions = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }]; const result = renderBody('@<script> alert(1)', mentions); expect(result).not.toContain('<script>'); }); ```

frontend/src/routes/profile/+page.server.ts

						
				@@ -1,10 +1,15 @@

				import { fail } from '@sveltejs/kit';

marcel commented