All user submitted data should be passed through check_markup or check_plain before display. This is a security concern and needs to be addressed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanbfuller’s picture

Status: Active » Needs review
FileSize
4.56 KB

This is a first pass and getting everything properly filtered. I may have missed a few things, but this should get the most obvious ones. Feel free to take another pass.

One question I have:

Wrapping the multichoice question answers in check_markup() gives them

tags and drops the answer to the next line under the selection element. It seems like check_plain() is not the way to go. I have a feeling that this is too restrictive, as some people might want bolds and italics (at the very least). Additionally, as it currently stands tinyMCE alters the answer textboxes to be richtext, bringing the expectation that formatting is allowed.

For this patch I added a div around the answer and a quick css rule to make p tags display inline. I feel that this also breaks expectations and I'm not happy with it yet. Any suggestions on this are welcome.

seanbfuller’s picture

FileSize
4.39 KB

Updated version that does a slightly better job of displaying the answers. It uses margins to position the answer text next to the checkbox or radio button, meaning that p tags and other block level markup work as intended and the answer text is also properly filtered. CSS tested in firefox, Netscape and IE.

gilcot’s picture

The patch adds $form['start'] just before $form['question'] into multichoice.module

  $form['start'] = array('#type' => 'markup', '#value' => '<div class="multichoice_form">');
  $form['question'] = array('#type' => 'markup', '#value' => check_markup($node->body, $node->format));

   // Create form

So i think that a $form['stop'] before the call of <code>drupal_get_form is missing (otherwise <div class="multichoice_form"> is not closed)

  $form['stop'] = array('#type' => 'markup', '#value' => '</div>');

  return drupal_get_form('multichoice_render_question', $form);

Or maybe i'm missing something?

webchick’s picture

That $form['start'] and $form['stop'] stuff is kind of a hack -- anywhere the module's outputting HTML, it should ideally be in a theme function.. so:

$form['question'] = theme('multichoice_question', check_markup($node->body, $node->format);

or something...

However, we should primarily be concerned with making sure the check_markups are in place; cleaning up the display layer can follow-up in additional patches.

webchick’s picture

Btw, Sean, don't bother waiting for other reviews to fix security problems... just commit directly.

seanbfuller’s picture

Status: Needs review » Fixed
gilcot’s picture

Status: Fixed » Needs review
FileSize
1.2 KB

it's like there's a parameter missing in check_markup() call

seanbfuller’s picture

Status: Needs review » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)

  • Commit 8d9b098 on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x by seanbfuller:
    #90710 Fixes a number of security issues regarding missing check_plain...
  • Commit 2f4ae9a on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x by seanbfuller:
    #90710 by gilcot: Oversight of third parameter in check_markup call that...

  • Commit 8d9b098 on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages by seanbfuller:
    #90710 Fixes a number of security issues regarding missing check_plain...
  • Commit 2f4ae9a on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages by seanbfuller:
    #90710 by gilcot: Oversight of third parameter in check_markup call that...

  • Commit 8d9b098 on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages, 2269219 by seanbfuller:
    #90710 Fixes a number of security issues regarding missing check_plain...
  • Commit 2f4ae9a on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages, 2269219 by seanbfuller:
    #90710 by gilcot: Oversight of third parameter in check_markup call that...

  • Commit 8d9b098 on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, 2269219 by seanbfuller:
    #90710 Fixes a number of security issues regarding missing check_plain...
  • Commit 2f4ae9a on 4.7.x-1.x, 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, 2269219 by seanbfuller:
    #90710 by gilcot: Oversight of third parameter in check_markup call that...