diff --git a/app/Queries/GitHubReadmeQuery.php b/app/Queries/GitHubReadmeQuery.php index 92f25ff6d..be407abf6 100644 --- a/app/Queries/GitHubReadmeQuery.php +++ b/app/Queries/GitHubReadmeQuery.php @@ -6,19 +6,12 @@ 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. */ @@ -26,8 +19,10 @@ 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()); } } diff --git a/app/Services/GitHubReadmeSanitizer.php b/app/Services/GitHubReadmeSanitizer.php new file mode 100644 index 000000000..217c9784a --- /dev/null +++ b/app/Services/GitHubReadmeSanitizer.php @@ -0,0 +1,79 @@ +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); + } +} diff --git a/tests/Unit/ArchTest.php b/tests/Unit/ArchTest.php index 899e685a2..aa7ae2e62 100644 --- a/tests/Unit/ArchTest.php +++ b/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.", + ); + } +}); diff --git a/tests/Unit/Services/GitHubReadmeSanitizerTest.php b/tests/Unit/Services/GitHubReadmeSanitizerTest.php new file mode 100644 index 000000000..98935b197 --- /dev/null +++ b/tests/Unit/Services/GitHubReadmeSanitizerTest.php @@ -0,0 +1,78 @@ +sanitize($payload); + + expect($clean)->not->toMatch('/with([ + 'a script tag' => '', + 'an image that throws' => '', + 'a javascript link' => 'click', + 'an svg holding a style' => '', + 'the math mutation' => '