CKEditor mangles tokens in URLs, due to bug in Xss::attributes()

Looks that's exactly ckeditor bug, probably this regexp in action https://github.com/ckeditor/ckeditor-dev/blob/major/core/htmldataprocess...

Is there an upstream bug report?

Oh, I might've misread #4 as attributing it to a bug in the upstream regex.

I think this is the same bug as #2544110: XSS attribute filtering is inconsistent and strips valid attributes - the attributes method assumes ":" characters are an attempt to use a dangerous schema such as "javascript:", and sanitizes the string by stripping everything before the colon.

Is it possible to expand tokens *before* the attribute sanitization takes place, I wonder?

Might this API page for function locale_string_is_safe provide any insight or help for this issue? It sounds very similar to the problem described here.

Note the comment header for the function locale_string_is_safe:

// Some strings have tokens in them. For tokens in the first part of href or
  // src HTML attributes, \Drupal\Component\Utility\Xss::filter() removes part
  // of the token, the part before the first colon.
  // \Drupal\Component\Utility\Xss::filter() assumes it could be an attempt to
  // inject javascript. When \Drupal\Component\Utility\Xss::filter() removes
  // part of tokens, it causes the string to not be translatable when it should
  // be translatable.
  // @see \Drupal\Tests\locale\Kernel\LocaleStringIsSafeTest::testLocaleStringIsSafe()
  //
  // We can recognize tokens since they are wrapped with brackets and are only
  // composed of alphanumeric characters, colon, underscore, and dashes. We can
  // be sure these strings are safe to strip out before the string is checked in
  // \Drupal\Component\Utility\Xss::filter() because no dangerous javascript
  // will match that pattern.
  //
  // Strings with tokens should not be assumed to be dangerous because even if
  // we evaluate them to be safe here, later replacing the token inside the
  // string will automatically mark it as unsafe as it is not the same string
  // anymore.
  //
  // @todo Do not strip out the token. Fix
  //   \Drupal\Component\Utility\Xss::filter() to not incorrectly alter the
  //   string. https://www.drupal.org/node/2372127

Based on the last time I looked at this, my preference would be to try to come up with an extensible (or at least configurable) solution in #2635632: Drupal XSS filtering cuts valid attributes, make it configurable / use a blacklist and mark all the other issues as duplicate - with maybe a follow-up to improve the default list that core uses.

If we're going to look at changing token formatting, that should stay its own issue I think. I'd like to have one issue for the Xss::attributes() bug itself though.

Is there a solution for this problem? There is so much text an links but I have exactly this bug and I'm looking for a patch.

So any movement on those sub issues? =) bumped into the same issue today, possibly related to this one: #2903336: Allow tokens for url of the Link field

I ran into this same problem last night, and I did some debugging to come across the same problem you did. I guess my Google skills aren't as strong as I thought. I tried to see if there was an existing issue before going into the weeds of Drupal core debugging. What brought me here is that I thought I had a workaround, but it isn't satisfactory.

Same thing is happening to me when trying to use a token for the class attribute value on an a tag when I use a text editor type that has "Limit allowed HTML tags and correct faulty HTML" filter enabled (for example, <a class="[somegroup:sometoken]" ...></a>). I am using the https://www.drupal.org/project/token_filter module. I was able to get it to at least render in the site by putting the token replacement first in the text editor's "Enabled filters", but when I load a page for editing, the token gets mangled when the content is loaded in CKEditor. Doing a view-source in the browser on the edit page shows that the data-editor-value-original attribute on the editor textarea is correct with the proper token, but the actual content of the textarea isn't. So, it does, indeed, look like this is getting mangled before getting into CKEditor on the client-side JS. I debugged the rendering on the site part, but I haven't debugged this part. My educated guess is that the XSS utility is still mangling the content that ends up getting rendered in the CKEditor when trying to edit a page (and after re-reading the complete issue here, that definitely was mentioned and is part of this issue).

For my specific case, I was able to come up with a complete workaround now. It involved creating a custom filter for the text editor, using the solution to this issue, https://www.drupal.org/node/2943974, as my inspiration. Basically, use | to separate the token instead of : since that doesn't get stripped by the XSS utility. The custom filter comes in so that you can transform the token to the correct format it needs to be in to be recognized for replacement.

I have my custom filter and then the token_filter module's filter as the first two items in the "Filter processing order". I haven't tested it, but this might still work if your processing order is different, but obviously, the custom filter would at least need to come some time before the token filter. However, if you don't want to process tokens until after the "Limit allowed HTML..." filter, then both the custom filter and the token filter should be after that one.

Sorry, but I don't have time right now to share a clean version of the full feature, but hoping that the idea could point people in the right direction. To help you along, here is the process function from my custom filter:

public function process($text, $langcode) {
    //TJM: Replace tokens in our editor-safe format (just using '|' instead of ':') with their normal token format.
    $processedText = preg_replace_callback(
      '/\[(?:[\w.\-]+\|)+[\w.\-]+\]/',
      function ($matches) {
        return str_replace('|', ':', $matches[0]);
      },
      $text
    );
    
    if($processedText === NULL) {
      //TJM: Default to being non-destructive if the replacement call somehow returns an error.
      $processedText = $text;
    }

    return new FilterProcessResult($processedText);
  }

I also used the filter class from token_filter module as a guide to set up my custom filter since I was new to writing custom filters prior to this, but it was pretty easy. For reference, https://git.drupalcode.org/project/token_filter/blob/8.x-1.x/src/Plugin/.... You only really need the "process" function for your simple custom filter.

It is kind of annoying to have to do this and have this structure for tokens all over your content wherever you use them (could make maintenance annoying later on), but this issue hasn't been fixed in 2 years and I really wanted to be able to use tokens right now.

Using this approach allows the content to render properly on the site, but it also keeps the CKEditor content correct when you go to edit a page. In my previous comment, I explained how I was able to get the first part to work, but this custom filter completes the workaround and allows happy editing and usage of tokens!

+1

In addition to #39 a little more completed workaround example:
Place FilterTokenAttributes.php into my_module/src/Plugin/Filter folder with content:

<?php

namespace Drupal\my_module\Plugin\Filter;

use Drupal\filter\FilterProcessResult;
use Drupal\filter\Plugin\FilterBase;

/**
 * A filter that converts | in tokens into :.
 *
 * @Filter(
 *   id = "filter_token_links",
 *   title = @Translation("Token Attributes Filter"),
 *   description = @Translation("Use tokens in html attributes e.g. [site|name] instead of [site:name]."),
 *   type = Drupal\filter\Plugin\FilterInterface::TYPE_TRANSFORM_IRREVERSIBLE,
 * )
 */
class FilterTokenAttributes extends FilterBase {

  /**
   * {@inheritdoc}
   */
  public function process($text, $langcode) {
    $processedText = preg_replace_callback(
      '/\[(?:[\w.\-]+\|)+[\w.\-]+\]/',
      function ($matches) {
        return str_replace('|', ':', $matches[0]);
      },
      $text
    );
    return new FilterProcessResult($processedText ?? $text);
  }

}

Ohhh - that is a very creative solution! :)

You can even use the same token format and just add two filters: One before and one after the filtered HTML filter:

- Convert tokens to use ! instead of :
- Filter HTML (like normal)
- Convert tokens to use : instead of !

That is an effective core work-around and as long as the token regexp is safe, also a safe approach.

This can then be put into a contrib module: token_xss

This can _also_ help with the colons in class problem.

We managed to correct this problem by testing directly the presence of brackets at the beginning and at the end of the chain to be tested in the Xss::attributes() function in order to bypass the call to UrlHelper::filterBadProtocol().

Tokens are now correctly rendered and are no longer mangled when editing the node.

Here is the fixed patch (tested with Drupal 8.9.x).

@tido The patch applies on 8.9.7 and basically it works. But there are issues when a token in a href is extended. For instance if you have a custom user orders-page at /user/[uid]/orders than href="[current-user:url:path]/orders" won’t work. It will strip of anything until the last colon, i.e. href="path]/orders". I’m not sure if this is a proper use case and has to be considered.

Rerolling patch against Drupal 9.2.x.

Avoid notice on empty string.

@tom.moffett #39 and @dmitry.korhov #41, would that be possible via the custom filter module?

It has a field for the Pattern & Replacement text https://www.drupal.org/node/210553

Couldn't figure out if i just need to put into Pattern

/\[(?:[\w.\-]+\|)+[\w.\-]+\]/

and inside Replacement text "|" or would I need to utilise PHP Code perhaps to adopt/paste in yours?

Thanks for any help you can give, great solution I'd like to try in drupal 7.

Patch against Drupal 8.9.x.

I think we only care that it starts with a token since we want to allow it to be a valid protocol.

So, all of the following should work:

<a href="[my:token:here]">link text here</a>
<a href="[my:token:here]/">link text here</a>
<a href="[my:token:here]/[user:id]/profile">link text here</a>
<a href="[my:token:here]/blog/[user:id]">link text here</a>

Re-rolled #51 against Drupal 9.2.x

#51 requires the token to be the only thing present in the first "part" (split by /) of the URL. This doesn't work for my case, a URL such as <a href="[site:url]user/[commerce_subscription:uid:entity:uid]/orders">billing</a>.

Here is an adjustment to allow any URL that begins with a token to validate, patched against 8.9.x and I expect it to apply to 9.x branches.

Note, looking at the regex and the logic, this should allow the case noted in #53.

#55 works for me. The token is maintained through multiple edits and loading of the content in the editor.

Regarding #53, My url has multiple tokens in it, and they all work as expected.

According to the issue description, fixing this replacement is the only task. I believe this is RTBC, but since I haven't seen anyone change the status of this ticket yet I'm only moving to Needs Review.

Definitely can't RTBC this one without adding test coverage. Looks like #14 has a good start to the tests. There are some other cases that have been brought up in the comments since then though.

None of these patches apply to 9.1.11.

my url part of a link in editor is simply a single url token.. and on save it is replaced with "url]".

i was sure on an older project which was using 8.8.8, this worked correctly.

hmm, i think #55 would work but i think the patch has a typo, the last line is this:

$thisval = $skip_protocol_filtering ? $match[1] : UrlHelper::filterBadProtocol($match[1]);

but pretty sure $thisval is wrong, and should just be $value. Once i change it to that, the token is not stripped and things work as expected.

corrected patch attached.

Looks like #60 is the difference between 8.9.x and 9.x branches.

https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/lib/Drupal/C...

https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/lib/Drupal/C...

Ah, I see, they changed the variable name.. which is good, because why would anyone ever name a variable $thisval? lol.

Here's a fail test in the spirit of #14, but with many more cases gathered from other comments in the issue.

The (partial) fix is identical to the fix from #55 except that it can handle tokens in src in addition to href. I call it a partial fix because one of the test cases still fails.

NOTE: The case that is still failing is <img src="foo[my:token:here]/[another:token]">, which is getting changed to <img src="here]/[another:token]">. The output of the test is stripped of various elements. If you run the test locally you can see the true output.

I'm starting to wonder if there's any way to allow tokens in src or href securely.

Before that though, note there's a typo in my patch in #63.

+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -266,6 +266,18 @@ protected static function attributes($attributes) {
+            $attributes_allowing_tokens = ['src', 'html'];

This should be href, not html.

Back to security. Certainly the current approach (#55 or #63 with typo fixed) is concerning. If I enable the contrib token_filter module and add it to the standard Basic HTML format with the option Replace empty values selected, I can very easily do some XSS.

I just enter this into the CKEditor:

<a href="[fake:token]javascript:alert('gotcha!');">XSS Link</a>

That comes out the other side as

<a href="javascript:alert('gotcha!');">XSS Link</a>

And it could never be as simple as not allowing the string javascript: because I could do this:

<a href="[fake:token]java[fake:token]script:alert('gotcha!');">XSS Link</a>

Any thoughts anyone?

Restoring previous status

Here's a new patch that just fixes the typo in the patch from #63.

We still need discussion on the questions raised in #65.

Marked #2851825: UrlHelper::filterBadProtocol() destructively truncates tokens in ckeditor link href as a duplicate

This in turn is probably a duplicate of #2544110: XSS attribute filtering is inconsistent and strips valid attributes but I think we need to do a proper consolidation if so because there are competing approaches between the two issues.

I dug into this issue and #2544110: XSS attribute filtering is inconsistent and strips valid attributes with the intention of verifying that they were duplicates. What I found was that they are both intent on modifying Drupal\Component\Utility\XSS's filtering in order to prevent XSS class from filtering out/mangling data from 2 different kinds of contexts:

1. Tokens being used inside href or src attributes, e.g. <a href="[node:url:absolute]">link</a>

in a body field that uses CKEditor and enables "Limit allowed HTML tags and correct faulty HTML" in the text format configuration.

2. Valid attribute name/value combinations that provide RDF metadata, e.g. rel="schema:author" or property="foaf:name"

Right now, as per the scope described in each issue summary, they are not duplicates, but if the scope was widened to consider both use cases/contexts, they could be combined into 1 issue, ensuring a compatible solution. Considering that they're both dealing with XSS class, that might be desirable.

As it stands, the patches in each issue do not solve the other's problem:

1. The patch in #2944173-69: CKEditor mangles tokens in URLs, due to bug in Xss::attributes() is saying, “hey XSS::attributes, allow tokens in this hardcoded safelist of attributes (src, href)“.
2. The patch in #2544110-100: XSS attribute filtering is inconsistent and strips valid attributes is saying, “hey everyone in the XSS class, we’re gonna allow this safelist of attributes (alt, title) to bypass XSS::filter. And btw, protect these rdfaAttributes too.”

We do NOT want to allow href or src to bypass filtering altogether I would think! Which is what would happen if we used the attribute safelist approach in #2544110: XSS attribute filtering is inconsistent and strips valid attributes and just added in href and src. In this issue, we just want it to allow tokens and not mangle them up. Assuming that tokens aren’t an XSS vector (which is an open question for discussion in #65.

Could we combine these issues and their solutions? Maybe it would make sense to do so, to ensure a compatible solution that solves both problems (href, src token mangling, and RDFa Attribute handling in XSS class). We would need to increase the scope in the one that stays open. Is that worth doing? What do you think?

Final note, if this issue remained open, it would need an issue summary update, because the problem is still there (token mangling) but different in Drupal 9.4.x (the token is mangled on save, not after a re-edit only). This was mentioned in #59. But I am waiting to see what others think about combining the issues before doing so.

Looks like #2544110: XSS attribute filtering is inconsistent and strips valid attributes has been closed as a duplicate of a fixed issue. I've tested this issue again in 9.5.x and it's still a problem, although there are some minor differences in the results described in the issue summary.

I'm doing a bit more testing and plan to update the issue summary with some updated details and also some steps to test using Token Filter so that you can actually test with working tokens.

RE #65: It is a very well-made point so thank you for that.

I feel like it shouldn't be \Drupal\Component\Utility\Xss::attributes() responsibility. If it can happen after token replacement using token_filter then I think it should be dealt with in token_filter. Once the token is replaced using token_filter then src and href should be passed to \Drupal\Component\Utility\UrlHelper::filterBadProtocol().

I ran into this issue today, and had to implement a messy workaround. It would great if this could get more attention.

Hi all,

I used the 2 patch files provided by @danflanagan8. and turned it into a merge request.
I added the href property alongside the src and html property.

Any feedback is welcome

Best regards,
Richard Hoogstad

The patch of MR10819 works for me, thank you @richard_hoogstad.

Triaged as part of BSI.

Rebased the MR which was green but well behind HEAD.

Still NW based on an IS update requested in #76 as well as questioning the direction of the solution in #65