Skip to content
Snippets Groups Projects

XSS attribute filtering is inconsistent and strips valid attributes

Open XSS attribute filtering is inconsistent and strips valid attributes
issue/drupal-2544110:2544110-xss-attribute-filtering into 11.x
6 unresolved threads
Open Lucas Hedding requested to merge issue/drupal-2544110:2544110-xss-attribute-filtering into 11.x
6 unresolved threads

Closes #2544110

Merge request reports

Members who can merge are allowed to add commits.

Merged results pipeline #268233 passed

Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal

#268234

    Merged results pipeline passed for e97707e3

    Approval is optional
    Ready to merge by members who can write to the target branch.

    Merge details

    • The source branch is 951 commits behind the target branch.
    • 1 commit will be added to 3.0.x.
    • Source branch will not be deleted.

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/lib/Drupal/Component/Utility/Xss.php
    41 'class',
    42 'datetime',
    43 'mailto',
    44 'media',
    45 'name',
    46 'property',
    47 'rel',
    48 'rev',
    49 'sizes',
    50 'title',
    51 'typeof',
    52 'value',
    53 ];
    54
    55 /**
    56 * The default list of Unsafe attributes that must be sanitized by filter().
    • quietone
      quietone @quietone ·
      Maintainer
      Suggested change
      56 * The default list of Unsafe attributes that must be sanitized by filter().
      56 * The default list of unsafe attributes that must be sanitized by filter().
    • Please register or sign in to reply
  • quietone
    quietone @quietone started a thread on the diff
    • core/lib/Drupal/Component/Utility/Xss.php
    229 270 // starting with "javascript:"). However, for some non-URI
    230 271 // attributes performing this filtering causes valid and safe data
    231 272 // to be mangled. We prevent this by skipping protocol filtering on
    232 // such attributes.
    273 // such attributes. Adding check to skip angular attributes (ng).
  • quietone
    quietone @quietone started a thread on the diff
    • core/lib/Drupal/Component/Utility/Xss.php
    230 271 // attributes performing this filtering causes valid and safe data
    231 272 // to be mangled. We prevent this by skipping protocol filtering on
    232 // such attributes.
    273 // such attributes. Adding check to skip angular attributes (ng).
    233 274 // @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
    234 275 // @see http://www.w3.org/TR/html4/index/attributes.html
    235 $skip_protocol_filtering = str_starts_with($attribute_name, 'data-') || in_array($attribute_name, [
    236 'title',
    237 'alt',
    238 'rel',
    239 'property',
    240 'class',
    241 'datetime',
    242 ]);
    276 $skip_protocol_filtering = str_starts_with($attribute_name, 'data-') ||
    277 str_starts_with($attribute_name, 'ng-') ||
  • quietone
    quietone @quietone started a thread on the diff
    • core/lib/Drupal/Component/Utility/Xss.php
    341 395 return !isset($html_tags[strtolower($elem)]);
    342 396 }
    343 397
    398 /**
    399 * Strips bad protocols from attribute values.
  • quietone
    quietone @quietone started a thread on the diff
    • core/lib/Drupal/Component/Utility/Xss.php
    341 395 return !isset($html_tags[strtolower($elem)]);
    342 396 }
    343 397
    398 /**
    399 * Strips bad protocols from attribute values.
    400 *
    401 * @param string $name
    402 * The attribute name.
    403 * @param string $value
    404 * The attribute value.
    405 *
    406 * @return string
    407 * The attribute value, stripped of any bad protocols.
  • quietone
    quietone @quietone started a thread on the diff
    • core/tests/Drupal/Tests/Component/Utility/XssTest.php
    508 511 'Link tag with rel attribute',
    509 512 ['a'],
    510 513 ],
    514 [
    515 '<a href="mailto:me@example.com">Drupal</a>',
    516 '<a href="mailto:me@example.com">Drupal</a>',
    517 'Link tag with rev attribute',
    518 ['a'],
    519 ],
    • Comment on lines +514 to +519
      quietone
      quietone @quietone ·
      Maintainer

      I also think that while we are here, and this is adding over 50% more test cases that the dataprovider is re-worked a bit. I think the message should become and array key and then the message does not need to be passed to the test method. Like this,

            'Link tag with rev attribute' => [
        '<a href="mailto:me@example.com">Drupal</a>',
        '<a href="mailto:me@example.com">Drupal</a>',
        ['a'],
      ],
    • Please register or sign in to reply
    Please register or sign in to reply