Revisiting Color in Markup

[perlygatekeeper]

Changes to the sanitize (step 2):

color_transformer = lambda do |env|
  node = env[:node]
  
  # Only process element nodes with a style attribute
  return unless node.type == Nokogiri::XML::Node::ELEMENT_NODE
  return unless node['style']

  # Parse each declaration
  allowed_declarations = node['style'].split(';').filter_map do |declaration|
    property, value = declaration.split(':').map(&:strip)
    next unless property == 'color'

    # Allow only:
    # - Named colors: red, blue, darkslategray, etc. OR NOT?
    # - Hex colors: #fff, #ff6600
    # - RGB: rgb(0, 128, 255)
    # - HSL: hsl(120, 100%, 50%)
    valid_color = value.match?(
      /\A(
        \#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})        | # hex
        rgb\(\s*\d{1,3}\s*,\s*\d{1,3}\s*,\s*\d{1,3}\s*\)  | # rgb()
        hsl\(\s*\d{1,3}\s*,\s*\d{1,3}%\s*,\s*\d{1,3}%\s*\) | # hsl()
        [a-zA-Z]{2,50}                              # named color
      )\z/x
    )

    "color: #{value}" if valid_color
  end

  if allowed_declarations&.any?
    node['style'] = allowed_declarations.join('; ')
  else
    node.remove_attribute('style')
  end
end

--- THEN ADD THIS TO THE SANITIZE CONFIG ---

config = {
  elements: ['p', 'span', 'em', 'strong'],
  attributes: {
    :all => ['style']
  },
  css: {
    properties: ['color']
  },
  transformers: [color_transformer]
}

Sanitize.fragment(html, config)

Originally posted by @perlygatekeeper in #1440

[perlygatekeeper]

Benefits of this is that it relies on the the sanitize gem's own configuration capabilities in a clean and clear way, applies on on step 2 only, very clearly and a customizable way.

What gets through vs blocked:

Input style value           | Regex match         | Result
color: red                  | ✅ named             | allowed
color: #fff                 | ✅ hex short         | allowed
color: #ff6600              | ✅ hex long          | allowed
color: rgb(0, 128, 255)     | ✅ rgb               | allowed
color: hsl(120, 100%, 50%)  | ✅ hsl               | allowed
color: expression(alert(1)) | ❌                   | stripped
color: url(evil.com).       | ❌                   | stripped
color: red; position: fixed | ⚠️                   | only color: red kept
color: currentColor.        | ✅ named fallthrough | allowed
color: rgba(0,0,0,0)        | ❌ not in regex      | blocked*

[BraianS]

Any news on this?

[bakano69-del]

Cambios en el desinfectante (paso 2):

color_transformer = lambda do |env|
  node = env[:node]
  
  # Only process element nodes with a style attribute
  return unless node.type == Nokogiri::XML::Node::ELEMENT_NODE
  return unless node['style']

  # Parse each declaration
  allowed_declarations = node['style'].split(';').filter_map do |declaration|
    property, value = declaration.split(':').map(&:strip)
    next unless property == 'color'

    # Allow only:
    # - Named colors: red, blue, darkslategray, etc. OR NOT?
    # - Hex colors: #fff, #ff6600
    # - RGB: rgb(0, 128, 255)
    # - HSL: hsl(120, 100%, 50%)
    valid_color = value.match?(
      /\A(
        \#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})        | # hex
        rgb\(\s*\d{1,3}\s*,\s*\d{1,3}\s*,\s*\d{1,3}\s*\)  | # rgb()
        hsl\(\s*\d{1,3}\s*,\s*\d{1,3}%\s*,\s*\d{1,3}%\s*\) | # hsl()
        [a-zA-Z]{2,50}                              # named color
      )\z/x
    )

    "color: #{value}" if valid_color
  end

  if allowed_declarations&.any?
    node['style'] = allowed_declarations.join('; ')
  else
    node.remove_attribute('style')
  end
end

--- LUEGO AÑADE ESTO A LA CONFIGURACIÓN DE DESINFECTAR ---

config = {
  elements: ['p', 'span', 'em', 'strong'],
  attributes: {
    :all => ['style']
  },
  css: {
    properties: ['color']
  },
  transformers: [color_transformer]
}

Sanitize.fragment(html, config)

Publicado originalmente por@perlygatekeeperen #1440

[asclepiadae]

Within the context of the sanitize gem being intentionally strict, I would suggest that the rule be modified as such:

- # Allow only:
-    # - Named colors: red, blue, darkslategray, etc. OR NOT?
-    # - Hex colors: #fff, #ff6600
-    # - RGB: rgb(0, 128, 255)
-    # - HSL: hsl(120, 100%, 50%)
+ # Allow only hex colors: #fff,#ff6600
   valid_color = value.match?(
      /\A(
+         \#([0-9a-fA-F]{6}|[0-9a-fA-F]{3})        | # hex, prefer full three-byte form but fallback to shorthand
-          \#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})        | # hex
-          rgb\(\s*\d{1,3}\s*,\s*\d{1,3}\s*,\s*\d{1,3}\s*\)  | # rgb()
-          hsl\(\s*\d{1,3}\s*,\s*\d{1,3}%\s*,\s*\d{1,3}%\s*\) | # hsl()
-          [a-zA-Z]{2,50}                              # named color
      )\z/x
   )

I'd also suggest adding that if a shortform hex was matched, it gets automatically expanded to a full three-byte form (e.g. #09C -> #0099CC) to ensure global consistency of what value is saved and then later passed along to a renderer.

This allows for full 24 bits "true colour" in a developer-focused, globally 'standard' (insomuch as there is one), and concise format while limiting the opening of any abuse to it's absolute minimum. At a quick check I believe this doesn't open any risk to injection, homograph attacks - only the possibility of having lime green text on a white background.

Now, this doesn't address why #369 and #1440 were closed, not in a small part because GitHub and @zkoppert provided little direct feedback to address the long-standing issue (and with #1440 closed on cart vs horse semantics).

It's worth noting that CommonMark, which GFM is based on, also does not have support for color attributes. Discussion about color alone get redirected (and then lost) in a larger discussion on supporting a broad scope of attributes.