diff --git a/.zyra/issue-122-comment.md b/.zyra/issue-122-comment.md new file mode 100644 index 0000000..753edfa --- /dev/null +++ b/.zyra/issue-122-comment.md @@ -0,0 +1,71 @@ +Reactie voor GitHub-issue #122 (ginkelsoft-development/buildora) +================================================================= + +LET OP: vanuit deze werkplaats is er geen GitHub-toegang (geen `gh` CLI, +geen credentials) en kan deze reactie dus niet zelf geplaatst worden. Plak +onderstaande tekst als comment op issue #122, bijvoorbeeld met: + + gh issue comment 122 --repo ginkelsoft-development/buildora --body-file .zyra/issue-122-comment.md + +--- comment hieronder dit lijntje --- + +**Audit-conclusie: kwetsbaar (stored XSS, OWASP A03:2021 - Injection).** +Fix doorgevoerd op branch `zyra/issue-122-buildora-onderzoeken-vaststell-ce181a48`. + +**Bevinding** + +`RichTextField` en `EditorField` (`src/Fields/Types/RichTextField.php`, +`EditorField.php`) hadden vóór deze fix geen enkele sanitization: de ruwe +request-waarde ging ongewijzigd de database in. Meerdere plekken renderen +die opgeslagen waarde vervolgens **unescaped**: + +- `resources/views/components/input/richtext.blade.php:4` — `{!! $value !!}` +- `resources/views/form.blade.php` rendert per fieldtype de bijbehorende + `components.input.{type}`-view (dus ook `richtext.blade.php`) met de + opgeslagen/`old()`-waarde, op zowel de create- als edit-pagina. +- `resources/views/components/datatable.blade.php:228` — + ``: `RowFormatter::format()` + (`src/Datatable/RowFormatter.php:41-45`) zet de **rauwe** (niet + ge-escapte) fieldwaarde in de JSON-respons van de datatable-endpoint, en + Alpine's `x-html` injecteert die vervolgens als innerHTML in de + index-tabel van élk resource. Dit raakt overigens niet alleen + RichTextField/EditorField maar in principe elk veldtype — een aparte, + bredere bevinding die het waard is om los op te pakken. + +**Reproductie**: waarde `` opslaan in een +EditorField/RichTextField kwam vóór de fix ongestript terug in +`richtext.blade.php`'s `{!! $value !!}` en in de datatable-JSON — in beide +gevallen een klassieke stored-XSS-uitvoering in de browser van elke +gebruiker (incl. admins) die de pagina bekijkt. + +**Fix** + +- Nieuwe hook `Field::prepareForStorage(mixed $value): mixed` (identity by + default) die de controller vlak vóór `create()`/`update()` aanroept met + de ruwe request-waarde per veld + (`src/Http/Controllers/BuildoraController.php`). +- Nieuwe trait `Ginkelsoft\Buildora\Fields\Traits\SanitizesHtml`, gebruikt + door `RichTextField` en `EditorField`, die `prepareForStorage()` + overschrijft en de waarde door `ezyang/htmlpurifier` haalt: + - `->sanitize(bool $enabled = true)` — **standaard aan**. + - `->allowedTags(array $tags)` — configureerbare allow-list + (HTMLPurifier `HTML.Allowed`-syntax), met een veilige default + (basis-opmaak: `p`, `strong`, `em`, `ul/ol/li`, `a[href|title|target|rel]`, + tabellen, etc. — géén `script`, geen event-handler-attributen, geen + `javascript:`-URI's). +- Regressietests in + `tests/Unit/Fields/RichTextAndEditorFieldXssTest.php` bevestigen dat + ``, `onerror=`-attributen en `javascript:`-URI's + worden gestript, dat gewone opmaak behouden blijft, dat sanitization + standaard aan staat, dat `allowedTags()` configureerbaar is, dat + `sanitize(false)` expliciet uitgezet kan worden, en dat gewone + fieldtypes (zonder de trait) ongewijzigd blijven werken. + +**Nog open**: de bredere x-html/datatable-bevinding hierboven (geldt voor +alle fieldtypes, niet alleen RichText/Editor) valt buiten de scope van dit +issue en is niet in deze PR meegenomen. + +Draai lokaal: +`vendor/bin/phpunit tests/Unit/Fields/RichTextAndEditorFieldXssTest.php` +Volledige suite: `vendor/bin/phpunit --testdox` +Proefdraaien van de package (Orchestra Testbench): `.zyra/proef.sh` diff --git a/composer.json b/composer.json index fe7f5fa..1b6fd93 100644 --- a/composer.json +++ b/composer.json @@ -17,12 +17,13 @@ "require": { "php": "^8.1|^8.2|^8.3|^8.4", "bacon/bacon-qr-code": "^3.0", + "binarytorch/larecipe": "^2.0", + "ezyang/htmlpurifier": "^4.17", "laravel/framework": "^10.0|^11.0|^12.0", "livewire/livewire": "^3.0", "maatwebsite/excel": "^3.1", "pragmarx/google2fa": "^8.0", - "spatie/laravel-permission": "^6.16", - "binarytorch/larecipe": "^2.0" + "spatie/laravel-permission": "^6.16" }, "extra": { "buildora-version": "1.0.37", diff --git a/src/Fields/Field.php b/src/Fields/Field.php index 74e72dd..019a222 100644 --- a/src/Fields/Field.php +++ b/src/Fields/Field.php @@ -211,6 +211,29 @@ public function hideOnForms(): self return $this->hideFromCreate()->hideFromEdit(); } + /** + * Transform a raw incoming request value before it is persisted. + * + * Let op: dit is bewust een andere hook dan `setValue()`, dat in dit + * package al een gevestigde betekenis heeft (het vullen van + * `$field->value` vanuit een model/record voor weergave, zie o.a. + * FieldManager en de meeste Field-subklassen). `prepareForStorage()` + * doet het omgekeerde: het krijgt de ruwe waarde uit het request en + * geeft de waarde terug die opgeslagen mag worden. + * + * De basisimplementatie is de identity-functie. Fieldtypes die + * inkomende waarden vóór het opslaan moeten transformeren/saniteren + * (bv. RichTextField/EditorField die onveilige HTML strippen om stored + * XSS te voorkomen) overschrijven deze methode. + * + * @param mixed $value + * @return mixed + */ + public function prepareForStorage(mixed $value): mixed + { + return $value; + } + /** * Convert the field into an array representation. * diff --git a/src/Fields/Traits/SanitizesHtml.php b/src/Fields/Traits/SanitizesHtml.php new file mode 100644 index 0000000..cdd4019 --- /dev/null +++ b/src/Fields/Traits/SanitizesHtml.php @@ -0,0 +1,138 @@ +sanitize()` and + * `->allowedTags()`. + */ +trait SanitizesHtml +{ + /** + * Whether HTML sanitization is enabled for this field. Defaults to true + * ("secure by default" - least privilege for stored HTML). + * + * @var bool + */ + protected bool $sanitizeHtml = true; + + /** + * The tags (optionally with allowed attributes, using HTMLPurifier's + * `HTML.Allowed` syntax, e.g. `a[href|title]`) that survive + * sanitization. Anything not in this list (including `) never reaches the database. + * + * @param mixed $value + * @return mixed + */ + public function prepareForStorage(mixed $value): mixed + { + if (! $this->sanitizeHtml || ! is_string($value) || $value === '') { + return $value; + } + + return $this->purifyHtml($value); + } + + /** + * Run the value through HTMLPurifier using the configured allow-list. + * + * @param string $html + * @return string + */ + protected function purifyHtml(string $html): string + { + $config = HTMLPurifier_Config::createDefault(); + $config->set('HTML.Allowed', implode(',', $this->allowedTags)); + $config->set('URI.AllowedSchemes', ['http' => true, 'https' => true, 'mailto' => true]); + // Geen definitie-cache op schijf nodig (en dus geen schrijfbare map vereist). + $config->set('Cache.DefinitionImpl', null); + + return (new HTMLPurifier($config))->purify($html); + } +} diff --git a/src/Fields/Types/EditorField.php b/src/Fields/Types/EditorField.php index 02da460..c6e275f 100644 --- a/src/Fields/Types/EditorField.php +++ b/src/Fields/Types/EditorField.php @@ -3,12 +3,20 @@ namespace Ginkelsoft\Buildora\Fields\Types; use Ginkelsoft\Buildora\Fields\Field; +use Ginkelsoft\Buildora\Fields\Traits\SanitizesHtml; /** * Represents a WYSIWYG HTML editor field using a rich editor like CKEditor. + * + * De WYSIWYG-HTML uit deze editor komt rechtstreeks van de gebruiker en + * wordt elders ongesanitized gerenderd. Daarom wordt de inkomende waarde + * standaard door HTMLPurifier gehaald (zie {@see SanitizesHtml}) vlak vóór + * het opslaan, met een configureerbare allow-list van tags/attributen. */ class EditorField extends Field { + use SanitizesHtml; + /** * Create a new EditorField instance. * diff --git a/src/Fields/Types/RichTextField.php b/src/Fields/Types/RichTextField.php index 082d6f0..d1993b5 100644 --- a/src/Fields/Types/RichTextField.php +++ b/src/Fields/Types/RichTextField.php @@ -3,12 +3,21 @@ namespace Ginkelsoft\Buildora\Fields\Types; use Ginkelsoft\Buildora\Fields\Field; +use Ginkelsoft\Buildora\Fields\Traits\SanitizesHtml; /** * Represents a field for rendering rich text (HTML) content in views only. + * + * De opgeslagen waarde wordt elders ongesanitized gerenderd (o.a. + * `{!! $value !!}`), wat zonder maatregelen tot stored XSS leidt. Daarom + * wordt de inkomende waarde standaard door HTMLPurifier gehaald (zie + * {@see SanitizesHtml}) vlak vóór het opslaan, met een configureerbare + * allow-list van tags/attributen. */ class RichTextField extends Field { + use SanitizesHtml; + /** * Create a new RichTextField instance. * diff --git a/src/Http/Controllers/BuildoraController.php b/src/Http/Controllers/BuildoraController.php index 29f7056..4918bba 100644 --- a/src/Http/Controllers/BuildoraController.php +++ b/src/Http/Controllers/BuildoraController.php @@ -74,6 +74,15 @@ public function store(Request $request, string $model): RedirectResponse $finalData = array_merge($allowedRequestData, $validatedData); foreach ($resource->resolveFields($modelInstance) as $field) { + if (array_key_exists($field->name, $finalData)) { + // Geeft elk fieldtype de kans om de ruwe request-waarde te + // transformeren vóór het opslaan (bv. RichTextField en + // EditorField saniteren hier hun WYSIWYG-HTML om stored XSS + // te voorkomen). Field::prepareForStorage() is voor de + // meeste fieldtypes de identity-functie. + $finalData[$field->name] = $field->prepareForStorage($finalData[$field->name]); + } + if ($field instanceof FileField && $request->hasFile($field->name)) { $uploadedFile = $request->file($field->name); $disk = $field->getDisk() ?? 'public'; @@ -163,6 +172,13 @@ public function update(Request $request, string $model, int $id): RedirectRespon $finalData = array_merge($allowedRequestData, $validatedData); foreach ($resource->resolveFields($modelInstance) as $field) { + if (array_key_exists($field->name, $finalData)) { + // Zie store(): transformeert/saniteert de ruwe request-waarde + // vóór het opslaan (o.a. stored-XSS-preventie voor + // RichTextField/EditorField). + $finalData[$field->name] = $field->prepareForStorage($finalData[$field->name]); + } + if ($field instanceof FileField && $request->hasFile($field->name)) { $uploadedFile = $request->file($field->name); $disk = $field->getDisk() ?? 'public'; diff --git a/tests/Unit/Fields/RichTextAndEditorFieldXssTest.php b/tests/Unit/Fields/RichTextAndEditorFieldXssTest.php new file mode 100644 index 0000000..0a2235c --- /dev/null +++ b/tests/Unit/Fields/RichTextAndEditorFieldXssTest.php @@ -0,0 +1,167 @@ +alert(1)` opgeslagen via deze fieldtypes kwam + * dus ongestript terug in de gerenderde output = stored XSS (OWASP + * A03:2021 - Injection). + * + * Fix: BuildoraController::store()/update() roept nu + * `$field->prepareForStorage()` aan met de ruwe request-waarde vlak vóór + * het opslaan. RichTextField en + * EditorField gebruiken de `SanitizesHtml`-trait om die waarde standaard + * (`sanitize(true)` is de default) door HTMLPurifier te halen met een + * configureerbare allow-list (`->allowedTags()`), zodat `'; + + $sanitized = $field->prepareForStorage($payload); + + $this->assertStringNotContainsString('assertStringNotContainsString('alert(1)', $sanitized); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function it_strips_an_inline_event_handler_disguised_as_a_normal_tag(string $fieldClass): void + { + $field = $fieldClass::make('content'); + + $payload = ''; + + $sanitized = $field->prepareForStorage($payload); + + $this->assertStringNotContainsString('onerror', $sanitized); + $this->assertStringNotContainsString('alert(', $sanitized); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function it_strips_a_javascript_uri_from_a_link(string $fieldClass): void + { + $field = $fieldClass::make('content'); + + $payload = 'klik'; + + $sanitized = $field->prepareForStorage($payload); + + $this->assertStringNotContainsString('javascript:', $sanitized); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function it_keeps_ordinary_formatting_from_the_default_allow_list(string $fieldClass): void + { + $field = $fieldClass::make('content'); + + $payload = '

Hallo wereld

'; + + $sanitized = $field->prepareForStorage($payload); + + $this->assertStringContainsString('

', $sanitized); + $this->assertStringContainsString('wereld', $sanitized); + $this->assertStringContainsString('

  • een
    • ', $sanitized); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function sanitize_is_enabled_by_default(string $fieldClass): void + { + $field = $fieldClass::make('content'); + + $this->assertTrue($field->isSanitizeEnabled()); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function allowed_tags_are_configurable_per_field(string $fieldClass): void + { + $field = $fieldClass::make('content')->allowedTags(['b']); + + $sanitized = $field->prepareForStorage('

    tekst

    vet'); + + //

    staat niet meer op de (nu strengere) allow-list en verdwijnt, + // maar de inhoud blijft staan; is nog wel toegestaan. + $this->assertStringNotContainsString('

    ', $sanitized); + $this->assertStringContainsString('vet', $sanitized); + $this->assertStringContainsString('tekst', $sanitized); + } + + /** + * @dataProvider richTextAndEditorFieldProvider + * @test + */ + public function sanitize_can_be_explicitly_disabled(string $fieldClass): void + { + $field = $fieldClass::make('content')->sanitize(false); + + $payload = ''; + + $this->assertFalse($field->isSanitizeEnabled()); + $this->assertSame($payload, $field->prepareForStorage($payload)); + } + + /** + * Sanity check: het gedrag van gewone fieldtypes (die de sanitization + * trait niet gebruiken) blijft ongewijzigd - Field::prepareForStorage() + * is de identity-functie. + * + * @test + */ + public function a_regular_field_leaves_the_value_untouched(): void + { + $field = TextField::make('title'); + + $payload = ''; + + $this->assertSame($payload, $field->prepareForStorage($payload)); + } +}