13 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 08-dashboard-widgets-vollimplementierung | 2026-07-01T00:00:00Z | standard | 24 |
|
|
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.
// 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:
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:<ipv4> — 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:
// 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:
// 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);
}
// 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 <p> at line 271 renders {error} directly, so users see link.error in the UI instead of "Fehler beim Laden des Links" / "Error loading link".
// 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:
} 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.
// 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:
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 <div> at line 59 that acts as the click-to-close backdrop carries aria-hidden="true". This attribute propagates to all descendants, including the inner <div role="dialog"> at line 66. Screen readers and other assistive technologies will therefore ignore the entire modal — the widget catalog is completely inaccessible when it opens.
// Problematic:
<div
className="fixed inset-0 z-50 flex items-center justify-center"
onClick={onClose}
aria-hidden="true" // ← hides everything inside, including the dialog
>
<div role="dialog" aria-modal="true" aria-label={t('catalogTitle')}>
...
</div>
</div>
Fix: Remove aria-hidden from the outer wrapper and apply it only to the backdrop overlay; the dialog should remain visible to assistive technologies:
<div
className="fixed inset-0 z-50 flex items-center justify-center"
onClick={onClose}
>
{/* Backdrop — visually only, hidden from AT */}
<div className="fixed inset-0 bg-black/50" aria-hidden="true" />
{/* Dialog — accessible */}
<div
ref={dialogRef}
role="dialog"
aria-modal="true"
aria-label={t('catalogTitle')}
...
>
WR-04: calculator-widget.tsx — "M" button is a duplicate of "MR" with identical handler
File: apps/web/src/components/dashboard/widgets/calculator-widget.tsx:339-340
Issue: The memory button row renders two buttons that both call memoryRecall:
<button type="button" className={memBtn} onClick={memoryRecall} disabled={memory === 0}>MR</button>
<button type="button" className={memBtn} onClick={memoryRecall} disabled={memory === 0}>M</button>
Standard calculator conventions define "M" as a memory-panel / memory-view button distinct from "MR" (memory recall). Here both call the same function. This appears to be a copy-paste error when implementing the "M" button. The button provides no unique functionality and may confuse users expecting memory-panel behavior.
Fix: If "M" is intended to display the stored memory value (common in modern calculators), implement a distinct handler. If the label is meant to be "MS" (memory store), replace the onClick and label accordingly:
{/* If M = MS (memory store) */}
<button type="button" className={memBtn} onClick={memoryStore}>MS</button>
WR-05: schema.prisma — FavoriteLink.widgetId has no FK relation to WidgetInstance
File: apps/api/prisma/schema.prisma:237-252
Issue: FavoriteLink.widgetId is a plain String with an index but no @relation referencing WidgetInstance. When a WidgetInstance record is deleted (e.g., user removes a widget from their dashboard), all associated FavoriteLink rows are silently orphaned. The orphaned rows will accumulate in the database without any mechanism to identify or clean them up.
model FavoriteLink {
id String @id @default(uuid())
userId String
tenantId String
widgetId String // ← no @relation, no onDelete cascade
...
@@index([widgetId])
}
Fix: Add a relation to WidgetInstance with cascade delete, or add an application-level cleanup hook in the widget removal service. The Prisma-schema approach:
model FavoriteLink {
id String @id @default(uuid())
userId String
tenantId String
widgetId String
widgetInstance WidgetInstance @relation(fields: [widgetId], references: [id], onDelete: Cascade)
...
}
model WidgetInstance {
...
favoriteLinks FavoriteLink[]
}
Info
IN-01: create-widget.dto.ts — stale comment
File: apps/api/src/dashboard/dto/create-widget.dto.ts:4
Issue: The JSDoc comment reads "widgetType must be one of the four supported types" but the @IsIn([...]) decorator lists eight types. The comment was not updated when Phase 8 types were added.
Fix: Update the comment: "widgetType must be one of the eight supported types."
IN-02: calculator-widget.tsx — "Fehler" error string hardcoded in German
File: apps/web/src/components/dashboard/widgets/calculator-widget.tsx:25,109
Issue: The display error string "Fehler" is hardcoded at two points: in formatNumber (line 25) and compared against in displayIsError (line 109). English locale users see "Fehler" on division-by-zero or arithmetic overflow instead of "Error". The rest of the widget uses useTranslations('widgets').
Fix: Add a translation key and use it:
// en.json / de.json under "widgets.calculator":
"error": "Error" // de: "Fehler"
// In CalculatorWidget:
const errorLabel = t('calculator.error');
const displayIsError = display === errorLabel;
// In formatNumber (make it accept the label or keep it pure and check outside):
// Alternative: return a sentinel like '!ERR!' and map to translated string at render time
IN-03: page.tsx — hardcoded "Loading..." text instead of translation key
File: apps/web/src/app/(portal)/page.tsx:55
Issue: The loading fallback renders <p>Loading...</p> as a hardcoded English string. Every other loading state in the codebase uses t('common.loading') (which exists in both de.json and en.json).
Fix:
// Replace:
<p className="text-sm text-muted-foreground">Loading...</p>
// With:
<p className="text-sm text-muted-foreground">{t('common.loading')}</p>
Reviewed: 2026-07-01 Reviewer: Claude (gsd-code-reviewer) Depth: standard