Contests

Please include the link to your sandbox project page and set this to needs review.

Hello,

Here is a quick review :

• Rename your readme.txt into README.txt
• In all your files, take the $Id$ line off, this is not necessary anymore with Git
• In your .install file, there is some trailing whitespaces, and indent should be 2 spaces only, not 4
• In your .admin.inc file
• There is a lot of trailing whitespaces.
• Say which hook you're implementing (for example, when you use contests_settings_form(), say you're implementing hook_settings_form())
• Indents should be 2 spaces, not 4
• From line 148 to 163, you're using $_GET values without checking them. Please use check_plain() to sanitize them
• Same line 256, especially since you're using these values line 288 in a query. There is a high risk of SQL injection there
• Comment all your functions (contests_submissions_header())
• In your .compete.inc file, a lot of trailing whitespaces too, same for indent and comments (Implementation of hook_...)
• In your .module file
• Trailing whitespaces, indents, comments are right there.
• Line 146, you're using some non-sanitized variables, please check_plain() them, or this can lead to XSS attacks
• Same at line 175
• Same in your contests_load($node) function

I haven't checked your .js file.

You should get your module through the Coder module, it will help you to find trailing whitespaces, and fix your indents problems.

But mostly, you MUST fix your SQL injections and XSS attacks risks. Absolutely.

Otherwise, the module looks good, just talking about feature :).

Have a nice day!

@Ralt

* Say which hook you're implementing (for example, when you use
contests_settings_form(), say you're implementing hook_settings_form())

There is no such hook, that is just a form builder function.

- #default_value is already checked by FAPI. However, not sure why it's necessary at all to define the get values as default value, though.

- As long as placeholders are used, there is no need for additional sanitation of $_GET values. XSS protection should happen when something is displayed, not when it is stored.

Hello,

@Berdir : my bad! I guess I still need to learn, heh.

Then, the module just needs to get past the coder module to remove trailing whitespaces and fix indents problems, and it will be then good to go, I think.

Hi.

I have noticed that you didn't changed the readme.txt file name to README.txt

Also you should change the SQL Insert and Update statements (contest_insert and contest_update functions) by the drupal_write_record function: http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_wr...

Kind regards.

Hi.

I'm getting this errors testing the Contest module:

warning: Missing argument 2 for variable_get() in contests.module on line 267

warning: Missing argument 2 for variable_get() in contests.admin.inc on line 36

Also I am trying to "Compete" a question but it doesn't work for me.

Please, be sure all the files ends with a blank new line.

Kind regards.

(Jumping in on what looks like an abandonded review process.)

In my opinion this project is ready to go. The submitter has made all the requested changes, and has met all of the requirements for inclusion. I've reviewed the code and in my opinion it's very good; higher quality and better documented than many other modules I've seen. Plus, it works -- it does what it says it does, and it's simple.

My only concern is that you could build something very similar to this using the webform module, so there's some duplicated functionality here. However, it's been two months since the submitter first posted this, so it seems to me it'd be quite unfair to come back and demand a justification for it at this stage. It's simpler and lighter weight than webform.

I say he's done. Changing status to reviewed and tested.

It looks to me like there are critical security issues here. You're doing a direct query of the node table, not doing a db_rewrite_sql() on it in case there are node access constraints, not checking to see if a node is disabled. And you're displaying the user-entered title without sanitizing it.

In contests_compete_form_submit() I see

$object->answer = $form_state['values']['answerbox'];

without any definition of $object. Is nobody running with errors being output? One of my favorite rants: http://randyfay.com/node/76

You don't put the version in the .info file. The packaging system puts that there.

It looks to me like all reviewers should have a look at http://drupal.org/writing-secure-code, since security issues are the most important reason for this whole process. And here we have a raw query of the node table and raw output of the results, without respecting permissions, node access, or anything else. Ooops.

Oops indeed. Mea culpa.

No activity in several months. Reopen and set the status to "needs review" if you are still pursuing this application.