Image alternative text loses text preceding colon upon leaving plain-text editor or upon saving node

Same problem.
I'm trying to put a token to the alt field: "[node:title]"
After saving the node remains just the end of it: "title]"

I don't think that this is CKEditor related; see the related core issue.

Patch at #97 solved the problem for me…

Edit: Note that this patch is an explicit requirement for use of the Enitity Embed module !

See patch in #2105841-111: Xss filter() mangles image captions and title/alt/data attributes

The attached patch is a re-roll of the last D7 patch from #2105841: Xss filter() mangles image captions and title/alt/data attributes that applies cleanly against the latest 7.x branch.

Had been using the old patch in #2105841 and this new one in #15 to meet the requirement for the Entity Embed module. Works as expected.

Entity Embed Requirements

7.x-3.x

Drupal 7.37 or later patched with #2105841-97: Xss filter() mangles image captions and title/alt/data attributes.

• Editor CKEditor - A submodule of the Editor module.
• Entity Reference - 7.x-1.2 or later.

The D8 parent is rather long.

For the purposes of reviewing this backport, here are a couple of highlights from it:

* #64 has a summary of the problem and the approach to fixing it by @pfrenssen.
* #20 to #32 suggest, discuss and review the idea of whitelisting certain attributes as safe / not requiring filtering.
* #107 @David_Rothstein reconsiders whether the D7 backport patch is safe to commit as is to D7 due to concerns about arbitrary whitelisting of data- attributes.

This NEEDS WORK as the patch is incomplete:

The new function is never called outside of tests.

Also the tests do test the wrong thing - filter_xss SHOULD be identical to the with attribute filtering.

As applying e.g. #15 to a site is a potential serious security issue, I am marking this issue as "DO NOT USE".

We can re-visit once we have a good patch.

(I was searching for something that won't filter out `:` from `class` to use Tailwind and landed here).

D8 patch is here for reference:

https://git.drupalcode.org/project/drupal/commit/2febbff

Thanks,

Fabian

@Fabianx, thank you for highlighting this. It's clear the patch in #15 is incomplete.

I have reviewed the D8 patch you referenced and modified the code to include filter_xss_data_attributes as part of filter_xss.

I also modified the tests to simply call filter_xss, since the data attributes function is now part of what filter_xss does.

Please review and provide feedback, and I'll update as necessary. Thanks.

Thanks @Fabianx and @ron_s! Attached is an interdiff for patch #15 & #19.

We ran into a similar issue for Backdrop: https://github.com/backdrop/backdrop-issues/issues/1201.

The problem was that filter_xss() has a portion for stripping unknown protocols (e.g. http, ftp, news, mailto, etc). What was happening was that a caption like data-caption="Image: The Caption" was being parsed to strip any unknown protocols. So "Image:" was considered a protocol, which wasn't in the white list, and so was stripped out.

To fix this issue, we skipped the checking of protocols on all data-attributes (as well as alt and title, added later).

Since we don't know what type information could be stored in data attributes, we decided we shouldn't assume it was safe to strip protocols, or anything else for that matter. We decided that it should be left up to whatever code is using the attribute to decide if and how the attribute value is sanitized.

In our case, the data-caption attribute is something that is supported in core. We run filter_xss() on the attribute value when the image tag with the data-caption attribute is replaced with figure and figcaption tags (and when the data-caption attribute is removed).

We chose not to run filter_xss() on any unknown data attribute value, since we were unsure of what might happen if the data attribute contained only JSON (for example).

I'm attaching a patch here that's a D7 equivalent of our fix, for comparison. (We added the same treatment for alt and title later on)

Here's a patch that includes the changes for alt, title, and data-attributes, as well as tests for those changes. This patch should be safe to use.

Changing issue status to NR, but I want to highlight that this patch is quite different from the patch in #19 and is not an iteration on that approach.

The approach in #19 more closely matches the filtering in Drupal 8, but I believe running all data attributes through filter_xss_admin() could have serious consequences. Since data attributes often contain JSON (for example: data-x1="{ foo: 'something', bar: 'something else' }") I think this approach will be less likely to cause problems for existing sites.

The attached patch is an iteration of the patch in #19 that should fix the failing test.

I'm not necessarily trying to make a statement about whether the approach used in patch #19 should be used instead of the approach used in #24 but noticed #19's tests were failing for a relatively trivial reason (due to self-closing img tag being generated by the filter_dom_* functions used in the new filter_xss_data_attributes() function).

#25 no longer applies cleanly to 7.x-dev after the changes from SA-CORE-2021-002. The attached patch is a re-roll of #25 that should apply cleanly to the latest 7.x-dev

@joegraduate, thank you for the re-roll.

This is a more limited version of the agreed approach. It does not address data- attributes, but does fix the class attribute, which is particularly useful for tailwind.

Reported at #3396480: A colon in alt text breaks the image description for Drupal.org

I created a MR from the last patch because it failed to apply. I also fixed a comment, which was repeating are twice, and used array() for an array literal instead of the short syntax.

The patch from that MR applied to the 7.x branch, even though I applied patches from other issues.