Xss::filterAdmin() incorrectly filters datetime attribute

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Not sure if this affects ckeditor

But tested this following the example in #13

$dbd = '#DBD';
$value = Xss::filterAdmin($dbd);

Using breakpoint I see $value does still has datetime. Did I miss a step?

Thanks for your work on this, @smustgrave. In your screenshot, the datetime attribute is the issue, not the #DBD text

1978-11-19T05:00:00Z is having the longest potential "protocol" left-trimmed, since Xss::filterAdmin treats any text followed by a colon as a protocol. That results in datetime="00Z", which is no longer a valid datetime attribute.

@neclimdul, thanks for cleaning up the patch.

Thanks for the update.

Can confirm the issue and that the patch fixes the issue

Think we should extend the tests though. The issue summary mentions

<time datetime>
<del datetime>
<ins datetime>

but we only test for time.

Added a related ticket where we fixed something very similar.

Updated tests to include all elements with the datetime attribute. This doesn't give us any extra code coverage, but perhaps it helps prevent a regression in the future. There's an alternate patch if a single test scenario is preferred to three.

Nope you got it just right. Will mark it after the tests finish

Thank you for adding the additional tests!

The last submitted patch, 23: 2692451-23.patch, failed testing. View results →

Needs a reroll of the patch - no longer applies.

Adding Reroll for 10.1.x. Below are the errors that occurred while applying the patch as the patch failed to apply.
error: patch failed: core/tests/Drupal/Tests/Component/Utility/XssTest.php:525
error: core/tests/Drupal/Tests/Component/Utility/XssTest.php: patch does not apply

The last submitted patch, 28: 2692451-28.patch, failed testing. View results →

🐛 XSS::filter and filter_xss can create malformed attributes when you would expect them to be stripped Fixed was the conflicting patch.

Without the extra unrelated change in #28

The last submitted patch, 30: 2692451-30.patch, failed testing. View results →

Spurious test failure disappeared after re-running the tests, back to NR.

Test coverage looks good. There are a number of tickets around that Xss.php file.

+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
@@ -542,6 +542,22 @@ public function providerTestAttributes() {
+        '<del datetime="1789-08-22T12:30:00.1-04:00">deleted text</del>',
+        '<del datetime="1789-08-22T12:30:00.1-04:00">deleted text</del>',
+        'Del with datetime attribute',
+        ['del'],

this case is being merged into the img case above it - bad merge?

I see, yes it does look like a bad merge.

Back to RTBC

Committed and pushed 5af2ad5feb to 10.1.x and 966d2319e6 to 10.0.x and dbf142e91e to 9.5.x. Thanks!

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