Skip to content

Commit 52a4475

Browse files
committed
Fix webhook payload encoding failure
Add a fail-closed guard in WebhookChannel when alert payloads cannot be serialized as JSON. Non-serializable payloads are no longer signed, sent, or reported as delivered. Add regression coverage for invalid UTF-8 and non-finite float payloads. Extract the canonical Webhook Channel spec and archive the pilot artifacts.
1 parent c4d8321 commit 52a4475

11 files changed

Lines changed: 825 additions & 0 deletions

File tree

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
# Analyze — webhook-json-encode-guard
2+
3+
> Artefatto storico archiviato. L'`analyze` verifica la **coerenza documentale** del change:
4+
> che codice, test, spec e regole di progetto non si contraddicano. Non è validazione del
5+
> comportamento reale — quella proviene dal RED eseguito (vedi `runtime-red-probe.md`).
6+
7+
## Coerenza con le regole di progetto (CLAUDE.md)
8+
9+
- **No singleton / static / final:** il change non introduce metodi/proprietà static né classi/metodi final. `WebhookChannel` resta non-final, `send()` resta metodo d'istanza. ✅
10+
- **Sicurezza — escape/sanitize:** il guard non introduce input/output utente nuovo; il messaggio d'errore è una stringa fissa localizzata via `__()`. ✅
11+
- **Anti-SSRF:** l'outbound HTTP resta interamente su `HttpClientInterface::post()`; il guard anzi *previene* una chiamata su corpo malformato. Nessuna chiamata HTTP diretta introdotta. ✅
12+
- **WPCS:** indentazione tab, Allman, PHPDoc invariata; `composer phpcs` 0 errori. ✅
13+
- **TDD:** test scritti prima (RED osservato), poi fix (GREEN). ✅
14+
15+
## Coerenza tra artefatti
16+
17+
- `current-state.md` (bug) ↔ `change-delta.md` (delta) ↔ `specs/channels/webhook.md` (stato): la sezione "Codifica del payload → Fail-closed" della spec descrive esattamente l'esito del guard implementato.
18+
- `DR-WEBHOOK-001` (active) registra la scelta fail-closed e indica cosa aggiornare se in futuro si scegliesse la normalizzazione: spec, sezione Codifica, Contratto di ritorno, test. Coerente con il codice.
19+
- Il "Contratto di ritorno" della spec (`success`/`error`) corrisponde alla shape ritornata da `send()` nel ramo guard e nei rami esistenti.
20+
- La sezione "Verifica richiesta" della spec elenca rischi coperti dai test presenti (happy path, firma, fail-closed non inviato, errore trasporto propagato).
21+
22+
## Punti di attenzione documentale
23+
24+
- `package-lock.json` riporta un bump `version 0.5.0 → 0.6.2` causato da `npm install` (sincronizzazione da `package.json`), **estraneo** a questo change. Va escluso. È un sintomo del finding di review sul disallineamento di versione, da trattare separatamente.
25+
- La spec dichiara la **delega** al client HTTP per anti-SSRF/timeout/limiti, senza certificarne il livello: coerente con il principio "la spec del canale documenta la delega, non certifica un altro layer".
26+
27+
## Limite dell'analyze
28+
29+
Questa analisi è documentale. La prova che il comportamento reale sia quello descritto viene
30+
**solo** dal RED eseguito e dal GREEN eseguito sul codice reale, non dalla coerenza tra documenti
31+
né dal consenso tra reviewer.
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# Change delta — webhook-json-encode-guard
2+
3+
> Artefatto storico archiviato (linguaggio di cambiamento). Il delta documenta la transizione
4+
> dallo stato precedente a quello corrente; **non è fonte attiva**.
5+
> La fonte attiva del comportamento è [specs/channels/webhook.md](../../../specs/channels/webhook.md).
6+
7+
## Obiettivo del change
8+
9+
Rendere `WebhookChannel::send()` fail-closed quando il payload non è serializzabile in JSON,
10+
in modo che un payload malformato non venga firmato, inviato e riportato come consegnato.
11+
12+
## Delta di comportamento
13+
14+
| Aspetto | Prima | Dopo |
15+
| --- | --- | --- |
16+
| `json_encode()` ritorna `false` | firma su stringa vuota, POST del corpo spurio `"false"`, esito `success=true` su 2xx | nessuna firma, nessuna POST, esito `success=false` con `error` non vuoto |
17+
| Payload valido (happy path) | invariato | invariato |
18+
| Firma HMAC per payload valido | invariata | invariata |
19+
20+
## Delta di codice (produzione)
21+
22+
`src/Channels/WebhookChannel.php` — guard aggiunto subito dopo `json_encode()` e **prima** di
23+
`hash_hmac()` e `http_client->post()`:
24+
25+
$raw_body = json_encode( $payload );
26+
27+
if ( false === $raw_body ) {
28+
return [
29+
'success' => false,
30+
'error' => __( 'Failed to encode webhook payload.', 'ops-health-dashboard' ),
31+
];
32+
}
33+
34+
## Delta di test
35+
36+
`tests/Unit/Channels/WebhookChannelTest.php`:
37+
38+
- helper `create_http_client_spy( &$called )` — mock `post()` con `andReturnUsing` che registra l'invocazione e ritorna successo (così `send()` ritorna sempre normalmente);
39+
- `test_send_returns_failure_when_payload_contains_invalid_utf8` — payload `[ 'x' => "\xB1\x31" ]`;
40+
- `test_send_returns_failure_when_payload_contains_non_finite_float` — payload `[ 'value' => NAN ]`.
41+
42+
## Decisione di prodotto
43+
44+
Fail-closed (non normalizzazione/coercizione). Registrata come `DR-WEBHOOK-001` (active) nella
45+
spec canonica.
46+
47+
## Confini espliciti (non toccati)
48+
49+
`HttpClient`, altri canali, `AlertManager`, storage/settings/admin, anti-SSRF/redaction,
50+
metadati composer/package. Nessun refactor. Nessun uso di `wp_json_encode()`, `@json_encode()`,
51+
`set_error_handler()`.
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# Closeout — webhook-json-encode-guard
2+
3+
> Artefatto storico archiviato. Evidenze red/green e stato di chiusura del change.
4+
5+
## RED evidence
6+
7+
Comando (PRIMA del fix): `vendor/bin/phpunit --testsuite=unit --filter WebhookChannelTest`
8+
9+
Tests: 17, Failures: 2. (gli altri 15 verdi)
10+
11+
1) test_send_returns_failure_when_payload_contains_invalid_utf8
12+
"post() must NOT be called when the payload cannot be encoded"
13+
Failed asserting that true is false. (WebhookChannelTest.php:556)
14+
2) test_send_returns_failure_when_payload_contains_non_finite_float
15+
"post() must NOT be called when the payload cannot be encoded"
16+
Failed asserting that true is false. (WebhookChannelTest.php:591)
17+
18+
Motivo del fallimento: **corretto**. `json_encode()` ritorna `false` silenziosamente (nessun
19+
Warning/ErrorException), `send()` prosegue e chiama `post()` → lo spy imposta `$called=true`.
20+
RED per "post() was called", coerente con la Runtime RED Probe.
21+
22+
## GREEN evidence
23+
24+
- `composer test:unit -- --filter WebhookChannelTest` → OK (17 tests, 28 assertions)
25+
- `composer phpcs` → 35/35, 0 errori
26+
- `composer analyse` (PHPStan level 6) → [OK] No errors
27+
- Suite unit completa → OK (639 tests, 1508 assertions); 1 risky **pre-esistente**
28+
(`CheckRunnerTest::test_clear_results_deletes_from_storage`, file non toccato — vedi
29+
`pilot-retrospective.md › Follow-up debt`).
30+
31+
## Changed files
32+
33+
- `src/Channels/WebhookChannel.php` (+9) — guard fail-closed in `send()`.
34+
- `tests/Unit/Channels/WebhookChannelTest.php` (+98) — 2 test + helper `create_http_client_spy()`.
35+
- `specs/channels/webhook.md` (new) — spec canonica (fonte attiva).
36+
- `changes/archive/2026-06-21-webhook-json-encode-guard/` (new) — artefatti storici.
37+
38+
## Scope deviations
39+
40+
none. Non modificati: `HttpClient`, altri canali, `AlertManager`, storage/settings/admin,
41+
anti-SSRF/redaction, metadati composer/package. Assenti nel diff: `wp_json_encode()`,
42+
`@json_encode()`, `set_error_handler()`.
43+
44+
## Excluded / unrelated
45+
46+
- `package-lock.json` — bump `0.5.0 → 0.6.2` da `npm install`, estraneo al change. **Escluso**
47+
dal commit (lasciato `M` nel working tree per decisione del maintainer).
48+
49+
## Versioning decision (maintainer)
50+
51+
Da versionare nel commit del change (branch dedicato):
52+
53+
- `src/Channels/WebhookChannel.php`, `tests/Unit/Channels/WebhookChannelTest.php` (codice + test);
54+
- `specs/channels/webhook.md` (spec canonica);
55+
- `changes/archive/2026-06-21-webhook-json-encode-guard/**` (artefatti d'archivio);
56+
- `CODE_REVIEW_v0.6.2.md`, `PILOT_CANDIDATE_REPORT.md` (report del programma pilota).
57+
58+
Escluso dal commit: `package-lock.json` (lasciato com'è).
59+
60+
## Closeout readiness
61+
62+
READY → ARCHIVED. Commit in attesa di via libera esplicito del maintainer (no commit/tag finora).
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Current state (pre-change) — webhook-json-encode-guard
2+
3+
> Artefatto storico archiviato. Cattura lo stato del canale Webhook **prima** del change.
4+
> La fonte attiva del comportamento corrente è [specs/channels/webhook.md](../../../specs/channels/webhook.md).
5+
6+
## Componente
7+
8+
`OpsHealthDashboard\Channels\WebhookChannel` — canale di alert che invia una HTTP POST JSON
9+
con firma HMAC opzionale, tramite `HttpClientInterface::post()`.
10+
11+
## Comportamento osservato prima del change
12+
13+
In `WebhookChannel::send()` il payload veniva serializzato con `json_encode( $payload )`
14+
senza verificare il valore di ritorno. In caso di fallimento della serializzazione:
15+
16+
- `json_encode()` ritorna `false` (silenziosamente, senza emettere PHP Warning);
17+
- `hash_hmac( 'sha256', false, $secret )` calcolava la firma sulla stringa vuota;
18+
- `HttpClient::post( $url, false, ... )` riceveva `false` e lo ri-serializzava nella stringa
19+
letterale `"false"`, inviata come corpo;
20+
- su risposta HTTP 2xx, `send()` restituiva `success = true`.
21+
22+
Effetto netto: un payload non rappresentabile in JSON veniva trasmesso come corpo spurio,
23+
con una firma che non corrispondeva al corpo, e l'esito veniva riportato come consegnato.
24+
La perdita dell'alert era completamente silenziosa.
25+
26+
## Causa scatenante realistica
27+
28+
Il payload include `message` e `details` provenienti dall'output dei check. `ErrorLogCheck`
29+
legge righe di log via `fread()`; byte UTF-8 non validi (path latin-1, frammenti binari di
30+
stack trace) sopravvivono alla redazione (solo regex, nessuna normalizzazione UTF-8) e
31+
raggiungono il payload. `json_encode()` ritorna `false` (JSON_ERROR_UTF8) su UTF-8 non valido,
32+
e analogamente su valori float non finiti (`NAN`/`INF`).
33+
34+
## Origine
35+
36+
Code review v0.6.2 — finding correctness (medium) su `WebhookChannel.php:104-112`.
37+
Selezione come pilota in [PILOT_CANDIDATE_REPORT.md](../../../PILOT_CANDIDATE_REPORT.md) (Candidate 1).
38+
39+
## Vincoli rilevanti dello stato corrente
40+
41+
- PHP 7.4+ (osservato in CI/dev: 8.3.31).
42+
- `phpunit.xml.dist`: `convertWarningsToExceptions="true"` — ma `json_encode()` non emette
43+
warning, quindi non viene convertito in eccezione (verificato con probe).
44+
- Tutto l'outbound HTTP deve passare per `HttpClientInterface` (regola anti-SSRF di progetto).
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# Key lesson
2+
3+
I gate che hanno funzionato sono stati quelli ancorati alla verità di terra:
4+
5+
- eseguire il test reale;
6+
- osservare il RED reale;
7+
- leggere l'artefatto realmente prodotto;
8+
- verificare il working tree reale.
9+
10+
Il consenso tra reviewer o tra AI non è validazione.
11+
12+
Il maintainer resta il gate, soprattutto quando più reviewer concordano: modelli diversi
13+
possono condividere lo stesso punto cieco.
14+
15+
Questa lezione va considerata parte del workflow:
16+
17+
- `analyze` verifica coerenza documentale;
18+
- il RED eseguito verifica il comportamento reale;
19+
- la review del file prodotto verifica l'artefatto reale;
20+
- nessun passaggio AI↔AI sostituisce il giudizio del maintainer.
21+
22+
---
23+
24+
# Pilot retrospective — webhook-json-encode-guard
25+
26+
## Cosa è andato bene
27+
28+
- Il pilota era piccolo, localizzato e a basso rischio: un solo metodo di produzione, due test, una spec nuova.
29+
- La Runtime RED Probe ha smontato un'assunzione teorica (il timore che `convertWarningsToExceptions` rendesse il RED "sbagliato"): il probe reale ha mostrato che `json_encode()` non emette warning, quindi il guard `json_encode` semplice era sufficiente, senza `@`/`set_error_handler`/`wp_json_encode`.
30+
- Il RED ha fallito per il motivo giusto ("post() was called"), confermato sul codice reale prima del fix.
31+
- I gate di qualità (phpcs, phpstan, suite unit completa) erano già a baseline pulita, rendendo il GREEN inequivocabile.
32+
33+
## Cosa ha richiesto correzione in closeout
34+
35+
- La spec iniziale "certificava" le protezioni del client HTTP nella sezione Trasporto: corretta in linguaggio di **delega**, non di certificazione.
36+
- La spec doveva contenere una **Decision record attiva** (`DR-WEBHOOK-001`), non solo prosa descrittiva.
37+
- Necessità di mantenere il Markdown render-safe (nessun fence annidato).
38+
39+
## Punto cieco condiviso
40+
41+
Più passaggi di analisi (incluse revisioni AI) possono concordare e restare comunque
42+
incompleti. La validazione è arrivata dall'esecuzione reale e dal giudizio del maintainer,
43+
non dall'accordo tra revisori.
44+
45+
## Follow-up debt
46+
47+
- **Risky test pre-esistente:** `OpsHealthDashboard\Tests\Unit\Services\CheckRunnerTest::test_clear_results_deletes_from_storage`
48+
(`tests/Unit/Services/CheckRunnerTest.php:309`) — segnalato da PHPUnit come "This test did not
49+
perform any assertions". **Pre-esistente**, in un file non toccato da questo change.
50+
**Non corretto qui** (fuori scope). Da affrontare come change separato.
51+
- **Disallineamento versione `package-lock.json`** (`0.5.0``0.6.2`, sincronizzato da
52+
`npm install`): sintomo del finding di review sul disallineamento delle versioni tra
53+
`package.json`, `composer.json`, header plugin, `readme.txt`. Da trattare separatamente.
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Plan — webhook-json-encode-guard
2+
3+
> Artefatto storico archiviato. Piano approvato dal maintainer prima dell'implementazione.
4+
5+
## Approccio
6+
7+
TDD red→green, scope minimo, fail-closed. Decisione validata dalla Runtime RED Probe
8+
(verità di terra: probe reale + RED reale), non da consenso tra reviewer.
9+
10+
## Sequenza approvata
11+
12+
1. **RED test (UTF-8 invalido)** — aggiungere `test_send_returns_failure_when_payload_contains_invalid_utf8` con spy su `post()` e flag `$called`.
13+
2. **RED test (NAN/INF)** — aggiungere `test_send_returns_failure_when_payload_contains_non_finite_float`.
14+
3. **Verifica RED**`composer test:unit -- --filter WebhookChannelTest`; i nuovi test devono fallire perché `post()` viene chiamato (non per Warning/ErrorException).
15+
4. **Fix guard** — in `WebhookChannel::send()`, dopo `json_encode()` e prima di `hash_hmac()`/`post()`, ritornare fail-closed se `false === $raw_body`.
16+
5. **Verifica GREEN**`composer test:unit -- --filter WebhookChannelTest`, `composer phpcs`, `composer analyse`.
17+
6. **Closeout + bozza spec** — preparare evidenze, `specs/channels/webhook.md` in linguaggio di stato; nessun archive finché non approvato.
18+
19+
## Vincoli decisi
20+
21+
- Fix limitato a `WebhookChannel::send()`.
22+
- Niente `wp_json_encode()`, `@json_encode()`, `set_error_handler()`.
23+
- Messaggio d'errore utente generico e localizzato (no `json_last_error_msg()` esposto).
24+
- Nessun commit/tag durante il pilota.
25+
26+
## Rischio mitigato
27+
28+
Un payload malformato non deve essere inviato e riportato come consegnato.

0 commit comments

Comments
 (0)