Support SVG files for theme logo setting

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Updated default theme path in tests, attached patch for same. please review

Leaving the issue summary tag for possible cleanup.

This seems close but think the tests should be expanded to test all the extensions being added.

Also a change record should be written for it.

There's a long-standing concern about allowing SVG uploads to Drupal sites due to the security problems inherent in the format. At the same time, site owners want full control over their sites and should be able to do things that could potentially be harmful to their sites because that's their decision.

There are 3rd party libraries that can filter SVG content to protect a site, but I don't see that being used here. Therefore, this would allow someone with the "administer themes" permission to upload unfiltered SVG files to the site, which would then be viewed by all visitors.

One method of absolving core developers of having to somehow protect every site from every possible security vulnerability, while also allowing individual site owners to do what they want with their own site, has been the use of "restricted access" permissions to protect certain site functionality. For example the permission for controlling field configuration is a "restricted" permission, because you could potentially open a site up to security vulnerabilities based upon how you build fields on an entity type.

The concern with this issue is that (IIRC) this would be the first part of Drupal core that officially allowed uploading SVG files to the site. However, no security tools are being added to protect the site from an unscrupulous user who discovered they'd been given the "administer themes" permission. Furthermore, the "administer themes" permission is not a "restricted" one, therefore some level of protection is warranted.

Therefore, from a security standpoint IMHO this needs further work - either an SVG security tool be added to core and properly implemented, or the "administer themes" permission be changed to a "restricted access" one.

@DemianMcKenna, There are definitely easy workarounds without a patch where you can upload SVG to a server via a file upload, and a content block.

This feature request is really just a nice to have, so site owners can use the thing they want.

We can't be the SVG police in the same way we can't be the text formatter police. The best thing we can do is to inform users that they should be concerned about arbitrarily uploading files from untrusted sources (This isn't SVG specific advice).

It was mentioned way back in #147 that we should consider rearchitecting how image toolkits are handled, but that did not seem to go over very well.

We can't be the SVG police in the same way we can't be the text formatter police.

But by default Drupal does police HTML content that is rendered on the page via content inserted through text fields. Therefore, we also need to either a) provide filtering for SVG files, or b) change the permission associated with theme administration to one that puts the responsibility squarely on the head of the site owner/builder - that's what the "restricted access" permission flag indicates.

This builds on #168 and changes the "administer themes" permission to be a "restricted access" permission, which IMHO would fulfill the security requirements.

Removing issue summary update which appeared to have been done.

Can a simple change record be written that announce svgs are now allowed?

@DamienMcKenna can we remove the Needs security review tag?

With my "security team" hat on I agree with DamienMcKenna's prior comments that if svgs are allowed to be uploaded then we should mark that permission as "restrict access".

Is this tradeoff worth it to allow uploading of svgs in exchange for marking "administer themes" as a restricted permission?

An option to not have to add "restrict access: true" for the permission would be to filter the SVG similarly to how text formats filter text - an option for that seems to be https://www.drupal.org/project/svg_sanitizer → though if we go that route we should try to also do it for core file upload fields.

Removing the "Needs security review" tag since I think Damien and I have provided that perspective, but it might need ongoing review so feel free to add back and ping if needed.

Those tradeoffs and options seem like questions for a framework or product manager.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -236,7 +236,9 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
-          'file_validate_is_image' => [],
+          'file_validate_extensions' => [
+            'png gif jpg jpeg apng svg',
+          ],

This will need updating for https://www.drupal.org/node/3363700 →

Making title more descriptive and changing category. IMO this seems like a feature, to add support for more formats.

Re-rolled #173 and made change suggested in #177.

With patches please include an interdiff. Also if possible MRs are recommended now too.

But moving to NW for the change record.

The patch #179 works fine with latest Drupal 10.2.2 with PHP 8.2

So to clarify, this just needs a change record → and the patch in #179 → turned into a merge request to move it forward?

thejimbirch → changed the visibility of the branch 2259567-parsonsj to hidden.

thejimbirch → changed the visibility of the branch 2259567-unnecessary-restrictions-on to hidden.

Hid some stale files and stale merge requests that were against 9.2 and 9.3.

I don't think that's quite it, but that would be helpful.

I updated the issue summary to remove an outdated remaining task and add some option items from comment 176.

thejimbirch → changed the visibility of the branch 9.2.x to hidden.

thejimbirch → changed the visibility of the branch 10.2.x to hidden.

thejimbirch → changed the visibility of the branch 10.3.x to hidden.

thejimbirch → changed the visibility of the branch 11.x to hidden.

twills → made their first commit to this issue’s fork.

MR Added created from patch in comment 179. Draft change record created.

Hiding the patch as the work is in the MR now.

Unfortunately think the test needs to be tweaked some as it seems to be passing without the update. See https://git.drupalcode.org/issue/drupal-2259567/-/jobs/1161166

Testing manually though when I upload a svg it doesn't appear to render.

Was on standard profile using olivero as frontend theme.

Removing tag for CR.

Ah found out the problem. The svg wasn’t saving but that wasn’t being checked if the form threw any errors. And when it went to check the logo_path it had the old image png value which was valid because the form never saved.

I added an assertion for the verification message the form saved

But also cleared the logo at the end of the loop so new value can be added and doesn't fall back to the previous image in the loop.

We now get a test only failure https://git.drupalcode.org/issue/drupal-2259567/-/jobs/1163711

Didn't make any change to the solution so think I'm fine with marking

Back to needs review for that question. I feel like allowing someone to administer themes allows them to deface your site, apart from their ability to upload malicious SVGs, so it probably make sense to add it?

Responded

Trying to sort out issue credit. I've tried to credit everyone who has moved this issue on over years with patches, helpful comments. mentoring at cons, working on it at cons. If I've made a mistake please let me know.

Drupal does provide ample footguns especially behind the restrict access type permissions. I think that this functionality is expected out-of-the-box and therefore we should support it.

Committed and pushed 537f11dcb5 to 11.x and 340845698d to 10.3.x. Thanks!

I'm a little bit amused that we've supported svg upload in favicon since #864938: Can't upload .ico favicon → ... TIL.

Automatically closed - issue fixed for 2 weeks with no activity.