Problem/Motivation

  • Create an Article node. For the Body field, choose the "Full HTML" format, click the "Source" button in the CKEditor toolbar, and enter the following HTML:
    <iframe src="http://example.com/"></iframe>
<!--break-->
stuff below the fold
  • Save that article. Notice that the iframe and the "stuff below the fold" is displayed, because the article is being shown in the "Full" view mode.
  • Go to the home page. Notice that nothing is shown for the article's teaser, not even the iframe.
  • This is because for the teaser, text_summary() is called and passed a FilteredMarkup object, marking the string as safe for rendering, because it's already been processed by a text format (in this case, by "Full HTML" which allows iframes). However, text_summary() then extracts the part before <!--break-->, and returns it as a plain string. Because it's a plain string, not a MarkupInterface object, when it gets rendered it gets passed to Xss::filterAdmin(), which strips out the iframe.
  • We're running into this problem in #2940029-89: Add an input filter to display embedded Media entities, where the iframe is a rendered YouTube video. In other words, this bug prevents the "Media Embed" filter from working as expected when embedded videos should otherwise be displayed in a content teaser.

Proposed resolution

Fix text_summary() to return a MarkupInterface object when a MarkupInterface object is passed in.

Remaining tasks

This issue is tagged with "Needs security review", because we should make sure that the proposed resolution is in fact a safe thing to do. In other words, if an HTML string has been determined to be safe to render, is it valid to assume that a fragment of it is also safe to render?

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#35 3067124-35.patch1.41 KBjunaidpv
#26 3067124_d_version.png66.04 KBnitin_lama
#26 3067124_ss_node.png90.18 KBnitin_lama
#26 3067124_ss_home.png64.83 KBnitin_lama
#26 3067124_94_after_patch.png55.23 KBnitin_lama
#26 3067124_94_before_patch.png57.41 KBnitin_lama
#24 3067124-24.patch1.59 KBjunaidpv
#18 3067124-18.patch1.59 KBhmendes
#12 Before--patch.jpg239.41 KBranjith_kumar_k_u
#12 After---patch.jpg251.83 KBranjith_kumar_k_u
#7 Screen Shot 2020-07-24 at 11.16.49 AM.png283.07 KBtanubansal
#7 Screen Shot 2020-07-24 at 11.16.41 AM.png276.75 KBtanubansal
#4 text_summary_markup_1.patch1.62 KBruuds
#3 text_summary_markup.patch1.87 KBruuds

Issue fork drupal-3067124

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

effulgentsia commented
Issue summary: View changes
ruuds’s picture

ruuds commented
Status: Active » Needs review
StatusFileSize
new text_summary_markup.patch1.87 KB

See the attached patch which solves this issue for me.

When not using a <!--break-->, https://www.drupal.org/project/drupal/issues/221257 will still be an issue in some cases.

ruuds’s picture

ruuds commented
StatusFileSize
new text_summary_markup_1.patch1.62 KB

Sorry, that's not correct. Although it solved the issue, text_summary should only return a MarkupInterface when $text is also a MarkupInterface. As stated...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tanubansal’s picture

tanubansal commented
StatusFileSize
new Screen Shot 2020-07-24 at 11.16.41 AM.png276.75 KB
new Screen Shot 2020-07-24 at 11.16.49 AM.png283.07 KB

Tested via below mentioned steps on 9.1
1. Create an Article node. For the Body field, choose the "Full HTML" format, click the "Source" button in the CKEditor toolbar, and enter the following HTML:

stuff below the fold

2. Save that article. Notice that the iframe and the "stuff below the fold" is displayed, because the article is being shown in the "Full" view mode.
3. Go to the home page. Notice that nothing is shown for the article's teaser, not even the iframe.

Same issue can be replicated on drupal 9.1
Can someone provide the patch for 9.1?

ruuds’s picture

ruuds commented

I've retested the patch on 9.1.x, which succeeded. See #4.

tanubansal’s picture

tanubansal commented

Thanks @Ruuds
After adding a patch, its working fine
RTBC + 1

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Related issues: +#2922569: Text trimmed by "Trimmed" or "Summary or trimmed" field formatters will get run through Xss::filter() potentially changing their content

I think this is addressing the same problem as a much older issue:

#2922569: Text trimmed by "Trimmed" or "Summary or trimmed" field formatters will get run through Xss::filter() potentially changing their content

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new After---patch.jpg251.83 KB
new Before--patch.jpg239.41 KB

The last patch works fine,these are the changes

Before patch
before patch

After patch
after patch

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Woah, I ran into this same problem in a migration and found this and #2922569: Text trimmed by "Trimmed" or "Summary or trimmed" field formatters will get run through Xss::filter() potentially changing their content. Surprising that so few of us are affected.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Bug Smash Initiative

Adding to the Bug Smash Initiative because this is clearly a bug that should be smashed. Once security review is done, it'll need tests.

volkswagenchick’s picture

volkswagenchick
she/her
Primary language English
Location San Francisco Bay Area
commented
Issue tags: +NorthAmerica2021

Adding NorthAmerica2021 tag for visbility.

DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

hmendes’s picture

hmendes commented
StatusFileSize
new 3067124-18.patch1.59 KB

Did a re-roll of the patch from #4 to the 9.4.x version

quietone’s picture

quietone commented
Issue tags: +Needs tests
Related issues: +#2835615: TextTrimmedFormatter not trimming at periods followed by non-breaking space, +#221257: text_summary() should output valid HTML and Unicode text

This was the daily random bug a few days ago in #bugsmash. Adding some related issues. And tagging for tests.

quietone’s picture

quietone commented
Status: Needs review » Needs work

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

junaidpv’s picture

junaidpv
Location Kannur, Kerala
commented
Status: Needs work » Needs review
StatusFileSize
new 3067124-24.patch1.59 KB

Just a reroll of #18 for current 9.5.x

Status: Needs review » Needs work

The last submitted patch, 24: 3067124-24.patch, failed testing. View results

nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: Unassigned » nitin_lama
StatusFileSize
new 3067124_94_before_patch.png57.41 KB
new 3067124_94_after_patch.png55.23 KB
new 3067124_ss_home.png64.83 KB
new 3067124_ss_node.png90.18 KB
new 3067124_d_version.png66.04 KB

Patch #18 working fine for 9.4.x version

Before:
validation

After:
validation

I tried replicating the issue on 9.5.10-dev, 10.1.0-dev, 11.0-dev and it seems like the issue is not existing on these versions, not sure why. I've done the steps mentioned in the IS. I am attaching the ss for reference.

Homepage:
validation

Node page:
validation

Version:
validation

nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: nitin_lama » Unassigned

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented

Triaging my issue queue and the code in question is still relatively the same, though this is turning into a service helper in #3568387: Move text_summary to TextSummary service and deprecate

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

sivaji_ganesh_jojodae opened merge request !14857

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs work » Needs review

I’ve created a draft MR for this issue to preserve MarkupInterface when text_summary() receives safe markup.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

@sivaji_ganesh_jojodae same deal as the other ones

This was tagged for tests which were skipped

Also please verify pipeline is passing too before review.

And have to ask did you review the changes or take the last patch for an MR?
Will help with the review process

Thanks

godotislate’s picture

godotislate
he/him
commented

Since #3568387: Move text_summary to TextSummary service and deprecate is in, this work will need to be done in the new TextSummary::generate() method that replaces text_summary().

junaidpv’s picture

junaidpv
Location Kannur, Kerala
commented
StatusFileSize
new 3067124-35.patch1.41 KB

Re-rolled #24 for 11.3.x