Repository navigation
Conversation
…eaders file RenderedPage keeps every header of the response it was rendered from, and withContent() carries them through listeners (minification included). With headers_file set, the headers that describe a page (CSP, HSTS, X-Robots-Tag, Link, ...) are written in the _headers format of Cloudflare Pages and Netlify: what all pages share in one /* rule, per-path rules only for what differs, and a filtered run merges with the pages already listed. Closes bakslashHQ#2
mtarld
left a comment
There was a problem hiding this comment.
I think we can make this simpler, mostly by dropping the merge on filtered runs I asked for in #2 (see inline). Plus, the /* rule turns out to be fragile on a real site: a redirect or a removed route is enough to lose it, and to go past the 100 rules on a large site.
Link and X-Robots-Tag in the written list are fine to me.
| - `RenderedPage::$headers` keeps every header of the response a page was rendered from; | ||
| `RenderedPage::withContent()` returns a copy with other content, keeping the format and | ||
| the headers, and the minification listener uses it (#2). | ||
| - `headers_file` writes the headers that describe each page (`Content-Security-Policy`, | ||
| `Strict-Transport-Security`, ...) in the `_headers` format of Cloudflare Pages and Netlify: | ||
| shared headers in one `/*` rule, merged with the existing file on filtered runs (#2). |
There was a problem hiding this comment.
| - `RenderedPage::$headers` keeps every header of the response a page was rendered from; | |
| `RenderedPage::withContent()` returns a copy with other content, keeping the format and | |
| the headers, and the minification listener uses it (#2). | |
| - `headers_file` writes the headers that describe each page (`Content-Security-Policy`, | |
| `Strict-Transport-Security`, ...) in the `_headers` format of Cloudflare Pages and Netlify: | |
| shared headers in one `/*` rule, merged with the existing file on filtered runs (#2). | |
| - Keep the response headers of a rendered page in `RenderedPage::$headers`, and add | |
| `RenderedPage::withContent()` (#2). | |
| - Add a `headers_file` option, writing the page headers in the `_headers` format of | |
| Cloudflare Pages and Netlify (#2). |
| /** | ||
| * A copy carrying other content, keeping the format and the headers: what a listener | ||
| * transforming the markup should return. | ||
| */ |
| Git integration's output directory at it. `_redirects` and `_headers` files placed in the | ||
| output are honoured. | ||
|
|
||
| ### Security headers |
There was a problem hiding this comment.
I think this section can be way shorter: the config, the file example and the nonce note are enough IMO. The GitHub Pages paragraph is off topic, and the X-Robots-Tag note is expected when using --allow-debug.
| ->scalarNode('headers_file') | ||
| ->info('Write the headers that describe each page (Content-Security-Policy, Strict-Transport-Security, ...) to this file of the output, in the "_headers" format of Cloudflare Pages and Netlify (e.g. "_headers"). Disabled when null.') | ||
| ->defaultNull() | ||
| ->validate() | ||
| ->ifTrue(static fn (mixed $value): bool => \is_string($value) && ('' === trim($value, '/') || str_contains($value, '..'))) | ||
| ->thenInvalid('The headers file must be a path inside the output, got %s.') | ||
| ->end() | ||
| ->end() |
There was a problem hiding this comment.
| ->scalarNode('headers_file') | |
| ->info('Write the headers that describe each page (Content-Security-Policy, Strict-Transport-Security, ...) to this file of the output, in the "_headers" format of Cloudflare Pages and Netlify (e.g. "_headers"). Disabled when null.') | |
| ->defaultNull() | |
| ->validate() | |
| ->ifTrue(static fn (mixed $value): bool => \is_string($value) && ('' === trim($value, '/') || str_contains($value, '..'))) | |
| ->thenInvalid('The headers file must be a path inside the output, got %s.') | |
| ->end() | |
| ->end() | |
| ->scalarNode('headers_file') | |
| ->info('Write the headers that describe each page to this file of the output, in the "_headers" format of Cloudflare Pages and Netlify (e.g. "_headers"). Disabled when null.') | |
| ->defaultNull() | |
| ->end() |
Flysystem's WhitespacePathNormalizer already throws PathTraversalDetected on .. and strips the leading / (same for the ltrim() below), so I think we can drop this validation and testRejectsAHeadersFileOutsideTheOutput. Plus, str_contains($value, '..') rejects foo..bar.
| ltrim($file, '/'), | ||
| $cleanUrls, | ||
| ]) | ||
| ->addTag('kernel.event_listener', ['event' => PageRenderedEvent::class, 'method' => 'onPageRendered', 'priority' => -512]) |
There was a problem hiding this comment.
The changes of a user listener registered below -512 are missed. What about decorating bakslash_static_site.dumper instead? dump() gets the final page, and the decorator only has to record it when the inner dumper doesn't throw, so no need to filter on $event->uris anymore.
| /** | ||
| * The URL the host serves the written file at: what its rules match against. | ||
| */ | ||
| private function servedPath(string $uri, string $format): string |
There was a problem hiding this comment.
This duplicates the match of FlysystemDumper::dump(), so both will diverge at the first change. What about computing the written path in a single place, and deriving the served URL from it? It would fix /page.html as well, as Cloudflare Pages serves it at /page.
| default => $uri.'.'.$format, | ||
| }; | ||
|
|
||
| return $this->siteContext->basePath().$path; |
There was a problem hiding this comment.
I'm not sure about the base path. Pages are written at the root of the output, so a host serving it (behind a proxy stripping /docs or not) receives /blog, never /docs/blog. And if the output is a subdirectory of a larger deployment, the _headers file is ignored, as it must be at the root.
Am I missing a case where the prefix matches?
| ?? 'html'; | ||
|
|
||
| return new RenderedPage($content, $format); | ||
| return new RenderedPage($content, $format, $this->headers($response)); |
There was a problem hiding this comment.
| return new RenderedPage($content, $format, $this->headers($response)); | |
| return new RenderedPage($content, $format, $response->headers->all()); |
null values end up as '' in written(), which already skips them, so headers() can go. The @param of RenderedPage::$headers then has to be list<string|null>, as HeaderBag::all() returns.
|
|
||
| public function testWritesTheHeadersOfThePagesToTheHeadersFile(): void | ||
| { | ||
| $kernel = $this->kernel = new HeadersFileKernel('test', true); |
There was a problem hiding this comment.
What about booting the kernel with debug: false? The test would then check what a real build writes, without the X-Robots-Tag coming from the debug mode.
| * | ||
| * @return list<string> | ||
| */ | ||
| private static function layout(array $pages): array |
There was a problem hiding this comment.
AFAIK Cloudflare silently ignores the rules past the 100th, and nothing tells it here (150 pages with a different Link give 151 rules). Should we at least log a warning, or even throw?
Closes #2.
Built along the lines of your answer on the issue.
What changes
RenderedPagegainsarray $headers = [], holding every header of the response the page was rendered from, keyed by lowercase name asHeaderBag::all()gives them. Nothing is filtered there, so listeners can readX-Robots-Tag,Linkor anything else. Redirect stubs carry none.RenderedPage::withContent()returns a copy with other content, keeping the format and the headers.MinifyListeneruses it, so minification no longer drops them, and so does the README'sRewriteLinksexample.headers_file(defaultnull) registers a listener that collects the headers onPageRenderedEventand writes the file onGenerationFinishedEvent, from$event->uris. Only pages actually written get a rule. The listener runs at -512, after the minifier and user listeners.X-Content-Type-Options,X-Frame-Options,Referrer-Policy,Permissions-Policy, the threeCross-Origin-*,X-Robots-TagandLink.Set-Cookie,Cache-Control,Vary,Date,ETagand the like never are. The list isn't configurable, as you suggested./*rule, and a page only gets a rule of its own for what differs. A header whose value differs between pages is never put in/*, so no!detaching or value joining is involved.--route/--pathrun reads the pages already listed there, updates its own, and lays everything out again, so/*stays right for the whole site. A page covered by/*alone is listed as a comment, so it isn't forgotten. Everything outside the markers (hand-written rules) is kept verbatim.<path>/underclean_urls, and<path>.<format>for other formats. A value containing a line break is dropped, so it cannot forge another rule. Aheaders_fileescaping the output (..) is rejected.X-Robots-Tag: noindexwhenkernel.debugis on, so generating with--allow-debugwrites it. The functional test caught this one.Backward compatibility
Additive: existing
new RenderedPage($content, $format)calls keep working, and nothing is written unlessheaders_fileis set.Tests
19 new tests:
/*rule and per-path differences;/*-only page surviving a partial run;composer test -- --fail-on-deprecation,composer stanandcomposer cspass on PHP 8.5 with the highest dependencies, and the suite passes on PHP 8.3 with the lowest (Symfony 7.4, PHPUnit 12).One choice to check with you:
LinkandX-Robots-Tagare in the written list, since they describe the page too. Happy to drop either if you'd rather leave them to a listener.