Problem/Motivation

The security fixes in 8.x-1.8 included the introduction of checkPlain() in colorbox.js, which strips out the HTML in caption text so it gets presented as a string. This means that links in custom captions (including those generated by tokens from link fields) no longer work as links. For example:

8.x-1.7:

Caption links working in Colorbox 8.x-1.7

8.x-1.8:

Caption links broken in Colorbox 8.x-1.8

The same issue occurs in 7.x-2.16.

With hundreds of thousands of reported users of this module, this must have broken a very large number of sites that use links in Colorbox captions.

Also, the ability to include tokens of link fields in custom captions still remains, but does not function as expected.

Steps to reproduce

  1. Configure an image type with Colorbox as the format.
  2. Choose "Custom (with tokens)" for Caption.
  3. Insert a link either using a token from a link field, or just typing a link such as <a href="https://www.drupal.org">Drupal</a>.
  4. View the caption in the Colorbox window.

These are my settings for the above images:
Colorbox settings

Proposed resolution

Find a way to securely re-enable HTML in custom captions, and implement it.

Remaining tasks

  1. Establish if checkPlain() in colorbox.js is really necessary. The new Xss::filter($caption) in colorbox.theme.inc may provide sufficient security.
  2. If checkPlain() is not necessary, regress that part of the security fixes from 8.x-1.8.
  3. If checkPlain() really is necessary, investigate other ways that HTML in captions could be securely re-enabled.
  4. If no solution is found, forbid the use of link tokens in custom captions if possible.
CommentFileSizeAuthor
#19 colorbox-enable-caption-html-3263032-19.patch68 bytesdrupalfan2
#2 colorbox-enable-caption-html-3263032-2.patch428 bytesnick hope
Colorbox-format-settings.png27.69 KBnick hope
Colorbox-8pt1pt8-caption-links-broken.png289.09 KBnick hope
Colorbox-8pt1pt7-caption-links-working.png317.57 KBnick hope

Issue fork colorbox-3263032

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Nick Hope created an issue. See original summary.

nick hope’s picture

nick hope commented
StatusFileSize
new colorbox-enable-caption-html-3263032-2.patch428 bytes

This patch simply removes the following lines from colorbox.js, so that html is presented as html and not a string:

if (title) {
    extendParams.title = Drupal.checkPlain(title);
}

Note that this patch may re-introduce a security vulnerability. However 8.x-1.8 also introduced Xss::filter($caption) in colorbox.theme.inc. It might be that is sufficient security, and that checkplain() is not necessary as well. I don't have the experience to know.

richard.lampitt’s picture

richard.lampitt commented

#2 Is correct - the removal of HTML output in colorbox.js is the fastest and most robust way to prevent the XSS security vulnerability.

For this particular usecase, If HTML output is needed, it will need to be sanitized very thoroughly. Allow only &lt;a> tags, only the attributes src, class, id and perhaps target; with src constrained to display images only from the current site.

I don't have the expertise to do this, unfortunately. I gave it a go when the module was in limbo, but ended up just reverting to disabling all HTML as it was faster and more robust in the time I had.

nick hope’s picture

nick hope commented

Thank you @richard.lampitt.

Selective sanitization would seem like a good idea. PHP's strip_tags() with an allowed_tags parameter may help here (https://www.php.net/manual/en/function.strip-tags.php). Googling for a Javascript equivalent turns up a few ideas such as this and this.

I'm guessing that HTML coming only directly from tokens may be less of a security risk than HTML typed into a caption. If so, perhaps there is a way to allow HTML from tokens to bypass the sanitization.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

@Nick Hope and @richard.lampitt, thanks for your input. We're working on a solution for this and hope to have it approved by the security team soon.

paulmckibben opened merge request !9

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented
Assigned: Unassigned » paulmckibben
Status: Active » Needs review

I have a candidate fix for this in this issue fork: https://git.drupalcode.org/issue/colorbox-3263032

It involves using the DOMPurify library to sanitize HTML captions. If you install the changes from the above issue fork, you can use drush colorbox:dompurify to install the library. It looks for the library in libraries/DOMPurify/dist/purify.min.js.

Does this fix the issue without introducing any new problems?

drupalfan2’s picture

drupalfan2 commented

Is there an easier way beside DOMPurify to protect the title html code to use it in patch #2?

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

@drupalfan2, unfortunately, not that I have been able to find, because it is extremely challenging to sanitize HTML. However, if you are able to only use plain text in captions, DOMPurify is optional.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

I've modified the issue fork to allow developers to customize the allowed tags and attributes sanitized by DOMPurify beyond the few inline tags and link attributes allowed by default.

paulmckibben credited dsnopek.

paulmckibben credited greggles.

paulmckibben credited jenlampton.

paulmckibben credited poiu.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

  • paulmckibben committed 127c62e on 8.x-1.x 
    Issue #3263032: HTML links no longer work in custom captions
paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

drupalfan2’s picture

drupalfan2 commented
StatusFileSize
new colorbox-enable-caption-html-3263032-19.patch68 bytes

This problem still exists in Colorbox 8.x-1.10, maybe because DOMPurify is not installed.

Until this problem is solved we can use this new patch file.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

@drupalfan2, the better solution is to install DOMPurify. Your patch opens up the potential for XSS. I strongly recommend AGAINST installing the patch in #19.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I unpublished comment #19 which provided an unsafe solution as a patch.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

Thank you, @greggles.

drupalfan2’s picture

drupalfan2 commented

The command drush colorbox:dompurify is not working.
Error: The drush command 'colorbox:dompurify' could not be found.
Clearing the drush cache does not help.

I installed dompurify manually by downloading it from https://github.com/cure53/DOMPurify/releases .

HTML Links in Colorbox Text: I can not get this work in Colorbox 1.10. It works fine in Colorbox 2.0.0.
Why?
Is it necessary to uninstall and re-install the colorbox module?
Or what else has to done to get html links work in colorbox 1.10?
And what is new in Colorbox 2.0.0?