Refactor Xss::attributes() to allow filtering of style attribute values

Unfortunatly the patch doesn't do anything. After applying the patch and the hook in #4 the style attribute is still being removed.

@fabianderijk the "Limit allowed HTML" filter (Drupal\filter\Plugin\Filter\FilterHtml) is the culprit here - it uses Xss::filter() which AFAICT doesn't use the refactored methods. Line 87 of core/Drupal/Component/Utility/Xss.php strips out the style attribute here.

Should that also be refactored, and we could implement an alter hook in Drupal\filter\Plugin\Filter\FilterHtml to override the class? Or should it all be done in the latter?

I couldnt get the patch and the custom hook to work either.
The documentation for the hook says:

"Is only called when an EditorXssFilter will effectively be used; this hook does not allow one to alter that decision."

So i assume that is part of the problem?

I've adjusted the patch to make it a little.. simpler.
You dont need any custom hooks to get this patch to work, BUT BE WARNED:

This will allow style="" on every text-formatter. If you have any Wysiwyg editors that are open to non-trusted sources, you should NOT add this patch, as it would introduce a XSS security issue!

Patch #9 kinda works but I still have an issue in the rendered markup:
I'm able to use the CKeditor cell alignment (<td style="text-align:right">), which is applied and visible while editing, but the style attribute disappear on rendering (when I view the node). If I re-edit the node, the alignment is still present and inspecting the code (or using CKE "Source") correctly displays the style attribute, meaning it gets saved.

The big question: Is there am extra XSS filtering upon rendering? If yes, how can I avoid it? (possibly in the less disruptive and most secure way...)

@elgandoz You also need to allow "style" for the tags

/admin/config/content/formats/manage/basic_html

@ras-ben , thanks for your prompt answer, but the style attribute is already there. If it wasn't, I wouldn't even be able to select the alignments (or other) options from the CKEditor table properties dialog.

The difference here is that before the patch, the style attribute was stripped upon saving (coming back to the editing form the inline-style was gone), while now they are saved but removed on the rendering only.

I will also try this on a vanilla setup in order to confirm.

EDIT: I found what's causing this problem: I am using attributes values (<td rowspan colspan id headers style="text-align">). The key here is style="text-align" instead of just style.

From the "Allowed HTML tags" help text:

A list of HTML tags that can be used. By default only the lang and dir attributes are allowed for all HTML tags. Each HTML tag may have attributes which are treated as allowed attribute names for that HTML tag. Each attribute may allow all values, or only allow specific values. Attribute names or values may be written as a prefix and wildcard like jump-*. JavaScript event attributes, JavaScript URLs, and CSS are always stripped.

CKEditor receives the available options for its dialogs from the "Allowed HTML tags" list: the difference here is that if I leave just styles it adds a lot of options to the table cell properties that I explicitly don't want the authors to use (see below screenshots).

Listed here are the different cases:

1. If I add only style all the possible options are included in the dialog, Drupal saves and render the inline styles
2. If I add the style="text-align" the options are reduced (as expected), Drupal saves correctly the data but it doesn't render the inline style
3. If I add the style="text-align:right" interestingly the options are completely removed (CKE doesn't recognize them), but if the inline styles are already saved in the markup, Drupal renders it correctly

Basically in case we define attributes values, the rendering and CKE options are mis-aligned.

Despite it may seem very specific to my case, I reckon this could affect all the other CKE widgets/plugins; in my specific case I was using the tables one.

In a nutshell: patch #9 works only if you want to allow all styles and you don't mind adding all the options in CKE dialogs.
It currently works as a workaround but it should retain the CKEditor compatibility if we want to fix this in core.

I'll try to see if there's a way to rectify this.

#9 works for me as I wanted to offer all options for tables to the editor.

Just thought i'd re-add my comment before, as it's VERY important to note:

If you have any Wysiwyg editors that are open to non-trusted editors/users, you should NOT add this patch, as it would introduce a XSS vulnerability!

This is a #9 patch that supports Drupal 9.1.

This patch work fo me, but still have question, did that patch Patch 17 not introduce XSS vulnerability!!!

@ckhalilo The previous #9 and #17 (re-roll) patches are the same. In comments #9 and #16 the risk is mentioned.

Thank you

Question would this issue be why certain classes of mine get stripped out? A library I have to use has classes like mobile-lg:grid-col-3 and they get removed while others remain.

Applied the #23 without a problem. Thanks!

Creating a new patch as per Drupal 9.

Fixing the issue with $value variable.

patched 8.9.14 with 24 without problems

@Drumanuel is the patch applied to drupal version 8.9.14?

For some reason hook_editor_xss_filter_alter() is not firing anymore, so there is no way to use the '\\Drupal\\editor\\EditorXssFilter\\InlineStyles' defined in patch #24. style attribute is stripped out from the output.

Reroll #27 for Drupal 9.1.8. Same security considerations from #9 apply here.

Security risk

While I like the spirit of #2, I'm not sure Drupal core will be willing to take this risk.

Allowing inline styles on user input is a risk, period. Selectively allowing inline styles is less risky, but any bug in the logic that does the selective allowing again implies a security risk.

I'm also not certain that #2 is truly safe. For example, what about using display: none; or color: #fff to make content invisible to a reviewer?

Best practices when theme dictates look and feel

Besides the security implications, using inline styles is generally frowned upon. It makes sense only for sites where the site is willing to grant content authors very far-reaching permissions to deviate from the site's theme. To ensure uniformity, it'd be better to add a class attribute to your tag (e.g. <table class="comparison"> or <table class="reference">) using https://ckeditor.com/cke4/addon/stylescombo (which Drupal core ships with by default). Then the front end developer can specify the styling once.

Landing pages when a site has advanced content authors

That being said, there absolutely are scenarios where you want to fully empower (trusted!) content authors to do anything they want. Typical example: landing pages. In this case, I suggest using the Full HTML text format — or really, any text format without the Limit allowed HTML tags and correct faulty HTML filter (filter plugin ID: filter_html).

Backwards compatibility & visual regressions

Finally, we cannot make the change that is being proposed here without breaking backwards compatibility. If we change filter_html to start allowing certain inline styles to make it into the filtered output, then any site that updates may suddenly suffer from visual regressions.

After updating core from 8.9.14 to 8.9.16, patch #24 although it applies it does not fix the problem any longer.
Any ideas?

Re-rolled patch #33. Attached interdiff for same.

Re-rolled patch #36 for 9.2.x core.

The same warning applies:
If you have any Wysiwyg editors that are open to non-trusted sources, you should NOT add this patch, as it would introduce a XSS security issue!

I see the patch from comment #24 no longer works in Drupal 8.9.17

Ignore Patch in incompatible version patch

hook_editor_xss_filter_alter() is not firing anymore, so there is no way to use the '\\Drupal\\editor\\EditorXssFilter\\InlineStyles' defined in patch #24
Updated the Patch to work with latest 8.9

hook_editor_xss_filter_alter() is not firing anymore, so there is no way to use the '\\Drupal\\editor\\EditorXssFilter\\InlineStyles' defined in patch #24
Updated the Patch to work with latest 8.9.x

I simplified the code logic of the patch #24. It will make the XSS filter work with style attributes that are not URI types. Regarding to the proposal of overriding the XSS inline filtering, you can still do so by using the code in #4. You can find the new patches for Drupal Core 9.2.x, 9.1.x and 8.9.x.

I already tested the 9.2.x patch in a live project that uses CkEditor and the module CKEditor Table Selection. Below you can see a screenshot with the test results formatting a table in the editor.

Please try the other versions of the patches and validate if they are working as expected.

Thanks to @adamfranco for the good proposal fixing this issue.

Best regards.

Sorry but I just realised that the patch created for 8.9.x is not correct. Attaching the right file here:

#43 is working for me on D9.2. But is it safe to apply this patch on a live site?

Actually #43 is not working entirely... some needed styles are still stripped, such as "text-decoration:none"

I'm attempting to use the views Global: Custom text field to generate tables and table rows/cells containing inline styles, but some of the styles are getting stripped on output. The Global: Custom text field doesn't use anything assistive like CKEditor. So why are we not allowed to put whatever we want in our views custom text fields?

Any advice on how I can get my views to output inline styles via a Global: Custom text field?

EDIT: described my workaround for this in issue #3256957

Everybody here seems to be concerned only with text editor filtering, but there is an awful lot of other code that also runs markup through Xss. What I would like is a nice clean way in code to replace Drupal\Component\Utility\Xss with my own extending class and have that used everywhere. If Xss was a service this would be simple.

Due to potential security headaches and comments from core maintainers above, I doubt this issue will ever be anything more than a patch, or a collection of techniques to get around the problem.

Background: I've been messing around with Xss::attributes() to try to allow a specific use-case in a views rendered entity field (ie not sourced from any user-facing input), which is:

<div class="some classes here" style="--varname1: value; --varname2: value">

The style attribute is added by a paragraphs behavior plugin, and the values are validated. This allows passing dynamic values through to css, scoped to the element they are defined on:

div.some.classes.here {
  background-color: var(--varname1);
}

This is a hugely useful technique, but a modification of how Xss::attributes() operates is required.

I've come to a different solution for my views rendered entity use-case above, patching core to provide a simple alter hook directly in Xss::attributes(). Nothing changes unless a module implements the hook. Attributes are not passed by reference so are not alterable within the hook, only the criteria by which core filters them.

In my custom module I implement the hook, checking an explicit data- attribute that my module also sets on the element render array to determine whether to allow style attributes to pass through Xss:attributes unmolested - so this occurs for that element's attributes only:

function MYMODULE_xss_attributes_alter(array &$data) {
  // We don't care unless our data flag AND a style attribute is present.
  if (strpos($data['attributes'], 'data-mymodule-allow-styles) === FALSE
      || strpos($data['attributes'], ' style="') === FALSE) {
    return;
  }
  // Prevent protocol filtering or skipping the style attribute.
  $data['skip_protocol_filtering'][] = 'style';
  foreach ($data['strip_attribute_names'] as $delta => $value) {
    if ($value === 'style') {
      unset($data['strip_attribute_names'][$delta]);
    }
  }
}

I attach a patch that provides this alter hook for anyone wishing to pursue this approach. I don't recommend anyone uses this in production unless they really know what they are doing. In particular, be sure to check the provided attributes are what you expect. The code above could possibly be expanded to check whether the attributes include only styles that my module expects.

@John Pitcairn I like your solution. Your patch has wrong paths of Xss.php file, I fixed it.

I found the patch in #53 useful but i ended up removing it as it's a bit of a sledgehammer solution that affects everything.

I think a better approach would be to simply move the code that defines that config into a separate method rather than calling a hook. You can already use hook_editor_xss_filter_alter to specify your own Xss class and that includes the text format that is currently being used so you can lock it down with more specific conditions.

Then all you need to do is create an Xss filter class that overrides the config and anything you needs and write a hook to enable it for certain format conditions.

Added reroll of the patch #53 on Drupal 9.4.x. and fixed Drupal CS issue of that patch #53.

Added reroll of the patch #56 on Drupal 9.5.x.

Just renamed #56 and #58. So will not confuse.

Tried to fix CCF in #59.
Please review.

@very_random_man re #55: hook_editor_xss_filter_alter() and swapping the class used is only useful if the filtering is happening via a text editor. It's of no help if something else like Views is calling Xss::filter().

The patch from #60 has not applied for D9.5.9
Re-roll for 9.5.9

#60 missing "datetime", #62 was working fine for me in 9.5.9, so rerolled the same patch for 10.1.x

The proposed patch now only applies to attributes using single quotes. I'm updating the patch (for 9.5.x) to work for double quotes as well.

This should be moved over to D10 as D9 is approaching EOL and D10 is where such maintenance should reside.

I have come across this recently and I tried to organize the reasoning behind the whole issue and come to a conclusion.

Impact

First, I think I understand the reasoning of the current behavior, but here is the result - which I think is worse than the intentions.

1. Security-aware developer puts a filter in the text format, such as HTMLPurifier or HtmLawed, in order to find a balance between allowing some inline styles capabilities but mitigating at least some risks.
2. Drupal core says, sorry developer, you cannot have a partly secure setup. If you need inline styles, go ahead and remove your security filter so that you have a completely insecure setup.
3. Client goes "I came to Drupal from X system for being a great CMS, but I cannot do such a basic thing like setting the size of some text and adding some custom padding, Drupal is so limiting me".
4. Account manager sees the dollars going away, ok developer let's remove the security filter, content editors are trusted users.
5. Attacker gains access to a content editor's account and engineers an XSS attack, or click-jacking, or whatever pleases, through script tags or style attributes.
6. Administrator user views or edits that content item and is subjected to the attack.

Drupal core allows me as a developer/site builder to have a completely insecure text editing configuration, if that's what I deem appropriate for my use case. But it prevents me from having a reasonably secure setup.

Proposed solutions

First, the idea is that the content is stored "as is" in the database and the filtering happens during rendering. This is intentional because if something would be filtered before saving it into the database, that's gone forever leading to errors. This is exactly what happens though with this issue and it is inconsistent with this design.

What are the disadvantages of delegating the rendering to the text editor? CKEditor has its own system for this, being Advanced Content Filtering in v4 or General HTML Support in v5. Provide the default configuration for what is provided by Drupal core i.e. CKEditor + the default text formats, and let developer configure the text editor as needed if they customize their setup.

Alternatively, would it make sense to load the same security filters that are configured for the frontend rendering - which I believe are determined by the FilterInterface::TYPE_HTML_RESTRICTOR constant, load them in the text editing rendering i.e. in the Standard editor XSS filter?

This way I get the same result in both frontend and text editing rendering. It does violate though the design of not overriding the original data. Could be offered though as an option to the developer to decide.

Keeping the HTML code safe is a massive subject, and that's why there exist specialized libraries for it. Trying to reinvent this within Drupal core (referring to allowing simple properties only within inline styles), will only deliver a mediocre solution, locking developers into it - unless you get into complex efforts to override/extend/replace classes, increase code maintenance for core and contrib/custom modules. Better let developers use the tools that specialize in solving these issues.

I have just reviewed this this week and I am fairly unfamiliar with components involved, so if I'm misunderstanding this part of the stack please do correct me.

Documentation/help

Lastly, after a solution is decided, I think what is really missing is guidance for the developer/site builder. I think that the majority of developers/site builders would not be familiar with all these details, the security risks, what alternative solutions to provide to text editors. So let's create a documentation page with advice like the ones @Wim Leers suggested, plus an explanation of this issue i.e. the frontend/text editing rendering and how to properly configure both the text editor and the text filters, and link to that documentation in the text format create/edit pages.

Re #66: yes, but this issue does not just affect text formats/editors.

It also affects things like views rewrites, where you might want to rewrite and add a style attribute that is benign - passing a css property value through to a stylesheet for example. No text editor is involved there.

Update #43 for 10.1

I have update the #64 patch for the last core update 10.2.3

update patch with LF format

the final patch rename correctly.

This is a reroll of #64/#71 for 10.2.x.

Recommend using MRs

Feature will need test coverage

Issue summary should use standard template for reviews and committers

Re-rolling #43/68 for 10.2.3

Patch for 10.4.x

Patch for 11.0.x

This patch is for 11.1.x

I am seeing

\Drupal::moduleHandler()->alter('xss_attributes', $attribute_data);

in the latest patches. It seems that this line (among other things) is causing the pipeline to fail. The container is not available at the stage the code is executed at so using \Drupal:: to access ModuleHandler service does not work.

I notice that none of the other utility classes inside the same utility? folder contain the string: '\Drupal::'

#34 needs to be addressed. This issue cannot progress until those comprehensive security concerns are responded to in equal depth. Has anyone attempted this? Otherwise I think the issue status should be changed to Postponed..

Comment #34 is long out of date. What we are doing now is allowing the XSS tag list to be altered by modules.

Re: #82, Okay i see. But does that mean that the hooks? should contain like a packet of cigarettes, a 'health warning'? Not sure what the policy on hook security is? Are there hooks that knowingly reduce the level of security achieved by core?

Alternatively, do you mean that by virtue of the fact that an extension point is being exposed so users can choose whether or not to add non-core tags that the security concerns at #34 are irrelevant?

A module could alter the XSS tag list to make it more secure, so the security concern is not relevant to the solution that is being proposed.

Patch for current 10.5.x

It looks like the patch for #85 has collided with Commit 26caa6b1 related to https://www.drupal.org/project/drupal/issues/3511566

The change to line 219 ...

-              in_array($attribute_name, ['style', 'srcdoc']) ||
+              in_array($attribute_name, $attribute_data['strip_attribute_names']) ||

... implies 'strip_attribute_names' should be set to ['style', 'srcdoc'] rather than ['style'].

Until a better patch is rolled, you can a) use the patch at #77 which works on 10.5.4 or b) have the implementation of hook_xss_attributes_alter() set 'strip_attribute_names' correctly

I have reworked patch #74 for Drupal core 10.5.6

TL;DR

Hooks don't work for Xss (core/lib, no DI). This patch adds two granular options: extend the class and override, or pass attribute filters directly to filter(). allowAttribute() included as alternative to hook proposals, but discouraged. No BC break — existing tests should pass.

Detailed

Introducing hooks is not applicable here. Xss is inside core/lib and must be isolated
from the rest of Drupal, DI is not allowed. So I went with configuration on the class
itself.

The refactoring does the same thing as other patches — extracting the skip decision
and value processing out of attributes() into shouldSkipAttribute(),
filterAttributeValue(), and needsProtocolFiltering(). The revision is that
protocol filtering was originally done before checking whether the attribute is
skipped. I moved it into filterAttributeValue(), which only runs if the attribute
makes it through. This way the method actually does something useful to override.

The real benefit here is the more granular approach. It covers class extension and a
new argument to filter().

Class extension: EditorXssFilter\Standard already extends Xss, and
hook_editor_xss_filter_alter already lets you swap it out. What was missing is
anything to actually override — the attribute logic was all inlined. Now you can
provide a new Standard via the hook and reuse the parent, only overriding what you need.

The new argument: a third parameter to filter() for attribute filters — a map of
attribute name to a sanitizer callback, or TRUE to allow with default filtering. If an
attribute is in that map, it won't be skipped. No side effects, you pass what you need
and get filtered output. FilterHtml is the natural fit here. Similar to
Extended HTML Filter, it keeps the scope local to the filter.

I also added Xss::allowAttribute() for global registration on the static class. I
strongly discourage this one — in wrong hands it opens too much. There is obviously
demand for it though, and developers must be able to extend the functionality when
they know what they do. It also allows creating a module that intelligently filters all style attributes. I included it in the patch to give an
alternative to the hook proposals. I don't think it should be accepted in this form
and requires more discussion.

The patch doesn't break existing functionality — existing tests should pass, API evolves but doesn't break.

Extending FilterHtml with the new argument (no core FilterHtml patch needed)

// ./mymodule.module
// Override existing filter_html implementation
function mymodule_filter_info_alter(array &$info) {
  if (isset($info['filter_html'])) {
    $info['filter_html']['class'] = MyFilterHtml::class;
    $info['filter_html']['title'] = \t('[MyModule] Limit allowed HTML tags and correct faulty HTML');
  }
}

// ./src/Plugin/Filter/MyFilterHtml.php
/**
 * Provides an extended filter to limit HTML tags with the ability to use "style" attribute.
 */
class MyFilterHtml extends FilterHtml {

  /**
   * {@inheritdoc}
   */
  public function process($text, $langcode) {
    $restrictions = $this->getHtmlRestrictions();
    // Defines style attribute filter.
    $attribute_filters = [
      'style' => [$this, 'sanitizeStyleAttribute'],
    ];
    unset($restrictions['allowed']['*']);
    $text = Xss::filter($text, array_keys($restrictions['allowed']), $attribute_filters);
    return new FilterProcessResult($this->filterAttributes($text));
  }

  /**
   * {@inheritdoc}
   */
  public function getHTMLRestrictions() {
    $restrictions = $this->restrictions ?? parent::getHTMLRestrictions();
    // Remove global configuration for style attribute.
    // If set to FALSE, it is disallowed globally in HTMLRestrictions.
    // If set to TRUE, it is too much for all and may fail validation.
    /* @see \Drupal\ckeditor5\HTMLRestrictions */
    unset($restrictions['allowed']['*']['style']);
    // Allow style attribute in span elements.
    if (isset($restrictions['allowed']['span'])) {
      $restrictions['allowed']['span']['style'] = TRUE;
    }
    return $restrictions;
  }

  /**
   * Attribute filter for style attribute.
   *
   * @param string $attribute_value
   *   Attribute value to process.
   *
   * @return string
   *   Sanitized attribute value.
   */
  public function sanitizeStyleAttribute($attribute_value) {
    $allowed_style_props = [
      'color',
    ];

    $filtered_props = [];
    foreach (explode(';', $attribute_value) as $prop) {
      // Remove whitespaces
      $prop = trim($prop);
      // Check if the property is in the whitelist.
      if (preg_match('/^([a-z\-]+?):.+?$/', $prop, $matches) && in_array($matches[1], $allowed_style_props, true)) {
        $filtered_props[] = $prop;
      }
    }

    if (!empty($filtered_props)) {
      $filtered_props = implode(';', $filtered_props);
      // Add a semicolon at the end if it does not exist.
      return str_ends_with($filtered_props, ';') ? $filtered_props : $filtered_props . ';';
    }

    return '';
  }

}

Global allowAttribute usage

# mymodule.services.yml
services:
  mymodule.xss_config_subscriber:
    class: Drupal\mymodule\EventSubscriber\XssConfigSubscriber
    tags:
      - { name: event_subscriber }

class XssConfigSubscriber implements EventSubscriberInterface {

  public static function getSubscribedEvents() {
    return [
      KernelEvents::REQUEST => ['onRequest', 2048],
    ];
  }

  public function onRequest() {
    Xss::allowAttribute('style', [CssSanitizer::class, 'sanitize']);
    Xss::allowAttribute('data-custom');
  }
}

Subclassing for the editor XSS filter

function mymodule_editor_xss_filter_alter(&$editor_xss_filter_class, FilterFormatInterface $format, FilterFormatInterface $original_format = NULL) {
  $editor_xss_filter_class = MyXssStandard::class;
}

class MyXssStandard extends Standard {

  /**
   * {@inheritdoc}
   */
  protected static function shouldSkipAttribute($attribute_name, array $attribute_filters) {
    if ($attribute_name === 'style') {
      return FALSE;
    }
    return parent::shouldSkipAttribute($attribute_name, $attribute_filters);
  }

  /**
   * {@inheritdoc}
   */
  protected static function filterAttributeValue($attribute_name, $value, $attribute_filters) {
    if ($attribute_name === 'style') {
      return $value;
    }
    return parent::filterAttributeValue($attribute_name, $value, $attribute_filters);
  }

}