Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task: this is part of the Twig autoescape meta-issue (#1825952: Turn on twig autoescape by default)
Issue priority Major: existing code incorrectly marked strings as safe. Also #2337747: HTML entities/tags in the item title are double encoded for feeds using fields, which was marked as major, was closed as a dup of this.
Prioritized changes Security change and follow up from a recent critical change.
Disruption There will be disruption for any contrib modules using format_xml_elements, which is eliminated by this change. Comment #13 suggests this to be a very limited number though it is hardly a thorough examination of contrib.

Follow-up to: #1825952: Turn on twig autoescape by default

Problem/Motivation

In #1825952: Turn on twig autoescape by default, format_xml_elements() assembles XML markup based on input and it is subsequently marked as safe. However, there is no guarantee that the rendered markup is actually safe.

This is not a security regression because the same is true in HEAD; format_xml_elements() will render whatever the caller tells it to, so it's the caller's responsibility to sanitize the input. However, this is one of the only places that we are marking as safe markup strings that are not explicitly known to be safe.

Proposed resolution

Remove format_xml_elements and use the existing preprocess functions to prepare variables for use in views-view-row-rss.html.twig.

Remaining tasks

  • Create patch
  • Update/Add tests
  • Create change record

User interface changes

None.

API changes

Remove format_xml_elements.

Postponed until

#1825952: Turn on twig autoescape by default

Files: 
CommentFileSizeAuthor
#42 2296885-42-39.interdiff.txt1.14 KBmikeker
#42 2296885-42_format-xml-elements.patch14.66 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,030 pass(es). View
#39 2296885-39-24-docs-update.interdiff.txt627 bytesmikeker
#39 2296885-39-24-fixes-tests.interdiff.txt790 bytesmikeker
#39 2296885-39_format-xml-elements.patch14.29 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,783 pass(es). View
#24 2296885-24-20.interdiff.txt5.57 KBmikeker
#24 2296885-24-remove-format_xml_elements.patch13.21 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2296885-24-remove-format_xml_elements.patch. Unable to apply patch. See the log in the details link for more information. View
#20 2296885-20-17.interdiff.txt2.07 KBmikeker
#20 2296885-20-remove-format_xml_elements.patch7.84 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,152 pass(es), 91 fail(s), and 6 exception(s). View
#17 2296885-17-14.interdiff.txt3.19 KBmikeker
#17 2296885-17-remove-format_xml_elements.patch5.57 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,639 pass(es). View
#8 2296885-8-xml-cleanup.patch5.03 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,496 pass(es). View
#7 2296885-7-xml-cleanup.patch1.84 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,182 pass(es). View

Comments

xjm’s picture

Status: Postponed » Active
dawehner’s picture

Don't we have an issue to remove format_xml_elements() already? Its not really such an helpful API and note: we do have no use for it anymore in core.

jibran’s picture

Issue tags: +SafeMarkup
mikeker’s picture

Assigned: Unassigned » mikeker

Other than format_xml_elements calling itself, the only place I could find a usage (outside of tests) was in Views. Specifically: template_preprocess_views_view_rss() and template_preprocess_views_view_row_rss(). My take is those should be handled in the template and not preprocessed. See also #2337747: HTML entities/tags in the item title are double encoded for feeds using fields.

As @dawehner said in #3, we should probably remove this function and move it's processing to the two relevant Views templates.

I'll look through the tests and see if there's anything lurking there...

mikeker’s picture

mikeker’s picture

Status: Active » Needs review
FileSize
1.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,182 pass(es). View

Attached patch cleans up Views' RSS formatting and removes the last call to format_xml_element in core outside of tests. I haven't touched format_xml_elements() yet -- wanted to see what the testbot thinks first.

If the testbot comes back green, I'll go ahead and start removing format_xml_elements from the tests and replacing it with the actual XML that should be generated in those tests.

I welcome any feedback on this approach -- I've never worked on removing things from core before... :)

mikeker’s picture

Issue consolidation: Adds the test from #2337747-12: HTML entities/tags in the item title are double encoded for feeds using fields which I'm marking as a dup of this issue as the underlying fix will take care of both.

Also, what idiot marked this as related to itself in #6? /me: ducks out of sight...

dawehner’s picture

+1 to inline the XHML, at remove the usage of format_xml_elements().

Do we consider format_xml_elements() as something removable at this point?

mikeker’s picture

@dawehner: I haven't been able to find any previous discussion about removing format_xml_elements(). Would love to hear what others think about removing it...

joelpittet’s picture

I'm for it!

Though we may need to check on contrib usage at beta stage but at the least we could deprecate it.

mikeker’s picture

Issue tags: +Needs change record

For what it's worth this, for D7, shows only six calls, all within core and Views. The list of modules tracked is by no means exhaustive, but it's not insubstantial either.

I would guess there would be minimal impact on contrib. Regardless, this would need a change record.

mikeker’s picture

FileSize
6.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,500 pass(es), 5 fail(s), and 2 exception(s). View

Rerolled against latest HEAD.

mikeker’s picture

Priority: Normal » Major

Since #2337747: HTML entities/tags in the item title are double encoded for feeds using fields was bumped to Major in comment #3 and then closed as a dup of this issue, I'm bumping this to Major as well.

Status: Needs review » Needs work

The last submitted patch, 14: 2296885-14-remove-format_xml_elements.patch, failed testing.

mikeker’s picture

FileSize
5.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,639 pass(es). View
3.19 KB

Ack, totally goofed the reroll in #14. Rerolled the reroll and removed field_langcode field_langcode_add_to_query from the test view as per this CR.

mikeker’s picture

Status: Needs work » Needs review
Mile23’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

$ git apply 2296885-17-remove-format_xml_elements.patch 
error: patch failed: core/modules/views/src/Tests/Plugin/DisplayFeedTest.php:74
error: core/modules/views/src/Tests/Plugin/DisplayFeedTest.php: patch does not apply
warning: core/modules/views/views.theme.inc has type 100755, expected 100644
error: patch failed: core/modules/views/views.theme.inc:912
error: core/modules/views/views.theme.inc: patch does not apply
mikeker’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,152 pass(es), 91 fail(s), and 6 exception(s). View
2.07 KB

Rerolled.

Also realized that I never actually removed format_xml_elements from common.inc. This patch does that as well.

mikeker’s picture

Issue summary: View changes

Updated proposed resolution and remaining tasks in the issue summary.

Status: Needs review » Needs work

The last submitted patch, 20: 2296885-20-remove-format_xml_elements.patch, failed testing.

mikeker’s picture

Issue summary: View changes

Added beta eval.

mikeker’s picture

Status: Needs work » Needs review
FileSize
13.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2296885-24-remove-format_xml_elements.patch. Unable to apply patch. See the log in the details link for more information. View
5.57 KB

In re-reading this issue, it looks like I was waiting for... something... before removing format_xml_elements. The attached patch removes the code and fixes the tests that were relying on it.

In the CR, we will need to mention that contrib modules subclassing the Views RSS style plugin will need to tweak the output of getChannelElements to return a render array instead of an array to be consumed by format_xml_elements.

mikeker’s picture

Issue summary: View changes

Fixed HTML in beta eval, tweaking wording in "disruption" section.

Status: Needs review » Needs work

The last submitted patch, 24: 2296885-24-remove-format_xml_elements.patch, failed testing.

The last submitted patch, 24: 2296885-24-remove-format_xml_elements.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review

Since the testbot came back green after #28.

mikeker’s picture

Patch still applies cleanly -- rerunning tests, just in case.

mikeker’s picture

Assigned: mikeker » Unassigned
Issue tags: -Needs reroll

Patch applies cleanly to today's HEAD and the testbot is green as of 27 April.

Sorry, forgot to unassign myself and remove the reroll tag.

mikeker’s picture

Issue tags: -Needs change record

Heh... Also forgot that I started a draft CR about a month ago:

https://www.drupal.org/node/2468139

catch’s picture

Title: Evaluate SafeMarkup use in format_xml_elements() » Remove format_xml_elements()

I'd personally be OK with removing format_xml_elements(), the chances of that being used anywhere are really minimal and I think it does more harm staying in since it's a red herring compared to serializer etc. Could use a second opinion from another committer though, because I do love removing things.

The issue for refactoring Views RSS is #2003108: Switch Views RSS to use Serializer/Zend by the way.

mikeker’s picture

@catch: Agreed, chances for disruption are minimal and the ability to override the default RSS markup is nice. Thanks for the link to the related issue, I've added a link to this issue over there.

xjm’s picture

I agree with the beta evaluation and @catch's assessment in #34; I think it's actually a good thing to have a BC break in this case.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1 to remove it. Xml is less common than before, a template also works pretty good.
In case you want to, there is also tools like https://github.com/nb/oxymel and https://github.com/iwyg/xmlbuilder to make xml building really easy

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2296885-24-remove-format_xml_elements.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
14.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,783 pass(es). View
790 bytes
627 bytes

Rerolled and fixed tests. #2424533: Copy views templates to Classy added views-view-row-rss.html.twig (which is changed in this patch) to Classy. However there are no classes added by this template (it's an RSS feed, after all) so I removed it as part of this patch. See the fixes-tests interdiff.

There is also a minor doc change as shown in the docs-update interdiff.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I totally agree that there is no reason to have that template as part of classy.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

http://www.drupalcontrib.org/api/drupal/drupal%21includes%21common.inc/f... shows that in Drupal 7 only views and core used the method I agree that removing this makes sense.

I don't think this issue should remove core/themes/classy/templates/views/views-view-row-rss.html.twig since that is out of scope. Having the templates all in classy is not just about the templates using a class. It is about making things very easy to find.

mikeker’s picture

Status: Needs work » Needs review
FileSize
14.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,030 pass(es). View
1.14 KB

#41: Makes sense.

This patch updates the template in classy to use the same markup as that in core/modules/views/templates/views-view-row-rss.html.twig.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This patch updates the template in classy to use the same markup as that in core/modules/views/templates/views-view-row-rss.html.twig.

+1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed cc55e5c and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/src/Tests/Ajax/DialogTest.php
@@ -174,6 +174,7 @@ public function testDialog() {
+

Unrelated change - removed on commit.

  • alexpott committed 90433ce on 8.0.x
    Issue #2296885 by mikeker, dawehner: Remove format_xml_elements()
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

The change record here was still in "Draft" state. I went ahead and published it now.