SimpleAdvertisements

* your module uses taxonomy functions so it should depend on the taxonomy module
* there is an update function that only contains commented out code. Remove it.
* same for the empty validation function

* "// default": Comments should be more meaningful and on a new line, see http://drupal.org/node/1354#inline
* simpleads_install(): no need to use variable_set() in there, variable_get() can use default values anyway.
* "if ($uend_time <= $now)" always use "{}" around if/else statements, even if it is just one line. This often leads to bugs if you add lines later. See http://drupal.org/node/318#controlstruct
* same for for() loops
* lines in README.txt should not exceed 80 characters

• git release branch missing, see http://drupal.org/node/1015226
• Are you sure you want to implement hook_init()? If you need your javascript really on every page you can list it in the module file, see http://drupal.org/node/756722
• _simpleads_access_callback(): you don't need the "else" there.
• _simpleads_load_ads(): if you document function parameters please do it properly with @param, see http://drupal.org/node/1354#functions

Otherwise I think this is nearly ready. The simpleads name space is already taken http://drupal.org/project/simpleads but I think it should be no problem to take it over. See http://drupal.org/node/251466

Looks RTBC for me.

Now while we are waiting for the access grants would you be so kind and do a review of one of the other project applications as well? We have currently quite a backlog and need every help we can get. Just pick one from this list: http://drupal.org/project/issues/projectapplications?status=8

Suggested fixes

Please take a moment to make your project page follow tips for a great project page.

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

This code could be hardened against an attack:

398 $result .= '<a href="' . $ad['url'] . '" target="_blank"><img src="' . $ad['image'] . '" alt="' . $ad['alt'] . '" width="' . $ad_setting['ads_width'] . '" height="' . $ad_setting['ads_height'] . '" border="0"></a> ';

Specifically, the URL should be sent through url(), and the img tag should be built with something like theme_image which provides some sanity checks.

Required fixes

I think there are a few places open to cross site scripting. I created a new ad where every field contained: <script>alert('xss');</script>. When I browse around the admin pages I see a lot of popups.

Specifically, admin/content/simpleads and admin/content/simpleads/5/stat both showed popups.

The XSS needs to be fixed before this can be approved. The first three items are suggestions.

Looks like some great progress. I just noticed now that the term name is also vulnerable to xss on the admin page. It's important in this process to not only fix the bugs but also understand them and apply that new knowledge to other parts of the module. I should have included http://drupal.org/writing-secure-code in my earlier comment - it's a good resource to explain how to identify these problems and how to fix them.

• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• "$ad_group_name = check_plain($term->name);" don't filter on input, but filter on output. If you do it at both ends you may double escape things. The DB API will take care of SQL injection anyway. See also http://drupal.org/node/101495
• "'info' => t('Advertisement Block: !name', array('!name' => check_plain($data['block_info']))),": t() has a check_plain() built in, use '@name' instead and you should be fine. See also http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/format... for '!', '@' and '%'. Also in other places.

RTBC for me.

HI minnur,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.