test: pin the README sanitizer and guard mass assignment

app/Queries/GitHubReadmeQuery.php

@@ -6,28 +6,23 @@
 
 use App\Models\User;
 use App\Services\GitHub;
+use App\Services\GitHubReadmeSanitizer;
 use Illuminate\Http\Client\HttpClientException;
 use Illuminate\Support\Facades\Cache;
-use Symfony\Component\HtmlSanitizer\HtmlSanitizer;
-use Symfony\Component\HtmlSanitizer\HtmlSanitizerAction;
-use Symfony\Component\HtmlSanitizer\HtmlSanitizerConfig;
 
 final readonly class GitHubReadmeQuery
 {
-    /**
-     * Profile READMEs are mostly badges and banners, whose long proxied image URLs add up.
-     */
-    private const int MaxLength = 200_000;
-
     /**
      * Images uploaded to GitHub are served from links signed for five minutes.
      */
     private const int SignedImagesTtl = 240;
 
     private const int FailureTtl = 300;
 
-    public function __construct(private GitHub $github)
-    {
+    public function __construct(
+        private GitHub $github,
+        private GitHubReadmeSanitizer $sanitizer,
+    ) {
         //
     }
 
@@ -80,55 +75,6 @@ private function fetch(User $user): string
             ->replaceHeaders(['Accept' => 'application/vnd.github.html+json'])
             ->get("repos/{$login}/{$login}/readme");
 
-        return $response->notFound() ? '' : $this->sanitize($response->throw()->body());
-    }
-
-    /**
-     * Never trusted as is: only plain formatting, links and images from GitHub's hosts are kept. Classes, ids and
-     * styles are dropped, so a README can't take on the page's own styles, cover it or pose as part of it.
-     */
-    private function sanitize(string $html): string
-    {
-        $config = new HtmlSanitizerConfig()
-            // Only what a README is written with: a fixed list, so attributes like srcset or name, and elements like
-            // dialog, never come along with the browser's idea of what is safe.
-            ->allowElement('a', ['href', 'title'])
-            ->allowElement('img', ['src', 'alt', 'title', 'width', 'height', 'align'])
-            ->allowElement('p', ['align'])
-            ->allowElement('div', ['align'])
-            ->allowElement('h1', ['align'])
-            ->allowElement('h2', ['align'])
-            ->allowElement('h3', ['align'])
-            ->allowElement('h4', ['align'])
-            ->allowElement('h5', ['align'])
-            ->allowElement('h6', ['align'])
-            ->allowElement('th', ['align', 'colspan', 'rowspan'])
-            ->allowElement('td', ['align', 'colspan', 'rowspan'])
-            ->allowElement('ol', ['start'])
-            ->allowElement('details', ['open']);
-
-        foreach (['article', 'span', 'ul', 'li', 'table', 'thead', 'tbody', 'tr', 'pre', 'code', 'blockquote', 'summary', 'picture', 'br', 'hr', 'b', 'strong', 'i', 'em', 'sub', 'sup', 'del', 's', 'kbd'] as $element) {
-            $config = $config->allowElement($element);
-        }
-
-        // Any other element goes, but what it holds stays: GitHub's own wrappers like g-emoji or themed-picture, and
-        // the rarer text elements like samp or dl, keep their words. Only these go with everything in them.
-        $config = $config->defaultAction(HtmlSanitizerAction::Block);
-
-        foreach (['script', 'style', 'template', 'noscript', 'iframe', 'object', 'embed', 'svg', 'math', 'dialog', 'map', 'form', 'textarea', 'select', 'button', 'head', 'title', 'source', 'video', 'audio'] as $element) {
-            $config = $config->dropElement($element);
-        }
-
-        $config = $config
-            ->allowLinkSchemes(['https', 'http', 'mailto'])
-            ->allowRelativeLinks(false)
-            ->allowMediaSchemes(['https'])
-            ->allowMediaHosts(['camo.githubusercontent.com', 'github.com', 'raw.githubusercontent.com', 'user-images.githubusercontent.com', 'private-user-images.githubusercontent.com'])
-            ->allowRelativeMedias(false)
-            ->forceAttribute('a', 'rel', 'noopener noreferrer nofollow ugc')
-            ->forceAttribute('a', 'target', '_blank')
-            ->withMaxInputLength(self::MaxLength);
-
-        return mb_trim(new HtmlSanitizer($config)->sanitize($html));
+        return $response->notFound() ? '' : $this->sanitizer->sanitize($response->throw()->body());
     }
 }

app/Services/GitHubReadmeSanitizer.php

@@ -0,0 +1,79 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Services;
+
+use Symfony\Component\HtmlSanitizer\HtmlSanitizer;
+use Symfony\Component\HtmlSanitizer\HtmlSanitizerAction;
+use Symfony\Component\HtmlSanitizer\HtmlSanitizerConfig;
+
+/**
+ * Never trusts a profile README: only plain formatting, links and images from GitHub's hosts are kept. Classes,
+ * ids and styles are dropped, so a README can't take on the page's own styles, cover it or pose as part of it.
+ *
+ * It lives on its own so the rules can be exercised directly, against the payloads that have broken sanitizers
+ * before, without going near GitHub or the database.
+ */
+final readonly class GitHubReadmeSanitizer
+{
+    /**
+     * Profile READMEs are mostly badges and banners, whose long proxied image URLs add up.
+     */
+    private const int MAX_LENGTH = 200_000;
+
+    private HtmlSanitizer $sanitizer;
+
+    public function __construct()
+    {
+        $this->sanitizer = new HtmlSanitizer($this->config());
+    }
+
+    public function sanitize(string $html): string
+    {
+        return mb_trim($this->sanitizer->sanitize($html));
+    }
+
+    private function config(): HtmlSanitizerConfig
+    {
+        $config = new HtmlSanitizerConfig()
+            // Only what a README is written with: a fixed list, so attributes like srcset or name, and elements like
+            // dialog, never come along with the browser's idea of what is safe.
+            ->allowElement('a', ['href', 'title'])
+            ->allowElement('img', ['src', 'alt', 'title', 'width', 'height', 'align'])
+            ->allowElement('p', ['align'])
+            ->allowElement('div', ['align'])
+            ->allowElement('h1', ['align'])
+            ->allowElement('h2', ['align'])
+            ->allowElement('h3', ['align'])
+            ->allowElement('h4', ['align'])
+            ->allowElement('h5', ['align'])
+            ->allowElement('h6', ['align'])
+            ->allowElement('th', ['align', 'colspan', 'rowspan'])
+            ->allowElement('td', ['align', 'colspan', 'rowspan'])
+            ->allowElement('ol', ['start'])
+            ->allowElement('details', ['open']);
+
+        foreach (['article', 'span', 'ul', 'li', 'table', 'thead', 'tbody', 'tr', 'pre', 'code', 'blockquote', 'summary', 'picture', 'br', 'hr', 'b', 'strong', 'i', 'em', 'sub', 'sup', 'del', 's', 'kbd'] as $element) {
+            $config = $config->allowElement($element);
+        }
+
+        // Any other element goes, but what it holds stays: GitHub's own wrappers like g-emoji or themed-picture, and
+        // the rarer text elements like samp or dl, keep their words. Only these go with everything in them.
+        $config = $config->defaultAction(HtmlSanitizerAction::Block);
+
+        foreach (['script', 'style', 'template', 'noscript', 'iframe', 'object', 'embed', 'svg', 'math', 'dialog', 'map', 'form', 'textarea', 'select', 'button', 'head', 'title', 'source', 'video', 'audio'] as $element) {
+            $config = $config->dropElement($element);
+        }
+
+        return $config
+            ->allowLinkSchemes(['https', 'http', 'mailto'])
+            ->allowRelativeLinks(false)
+            ->allowMediaSchemes(['https'])
+            ->allowMediaHosts(['camo.githubusercontent.com', 'github.com', 'raw.githubusercontent.com', 'user-images.githubusercontent.com', 'private-user-images.githubusercontent.com'])
+            ->allowRelativeMedias(false)
+            ->forceAttribute('a', 'rel', 'noopener noreferrer nofollow ugc')
+            ->forceAttribute('a', 'target', '_blank')
+            ->withMaxInputLength(self::MAX_LENGTH);
+    }
+}

tests/Unit/ArchTest.php

@@ -95,3 +95,36 @@
         expect($file->getFilenameWithoutExtension())->toMatch('/^(List|Show|Create|Update|Delete)[A-Z]\w*Request$/');
     }
 });
+
+/*
+ * Mass assignment protection is off application-wide, so a model fills whatever it is handed. Every write builds
+ * its own array today; this keeps it that way, because one `->update($request->all())` would let a caller set a
+ * column no form request validates, like the one that decides who is an admin.
+ *
+ * It looks at what the write is handed first, which is the shape the mistake takes. A request copied into a
+ * variable first is beyond a file scan, and review still has to catch that one.
+ */
+test('nothing fills a model straight from a request', function (): void {
+    $writes = '/(?:->|::)(?:create|forceCreate|update|fill|forceFill)\(\s*(?:request\(\)|\$this->request|\$request)\s*->\s*(?:(?:all|validated|input)\(\)|(?:except|only)\()/';
+
+    // Strings and comments are blanked first, with PHP's own lexer, so that prose quoting the pattern, or a raw SQL
+    // statement, is never mistaken for a write.
+    $code = function (string $source): string {
+        $stripped = '';
+
+        foreach (token_get_all($source) as $token) {
+            $stripped .= is_array($token)
+                ? (in_array($token[0], [T_CONSTANT_ENCAPSED_STRING, T_ENCAPSED_AND_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true) ? '""' : $token[1])
+                : $token;
+        }
+
+        return $stripped;
+    };
+
+    foreach (File::allFiles(app_path()) as $file) {
+        expect($code($file->getContents()))->not->toMatch(
+            $writes,
+            "{$file->getRelativePathname()} hands a request straight to a model. Build the array by hand, from what the form request validated.",
+        );
+    }
+});

tests/Unit/Services/GitHubReadmeSanitizerTest.php

@@ -0,0 +1,78 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Services\GitHubReadmeSanitizer;
+
+it('strips the constructs that have broken sanitizers before', function (string $payload): void {
+    $clean = (new GitHubReadmeSanitizer)->sanitize($payload);
+
+    expect($clean)->not->toMatch('/<script|<iframe|<object|<embed|<svg|<math|<form|<style|<template|srcdoc|javascript:|\son[a-z]+\s*=|data:/i');
+})->with([
+    'a script tag' => '<script>alert(1)</script>',
+    'an image that throws' => '<img src=x onerror=alert(1)>',
+    'a javascript link' => '<a href="javascript:alert(1)">click</a>',
+    'an svg holding a style' => '<svg><style><img src=x onerror=alert(1)></style></svg>',
+    'the math mutation' => '<math><mtext><table><mglyph><style><img src=x onerror=alert(1)>',
+    'an iframe with its own document' => '<iframe srcdoc="<script>alert(1)</script>"></iframe>',
+    'a toggle handler' => '<details open ontoggle=alert(1)>x</details>',
+    'a form with a button' => '<form action="https://evil.example"><button>go</button></form>',
+    'a base tag' => '<base href="https://evil.example/">',
+    'a data uri image' => '<img src="data:image/svg+xml,<svg onload=alert(1)>">',
+    'a template' => '<template><img src=x onerror=alert(1)></template>',
+    'an object' => '<object data="x"></object>',
+    'an inline style' => '<div style="position:fixed;inset:0">overlay</div>',
+    'an attribute that only looks harmless' => '<p title="x" onclick="alert(1)">text</p>',
+]);
+
+it('keeps the formatting a README is written with', function (): void {
+    $clean = (new GitHubReadmeSanitizer)->sanitize('<h1 align="center">Hi</h1><p>Text <strong>bold</strong> and <code>x</code></p><ul><li>one</li></ul>');
+
+    expect($clean)
+        ->toContain('<h1 align="center">Hi</h1>')
+        ->toContain('<strong>bold</strong>')
+        ->toContain('<code>x</code>')
+        ->toContain('<li>one</li>');
+});
+
+it('forces a link to open safely, whatever it asked for', function (): void {
+    $clean = (new GitHubReadmeSanitizer)->sanitize('<a href="https://ok.example" target="_self" rel="opener">ok</a>');
+
+    expect($clean)
+        ->toContain('rel="noopener noreferrer nofollow ugc"')
+        ->toContain('target="_blank"')
+        ->not->toContain('rel="opener"')
+        ->not->toContain('_self');
+});
+
+it('drops a destination that is not an absolute http, https or mailto link', function (string $href): void {
+    expect((new GitHubReadmeSanitizer)->sanitize("<a href=\"{$href}\">x</a>"))->not->toContain('href');
+})->with([
+    'a protocol relative link' => '//evil.example/x',
+    'a javascript link' => 'javascript:alert(1)',
+    'a relative path' => '/relative/path',
+    'another scheme' => 'ftp://evil.example/x',
+]);
+
+it('keeps a mailto link, which a README may legitimately use', function (): void {
+    expect((new GitHubReadmeSanitizer)->sanitize('<a href="mailto:someone@example.com">mail</a>'))
+        ->toContain('href="mailto:');
+});
+
+it('serves an image only from a host GitHub proxies or serves itself', function (): void {
+    $sanitizer = new GitHubReadmeSanitizer;
+
+    expect($sanitizer->sanitize('<img src="https://evil.example/x.png" alt="x">'))->not->toContain('evil.example')
+        ->and($sanitizer->sanitize('<img src="https://camo.githubusercontent.com/abc" alt="badge">'))->toContain('camo.githubusercontent.com')
+        ->and($sanitizer->sanitize('<img src="https://raw.githubusercontent.com/a/b/main/c.png">'))->toContain('raw.githubusercontent.com');
+});
+
+it('keeps the words of a wrapper it does not know', function (): void {
+    expect((new GitHubReadmeSanitizer)->sanitize('<g-emoji>hi</g-emoji>'))->toBe('hi');
+});
+
+it('cuts a document longer than it is willing to parse', function (): void {
+    $clean = (new GitHubReadmeSanitizer)->sanitize('<p>'.str_repeat('a', 400_000).'</p>');
+
+    expect(mb_strlen($clean))->toBeLessThan(210_000);
+});