<strong>Strip HTML tags for issue titles in listings</strong>

Um.

I'm trying to help assure that critical issues get the attention they need, and by Drupal.org standards, critical means "keeping people from getting work done" (or security). Since this is neither, I'm setting the status back to normal.

I'm also tagging and putting in the "needs review" status, since this post so eloquently confirms the issue.

My mistake. Looks like JS isn't parsed: https://drupal.org/project/issues/search/1481160 Would help if I read the description before freaking out. :D

Unpublishing because this seems like a security issue with one of the underlying modules.

Oh, I just read the entire issue summary too, so not actually a security issue.

SearchAPI uses EntityAPI for Views. I think the relevant code is in EntityFieldHandlerHelper::render(), http://drupalcode.org/project/entity.git/blob/refs/heads/7.x-1.x:/views/...

Either $handler->get_value($values); should return the value more-filtered, which may have side effects; or array('html' => TRUE) should be taken out of render_entity_link()

The problem is that entity_views_handler_field_text assumes that the input is HTML, and runs it through:

  /**
   * Render a single field value.
   */
  public function render_single_value($value, $values) {
    return $this->sanitize_value($value, 'xss');
  }

In general, the difference between plaintext and HTML doesn't seem to have been properly thought through in the Entity API views integration.

Looking at the field handlers, defined in entity_views_get_field_handlers():

      'text'         => 'entity_views_handler_field_text',
      'token'        => 'entity_views_handler_field_text',

... it seems like the intent was that entity_views_handler_field_text handles only plain-text. So we could change this $this->sanitize_value($value, 'xss'); into a $this->sanitize_value($value);. But this is going to break any text field that is actually meant to contain some HTML.

For example, the $node->log property apparently is meant to be HTML (even if it is not documented anywhere), because it is passed to filter_xss() in node_revision_overview():

'<p class="revision-log">' . filter_xss($revision->log) . '</p>'

Alternatively, we could introduce a 'html' => TRUE key in the property info of type 'text' and properly handle that, but whatever default value we give to this property we are bound to break stuff.

'html' => TRUE
Yeah, something like this seems to be feasible - however I'm not sure it's necessary to do do it now for d7, though.

In Entity API property info we've already keys 'sanitzed', 'sanitze', together with the wrapper's options 'decode' and 'sanitize' it should be sufficient to control the output to be plain text or sanitized html.

So afaik making use of the wrapper's sanitization method in the views field handler combined with providing 'check_plain' as method should be enough.

Changing status per #2.

...coming from the now closed as a dupe of this one #2156041: Display issue titles that contain html tags properly in the issue queue.

Here's a screenshot from that issue of how #550428: Tags like "&nbsp;" or "<p>&nbsp;</p>" or "<br />" added to empty textareas. is displayed in the issue queue:

I actually don't think this belongs either to EntityAPI or views. It is legitimate for site builders to defined view fields that can contain HTML.
Concerning d.o. implementation, Summary field should be with the following settings:

• Strip HTML tags
• Allow these tags: <a>

We discussed this with fago, klauis, jthorson and valthebald. The conclusion was to modify generic views handler to be more context aware and check_plain() the output.

Does #18 move this back to Entity API?

It does!

After a long discussion with valthebald we pointed out that it will be enough to add a getter callback to the node title, which will run it trough on check_plain().

Iv'e updated the tests for this:

1. Changed the basig value getter test to check the body field, not the title.
2. Added a test for the tile field, which should be sanitized by default.
3. Added a test for the title field value raw getter function.
4. Removed the title test with the sanitize option, since it's the default behavior.

I talked about this with valthebald and we found out, that however the title should be sanitized by default, we should have a way to get the raw value. The tests now cover these needs.

(The attached patch should fail, it don't contains the previous fix.)

Here is the full patch, with a getter callback, and a raw getter callback.

The last patch contains an accidental kpr() call. But why didn't it break the test?

Here is the fixed one.

That's almost good, but please add a docblock for entity_node_get_raw_title()

Oh.. sorry, here is the new version.

Looks great to me.
@klausi: we have discussed and tried alternatives to the approach implemented in the last patch (like pass parameters to entity_views_handler_field_text::render_single_value() via hook_entity_property_info() in node.info.inc, but it render() has no direct access to entity definition.

1. +++ b/entity.test
   @@ -742,14 +742,17 @@ class EntityMetadataTestCase extends EntityWebTestCase {
   +    // Titles should be sanitized by default.
   +    $this->assertEqual(check_plain('<b>Is it bold?</b>'), $wrapper->title->value(), 'Title is sanitized.');
   

   Nope, that's not how the API works.

2. +++ b/entity.test
   @@ -758,7 +761,6 @@ class EntityMetadataTestCase extends EntityWebTestCase {
   -    $this->assertEqual(check_plain('<b>Is it bold?<b>'), $wrapper->title->value($options), 'Getting sanitized field.');
   

   This is how it's working. The existing test coverage shows the problem is not in the wrapper / property info.

it's probably the views handler not leveraging wrapper sanitization, i.e. entity_views_handler_field_text (or even more handlers) should use it.

Sat together with drunken_monkey to see how to fix this. Attached patch should be the right fix - but it needs testing. Also, there are more sanitize_value() calls in other field handler classes which need to be removed and get replaced by the comment now. Sanitization is taken care of by the wrapper now.

We should verify this change does not break HTML output of e.g. body fields (it should be fine though).

ops, please use this patch.

I have tested the patch from #38, it works well, the titles will be sanitized, the body texts will be displayed with the right text format.

Should we change the other handlers in this issue, or should we open a new one?

Great! It should be changed here, else it might become double sanitized for others.

Test hiccup? Marking for retest

Oh, and I confirm #38 solves d.o. issue (tested on https://sprint1-drupal.redesign.devdrupal.org/project/issues/search?proj... sandbox)

I have added the same lines to the other field handlers, but not tested it yet, also I'm not sure they are good everywhere.

The last patch passed, but I'm still not sure, that all the field handler are working right now. Can anyone suggest some scenarios which will check them?

1. +++ b/views/handlers/entity_views_handler_field_duration.inc
   @@ -123,7 +123,7 @@ class entity_views_handler_field_duration extends views_handler_field {
   +        $value .
   

   Should have a comment why it does no sanitization here.

2. +++ b/views/handlers/entity_views_handler_field_numeric.inc
   @@ -93,7 +93,9 @@ class entity_views_handler_field_numeric extends views_handler_field_numeric {
      public function render_single_value($value, $values) {
   -    return parent::render($values);
   +    // Sanitization is handled by the wrapper, see
   
   diff --git a/views/handlers/entity_views_handler_field_options.inc b/views/handlers/entity_views_handler_field_options.inc
   index a947d36..96c6999 100644
   
   +++ b/views/handlers/entity_views_handler_field_uri.inc
   @@ -93,7 +93,9 @@ class entity_views_handler_field_uri extends views_handler_field_url {
   -    return parent::render($values);
   +    // Sanitization is handled by the wrapper, see
   +    // EntityFieldHandlerHelper::get_value()
   +    return $values;
   

   This seems wrong, render does more than just sanitization. In the case of numbers I guess we can get away with just double sanitize.

3. +++ b/views/handlers/entity_views_handler_field_uri.inc
   @@ -93,7 +93,9 @@ class entity_views_handler_field_uri extends views_handler_field_url {
      public function render_single_value($value, $values) {
   -    return parent::render($values);
   +    // Sanitization is handled by the wrapper, see
   

   Again render does more.

For testing, best create a search api view that lists entities that have properties of each type + display them. Not sure which entity type would be best here.

I have reverted the render() calls, and added the comment.
I'm also created a veiw for testing, here is how it looks:

Having sanitize_value() removed from entity_views_handler_field_duration::render_single_value(), I think there's no reason for $value to sit on separate line of code.
Otherwise, looks fine for me (and still solves issue queue issue)

Yes, I agree, but I think that neither

return $this->sanitize_value($this->options['prefix'], 'xss') . $value . $this->sanitize_value($this->options['suffix'], 'xss');

or

    return $this->sanitize_value($this->options['prefix'], 'xss') . $value .
      $this->sanitize_value($this->options['suffix'], 'xss');

are better.

Makes sense. RTBC then?

Thanks, patch looks good and the results also. Thanks, for taking the time to properly test this also!

Committed.

Excellent! Thanks everyone!

Entity 1.5 is now deployed on Drupal.org.

Hooray!