HTML entities in Tour tip labels get double-escaped

HTML entities in Tour tip labels get double-escaped fixed, Adding before and after patch screenshot for reference.
Before Patch

After Patch

Can be move to RTBC.

I think it's a good catch! Thanks!!

The solution seens fine to me, but since it's a bug, I think it's important to have a test to cover this scenario.

Adding the tag and moving back to NW.

Hey! I reviewed the tests and update them to validate the HTML entities scenarios, I'm uploading two new patchs, the first one is the test-only patch which should fail, the second is the new patch.

Moving back to needs review.

Verified the issue exists
Verified the patch fixed the issue.

Attaching screenshots to show.

Needs a re-roll for 10.x

Re-rolled patch against 10.0.x. Please review

reviewing this

changes in patch as it shows fail.
applied new patch on Drupal 10:

Reviewed and verified. looks good.

Testing steps:

* Install new drupal version.
* go to /admin/config/development/configuration/single/import.
* import given yml file.
* apply patch.
* start tour.

Testing result:

Output is : What's New with text in bold

This is the admin page of the Drupal site.

After patch

Please review this new patch for 9.5 and 10

Applied patch against #16 in Drupal 10.1.x

Re-rolling patch #16 against 10.1.x. Kindly check & validate

#16 runs fine on 10.1 so the patches in #18 and 20 were not needed it appears.

Verified #16 also. Following the steps there and the steps in the IS.

The latest patch is failing tests and the last test run was in October. For some reason the patch is not being automatically retested. I will start a test now.

Please review this patch.

Thanks @harshitthakore, for removing the commented lines. I forgot to add space before the comment text.

Tested #32 on Drupal 10.1.x and following are my findings:

1. Issue summary is clear and steps to reproduce have been provided
2. Was able to reproduce the issue on Drupal 10.1
3. The patch applies cleanly and fixes the issue. Provided screenshots for reference as there was a code change since last manual test
4. No issues found in the code review
5. Don't think supporting test is required here

Marking as RTBC

--- a/core/modules/tour/src/TourViewBuilder.php
+++ b/core/modules/tour/src/TourViewBuilder.php

@@ -43,7 +43,7 @@ public function viewMultiple(array $entities = [], $view_mode = 'full', $langcod
-          'title' => Html::escape($tip->getLabel()),
+          'title' => Html::decodeEntities($tip->getLabel()),

This is dangerous. We are switching from something intended to sanitize the tip label to something that effectively un-sanitizes any previous sanitization. This could allow XSS to be injected in the tip label.

We should either consider using Markup objects/render arrays properly here, or use Xss::filter().

In order to make a decision of which of those options to use, we need to see where $tip->label is coming from. How is it defined? Is it already a Markup object, or is it a plain string? Etc. Just saying that "it is already escaped somewhere" without looking into where and how is insecure coding.

Retested #36 following the issue summary.

Posted before/after screenshots to issue summary.

#35 looks to have been addressed.

Confirmed that the title is not being used anywhere else than what was pointed out in #36.

Committed 18c67bf and pushed to 10.1.x. Thanks!

Leaving open for 10.0.x and 9.5.x backport.

Hey @lauriii, here's a patch for 9.5, #36 is applying corrcetly on 10.0.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Hi,

It seems that the changes in patch #42 are already present in 10.1.x branch. So, reroll is not needed for 10.1.x.

Thanks & Regards,
Mrinalini

#42 is not supposed to work with 10.1.x since it's only for 9.5.x. Moving back to RTBC.

Cherry-picked to 10.0.x and committed #42 to 9.5.x. Thank you!

Was reviewing this issue. I'm not understanding why smustgrave and ameymudras and possibly others were not given issue credit as they had genuine contributions IMO but this issue is closed so maybe it won't be changed.

I didn't credit @smustgrave, @ameymudras or anyone else for posting screenshots to this issue since this issue did not benefit from screenshots besides the one that was part of the original issue summary. What it needed was code review. What comes to crediting code review, we only give credit for substantial code reviews. In the case of this issue, it could have been explaining why the change that is being proposed in \Drupal\tour\TourViewBuilder is fine.

Thank you for the explanation. I’ll need to figure out how to better find issues to work on and then help my coworkers do the same. I need to better understand what actually requires testing given these issues I’ve been reviewing seem to not need it.