Titles in breadcrumbs are 'double-escaped'

The patch in #2 solved my immediate problem, but resulted in a warning:

Warning: array_key_exists(): The first argument should be either a string or an integer in Drupal\easy_breadcrumb\EasyBreadcrumbBuilder->build() (line 237 of modules/contrib/easy_breadcrumb/src/EasyBreadcrumbBuilder.php).

This patch casts the $title variable to a string before passing it to array_key_exists() to suppress this warning.

#7 Works, applies cleanly, and includes updated tests.

1. +++ b/src/EasyBreadcrumbBuilder.php
   @@ -232,9 +233,8 @@ class EasyBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   -
   

   Very minor nit-pick, this line removal seems unnecessary. Probably not worth holding up this patch, just noting it.

2. +++ b/src/EasyBreadcrumbBuilder.php
   @@ -232,9 +233,8 @@ class EasyBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   +            if ($title && array_key_exists((string)$title, $replacedTitles)) {
   

   Casting the title to a string seems reasonable to me since getTitleString() does the same thing in the case of FormattableMarkup.

There is a duplicate efforts in the issue queue at #2290941: Breadcrumb shows escaped HTML tags on core admin pages. Since that issue (2290941) does not include updates to the test, we should probably re-close it and continue with this one (2979389).

Rerolling. No functional changes from #7.

Thanks :)

There's an important followup at #2990464: Breadcrumb markup should allow some tags such as italics or strong.

Thanks @greggles.

Should we also update the source of the trouble?

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Controlle...

Namely the documentation says, "If the method returns a string and it is not marked safe then it will be auto-escaped." However, it's clearly not auto-escaped.

@Greg Boggs - please.

I've reverted in 8.x-1.x branch the introduced changes from this issue and under the related one I'm gonna attach a patch that delivers the feature request and does not introduce the reported vulnerability.

Thanks , :)

The patches don't apply in any of the versions of the module, so I just remade it against the branch 8.x-1.x from github.

Re-rolled patch for 8.x-1.12.

Hi Folks. This issue is closed because the approach in this patch introduces a flaw. Please do not use the patch in this thread on your website.

~G

Hi @Greg Boggs. If we shouldn't use this patch, can you point me to a version of this module where this has been resolved? I've tried 1.x-dev with no luck.

I believe this issue is where the latest work is:

https://www.drupal.org/project/easy_breadcrumb/issues/2990464

Thanks. I really needed a fix for this, so I just preprocessed the breadcrumb block and ran the titles through https://www.php.net/manual/en/function.htmlspecialchars-decode.php . I'm sure there's some reason this issue is more complicated and maybe that's not a secure solution, but it works well enough for me.

Sounds like a great solution to me. Patches encouraged. :)

The issue is still active in the latest stable version of Easy Breadcrumb (1.12.0).

Could not apply the patch from #21. However, the contained changes solved my issue. Rerolled the patch for version 1.12.0 of Easy Breadcrumb.

Please create an new stable release which is containing the fix.

Hi Peter! Thanks for your work so far on the patch. I believe you've coded a cross-site scripting vulnerability in your latest patch, can you take another look and ensure that the mark up object doesn't allow JavaScript?

Basically, it means that anyone with any ability to publish a crumb title would be able to become the admin user if a currently logged in admin loaded the page.

I currently cannot reproduce the issue anymore. However, there is executed the following code a little bit above the return. The code is creating markup from a title which has been already sanitized. Can you still reproduce the issue?

I can reproduce the issue, see attached screenshot. It is a term name.

The patch #28 resolved the issue but leads to the following warnings:

Warning: array_key_exists(): The first argument should be either a string or an integer in Drupal\easy_breadcrumb\EasyBreadcrumbBuilder->build() (line 405 of modules\contrib\easy_breadcrumb\src\EasyBreadcrumbBuilder.php).

In the patch the method getTitleString() is changed in a bad manner as it does not return a string anymore but a Markup instance. I suggest the attached patch as it turns the title string into the Markup instance when the build() method has done all its magic.

My first patch misses an "use" statement.

Now a patch really working :-/

#33 works for me. Just rerolled patch with fixed typo.

I believe this is fixed in the latest dev branch. Can you try? The patch in 33 is a security vulnerability. Do not use it on your website if you have any logins that are not Admin users.

Dev branch works!
Thanks Greg Boggs!

What character are you using with the extra escapes?

I'm not sure, see a screen shot from my dev site. Have you tried clearing the cache and double checking that you're on the latest dev? Before the switch to Markup, this was certainly broken.

See here https://www.drupal.org/project/easy_breadcrumb/issues/3056018#comment-13...

In some cases like in the title of a view as a page, the & is &amp it need a fix there is 2 patch files for 8.1.13 & 8.1.x dev version on the other link of the issue.