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('/',
+ 'an image that throws' => '',
+ 'a javascript link' => 'click',
+ 'an svg holding a style' => '',
+ 'the math mutation' => '