Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#7 | multichoice_check_markup.patch | 1.2 KB | gilcot |
#2 | quiz_markup2.patch | 4.39 KB | seanbfuller |
#1 | quiz_markup.patch | 4.56 KB | seanbfuller |
Comments
Comment #1
seanbfuller CreditAttribution: seanbfuller commentedThis 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.
Comment #2
seanbfuller CreditAttribution: seanbfuller commentedUpdated 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.
Comment #3
gilcot CreditAttribution: gilcot commentedThe patch adds
$form['start']
just before$form['question']
intomultichoice.module
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)Or maybe i'm missing something?
Comment #4
webchickThat $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:
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.
Comment #5
webchickBtw, Sean, don't bother waiting for other reviews to fix security problems... just commit directly.
Comment #6
seanbfuller CreditAttribution: seanbfuller commentedComment #7
gilcot CreditAttribution: gilcot commentedit's like there's a parameter missing in check_markup() call
Comment #8
seanbfuller CreditAttribution: seanbfuller commentedComment #9
(not verified) CreditAttribution: commented