# Task 6 Review: Endpoint stato/download/trigger PDF

**Diff:** ae5ec76..3666b9d  
**File:** `app/src/routes/api/dvr/[id]/pdf/+server.ts`  
**Reviewer:** subagent-review  
**Data:** 2026-06-24

---

## Auth Check — VERIFICATO ESPLICITAMENTE

**Risultato: PROTETTO. Nessuna vulnerabilità di esposizione dati.**

Lettura di `app/src/hooks.server.ts` (riga 26):
```ts
if (!user && !isPublic) {
    throw redirect(302, '/login?next=' + encodeURIComponent(path));
}
```
Le uniche route pubbliche sono `/login` e `/password-reset` (riga 12). La route `/api/dvr/[id]/pdf` non è in questo elenco, quindi qualsiasi richiesta non autenticata viene intercettata dall'hook globale con redirect 302 prima di raggiungere l'handler.

**Confronto con allegati** (`app/src/routes/api/allegati/[id]/+server.ts` riga 18):
```ts
if (!locals.user) throw error(401, 'Unauthorized');
```
L'endpoint allegati ha una guardia in-handler esplicita (401) in aggiunta all'hook. Il nuovo endpoint PDF manca di questa seconda linea di difesa, ma non crea una vulnerabilità reale: l'hook copre tutte le route non pubbliche in modo uniforme.

**Differenza di comportamento**: un utente non autenticato su `/api/dvr/[id]/pdf` riceve un redirect 302, non uno 401. Per una chiamata fetch() da JavaScript questo è silenziosamente degradante (l'app ottiene l'HTML della pagina di login, non un errore HTTP chiaro). Su allegati invece riceverebbe 401, che è più corretto semanticamente per un'API.

Questa è una **Minor** (inconsistenza di postura tra endpoint API, non un buco di sicurezza).

---

## Spec Compliance

### GET ?mode=status ✅
`app/src/routes/api/dvr/[id]/pdf/+server.ts:38-40`:
```ts
if (url.searchParams.get('mode') === 'status') {
    return json({ state: status.state, generatoIl: status.generatoIl });
}
```
Esatto: `200 { state, generatoIl }`. Conforme alla spec.

### GET download — logica canServe ✅
Righe 43-48:
```ts
const allowStale = url.searchParams.get('allowStale') === '1';
const canServe =
    status.state === 'ready' || (allowStale && status.state === 'stale' && status.r2Key !== null);

if (!canServe || !status.r2Key) {
    return json({ state: status.state }, { status: 409 });
}
```
La condizione `canServe` implementa correttamente la spec:
- `ready` → serve sempre
- `stale + allowStale=1 + r2Key presente` → serve la versione precedente
- tutti gli altri casi (generating, error, missing, stale senza allowStale, stale senza r2Key) → `409 { state }`

Il double-guard `!canServe || !status.r2Key` è prudente: protegge anche il caso in cui `state=ready` ma `r2Key` è `null` in DB (dato inconsistente). In quel caso restituisce `409`.

Nota: la spec dice "altrimenti `409 { state }`". Il caso `getObject` ritorna `null` (riga 57) restituisce `409 { state: 'missing' }` — tecnicamente `state` qui è stringa hardcoded `'missing'`, non il `status.state` del DVR. Questo è accettabile (l'oggetto R2 è fisicamente assente, lo state del DVR potrebbe essere `ready`) ma è un'asimmetria rispetto al pattern dei casi non-ready. **Minor**.

### GET download — streaming pattern ✅
Confronto con `allegati/[id]/+server.ts:41`:
```ts
return new Response(obj.body, { headers: { ... } });
```
Il nuovo endpoint usa identicamente `obj.body` (già `ReadableStream` via `transformToWebStream()` in `r2.ts:46`). Pattern corretto. `getObject` return shape verificata in `r2.ts:39-55`: `{ body: ReadableStream; contentType: string | undefined; size: number | undefined } | null`.

### GET download — filename con codiceFisico ✅
Riga 59:
```ts
const filename = `${agenzia?.codiceFisico ?? 'dvr'}_DVR.pdf`;
```
Usa `agenzie.codiceFisico` dalla tabella DB. Fallback a `'dvr'` se l'agenzia non esiste (ID orfano). Conforme alla spec `{codiceFisico}_DVR.pdf`.

### GET download — 409 se !ready ✅
I casi non-ready restituiscono tutti `json({ state }, { status: 409 })`. Conforme.

### POST — niente doppio trigger ✅
Righe 75-77:
```ts
if (status.state === 'generating') {
    return json({ state: 'generating' }, { status: 202 });
}
```
Se già `generating`, ritorna `202` senza chiamare `markGenerating` né `triggerPdfGeneration`. Conforme alla spec.

### POST — sequenza markGenerating → triggerPdfGeneration ✅
Righe 79-81:
```ts
await markGenerating(id, status.currentHash);
await triggerPdfGeneration(id);
return json({ state: 'generating' }, { status: 202 });
```
Ordine corretto: prima si scrive lo stato in DB, poi si dispatch il job GitHub Actions. Se `triggerPdfGeneration` lancia (token mancante, errore HTTP), l'eccezione propagherà e SvelteKit restituirà un 500. Il DB sarà in stato `generating` ma il job non è partito — stale lock. Questo non è un finding critico per l'uso previsto, ma è una fragilità: nessun cleanup se `triggerPdfGeneration` fallisce. **Minor** (edge case reale solo se il token manca in prod).

### ID non numerico → 400 ✅
Entrambi GET e POST: `parseInt` + `Number.isNaN` guard. Conforme.

---

## Strengths

- **Logica canServe chiara**: la condizione a riga 43-45 è leggibile e rispecchia esattamente la spec `allowStale` senza trucchi. Meglio del brief pseudo-code.
- **Pattern streaming identico ad allegati**: zero divergenza dal pattern già in prod. Buona disciplina.
- **Commento d'intestazione accurato**: il JSDoc a riga 17-22 descrive esattamente i 3 verbi e i loro comportamenti, utile per chi legge senza conoscere la spec.
- **Naming pulito**: `agenzia`, `canServe`, `allowStale` — nessuna abbreviazione oscura.
- **Gestione getObject null esplicita** (riga 57): non lascia propagare undefined, ritorna 409.

---

## Issues

### Minor — Postura auth inconsistente con allegati
**Severità: Minor**  
**File:** `app/src/routes/api/dvr/[id]/pdf/+server.ts` (non presente)  
vs `app/src/routes/api/allegati/[id]/+server.ts:18`

L'hook globale protegge la route, ma tutti gli altri endpoint API di questa app che servono dati del cliente hanno anche una guardia in-handler (`if (!locals.user) throw error(401, ...)`). L'assenza qui non crea un buco di sicurezza, ma:
1. Un client fetch() su un endpoint non autenticato riceve 302→HTML invece di 401 (comportamento inaspettato per API).
2. Se in futuro `/api/dvr/...` venisse spostato in un sub-path con regole diverse, questo endpoint non avrebbe fallback.

**Fix suggerito**: aggiungere `if (!locals.user) throw error(401, 'Unauthorized');` all'inizio di entrambi GET e POST, esattamente come `allegati/[id]/+server.ts:18`.

---

### Minor — Stale lock se triggerPdfGeneration lancia
**Severità: Minor**  
**File:** `:79-81`

`markGenerating` scrive il DB prima di `triggerPdfGeneration`. Se il dispatch GitHub fallisce (token mancante, rate limit, network), l'agenzia rimane bloccata in stato `generating` indefinitamente senza alcun meccanismo di cleanup. La prossima POST ritornerà `202` senza ri-triggerare.

Non bloccante per l'uso normale, ma in prod con token configurato correttamente non si manifesterà. Il problema emerge solo in scenari di errore di infrastruttura.

**Fix suggerito**: wrappare in try-catch e fare rollback dello stato (o aggiornare a `error`) se `triggerPdfGeneration` lancia, prima di ritornare 202.

---

### Minor — `409 { state: 'missing' }` hardcoded vs stato DVR reale
**Severità: Minor**  
**File:** `:57`

```ts
if (!obj) return json({ state: 'missing' }, { status: 409 });
```
Il campo `state` qui è la stringa `'missing'`, non `status.state` che potrebbe essere `'ready'` (se R2 ha perso il file ma il DB dice ready). Il client vede `state: 'missing'` per un'agenzia che internamente è `ready`, potenzialmente causando confusione nel frontend. 

**Fix suggerito**: `return json({ state: status.state, r2Missing: true }, { status: 409 })` oppure semplicemente `return json({ state: status.state }, { status: 409 })` (coerenza con gli altri casi 409).

---

### Minor — Content-Length non propagato (già noto al implementer)
**Severità: Minor** (già dichiarato nel report come concern noto)  
**File:** `:60-65`

`obj.size` è disponibile ma non viene passato come `Content-Length`. Nessun impatto funzionale, ma impedisce progress bar durante download di PDF grandi.

---

## Assessment

**Spec compliance: ✅ COMPLETA** — tutte e 3 le modalità (status/download/POST), tutti i guard (ID non numerico, canServe, allowStale, doppio trigger, getObject null) implementati correttamente.

**Auth check: PROTETTO** — l'hook globale copre `/api/dvr/[id]/pdf`; nessun dato esposto a non autenticati. Inconsistenza di stile rispetto ad allegati (guard in-handler mancante = Minor).

**Conteggio issues:**
- Critical: 0
- Important: 0
- Minor: 4 (auth in-handler, stale lock, state hardcoded, Content-Length)

**Task quality: Approved**

L'implementazione è corretta, conforme alla spec, e usa i pattern già consolidati nel progetto. I 4 Minor sono tutti miglioramenti difensivi o di UX, nessuno blocca il funzionamento. Il check `npm run check` riporta 0 errori. Approvato per merge.