Allowing styles to be applied to text fields - problem

For h5p content, I have added some font styling (e.g. font-size, background-color). I ahve altered the semantics so that semantics->font->size = true; and $field->font->background = true;. However the rendered html does not include these styles.

Looking into this a bit further I belive the problem could lie in the following module / function profiles\opigno_lms\modules\contrib\h5p\library\h5p.classes.php:_filter_xss_attributes() ~line3518

<?php if ($allowedStyles && $attrName === 'style') { // Allow certain styles foreach ($allowedStyles as $pattern) { if (preg_match($pattern, $match[1])) { $attrArr[] = 'style="' . $match[1] . '"'; break; } } break; } ?>

This seems to cater only for a single style to be checked against the allowed styles. If the styles="..." has a list of styles separated by semi-colons as expected, it seems to come up with not allowed (as the preg_match is looking for start of string).

I am testing a fix for this, and will post it here if anyone else has a similar issue, and concurs with my analysis.

 

falcon's picture

The CK Editor adds span tags per style, see https://h5p.org/node/18890 - that is why the code is the way it is. Have added some comments to the code now. I thought it was wrong too until I created the test node.

What are you trying to do. Is this rather strange approach problematic?

Thanks. I see what you mean. And how the code copes with the specific html generated by the CKeditor. Ideally I would like to be able to use styled divs and even nest them. We are importing the html so not using CKeditor solely. Is there a reason as no why it would not be a good idea to have the code cope with both eventualities (essentially an extra foreach loop). So that more than a single style could be coped with.

style="text-align: center;background-color:#ff0000;color:#ffffff;font-size:2em" I am still testing but this is the general idea <?php //$match[1] has the whole 'style' string of semicolon separated styles //we need to test these each to see if they area allowed and let them through if they are //split $match[1] into an array of individual styles $requestedStyleA = explode(";",$match[1]); $requestedStyleA = array_map('trim', $requestedStyleA); $acceptedStyleA = array(); foreach ($requestedStyleA as $requestedStyle) { foreach ($allowedStyles as $pattern) { if (preg_match($pattern, $requestedStyle)) { $acceptedStyleA[] = $requestedStyle; break; //out of foreach ($allowedStyles as $pattern) } } } $attrArr[] = 'style="' . implode(";",$acceptedStyleA). '"'; ?>

Thanks. I see what you mean. And how the code copes with the specific html generated by the CKeditor. Ideally I would like to be able to use styled divs and even nest them. We are importing the html so not using CKeditor solely. Is there a reason as no why it would not be a good idea to have the code cope with both eventualities (essentially an extra foreach loop). So that more than a single style could be coped with.

style="text-align: center;background-color:#ff0000;color:#ffffff;font-size:2em" I am still testing but this is the general idea <?php if ($allowedStyles && $attrName === 'style') { //$match[1] has the whole 'style' string of semicolon separated styles //we need to test these each to see if they area allowed and let them through if they are //split $match[1] into an array of individual styles $requestedStyleA = explode(";",$match[1]); $requestedStyleA = array_map('trim', $requestedStyleA); $acceptedStyleA = array(); foreach ($requestedStyleA as $requestedStyle) { foreach ($allowedStyles as $pattern) { if (preg_match($pattern, $requestedStyle)) { $acceptedStyleA[] = $requestedStyle; break; //out of foreach ($allowedStyles as $pattern) } } } $attrArr[] = 'style="' . implode(";",$acceptedStyleA). '"'; break; } ?>
thomasmars's picture

We definitively should have a solution that can cope with both use cases. Please let us know how it turns out for you. We would love a pull request if you find a good solution for https://github.com/h5p/h5p-php-library

Thanks Thomas. I will happily do so, but must confess to never having used github before, so do not know how to do a 'pull request'. I will have to check out what I need to do re git software and accounts first. (Unless you could direct me to a 'Getting started with git' that you recommend.)

thomasmars's picture

The Github help documentation is a pretty good I think :)

What would be the best way to supply test cases? Is there a standard way? And coding standards, I've seen them somewhere. I'd like to get this right!

icc's picture

Currently, I don't believe there are any test cases. This is the GPL part of the code and has been copied from Drupal 7 and then modified to support CSS styles, in a more or less restrictive/whitelist way. I imagine they have some test cases but not much related to CSS.

Coding wise, sticking to the standards of Drupal should be good enough: https://www.drupal.org/coding-standards