Problem/Motivation

When viewing the forum list, if there are replies that have not been viewed, the markup for the list of links to the new replies is escaped. See attached screenshot.

Proposed resolution

Convert the 'data' value from a string to an array that uses '#markup' on line 509 of forum.module

Remaining tasks

  • Fix issue
  • Create test coverage
  • Create patch
  • Review patch
  • Address #20, #22, and #24
  • Commit
Files: 
CommentFileSizeAuthor
#35 forum_new_replies-2488886-35.patch1.12 KBjoelpittet
#32 2488886-30-tests-only.patch527 bytesmikeker
#26 2488886-26-_forum-list-escaping.patch1.12 KBmikeker
#14 forum-2488886-14.patch3.58 KBcolinafoley
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,645 pass(es). View
#14 forum-2488886-test-only-14.patch3.06 KBcolinafoley
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,055 pass(es), 1 fail(s), and 0 exception(s). View
#12 interdiff.txt2.25 KBcolinafoley
#12 forum-2488886-12.patch3.58 KBcolinafoley
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,056 pass(es). View

Comments

colinafoley’s picture

Issue summary: View changes

I have solved the issue and written test coverage.

colinafoley’s picture

Issue summary: View changes
colinafoley’s picture

Issue summary: View changes
FileSize
3.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,820 pass(es), 1 fail(s), and 0 exception(s). View

Here's the patch.

colinafoley’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: forum-2488886-3.patch, failed testing.

Status: Needs work » Needs review

colinafoley queued 3: forum-2488886-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: forum-2488886-3.patch, failed testing.

colinafoley’s picture

FileSize
3.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,064 pass(es), 1 fail(s), and 0 exception(s). View

Rewrote assert to rely on xpath instead of strings.

Cottser’s picture

Status: Needs work » Needs review

:)

Wim Leers’s picture

Status: Needs review » Needs work

Looks great, pretty much only nitpicks :) Hopefully it comes back green!

  1. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -17,6 +17,20 @@
    +   * User account with limited access to forum pages
    ...
    +   * User account with limited access to forum pages
    

    Nit: Missing trailing periods.

    More important: they have different sets of permissions, but have the same description.

  2. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -27,14 +41,15 @@ protected function setUp() {
         // Create a test user.
    

    Now 2 test users are being created.

  3. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -68,4 +83,42 @@ function testForumIndexStatus() {
    +    // Login as the forum user to create a reply
    ...
    +    // Verify the new post link is not raw
    

    Missing trailing period.

  4. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -68,4 +83,42 @@ function testForumIndexStatus() {
    +    // Mockup a comment
    +    $edit = array();
    +    $edit['comment_body[0][value]'] = $this->randomMachineName();
    +
    +    // Create comment
    

    Just saying "Create comment." once should be sufficient.

  5. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -68,4 +83,42 @@ function testForumIndexStatus() {
     }
    +
    

    Extraneous newline addition.

The last submitted patch, 8: forum-2488886-8.patch, failed testing.

colinafoley’s picture

FileSize
2.25 KB
3.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,056 pass(es). View

Fixed the style changes

Wim Leers’s picture

Status: Needs work » Needs review
colinafoley’s picture

FileSize
3.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,055 pass(es), 1 fail(s), and 0 exception(s). View
3.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,645 pass(es). View

Adding test only patch.

The last submitted patch, 14: forum-2488886-test-only-14.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks!

Wim Leers’s picture

Title: Forum - new replies message is escaped » Forum - "new replies" message is escaped
colinafoley’s picture

Assigned: colinafoley » Unassigned
anavarre’s picture

+++ b/core/modules/forum/src/Tests/ForumIndexTest.php
@@ -68,4 +82,41 @@ function testForumIndexStatus() {
+    // Login as the forum user to create a reply.

Could be fixed on commit but I think s/Login/Log in would be more accurate (verb instead of noun).

Fabianx’s picture

RTBC + 1

larowlan’s picture

+++ b/core/modules/forum/src/Tests/ForumIndexTest.php
@@ -68,4 +82,41 @@ function testForumIndexStatus() {
+  public function testHistory() {

nit:Needs a doc block

xjm’s picture

Issue summary: View changes

(Embedding the screenshot, fixing the patch files order, adding reviewer credit.)

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks for the patch and screenshots. I verified in context that the text within this markup is composed of strings properly sanitized earlier in the code flow:

        $variables['topics'][$id]->new_text = '';
        $variables['topics'][$id]->new_url = '';

        if ($topic->new_replies) {
          // ...
          $variables['topics'][$id]->new_text = \Drupal::translation()->formatPlural($topic->new_replies, '1 new post in topic %title', '@count new posts in topic %title', array('%title' => $variables['topics'][$id]->label()));
          $variables['topics'][$id]->new_url = \Drupal::url('entity.node.canonical', ['node' => $topic->id()], ['query' => $query, 'fragment' => 'new']);
        }

      // ...
          $new_replies = '';
          if ($topic->new_replies) {
            $new_replies = '
new_url . '">' . $topic->new_text . ''; } $row[] = array( 'data' => array('#markup' => $topic->comment_count . $new_replies), 'class' => array('forum__replies'), );

I'm slightly concerned in general that using #markup is proliferating a bit and could be used as a pattern that people apply to keep their stuff from being escaped without thinking about sanitization, especially here where the content is being composed with concatenations stretched out across numerous lines, but that's probably too wide a scope to address here, especially given that this is blocking other work.

However, I found a couple more minor issues with the patch, which I think with #20 and #22 are enough that we should probably clean them up before committing:

  1. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -68,4 +82,41 @@ function testForumIndexStatus() {
    +    // Verify the new post message link is not escaped.
    

    So, this is very minor, but the assertions aren't actually verifying a message not escaped. They're verifying the correct markup for the message is present, or that said markup is not escaped. (I misunderstood at first and thought that we were deliberately not escaping the text itself, which seemed wrong.)

  2. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
    @@ -68,4 +82,41 @@ function testForumIndexStatus() {
    +  public function testHistory() {
    

    Why is this method called testHistory()? It has nothing to do with the history as far as I can see.

So let's clean those things up here. Thanks!

Fabianx’s picture

+++ b/core/modules/forum/forum.module
@@ -506,7 +506,7 @@ function template_preprocess_forums(&$variables) {
-            'data' => $topic->comment_count . $new_replies,
+            'data' => array('#markup' => $topic->comment_count . $new_replies),

I agree with xjm here,

Lets use:

SafeMarkup::format('%count!replies', [
  '%count' => $topic->comment_count,
  '!replies' => $new_replies,
]);

Using #markup would soon be unsupported anyway ...

In my RTBC + 1 I mainly looked at the tests ... Should've caught that ...

Ever since effulgentsia fixed SafeMarkup::format() to work like an auto-escape filter, it is a wonderful way to use to fix double-escaping issues in a secure way.

mikeker’s picture

Marked #2636074: forum-list.html.twig replies are double escaped as a dup of this issue. Removed the blocker tag since we managed to release 8.0.0 without this fix... :)

I've also opened #2642450: template_preprocess_forums generates too much HTML to address further cleanup of the forum preprocess functions -- there's a lot of HTML generated in the preprocess functions which makes it harder for custom themes to override.

This patch is from #2636074-4: forum-list.html.twig replies are double escaped and moves the verification into an existing tests which seems cleaner to me. Otherwise, the fix is nearly identical (this patch uses #prefix while #14 concatenates the two strings, see #2636074-7: forum-list.html.twig replies are double escaped for details).

(FYI: credit to @joelpittet for this patch, not me...)

mikeker’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +Twig
joelpittet’s picture

Thanks @mikeker:)

lauriii’s picture

Would it be possible to get test only patch for #26? Otherwise this looks RTBC for me!

mikeker’s picture

mikeker’s picture

mikeker’s picture

Wow, double-posted and still managed to forget the file attachment somehow... :)

Status: Needs review » Needs work

The last submitted patch, 32: 2488886-30-tests-only.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review

Back to NR as this was from the tests-only patch...

joelpittet’s picture

Thanks @mikeker. Here's the patch again.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Joel!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'm not sure that this fix is the best way to fix this BUT fixing forum markup is a much bigger and more complicated issue therefore I'm going to commit this bug fix as an interim fix. Ideally all the markup added in template_preprocess_forums() would be in twig templates.

Committed b422873 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 451852d on 8.1.x
    Issue #2488886 by colinafoley, mikeker, joelpittet, Wim Leers: Forum - "...

  • alexpott committed b422873 on
    Issue #2488886 by colinafoley, mikeker, joelpittet, Wim Leers: Forum - "...
joelpittet’s picture

Cottser’s picture

Thanks @joelpittet closing that in favour of the one @mikeker opened in #26, #2642450: template_preprocess_forums generates too much HTML.

joelpittet’s picture

Ah I missed that in my quick up-skim. Good catch.

Status: Fixed » Closed (fixed)

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