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

Left curly brackets get separated by HTML comments #111

Closed
enorm opened this issue Apr 12, 2017 · 9 comments
Closed

Left curly brackets get separated by HTML comments #111

enorm opened this issue Apr 12, 2017 · 9 comments
Labels

Comments

@enorm
Copy link

enorm commented Apr 12, 2017

Hi,

when I sanitize HTML code that includes multiple left curly brackets in a row, like
{{{
then they get converted to
{<!-- -->{<!-- -->{.

First question: Why is that? ^^

Second question: Can I disable this behavior? If yes, how?

The problem is, that I want to sanitize Handlebars templates, and I need to keep the Handlebars expressions which look like this: {{ expression }}

Here a minimal code example that describes the issue:

// omitted imports

public class HtmlSanitizerTest {

    private static final PolicyFactory policy = new HtmlPolicyBuilder()
            .allowCommonBlockElements()
            .allowCommonInlineFormattingElements()
            .allowStandardUrlProtocols()
            .allowStyling()
            .allowElements("span")
            .allowAttributes("class", "id").globally()
            .toFactory()
            .and(Sanitizers.TABLES);

    @Test
    public void handlebarsExpressionShouldNotBeChanged() {
        String expected = "{{ test }}";
        String actual = policy.sanitize(expected);
        Assert.assertEquals(expected, actual);

        /*
         * org.junit.ComparisonFailure:
         * Expected :{{ test }}
         * Actual   :{<!-- -->{ test }}
         */
    }
}

Best regards,
Norman

@mikesamuel
Copy link
Contributor

Please see https://github.com/OWASP/java-html-sanitizer/blob/master/docs/client-side-templates.md

Working as intended.

@enorm
Copy link
Author

enorm commented May 16, 2017

Thanks!

@plaa
Copy link

plaa commented Dec 19, 2017

So is there a way to disable this? We need to sanitize HTML code which can contain Handlebars placeholders within the HTML body text (not inside tags) and the sanitizer is breaking those.

@AntonMalyshev
Copy link

AntonMalyshev commented Dec 19, 2017

If that's working as expected then it's just unsuitable to sanitise most of HTML templates like mustache

@jmanico
Copy link
Member

jmanico commented Dec 19, 2017 via email

@AntonMalyshev
Copy link

Actually I have to validate templates before storing them and then I reuse them many times without validation.

@jmanico
Copy link
Member

jmanico commented Dec 19, 2017 via email

@mikesamuel
Copy link
Contributor

No, there is no way to turn off {{ mangling.

If you want to use untrusted handlebars templates you're going to need to use a sanitizer that understands the idiosyncrasies of handlebars. This sanitizer does not and would not do a good job even if there were a knob to allow {{...}} through.

<a href="{{x}}">link</a>

leads to XSS when x is javascript:alert(1), and

{{{x}}}

leads to XSS when x is <script>alert(1)</script>.

@plaa
Copy link

plaa commented Dec 20, 2017

If you want to use untrusted handlebars templates you're going to need to use a sanitizer that understands the idiosyncrasies of handlebars.

Exactly. But the inability to disable brace mangling prevents using the HTML sanitizer as part of such a solution. You're saying we need to write a separate HTML+CSS sanitizer for that purpose.

In our case the perfect solution would be to perform HTML sanitization without brace mangling, and then ensure that there are no instances of {{{ within the string. <a href="{{x}}">link</a> could be quoted to <a href="%7b%7bx%7d%7d">x</a> but braces in body text would remain untouched.

As a workaround, we're now replacing {{ with a magic string, feeding it through the sanitizer, replacing the magic string back, and then checking it doesn't contain {{{. It's certainly not ideal (due to inexact escaping within attributes) but I think it shouldn't pose any security risks.

Having the brace mangling enabled by default is a good thing, but it should be configurable like other aspects of the sanitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants