docs(08): add code review report
This commit is contained in:
@@ -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:<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:
|
||||
|
||||
```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 `<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".
|
||||
|
||||
```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 `<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.
|
||||
|
||||
```tsx
|
||||
// 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:
|
||||
|
||||
```tsx
|
||||
<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`:
|
||||
|
||||
```tsx
|
||||
<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:
|
||||
|
||||
```tsx
|
||||
{/* 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.
|
||||
|
||||
```prisma
|
||||
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:
|
||||
|
||||
```prisma
|
||||
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:
|
||||
|
||||
```json
|
||||
// en.json / de.json under "widgets.calculator":
|
||||
"error": "Error" // de: "Fehler"
|
||||
```
|
||||
|
||||
```typescript
|
||||
// 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:**
|
||||
|
||||
```tsx
|
||||
// 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_
|
||||
Reference in New Issue
Block a user