Issue #122 (buildora) onderzoeken: vaststellen of RichTextField/EditorField kwetsbaar zijn voor stored XSS doordat WYSIWYG-HTML ongesanitized wordt opgeslagen en unescaped gerenderd, en zo nodig fixen

.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` —
+  `<td x-html="formatCell(row, col.name)">`: `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 `<script>alert(1)</script>` 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
+  `<script>alert(1)</script>`, `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`

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",

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.
      *

src/Fields/Traits/SanitizesHtml.php

@@ -0,0 +1,138 @@
+<?php
+
+namespace Ginkelsoft\Buildora\Fields\Traits;
+
+use HTMLPurifier;
+use HTMLPurifier_Config;
+
+/**
+ * Trait SanitizesHtml
+ *
+ * Sanitizes WYSIWYG/HTML input before it is persisted, so stored values can
+ * safely be rendered unescaped (e.g. via `{!! $value !!}`) without exposing
+ * the application to stored XSS (OWASP A03:2021 - Injection).
+ *
+ * Sanitization runs in {@see self::prepareForStorage()}, which the
+ * controller calls with the raw request value right before the model is
+ * created/updated - i.e. "before save". Note this is a different hook than
+ * `setValue()`, which in this package already means "populate `$value` for
+ * display from a model" (see Field-subklassen elders). It is enabled by
+ * default and can be configured per field instance via `->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 <script>,
+     * event handlers such as onerror/onclick, and javascript: URIs) is
+     * stripped.
+     *
+     * @var array<int, string>
+     */
+    protected array $allowedTags = [
+        'p', 'br', 'hr',
+        'strong', 'b', 'em', 'i', 'u', 's', 'sub', 'sup',
+        'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
+        'ul', 'ol', 'li',
+        'blockquote', 'code', 'pre',
+        'a[href|title|target|rel]',
+        'span',
+        'table', 'thead', 'tbody', 'tr', 'th', 'td',
+    ];
+
+    /**
+     * Enable or disable HTML sanitization for this field.
+     *
+     * Disabling this is opt-in and puts the responsibility for safe
+     * rendering back on the developer - only do this when the value is
+     * fully trusted (e.g. seeded by an admin, never end-user input).
+     *
+     * @param bool $enabled
+     * @return static
+     */
+    public function sanitize(bool $enabled = true): static
+    {
+        $this->sanitizeHtml = $enabled;
+        return $this;
+    }
+
+    /**
+     * Configure the allow-list of tags (and attributes) kept after
+     * sanitization. Uses HTMLPurifier's `HTML.Allowed` syntax, e.g.
+     * `['p', 'a[href|title]', 'img[src|alt]']`.
+     *
+     * @param array<int, string> $tags
+     * @return static
+     */
+    public function allowedTags(array $tags): static
+    {
+        $this->allowedTags = $tags;
+        return $this;
+    }
+
+    /**
+     * Get the currently configured allow-list of tags.
+     *
+     * @return array<int, string>
+     */
+    public function getAllowedTags(): array
+    {
+        return $this->allowedTags;
+    }
+
+    /**
+     * Determine whether sanitization is currently enabled.
+     *
+     * @return bool
+     */
+    public function isSanitizeEnabled(): bool
+    {
+        return $this->sanitizeHtml;
+    }
+
+    /**
+     * Transform a raw incoming value before it is persisted.
+     *
+     * Called by the controller with the request value for this field right
+     * before create()/update(), so malicious markup (e.g.
+     * `<script>alert(1)</script>`) 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);
+    }
+}

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.
      *

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.
      *

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';