Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
When using a placeholder within a page title, the breadcrumbs strip the tags.
Code used:
t('Edit %label')
Expected:
Edit <em class="placeholder">Hello world</em>
Actual:
Edit <em class="placeholder">Hello world</em>
Proposed resolution
Allow breadcrumb links to have HTML
Remaining tasks
N/A
User interface changes
Tags aren't stripped from page titles
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#6 | 2267763-6.patch | 3.8 KB | pwolanin |
#6 | increment.txt | 1.06 KB | pwolanin |
#4 | breadcrumb-title-2267763-4.patch | 3.45 KB | tim.plunkett |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedLooks good but TitleResolverInterface should be clarified that getTitle() returns a safe value that may contain markup.
Comment #2
tim.plunkettWe don't know if its safe or not, we just know it can contain markup.
Keeping things safe is about #2264041: Add a test to ensure title callbacks are not vulnerable to XSS, this just brings breadcrumbs in line with page titles.
I don't think touching TitleResolverInterface is in scope here, we're just unbreaking PathBasedBreadcrumbBuilder
Comment #3
pwolanin CreditAttribution: pwolanin commentedSorry to have to disagree, but either the output of getTitle() is assured to be safe and so needs to be documented as such, or the breadcrumb title needs to filtered before you can set the html option to TRUE.
It seems like you are suggesting the latter is correct? If so, that's fine, but then this patch also needs to add a call to something like Xss::filterAdmin() to the title that comes from the title resolver.
Comment #4
tim.plunkettIn HtmlControllerBase:
The results of getTitle are assumed to be safe, I've documented that as such.
Comment #5
pwolanin CreditAttribution: pwolanin commentedDiscussed with tim in IRC. looks RTBC except the exact language - let me re-roll.
Comment #6
pwolanin CreditAttribution: pwolanin commentedHeres the doxygen fix, which I also got jbeach to read over for sanity.
Comment #7
webchickCommitted and pushed to 8.x. Thanks!
Comment #9
tim.plunkett