Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Html Sanitizer] There is no way to allow just any link scheme #49030

Open
TomaszGasior opened this issue Jan 19, 2023 · 4 comments
Open

[Html Sanitizer] There is no way to allow just any link scheme #49030

TomaszGasior opened this issue Jan 19, 2023 · 4 comments

Comments

@TomaszGasior
Copy link

TomaszGasior commented Jan 19, 2023

Symfony version(s) affected

6.2.2

Description

Html Sanitizer has hard-coded default collection of protocols allowed in <a href="url"> in HtmlSanitizerConfig:

    private ?array $allowedLinkSchemes = ['http', 'https', 'mailto', 'tel'];

There is a method HtmlSanitizerConfig::allowLinkSchemes() allowing to override it with custom list of allowed protocols. However, there is no way to just allow any protocol: to remove any restrictions from the URL's protocol.

This is already supported by UrlSanitizer::sanitize() but there is no way to pass null to $allowedSchemes which would remove any protocol related restrictions.

public static function sanitize(?string $input, array $allowedSchemes = null, bool $forceHttps = false, array $allowedHosts = null, bool $allowRelative = false): ?string

if ($url['scheme'] && null !== $allowedSchemes && !\in_array($url['scheme'], $allowedSchemes, true)) {

How to reproduce

<?php

namespace App\Command;

use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\HtmlSanitizer\HtmlSanitizer;
use Symfony\Component\HtmlSanitizer\HtmlSanitizerConfig;

#[AsCommand(name: 'example')]
class ExampleCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $customConfig = (new HtmlSanitizerConfig)
            ->allowElement('a', '*')
            ->allowLinkSchemes([]) // null would work but it's impossible because of typing; I don't want to hardcode any protocol here
        ;
        $sanitizer = new HtmlSanitizer($customConfig);

        $output =  $sanitizer->sanitize('<a href="news://somedomain.com">some text</a>');

        dd($output);
       // outputs: <a>some text</a>
    }
}

Possible Solutions

  • Allow null in HtmlSanitizerConfig::allowLinkSchemes() and related fields & getters.
  • Reorganize configuration to make it possible in more elegant way, maybe create new allowAllLinkSchemes() method.

Additional Context

This may need to be handled also in framework bundle: currently empty value (null, empty array) of framework.html_sanitizer.sanitizers.XXXX.allowed_link_schemes causes HtmlSanitizerConfig::allowLinkSchemes() to not be called at all.

if ($sanitizerConfig['allowed_link_schemes']) {

@TomaszGasior TomaszGasior changed the title [Html Sanitizer] There is no way to allow just all link schemes [Html Sanitizer] There is no way to allow just any link scheme Jan 19, 2023
TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this issue Jan 19, 2023
@xabbuh
Copy link
Member

xabbuh commented Jan 20, 2023

I started to work on this feature in #49035. There, @stof made me aware of the fact that allowing arbitrary schemes is not safe. So I think we should not make it possible. I vote for closing here therefore.

@TomaszGasior
Copy link
Author

TomaszGasior commented Jan 20, 2023

As I understand, @stof pointed out that having ability to too easily allow any scheme is not secure with just null as value. He suggested adding another property, which is similar to what I suggested in my issue.

Definitely, having no ability to allow any URL scheme is very bad idea: the final decision about what's allowed and what's is not should be done always by the developer, not by sanitizing library creator.

@stof
Copy link
Member

stof commented Jan 20, 2023

@TomaszGasior what is your use case for allowing any scheme (which will then accept links with a javascript scheme and so allow XSS as that's basically equivalent to allowing the onclick attribute) ?

@TomaszGasior
Copy link
Author

I don't want to add any restrictions to my users in regard of URL protocol. If javascript: in insecure then allowing all protocols and adding ability to blacklist just that one protocol would be fine.

TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this issue Jan 20, 2023
TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this issue Jan 21, 2023
TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this issue Jan 27, 2023
TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this issue Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants