Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bbell171 created an issue. See original summary.

bbell171’s picture

by changing line 198 in PollViewForm.php to

'#prefix' => '<dt class="choice-title">' . SafeMarkup::escape($options[$pid]) . "</dt>\n" . '<dd class="choice-result">',

HTML characters will display

dakala’s picture

Assigned: Unassigned » dakala
dakala’s picture

Attached patch fixes incorrect rendering of HTML entities in poll titles and answers.

edurenye’s picture

Status: Active » Needs review

Move to needs review to run the tests.

edurenye’s picture

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

This needs tests.

The last submitted patch, 4: special-html-characters-2550345-4.patch, failed testing.

The last submitted patch, 4: special-html-characters-2550345-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Pretty sure that this needs updates with the recent core changes. We also need tests.

Berdir’s picture

Status: Needs review » Needs work
dakala’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

Re-rolled patch after rework and added unit test.

Berdir’s picture

Status: Needs review » Needs work
+++ b/src/Controller/PollController.php
@@ -26,7 +25,6 @@ class PollController extends ControllerBase {
   public function pollTitle(PollInterface $poll) {
-    return Xss::filter($poll->label());
+    return $this->t($poll->label());
   }

No t(). That's for interface translation.

Berdir’s picture

We should remove all Xss::filter() and checkPlain() calls from the module.

The extend the test to also cover choices.

We need to make sure that both the question and the choices are escaped once and only once in all places (form, results, admin).

dakala’s picture

After upgrading to 8.0.2 and removing all Xss::filter() and checkPlain() calls, I noticed that all labels are being escaped properly without any explicit calls to Html::escape() or any other methods. Not sure if any changes in 8.0.2 have rectified the problem. I'll go ahead with tests to cover choices first.

Berdir’s picture

Hm.

what happens if you add something like <script>alert('XSS')</script> in there? Is that removed? It should be...

dakala’s picture

I'm afraid alert('XSS'); is not removed from the title or choice fields. Interestingly, it's the same with a basic page node too. alert('XSS'); is displayed as is, in both title and body fields.

Berdir’s picture

It's perfectly fine if that is shown. As long as it's not executed. The important part is that the script tags around it are removed or escaped.

Berdir’s picture

Ah, I just noticed I was missing code tags in my comment. Sorry :) See the edit

dakala’s picture

Yeah, it's still the same with the

tags added. The text is displayed with the tags but not executed - both poll and page node.
dakala’s picture

@berdir: Looks like we no longer require special handling of these characters with 8.0.2 version of core. Added more tests.

dakala’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/poll.tokens.inc
    @@ -69,7 +68,7 @@ function poll_tokens($type, $tokens, array $data, array $options, BubbleableMeta
             case 'winner':
               if (isset($winner)) {
    -            $replacements[$original] = $sanitize ? Xss::filter($winner) : $winner;
    +            $replacements[$original] = $sanitize ? $winner : $winner;
               }
    

    $sanitize as an option no longer exists. Remove the check and the variable completely.

  2. +++ b/src/Form/PollViewForm.php
    @@ -233,7 +232,7 @@ class PollViewForm extends FormBase {
             '#theme' => 'poll_meter',
    -        '#prefix' => '<dt class="choice-title">' . SafeMarkup::checkPlain($options[$pid]) . "</dt>\n" . '<dd class="choice-result">',
    +        '#prefix' => '<dt class="choice-title">' . $options[$pid] . "</dt>\n" . '<dd class="choice-result">',
             '#suffix' => "</dd>\n",
             '#display_value' => t('@percentage%', array('@percentage' => $percentage)) . $display_votes,
    

    Not sure if this one is safe since it's part of #prefix.

    This is already using a temlate, let's just move the prefix and suffix inside the template and provide it as #title => $options[$pid]

  3. +++ b/src/Tests/PollBlockTest.php
    @@ -44,7 +42,7 @@ class PollBlockTest extends PollTestBase {
         // the choices might.
    -    $this->assertText($poll->label(), SafeMarkup::format('@title Poll appears in block.', array('@title' => $this->poll->label())));
    +    $this->assertText($poll->label(), t('@title Poll appears in block.', array('@title' => $this->poll->label())));
    

    t() shouldn't be used for assertion messages, so this was actually correct. You can also just leave out the @title part, Drupal 8 now shows the original value if it can't be found.

tduong’s picture

Fixed as suggested above + some refactoring.

Berdir’s picture

Status: Needs review » Needs work
+++ b/src/Tests/PollHtmlSpecialCharactersTest.php
@@ -11,7 +11,7 @@
  */
-class PollHtmlSpecialCharactersTest extends PollTestBase {
+class PollHtmlSpecialCharactersTest  extends PollTestBase {

double space?

+++ b/templates/poll-meter.html.twig
@@ -15,6 +15,7 @@
+ * - title: The choice title of a poll.

I know I said that above. But, I think we don't use "choice title" usually, just choice. So lets use #choice and so on.

Berdir’s picture

Issue tags: -Needs tests
tduong’s picture

  • Berdir committed b6a61a0 on 8.x-1.x authored by tduong
    Issue #2550345 by dakala, tduong: Special characters in HTML are not...
Berdir’s picture

Status: Needs review » Fixed

Thanks, committed.

tduong’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

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

Bojan Zivkov’s picture

This problem still remains if poll is used as entity reference field in another content with "inline entity form" set as widget in form display.

ahmadhalah’s picture

The problem is still for me I deleted Html::escape from the reprocess hook then its worked fine