From 54e4731b36be6131931649e13414abd3390ece23 Mon Sep 17 00:00:00 2001 From: Schalli Date: Wed, 1 Jul 2026 10:59:02 +0200 Subject: [PATCH] docs(08): add code review report --- .../08-REVIEW.md | 354 ++++++++++++++++++ 1 file changed, 354 insertions(+) create mode 100644 .planning/phases/08-dashboard-widgets-vollimplementierung/08-REVIEW.md diff --git a/.planning/phases/08-dashboard-widgets-vollimplementierung/08-REVIEW.md b/.planning/phases/08-dashboard-widgets-vollimplementierung/08-REVIEW.md new file mode 100644 index 0000000..881d557 --- /dev/null +++ b/.planning/phases/08-dashboard-widgets-vollimplementierung/08-REVIEW.md @@ -0,0 +1,354 @@ +--- +phase: 08-dashboard-widgets-vollimplementierung +reviewed: 2026-07-01T00:00:00Z +depth: standard +files_reviewed: 24 +files_reviewed_list: + - apps/api/prisma/schema.prisma + - apps/api/src/app.module.ts + - apps/api/src/dashboard/dto/create-widget.dto.ts + - apps/api/src/favorites/dto/create-favorite.dto.ts + - apps/api/src/favorites/dto/update-favorite.dto.ts + - apps/api/src/favorites/favorites.controller.ts + - apps/api/src/favorites/favorites.module.ts + - apps/api/src/favorites/favorites.service.ts + - apps/api/src/favorites/icon-discovery.service.ts + - apps/web/src/app/(portal)/page.tsx + - apps/web/src/components/dashboard/widget-catalog-modal.tsx + - apps/web/src/components/dashboard/widget-registry.test.tsx + - apps/web/src/components/dashboard/widget-registry.tsx + - apps/web/src/components/dashboard/widgets/calculator-widget.test.tsx + - apps/web/src/components/dashboard/widgets/calculator-widget.tsx + - apps/web/src/components/dashboard/widgets/favorites-widget.test.tsx + - apps/web/src/components/dashboard/widgets/favorites-widget.tsx + - apps/web/src/components/dashboard/widgets/link-widget.test.tsx + - apps/web/src/components/dashboard/widgets/link-widget.tsx + - apps/web/src/components/dashboard/widgets/stopwatch-widget.test.tsx + - apps/web/src/components/dashboard/widgets/stopwatch-widget.tsx + - apps/web/src/lib/favorites-api.ts + - apps/web/src/messages/de.json + - apps/web/src/messages/en.json +findings: + critical: 2 + warning: 5 + info: 3 + total: 10 +status: issues_found +--- + +# Phase 08: Code Review Report + +**Reviewed:** 2026-07-01 +**Depth:** standard +**Files Reviewed:** 24 +**Status:** issues_found + +## Summary + +Phase 8 adds four new dashboard widgets (Calculator, Stopwatch, Favorites, Link) plus the backing NestJS Favorites API and server-side icon/favicon discovery. The overall architecture is solid: per-user ownership checks, SSRF-aware icon fetching, clean widget-registry wiring pattern, and good test coverage. Two blockers require fixing before shipping: an SSRF bypass via IPv4-mapped IPv6 addresses in the icon discovery service, and a data-exposure flaw in the Favorites list endpoint that returns all of a user's favorites when the required `widgetId` parameter is omitted. Five warnings round out the findings, mostly in the frontend error-message handling and a schema data-integrity gap. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### CR-01: SSRF bypass via IPv4-mapped IPv6 addresses in icon-discovery.service.ts + +**File:** `apps/api/src/favorites/icon-discovery.service.ts:55-68` + +**Issue:** `isPrivateIpv6` only blocks `::ffff:127.x`, `::ffff:10.x`, and `::ffff:192.168.x`. The entire RFC 1918 172.16–31.x.x range and the 169.254.x.x link-local range are absent. Because `net.isIP('::ffff:172.31.0.1')` returns `6`, the code takes the IPv6 branch and finds no matching prefix, concluding the address is public — allowing the server to make outbound requests to internal infrastructure. An authenticated user who enters `http://[::ffff:172.16.0.1]/` as a favorite URL triggers this path. + +```typescript +// Current — incomplete +function isPrivateIpv6(address: string): boolean { + const lower = address.toLowerCase(); + return ( + lower === '::' || + lower === '::1' || + lower.startsWith('fc') || + lower.startsWith('fd') || + lower.startsWith('fe80:') || + lower.startsWith('::ffff:127.') || + lower.startsWith('::ffff:10.') || + lower.startsWith('::ffff:192.168.') + ); +} +``` + +**Fix:** Add the missing IPv4-mapped IPv6 ranges. The simplest safe approach is to detect the `::ffff:` prefix, parse the embedded IPv4 quad, and delegate to `isPrivateIpv4`: + +```typescript +function isPrivateIpv6(address: string): boolean { + const lower = address.toLowerCase(); + + if ( + lower === '::' || + lower === '::1' || + lower.startsWith('fc') || + lower.startsWith('fd') || + lower.startsWith('fe80:') || + lower.startsWith('ff') + ) { + return true; + } + + // IPv4-mapped IPv6 ::ffff: — delegate to isPrivateIpv4 + const v4MappedMatch = lower.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/); + if (v4MappedMatch) { + return isPrivateIpv4(v4MappedMatch[1]); + } + + return false; +} +``` + +--- + +### CR-02: GET /favorites returns all user favorites when widgetId is omitted + +**File:** `apps/api/src/favorites/favorites.controller.ts:48-55` + +**Issue:** The `widgetId` query parameter is not validated. If a client omits it (`GET /favorites` without a `?widgetId=` query), NestJS assigns `undefined` to the `string`-typed parameter at runtime. The service passes `undefined` to Prisma's `where` clause: + +```typescript +// favorites.service.ts — list() +return this.prisma.favoriteLink.findMany({ + where: { userId, widgetId }, // widgetId is undefined → Prisma drops the field + orderBy: [{ position: 'asc' }, { title: 'asc' }], +}); +``` + +Prisma v7 silently drops `undefined` fields from `where` clauses. The query becomes `WHERE userId = ?` with no widget scope, returning **all** of the user's favorites across every widget instance. This breaks the per-widget isolation that T-08-06 / Pitfall 3 are designed to enforce, and exposes more data than the caller is entitled to. + +**Fix:** Add explicit validation in the controller with a `ParseUUIDPipe` or a `@IsUUID()` guard, and add a null-guard in the service: + +```typescript +// Controller — require widgetId as a valid UUID +import { ParseUUIDPipe } from '@nestjs/common'; + +@Get() +async list( + @Query('widgetId', ParseUUIDPipe) widgetId: string, + @Req() req: Request, +) { + const { userId } = this.extractContext(req); + return this.favoritesService.list(userId, widgetId); +} +``` + +```typescript +// Service — defensive guard +async list(userId: string, widgetId: string) { + if (!widgetId) throw new BadRequestException('widgetId is required'); + return this.prisma.favoriteLink.findMany({ + where: { userId, widgetId }, + orderBy: [{ position: 'asc' }, { title: 'asc' }], + }); +} +``` + +--- + +## Warnings + +### WR-01: link-widget.tsx — all error paths store raw i18n key instead of translated string + +**File:** `apps/web/src/components/dashboard/widgets/link-widget.tsx:66,110,147,158` + +**Issue:** Every `setError(...)` call in this file stores the literal string `'link.error'` rather than calling `t('link.error')`. The error `

` at line 271 renders `{error}` directly, so users see `link.error` in the UI instead of "Fehler beim Laden des Links" / "Error loading link". + +```typescript +// All four error sites are identical — representative example: +} catch { + setError('link.error'); // raw key, not t('link.error') +} +``` + +**Fix:** Replace each with the translated call: + +```typescript +} catch { + setError(t('link.error')); +} +``` + +--- + +### WR-02: favorites-widget.tsx — inconsistent error handling between load and action paths + +**File:** `apps/web/src/components/dashboard/widgets/favorites-widget.tsx:74,122,158,168` + +**Issue:** The `useEffect` load handler stores the raw key `'favorites.error'` (line 74), while the three action handlers (`handleAdd`, `handleSaveEdit`, `handleDelete`) all store `t('favorites.error')` — the translated string. A load failure will display the raw key to the user; action failures will display the correct translation. + +```typescript +// useEffect — raw key (wrong): +if (!cancelled) setError('favorites.error'); + +// handleAdd — translated (correct): +setError(t('favorites.error')); +``` + +**Fix:** Change line 74 to use the translation function: + +```typescript +if (!cancelled) setError(t('favorites.error')); +``` + +--- + +### WR-03: widget-catalog-modal.tsx — `aria-hidden="true"` on the dialog's containing div + +**File:** `apps/web/src/components/dashboard/widget-catalog-modal.tsx:59-61` + +**Issue:** The outermost wrapper `

` at line 59 that acts as the click-to-close backdrop carries `aria-hidden="true"`. This attribute propagates to all descendants, including the inner `
` at line 66. Screen readers and other assistive technologies will therefore ignore the entire modal — the widget catalog is completely inaccessible when it opens. + +```tsx +// Problematic: + +``` + +**Fix:** Remove `aria-hidden` from the outer wrapper and apply it only to the backdrop overlay; the dialog should remain visible to assistive technologies: + +```tsx +
+ {/* Backdrop — visually only, hidden from AT */} +