#520736 by grendzy, brianV, JoshuaRogers, kjy07, Damien Tournoud, greggles: Fixed SA-CORE-2009-007 forum module XSS.

3 files changed, 68 insertions, 25 deletions

diff --git a/modules/forum/forum.module b/modules/forum/forum.module
index 413903434..ea7f77f07 100644
--- a/modules/forum/forum.module
+++ b/modules/forum/forum.module
@@ -94,6 +94,13 @@ function forum_menu() {
     'access arguments' => array('access content'),
     'file' => 'forum.pages.inc',
   );
+  $items['forum/%forum_forum'] = array(
+    'title' => 'Forums',
+    'page callback' => 'forum_page',
+    'page arguments' => array(1),
+    'access arguments' => array('access content'),
+    'file' => 'forum.pages.inc',
+  );
   $items['admin/structure/forum'] = array(
     'title' => 'Forums',
     'description' => 'Control forum hierarchy settings.',
@@ -163,9 +170,8 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
   // Add action link to 'node/add/forum' on 'forum' page.
   if ($root_path == 'forum') {
     $tid = (isset($router_item['page_arguments'][0]) ? $router_item['page_arguments'][0] : 0);
-    $forums = forum_get_forums($tid);
-    $parents = taxonomy_get_parents_all($tid);
-    if ($forums || $parents) {
+    $forum_term = forum_forum_load($tid);
+    if ($forum_term) {
       $vid = variable_get('forum_nav_vocabulary', 0);
       $vocabulary = taxonomy_vocabulary_load($vid);
 
@@ -178,7 +184,7 @@ function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {
             '#theme' => 'menu_local_action',
             '#link' => array(
               'title' => t('Add new @node_type', array('@node_type' => node_type_get_name($type))),
-              'href' => 'node/add/' . str_replace('_', '-', $type) . '/' . $tid,
+              'href' => 'node/add/' . str_replace('_', '-', $type) . '/' . $forum_term->tid,
             ),
           );
         }
@@ -723,27 +729,54 @@ function forum_form($node, $form_state) {
 }
 
 /**
- * Returns a list of all forums for a given taxonomy id
- *
- * Forum objects contain the following fields
- * -num_topics Number of topics in the forum
- * -num_posts Total number of posts in all topics
- * -last_post Most recent post for the forum
+ * Returns a tree of all forums for a given taxonomy term ID.
  *
  * @param $tid
- *   Taxonomy ID of the vocabulary that holds the forum list.
+ *    (optional) Taxonomy ID of the forum, if not givin all forums will be returned.
  * @return
- *   Array of object containing the forum information.
+ *   A tree of taxonomy objects, with the following additional properties:
+ *    - 'num_topics': Number of topics in the forum
+ *    - 'num_posts': Total number of posts in all topics
+ *    - 'last_post': Most recent post for the forum
+ *    - 'forums': An array of child forums
  */
-function forum_get_forums($tid = 0) {
+function forum_forum_load($tid = NULL) {
   $cache = &drupal_static(__FUNCTION__, array());
 
+  // Return a cached forum tree if available.
+  if (!isset($tid)) {
+    $tid = 0;
+  }
   if (isset($cache[$tid])) {
     return $cache[$tid];
   }
 
-  $forums = array();
   $vid = variable_get('forum_nav_vocabulary', 0);
+
+  // Load and validate the parent term.
+  if ($tid) {
+    $forum_term = taxonomy_term_load($tid);
+    if (!$forum_term || ($forum_term->vid != $vid)) {
+      return $cache[$tid] = FALSE;
+    }
+  }
+  // If $tid is 0, create an empty object to hold the child terms.
+  else if ($tid === 0) {
+    $forum_term = (object) array(
+      'tid' => 0,
+    );
+  }
+
+  // Determine if the requested term is a container.
+  if (!$forum_term->tid || in_array($forum_term->tid, variable_get('forum_containers', array()))) {
+    $forum_term->container = 1;
+  }
+
+  // Load parent terms.
+  $forum_term->parents = taxonomy_get_parents_all($forum_term->tid);
+
+  // Load the tree below.
+  $forums = array();
   $_forums = taxonomy_get_tree($vid, $tid);
 
   if (count($_forums)) {
@@ -762,10 +795,12 @@ function forum_get_forums($tid = 0) {
   }
 
   foreach ($_forums as $forum) {
+    // Determine if the child term is a container.
     if (in_array($forum->tid, variable_get('forum_containers', array()))) {
       $forum->container = 1;
     }
 
+    // Merge in the topic and post counters.
     if (!empty($counts[$forum->tid])) {
       $forum->num_topics = $counts[$forum->tid]->topic_count;
       $forum->num_posts = $counts[$forum->tid]->topic_count + $counts[$forum->tid]->comment_count;
@@ -775,6 +810,7 @@ function forum_get_forums($tid = 0) {
       $forum->num_posts = 0;
     }
 
+    // Query "Last Post" information for this forum.
     $query = db_select('node', 'n');
     $query->join('users', 'u1', 'n.uid = u1.uid');
     $query->join('forum', 'f', 'n.vid = f.vid AND f.tid = :tid', array(':tid' => $forum->tid));
@@ -791,6 +827,7 @@ function forum_get_forums($tid = 0) {
       ->execute()
       ->fetchObject();
 
+    // Merge in the "Last Post" information.
     $last_post = new stdClass();
     if (!empty($topic->last_comment_timestamp)) {
       $last_post->created = $topic->last_comment_timestamp;
@@ -802,9 +839,10 @@ function forum_get_forums($tid = 0) {
     $forums[$forum->tid] = $forum;
   }
 
-  $cache[$tid] = $forums;
-
-  return $forums;
+  // Cache the result, and return the tree.
+  $forum_term->forums = $forums;
+  $cache[$tid] = $forum_term;
+  return $forum_term;
 }
 
 /**
diff --git a/modules/forum/forum.pages.inc b/modules/forum/forum.pages.inc
index 6b01048da..02c780136 100644
--- a/modules/forum/forum.pages.inc
+++ b/modules/forum/forum.pages.inc
@@ -9,16 +9,21 @@
 /**
  * Menu callback; prints a forum listing.
  */
-function forum_page($tid = 0) {
-  $topics = '';
+function forum_page($forum_term = NULL) {
+  if (!isset($forum_term)) {
+    // On the main page, display all the top-level forums.
+    $forum_term = forum_forum_load(0);
+  }
+
   $forum_per_page = variable_get('forum_per_page', 25);
   $sortby = variable_get('forum_order', 1);
 
-  $forums = forum_get_forums($tid);
-  $parents = taxonomy_get_parents_all($tid);
-  if ($tid && !in_array($tid, variable_get('forum_containers', array()))) {
-    $topics = forum_get_topics($tid, $sortby, $forum_per_page);
+  if (empty($forum_term->container)) {
+    $topics = forum_get_topics($forum_term->tid, $sortby, $forum_per_page);
+  }
+  else {
+    $topics = '';
   }
 
-  return theme('forums', array('forums' => $forums, 'topics' => $topics, 'parents' => $parents, 'tid' => $tid, 'sortby' => $sortby, 'forums_per_page' => $forum_per_page));
+  return theme('forums', array('forums' => $forum_term->forums, 'topics' => $topics, 'parents' => $forum_term->parents, 'tid' => $forum_term->tid, 'sortby' => $sortby, 'forums_per_page' => $forum_per_page));
 }
diff --git a/modules/forum/forum.test b/modules/forum/forum.test
index 2cd4909d8..2d8d8f902 100644
--- a/modules/forum/forum.test
+++ b/modules/forum/forum.test
@@ -268,7 +268,7 @@ class ForumTestCase extends DrupalWebTestCase {
 
     // Assert that the forum no longer exists.
     $this->drupalGet('forum/' . $tid);
-    $this->assertRaw(t('No forums defined'), 'The forum was not found');
+    $this->assertResponse(404, 'The forum was not found');
   }
 
   /**