Allowing styles to be applied to text fields - problem

Submitted by Dominic Baker on Fri, 07/15/2016 - 10:11

Forums:

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

Fri, 07/15/2016 - 15:35

Permalink

The CK Editor adds span tags

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?

Dominic Baker

Sat, 07/16/2016 - 09:31

Permalink

I would like to be able to style at a div level

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). '"';
				?>

Dominic Baker

Sat, 07/16/2016 - 09:44

Permalink

Sorry, more complete code


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

Tue, 07/19/2016 - 16:56

Permalink

Excellent,

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

Dominic Baker

Wed, 07/20/2016 - 09:17

Permalink

Will do, but....

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

Wed, 07/20/2016 - 10:40

Permalink

Github help

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

ahmad.curriki

Tue, 08/25/2020 - 12:09

Permalink

Pull request for allowing multiple style properties per element

I've created new pull request which handles both the use cases.
Take a look: https://github.com/h5p/h5p-php-library/pull/91

Also allowed some more common used style properties.

Dominic Baker

Fri, 07/22/2016 - 13:02

Permalink

Test cases

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

Fri, 07/22/2016 - 16:19

Permalink

Currently, I don't believe

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

ahmad.curriki

Fri, 08/21/2020 - 13:44

Permalink

Style attributes issues

I've added few extra plugins for CKEditor like Paste from Google Docs, Enhanced Image.

As these plugins could include multiple properties for style attributes seperated by ";".

These properties could be in h1 tags, or in div tags - see attached screenshots for your reference.

So the implemented checks in class "H5p.classes.php" which mentioned by "Dominic Bake" is causing issues becuase it strips the styles attributes from dom elements before saving it to the database and as a result whole layout gets disturbed.

How can I overcome this issue?

P.S: I've updated the CKEditor to 4.3 as most of the plugins are available for use for versions later than 4.3.

Attachments:

h5p_styles_issue.png

You are here

Allowing styles to be applied to text fields - problem