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.

Sign up for GitHub

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

Jump to bottom

$ClassName template variable should be sanitised to remove slashes #7586

Closed
2 tasks done
DorsetDigital opened this issue Nov 9, 2017 · 12 comments
Closed
2 tasks done

$ClassName template variable should be sanitised to remove slashes #7586

DorsetDigital opened this issue Nov 9, 2017 · 12 comments
Assignees
@tractorcow
Labels
affects/v4 complexity/low impact/low type/bug
Milestone
Recipe 4.1.0

Comments

@DorsetDigital
Copy link
Contributor

DorsetDigital commented Nov 9, 2017 •
edited by flamerohr
Loading

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:
@tractorcow
Copy link
Contributor

tractorcow commented Nov 10, 2017 •
edited
Loading

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 pushed a commit to open-sausages/silverstripe-simple that referenced this issue Nov 10, 2017
BUG Remove unnecessary unescaped $ClassName in body class
eb8c192 
Fixes silverstripe/silverstripe-framework#7586
@tractorcow tractorcow mentioned this issue Nov 10, 2017
Merged
@tractorcow
Copy link
Contributor

PR silverstripe/silverstripe-simple#50

@tractorcow tractorcow added this to the Recipe 4.0.1 milestone Nov 10, 2017
@DorsetDigital
Copy link
Contributor Author

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.

@kinglozzer
Copy link
Member

@tractorcow Could we add add something like the following to DBClassName instead?

public function forTemplate()
{
    return Convert::raw2htmlid($this->value);
}

@flamerohr
Copy link
Contributor

I'd be more in favour of @kinglozzer's suggestion and have requested that in the pull request :)

@tractorcow
Copy link
Contributor

tractorcow commented Nov 13, 2017 •
edited
Loading

@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
Copy link
Contributor

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;
}

@kinglozzer
Copy link
Member

kinglozzer commented Nov 14, 2017 •
edited
Loading

That’s 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?

@dhensby
Copy link
Contributor

dhensby commented Nov 14, 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.

@tractorcow
Copy link
Contributor

I think the issue is that slashes are valid in attributes, but not in CSS selectors.

@christopherdarling
Copy link
Contributor

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.

tractorcow pushed a commit to open-sausages/silverstripe-framework that referenced this issue Nov 14, 2017
API Add getShortName to DBClassName
f863573 
Fixes silverstripe#7586
tractorcow pushed a commit to open-sausages/silverstripe-simple that referenced this issue Nov 14, 2017
API Use ShortName of class instead of FQN for body class property
b0458e0 
Fixes silverstripe/silverstripe-framework#7586
@tractorcow tractorcow mentioned this issue Nov 14, 2017
Merged
@tractorcow
Copy link
Contributor

tractorcow commented Nov 14, 2017 •
edited
Loading

Ok, I've updated this with a ShortName property instead.

Because of auto-escaping, this will be escaped as XML safely in the template anyway, but also won't have any slashes anymore. :)

@tractorcow tractorcow modified the milestones: Recipe 4.0.1, Recipe 4.1.0 Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tractorcow tractorcow

Labels
affects/v4 complexity/low impact/low type/bug
Projects
None yet
Milestone
Recipe 4.1.0
Development

No branches or pull requests
6 participants
@christopherdarling @dhensby @tractorcow @flamerohr @kinglozzer @DorsetDigital