Xss::filterAdmin() strips namespaced attributes from elements [#2556051]

Problem/Motivation

#2466931: Valid Twig syntax is incorrectly escaped in Views rewrites introduced Xss::filterAdmin() as a #post_render callback in \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace(). This is stripping out namespaced attributes added by the RDF module.

Using viewsTokenReplace() replaces the token with proper value but it does not repects foaf vocabulary specification. When you render the image using viewsTokenReplace(), this is what the HTML looks like:

When I am rendering the image using renderRoot() this is what the HTML looks like:

Proposed resolution

Debug and fix the bug.

Steps to replicate the bug

See #6

Comment	File	Size	Author
#17	interdiff-15-17.txt	954 bytes	mpdonadio
#17	xss_filteradmin_-2556051-17.patch	1.51 KB	mpdonadio
#17	xss_filteradmin_-2556051-test-only.patch	1003 bytes	mpdonadio
#15	xss_filteradmin_-2556051-test-only.patch	923 bytes	mpdonadio
#8	viewstokenreplace_not-2556051-8.patch	582 bytes	subhojit777
#3	Views_base_url_test___views_base_url.png	111.68 KB	subhojit777
#2	Views_base_url_test___views_base_url.png	111.68 KB	subhojit777
#2	test_article___views_base_url.png	112.89 KB	subhojit777
#2	Views_base_url_test___views_base_url.png	120.49 KB	subhojit777
	localhost_views-base-url-8_sites_default_files_simpletest_verbose_Drupal_views_base_url_Tests_ViewsBaseUrlFieldTest-34-1_html.png	96.94 KB	subhojit777
	Views_base_url_test___Drupal.png	116.46 KB	subhojit777

Comments

Comment #1

23 August 2015 at 14:10

subhojit777 created an issue. See original summary.

Comment #2

subhojit777

he/him

Bengali

commented 23 August 2015 at 17:51

Status	File	Size
new	Views_base_url_test___views_base_url.png	120.49 KB
new	test_article___views_base_url.png	112.89 KB
new	Views_base_url_test___views_base_url.png	111.68 KB

2 files were hidden/shown/deleted

Status	File	Size
hidden	Views_base_url_test___Drupal.png	116.46 KB
hidden	localhost_views-base-url-8_sites_default_files_simpletest_verbose_Drupal_views_base_url_Tests_ViewsBaseUrlFieldTest-34-1_html.png	96.94 KB

I changed the views setting and now I am showing images both from direct rendering and token rendering. foaf:Image is added to direct rendering but not in case of token rendering.
rdf schema missing

mpdonadio in IRC said that this has nothing to do with views module, and there might be some problem with rdf module and he added to look into rdf_preprocess_image(). I debugged $variables array. Attribute foaf:Image was added to array during token rendering of image, but property schema:image is not present. schema:image property is present if I see the image in node page.

Comment #3

subhojit777

he/him

Bengali

commented 23 August 2015 at 17:53

Status	File	Size
new	Views_base_url_test___views_base_url.png	111.68 KB

$variables array of rdf_preprocess_image() in node page:
schema present in node page

$variables array of rdf_preprocess_image() in views page:
variables array in views page

Comment #4

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 23 August 2015 at 20:04

@subhojit777, can you export a view that generates the correct and incorrect output as in #2, but not using your custom module, and attach it to the issue? This does look like it could be a Views problem, but it is a little hard to tell where.

Comment #5

subhojit777

he/him

Bengali

commented 24 August 2015 at 04:43

Thanks @mpdonadio will look into it tonight.

Comment #6

subhojit777

he/him

Bengali

commented 24 August 2015 at 09:38

Steps:

Create few articles and upload images in them OR you can devel generate articles
Import views using the code below (note that there are two kinds, one working properly and other which is buggy)
Inspect element images in the view
The views which is rendering image directly will have typeof as foaf:Image
The views which is rendering image using tokens will have typeof as Image

Views with image rendering properly:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_image
  module:
    - image
    - node
    - user
id: test
label: test
module: views
description: ''
tag: ''
base_table: node_field_data
base_field: nid
core: 8.x
display:
  default:
    display_plugin: default
    id: default
    display_title: Master
    position: 0
    display_options:
      access:
        type: perm
        options:
          perm: 'access content'
      cache:
        type: tag
        options: {  }
      query:
        type: views_query
        options:
          disable_sql_rewrite: false
          distinct: false
          replica: false
          query_comment: ''
          query_tags: {  }
      exposed_form:
        type: basic
        options:
          submit_button: Apply
          reset_button: false
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          expose_sort_order: true
          sort_asc_label: Asc
          sort_desc_label: Desc
      pager:
        type: full
        options:
          items_per_page: 10
          offset: 0
          id: 0
          total_pages: null
          expose:
            items_per_page: false
            items_per_page_label: 'Items per page'
            items_per_page_options: '5, 10, 25, 50'
            items_per_page_options_all: false
            items_per_page_options_all_label: '- All -'
            offset: false
            offset_label: Offset
          tags:
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'
          quantity: 9
      style:
        type: default
      row:
        type: fields
        options:
          default_field_elements: true
          inline: {  }
          separator: ''
          hide_empty: false
      fields:
        title:
          id: title
          table: node_field_data
          field: title
          entity_type: node
          entity_field: title
          label: ''
          alter:
            alter_text: false
            make_link: false
            absolute: false
            trim: false
            word_boundary: false
            ellipsis: false
            strip_tags: false
            html: false
          hide_empty: false
          empty_zero: false
          settings:
            link_to_entity: true
          plugin_id: field
          relationship: none
          group_type: group
          admin_label: ''
          exclude: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: true
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_alter_empty: true
          click_sort_column: value
          type: string
          group_column: value
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
        field_image:
          id: field_image
          table: node__field_image
          field: field_image
          relationship: none
          group_type: group
          admin_label: ''
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: 0
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          click_sort_column: target_id
          type: image
          settings:
            image_style: ''
            image_link: ''
          group_column: ''
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
          plugin_id: field
      filters:
        status:
          value: true
          table: node_field_data
          field: status
          plugin_id: boolean
          entity_type: node
          entity_field: status
          id: status
          expose:
            operator: ''
          group: 1
      sorts:
        created:
          id: created
          table: node_field_data
          field: created
          order: DESC
          entity_type: node
          entity_field: created
          plugin_id: date
          relationship: none
          group_type: group
          admin_label: ''
          exposed: false
          expose:
            label: ''
          granularity: second
      title: test
      header: {  }
      footer: {  }
      empty: {  }
      relationships: {  }
      arguments: {  }
      display_extenders: {  }
    cache_metadata:
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url.query_args
        - 'user.node_grants:view'
        - user.permissions
      cacheable: false
  page_1:
    display_plugin: page
    id: page_1
    display_title: Page
    position: 1
    display_options:
      display_extenders: {  }
      path: test
    cache_metadata:
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url.query_args
        - 'user.node_grants:view'
        - user.permissions
      cacheable: false

Views with image rendering incorrectly:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_image
  module:
    - image
    - node
    - user
id: test_buggy
label: 'test buggy'
module: views
description: ''
tag: ''
base_table: node_field_data
base_field: nid
core: 8.x
display:
  default:
    display_plugin: default
    id: default
    display_title: Master
    position: 0
    display_options:
      access:
        type: perm
        options:
          perm: 'access content'
      cache:
        type: tag
        options: {  }
      query:
        type: views_query
        options:
          disable_sql_rewrite: false
          distinct: false
          replica: false
          query_comment: ''
          query_tags: {  }
      exposed_form:
        type: basic
        options:
          submit_button: Apply
          reset_button: false
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          expose_sort_order: true
          sort_asc_label: Asc
          sort_desc_label: Desc
      pager:
        type: full
        options:
          items_per_page: 10
          offset: 0
          id: 0
          total_pages: null
          expose:
            items_per_page: false
            items_per_page_label: 'Items per page'
            items_per_page_options: '5, 10, 25, 50'
            items_per_page_options_all: false
            items_per_page_options_all_label: '- All -'
            offset: false
            offset_label: Offset
          tags:
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'
          quantity: 9
      style:
        type: default
      row:
        type: fields
        options:
          default_field_elements: true
          inline: {  }
          separator: ''
          hide_empty: false
      fields:
        title:
          id: title
          table: node_field_data
          field: title
          entity_type: node
          entity_field: title
          label: ''
          alter:
            alter_text: false
            make_link: false
            absolute: false
            trim: false
            word_boundary: false
            ellipsis: false
            strip_tags: false
            html: false
          hide_empty: false
          empty_zero: false
          settings:
            link_to_entity: true
          plugin_id: field
          relationship: none
          group_type: group
          admin_label: ''
          exclude: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: true
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_alter_empty: true
          click_sort_column: value
          type: string
          group_column: value
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
        field_image:
          id: field_image
          table: node__field_image
          field: field_image
          relationship: none
          group_type: group
          admin_label: ''
          label: ''
          exclude: true
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: 0
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          click_sort_column: target_id
          type: image
          settings:
            image_style: ''
            image_link: ''
          group_column: ''
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
          plugin_id: field
        nothing:
          id: nothing
          table: views
          field: nothing
          relationship: none
          group_type: group
          admin_label: ''
          label: ''
          exclude: false
          alter:
            alter_text: true
            text: '{{ field_image }}'
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: 0
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: false
          plugin_id: custom
      filters:
        status:
          value: true
          table: node_field_data
          field: status
          plugin_id: boolean
          entity_type: node
          entity_field: status
          id: status
          expose:
            operator: ''
          group: 1
      sorts:
        created:
          id: created
          table: node_field_data
          field: created
          order: DESC
          entity_type: node
          entity_field: created
          plugin_id: date
          relationship: none
          group_type: group
          admin_label: ''
          exposed: false
          expose:
            label: ''
          granularity: second
      title: test
      header: {  }
      footer: {  }
      empty: {  }
      relationships: {  }
      arguments: {  }
      display_extenders: {  }
    cache_metadata:
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url.query_args
        - 'user.node_grants:view'
        - user.permissions
      cacheable: false
  page_1:
    display_plugin: page
    id: page_1
    display_title: Page
    position: 1
    display_options:
      display_extenders: {  }
      path: test-buggy
    cache_metadata:
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url.query_args
        - 'user.node_grants:view'
        - user.permissions
      cacheable: false

Comment #7

subhojit777

he/him

Bengali

commented 24 August 2015 at 09:53

Issue summary:

View changes

Comment #8

subhojit777

he/him

Bengali

commented 24 August 2015 at 10:37

Status:	Active	» Needs review
Issue tags:		+SafeMarkup
Parent issue:		» #2297711: Fix HTML escaping due to Twig autoescape

Status	File	Size
new	viewstokenreplace_not-2556051-8.patch	582 bytes

Comment #9

24 August 2015 at 11:02

Status:

Needs review

» Needs work

The last submitted patch, 8: viewstokenreplace_not-2556051-8.patch, failed testing.

Comment #10

subhojit777

he/him

Bengali

commented 24 August 2015 at 11:10

Just want to know whether I am going in the right direction before proceeding any further.

Comment #11

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 24 August 2015 at 12:26

@subhojit777, I don't think that is the right path. We need to trace this through the render process and see where that attribute is getting messed up. I will take a look at it tonight (I'm UTC-4).

Comment #12

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 24 August 2015 at 23:15

Assigned:	Unassigned	» mpdonadio
Issue tags:	-SafeMarkup

Confirmed. Let me play with this tonight to see where things are breaking down in the render process.

Comment #13

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 24 August 2015 at 23:58

Title:	viewsTokenReplace() not respecting foaf vocabulary specification for images	» Xss::filterAdmin() strips namespaced attributes from elements
Component:	views.module	» base system
Priority:	Normal	» Major
Issue summary:	View changes
Issue tags:		+SafeMarkup, +Needs issue summary update, +Needs beta evaluation, +Needs tests
Related issues:		+#2466931: Valid Twig syntax is incorrectly escaped in Views rewrites, +#2501403: Document SafeMarkup::set in Xss::filter

Looks like @subhojit777 was on the right path. The patch in #2466931: Valid Twig syntax is incorrectly escaped in Views rewrites introduced this problem. The Xss::filterAdmin() #post_render callback is stripping the namespace from the RDF attribute.

The culprit is the final preg_replace_callback():

    // Strip any tags that are not in the whitelist, then mark the text as safe
    // for output. All other known XSS vectors have been filtered out by this
    // point and any HTML tags remaining will have been deliberately allowed, so
    // it is acceptable to call SafeMarkup::set() on the resultant string.
    $return preg_replace_callback('%
      (
      <(?=[^a-zA-Z!/])  # a lone <
      |                 # or
      <!--.*?-->        # a comment
      |                 # or
      <[^>]*(>|$)       # a string that starts with a <, up until the > or the end of the string
      |                 # or
      >                 # just a >
      )%x', $splitter, $string);

The comment there is from #2501403: Document SafeMarkup::set in Xss::filter, but the SafeMarkup::set() was removed in #2506195: Remove SafeMarkup::set() from Xss::filter(). Can't quite tell what is going wrong in the regex there.

Going to make a unit test that demonstrates this.

Comment #14

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 25 August 2015 at 00:13

Assigned:	mpdonadio	» Unassigned
Issue tags:	-Needs tests

Enough for me for the night. Here is a test that demonstrates the problem. Run `phpunit tests/Drupal/Tests/Component/Utility/XssTest.php` from CLI to see what happens.

Not really sure what the proper parent issue is here, if if matters.

Comment #15

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 25 August 2015 at 00:14

Status:

Needs work

» Needs review

Status	File	Size
new	xss_filteradmin_-2556051-test-only.patch	923 bytes

Comment #16

25 August 2015 at 00:56

Status:

Needs review

» Needs work

The last submitted patch, 15: xss_filteradmin_-2556051-test-only.patch, failed testing.

Comment #17

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 25 August 2015 at 01:48

Status:

Needs work

» Needs review

Status	File	Size
new	xss_filteradmin_-2556051-test-only.patch	1003 bytes
new	xss_filteradmin_-2556051-17.patch	1.51 KB
new	interdiff-15-17.txt	954 bytes

Root cause is the attribute list that can contain semicolons in Xss::attributes() needs to be expanded. Not sure what others are needed for RDF.

Comment #18

25 August 2015 at 02:13

The last submitted patch, 17: xss_filteradmin_-2556051-test-only.patch, failed testing.

Comment #19

olli commented 25 August 2015 at 04:30

Comment #20

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 25 August 2015 at 10:50

Status:

Needs review

» Closed (duplicate)

Yeah, this is a dup of #2544110: XSS attribute filtering is inconsistent and strips valid attributes.

Xss::filterAdmin() strips namespaced attributes from elements

Problem/Motivation

Proposed resolution

Steps to replicate the bug

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Parent issue

Related issues

News items

Our community

Documentation

Drupal code base

Governance of community