Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.The File Styles module allows passed-in file objects to override style (and other) attributes for images, but it seems to prevent most of them (pretty much anything but width and height) from actually working.
This is an unfortunate limitation when combined with a WYSIWYG and the Media module. For example, CKEditor has a number of advanced settings that allow you to put borders around an image, float it left or right, etc, from within the WYSIWYG editor, all of which are done via style attributes. But the File Styles module currently strips these out and prevents them from being used.
This simple patch removes the limitation and allows any attribute to be passed through. It also allows us to remove an IE hack that was in the code (at least I think it does; to be safe we could still filter out empty width and height on the off chance that a file object passes that in, I guess - not sure if we should. But it removes the need to have that hack in there for code generated in the function itself.).
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | file-styles-support-style-attribute.patch | 2.5 KB | David_Rothstein |
Comments
Comment #1
David_Rothstein commentedAnd the patch....
Comment #2
JacobSingh commentedDoesn't this expose us to XSS attacks?
Comment #3
David_Rothstein commentedThe patch only changes a theme function, which should not be relied on for security. However, I suppose that doesn't mean no one is :)
I'm not familiar enough with all the places that use this, so it seems we'd need to at least look into that and perhaps accompany this with some documentation. But overall, I would think the module which provides the style override is responsible for security here. Especially because in some cases, it is unnecessary. In the example of Media and CKEditor, it is not the filter's job to protect against XSS, but rather the text format as a whole (which could occur via another filter like HTML Purifier or WYSIWYG Filter that runs after the media filter). And no filtering is necessary at all if it's part of a format like Full HTML that is unsafe by design.
Maybe this patch needs to be accompanied by one in other modules, though. For example, the media filter might want to have a settings checkbox ("remove all image attributes except width and height") that determines if it is taking responsibility on its own for providing secure output.
Comment #4
heather commentedThis is totally off topic, and tangential, but I would have thought the use of deprecated vspace and hspace attributes in CKEditor is a /bad idea/. I would have expected it to apply an appropriate class, and then for that class to be override-able. Then you could just allow for specific classes to be applied to an image, and not new attributes. My €0.02 - which is OT, sorry!
Comment #5
JacobSingh commentedIdeally, we ditch the ckeditor screen in Media.
Added http://drupal.org/node/856694#comment-3212766
to address the security issue, I think this one is ready to go!
Comment #6
JacobSingh commentedCommitted