Silverstripe-framework: $ClassName template variable should be sanitised to remove slashes
Currently, the $ClassName template variable injects the entire, fully-qualified classname into the page. On SS4 this inevitably includes backslashes which are invalid in CSS class names.
Suggest that the ClassName should be sanitised with Convert::raw2htmlid() or similar before being sent to the template.
PRs:
DorsetDigital
All 12 comments
For context this is coming from the default theme's Page.ss
<body class="$ClassName<% if not $Menu(2) %> no-sidebar<% end_if %>" <% if $i18nScriptDirection %>dir="$i18nScriptDirection"<% end_if %>>
Let's just remove it instead? :P
tractorcow
on 10 Nov 2017
tractorcow
on 10 Nov 2017
Removing it from the template doesn't cure the underlying issue though. If there's an expectation to be able to use the $ClassName in a template (as per SS3) then it's going to catch a lot of people out when they re-introduce the invalid code.
If fixing the specific issue is too onerous, would changing it to a different value be preferable? (something like $CSSClasses gives a cleaner output, albeit more verbose. At least this way, developers would have some guidance on what to use.
DorsetDigital
on 10 Nov 2017
@tractorcow Could we add add something like the following to DBClassName instead?
public function forTemplate()
{
return Convert::raw2htmlid($this->value);
}
kinglozzer
on 10 Nov 2017
I'd be more in favour of @kinglozzer's suggestion and have requested that in the pull request :)
flamerohr
on 13 Nov 2017
@kinglozzer you are hard-baking in an assumption about front-end use to the inner dbfield; How do you know that $ClassName will be used as a css class name? It could be a value in a form. E.g.
<input type="radio" value="$ClassName" />
Which is now broken because we've stripped out slashes.
tractorcow
on 13 Nov 2017
You could add a $BaseName property to classname which skipped the namespace.
<body class="$ClassName.BaseName<% if not $Menu(2) %> no-sidebar<% end_if %>" <% if $i18nScriptDirection %>dir="$i18nScriptDirection"<% end_if %>>
And you could still use CSS rules like:
.HomePage h1 {
font-size: 1.1em;
}
That鈥檚 a fair point @tractorcow, I like your suggestion of adding an extra method to DBClassName to sanitise it 馃憤. Perhaps we could have both BaseName() and Sanitised() for the FQCN or something?
kinglozzer
on 14 Nov 2017
I agree with @tractorcow - there should be no assumption that $ClassName should be esacped as a .ATT as default.
Use $ClassName.ATT or some other valid escaping.
dhensby
on 14 Nov 2017
I think the issue is that slashes are valid in attributes, but not in CSS selectors.
tractorcow
on 14 Nov 2017
Backslashes are valid in CSS, you just need to escape them e.g.
<body class="Namespace\PageType">
.Namespace\\PageType {}
But for what it's worth, I think adding BaseName() and/or Sanitised() would be a good idea as this will help keep selector lengths shorter.
christopherdarling
on 14 Nov 2017
Ok, I've updated this with a ShortName property instead.
- https://github.com/silverstripe-themes/silverstripe-simple/pull/50
- https://github.com/silverstripe/silverstripe-framework/pull/7600
Because of auto-escaping, this will be escaped as XML safely in the template anyway, but also won't have any slashes anymore. :)
tractorcow
on 14 Nov 2017
Related issues
maxime-rainville
路
3Comments
kinglozzer
路
4Comments
chillu
路
3Comments
jonom
路
5Comments
ntd
路
4Comments
Most helpful comment
You could add a $BaseName property to classname which skipped the namespace.
And you could still use CSS rules like: