fix: gotenberg_font_face quotes escaped in HTML context by marilenaRM · Pull Request #267 · sensiolabs/GotenbergBundle

marilenaRM · 2026-05-06T19:24:03Z

Q	A
Gotenberg API version ?	8.x
Bug fix ?	yes
New feature ?	no
BC break ?	no
Issues	Fix #240

Description

gotenberg_font_face was declared is_safe => ['css'] only, causing Twig's HTML auto-escaping to convert double quotes to " when the function was used inside a <style> tag.

Each method now manages its own htmlspecialchars sanitization independently, removing the shared generateFontFace helper that obscured the security boundary tied to is_safe declarations.

gotenberg_font_face was declared is_safe => ['css'] only, causing Twig's HTML auto-escaping to convert double quotes to " when the function was used inside a <style> tag. Each method now manages its own htmlspecialchars sanitization independently, removing the shared generateFontFace helper that obscured the security boundary tied to is_safe declarations.

esserj · 2026-05-07T07:33:40Z

+        $name = htmlspecialchars($name);
+        $basename = htmlspecialchars(basename($path));
+
+        return '<style>@font-face {font-family: "'.$name.'";src: url("'.$basename.'");}</style>';


why duplicate the escaping logic?

I'm not sure I see the benefit here?

good point! sorry for this, it's useless

no worries! Thanks for fixing it

esserj · 2026-05-07T07:36:30Z

+use Twig\Loader\ArrayLoader;
+use Twig\RuntimeLoader\FactoryRuntimeLoader;
+
+class GotenbergExtensionTest extends TestCase


Thanks for the tests!

Would you be able to also add tests that verify that the escaping is happening correctly please?
This will prevent potential future changes from sneaking exploits in.

something along the lines of

$template = $twig->createTemplate('<style>{{ gotenberg_font_face("</style><script>/*foo.ttf*/ alert('hey')</script><style>", "my_font") }}</style>');

for both font face and style tag

and its best to also do it for the name argument, otherwise we are only testing one

tests added!

StevenRenaux · 2026-05-07T09:47:33Z

+use Twig\Loader\ArrayLoader;
+use Twig\RuntimeLoader\FactoryRuntimeLoader;
+
+class GotenbergExtensionTest extends TestCase


Existing test about font live in https://github.com/sensiolabs/GotenbergBundle/blob/1.x/tests/Twig/GotenbergRuntimeTest.php

tests written in the right class, thanks!

Neirda24 · 2026-05-07T13:14:59Z

@marilenaRM : in the original issue linked in your description @Jean-Beru asked if

<style>
    {{ gotenberg_font_face('vendor/@fontsource-variable/montserrat/files/montserrat-latin-wght-normal.woff2', 'Montserrat Variable') | e('css') }}
</style>

would work. It could make things easier to maintain. WDYT ?

StevenRenaux · 2026-05-07T13:23:03Z

        return basename($path);
    }

    public function getFontStyleTag(string $path, string $name): string


Sorry but I don't get it. You update getFontStyleTag but the method related to gotenberg_font_face is getFontFace

It was confusing since I duplicated the html escaping, but I rolled back the changes in this file

marilenaRM · 2026-05-07T13:51:15Z

@marilenaRM : in the original issue linked in your description @Jean-Beru asked if
<style>
    {{ gotenberg_font_face('vendor/@fontsource-variable/montserrat/files/montserrat-latin-wght-normal.woff2', 'Montserrat Variable') | e('css') }}
</style>
would work. It could make things easier to maintain. WDYT ?
@Neirda24 :
Well, I'd say that | e('css') is not the right approach here, for two reasons:

It would produce invalid CSS.
Twig's CSS escaping encodes every non-alphanumeric character into \HEX notation. Since gotenberg_font_face() returns a complete CSS rule (including structural characters like @, {, }, :, "), applying | e('css') to it would turn the entire rule into something unreadable by the browser. The filter is designed to escape values inside an existing CSS rule, not a full CSS block.
It pushes the burden onto the template author.
Even setting aside the above, requiring developers to remember to add | e('css') (or | raw) every time they use the function would be fragile and error-prone. The function should be safe and correct out of the box.

Jean-Beru · 2026-05-07T14:40:17Z

+        // | e('css') encodes every non-alphanumeric character (e.g. @ → \40, { → \7B)
+        // which destroys the structure of the CSS rule
+        $this->assertStringNotContainsString('@font-face', $output);
+        $this->assertStringContainsString('\40', $output);


We should make an assertion on the whole string ($this->assertSame);

Jean-Beru · 2026-05-07T14:40:55Z

+    {
+        [$twig] = $this->createTwigEnvironment();
+
+        $output = $twig->createTemplate('<style>{{ gotenberg_font_face("foo.ttf", "my_font") | e("css") }}</style>')->render([]);


What about using a DataProvider to provide the expected value and the template?

Jean-Beru · 2026-05-07T14:45:00Z

+    private function createRuntime(): array
+    {
+        $builder = $this->createMock(BuilderAssetInterface::class);
+        $runtime = new GotenbergRuntime();
+        $runtime->setBuilder($builder);
+
+        return [$runtime, $builder];
+    }
+
+    /**
+     * @return array{Environment, MockObject&BuilderAssetInterface}
+     */
+    private function createTwigEnvironment(): array
+    {
+        [$runtime, $builder] = $this->createRuntime();
+        $builder->method('addAsset');
+
+        $twig = new Environment(new ArrayLoader(), ['autoescape' => 'html']);
+        $twig->addExtension(new GotenbergExtension());
+        $twig->addRuntimeLoader(new FactoryRuntimeLoader([
+            GotenbergRuntime::class => static fn () => $runtime,
+        ]));
+
+        return [$twig, $builder];
+    }


IMO, these methods add confusion by returning multiple values. We could create the Environment in the newly added test. Combined with a DataProvider, no code will be duplicated.

Jean-Beru

Some minor comments about test description.

Jean-Beru · 2026-05-11T07:15:49Z

+        // basename() strips directory parts on /, so </style> in a path naturally loses its </ —
+        // but opening tags like <script> survive and must be escaped by htmlspecialchars


IMO, we can remove this comment. Tests should not describe method implementation but only make assertions on the result.

Jean-Beru · 2026-05-11T07:22:50Z

+     */
+    public static function provideFontRenderingCases(): iterable
+    {
+        yield 'font_face_in_html_context' => [


The keys allow you to describe the test in a concrete sentence to improve readability. For example: "with gotenberg_font_face in a style tag". Can you update them?

Jean-Beru · 2026-05-11T07:24:07Z

+        // names are not processed by basename — the full injection string must be escaped
+        yield 'font_face_escapes_malicious_name' => [


This comment can be resumed in the key.

Jean-Beru · 2026-05-11T07:24:23Z

+        // | e('css') encodes every non-alphanumeric character (@ → \40, { → \7B, etc.),
+        // destroying the structure of the CSS rule — it is the wrong tool for a complete CSS rule
+        yield 'font_face_css_escape_filter_breaks_rule' => [


This comment can be resumed in the key.

marilenaRM force-pushed the fix/font-face-html-escaping branch 2 times, most recently from 9319d21 to f522a8e Compare May 6, 2026 19:54

marilenaRM force-pushed the fix/font-face-html-escaping branch from f522a8e to a37d862 Compare May 6, 2026 20:31

esserj reviewed May 7, 2026

View reviewed changes

StevenRenaux reviewed May 7, 2026

View reviewed changes

adjustments after reviews

fee6857

marilenaRM requested review from StevenRenaux and esserj May 7, 2026 14:22

Jean-Beru reviewed May 7, 2026

View reviewed changes

refactor tests

42c7836

marilenaRM requested a review from Jean-Beru May 7, 2026 15:43

Jean-Beru reviewed May 11, 2026

View reviewed changes

		// basename() strips directory parts on /, so </style> in a path naturally loses its </ —
		// but opening tags like <script> survive and must be escaped by htmlspecialchars

		// names are not processed by basename — the full injection string must be escaped
		yield 'font_face_escapes_malicious_name' => [

Conversation

marilenaRM commented May 6, 2026

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neirda24 commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marilenaRM commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jean-Beru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marilenaRM commented May 7, 2026 •

edited

Loading