HTML encoding: & in title causes & in breadcrumb

This may not be a perfect fix, but I made the following change to my local copy of the code by adding a call to htmlspecialchars_decode() to handle the &

custom_breadcrumbs.module, Line: 106

$trail[] = htmlspecialchars_decode(_custom_breadcrumbs_create_crumb($title, $path));

I also encountered this problem and instead changed line 205 to:

<?php
$crumb = decode_entities(check_plain($title));
?>

Edit:
This line was edited because I used the identifier for no link. It appears that decoding entities needs to be done throughout the module.

The issue seems to occur when token_replace() is called to resolve tokens in the breadcrumb. The problem is that token_replace will run filters to escape and clean up the resulting value (causing & => &). Soon after _custom_breadcrumbs_create_crumb() is called, and again will sanitize the values.

The patch here, bypasses the first set of text filtering. This will still result in a safe sanitized solution because _custom_breadcrumbs_create_crumb() will always be called after it.

I think encoding and then decoding is a little silly. I've changed line 205 to:

$crumb = $title;

which works fine :)

#3 works perfectly for me. Not sure I like the idea of not running the title through check_plain, even though its only trusted people posting content on our site.

Setting status accordingly.

#3 effectively does the same thing as #4, but #4 is less code.

In #3 we sanitise the link text early with check_plain() in _custom_breadcrumbs_create_crumb(), and then have to ask not to sanitise it again when replacing tokens.

In #4 we don't sanitise the link text, but it gets sanitised anyway in custom_breadcrumbs_node_view() where it calls token_replace().

So from a code point of view, I prefer the simplicity of #4 myself. But from a logical point of view, #3 means that the output of _custom_breadcrumbs_create_crumb() is always in HTML whether it's a link or just a text string.

incrn8 to address your concern in #5, it's not that we're not running check_plain on the text, we're eliminating the fact that it is being done twice.

Unless you're referring to solution in #4. I would not recommend #4 for a couple reasons.

1.) You're only addressing the issue if the result of the breadcrumb is not a link. If it is a link, your already filtered text will get filtered again by l(). You can enter in more than just a token as the title (i.e. Hello & Goodbye [user:name]) and the '&' will not be encoded correctly using #4.

2.) The filtering done in custom_breadcrumbs_node_view() during token_replace() will only apply to tokens that get replaced, and not the whole title. The final check_plain() on line 205 ensures the whole title result is filtered.

I think it's best to use #3 still, since we ensure both tokens and user entered text get passed to _custom_breadcrumbs_create_crumb() in the same state.

Then _custom_breadcrumbs_create_crumb() ensures that all code paths output filtered text.

patch in #3 worked great for me.

#1 worked for me. I also tried #2, that one didn't work for me.

Thanks a lot.

Patch in #3 works for me.

Is this still a problem in the latest dev branch? It would need to get fixed there first if it's still a problem. If so, this patch may help.

Same for the paths...

  foreach  ($paths as $index => $value) {
    $paths[$index] = token_replace($value, $types, array(
		'clear' => TRUE,
		'sanitize' => FALSE,));
  }

Works just fine. Thanks.

I found a very simple solution to this problem. The l() function that is used to create the breadcrumb link has an option 'html' that says that the link text may contain HTML. The _custom_breadcrumbs_identifiers_option() function is used to create the options array. So adding the 'html' option there fixes the problem. Here's a patch.

patch #15 fixes the problem for links, but not for text (last item on path)

None of the above solutions worked for me. Using strip_tags() did, though. Here's the patch. Created with git format-patch

I inadvertantly selected "Test with custom_breadcrumbs 7.x-1.x-dev" when I uploaded the patch in #17. It is actually a patch against the 7.x-2.x branch. Created this one with git diff this time.

Same idea patched for 7.x-1.x branch instead.

Closing this because is outdated. Only 7.2 branch and greater are supported.