[theme-reviewers] Fwd: [WordPress Themes] #1596: THEME: Simply Works Core - 1.3.3

[Chip Bennett]

All,

Please see Nacin's security review comments below.

Perhaps a good starting point for developing some security-related
Guidelines? (Note: this is mostly above my pay grade. I'll be in
watch-and-learn mode. But I definitely think we need some Theme-Review
guidance wrt security.)

And, thanks to Nacin for taking the time to perform the security review!

Chip

---------- Forwarded message ----------
From: WordPress Themes <theme-reviewers at lists.wordpress.org>
Date: Wed, Oct 20, 2010 at 8:53 AM
Subject: Re: [WordPress Themes] #1596: THEME: Simply Works Core - 1.3.3
To:


#1596: THEME: Simply Works Core - 1.3.3
--------------------------+-------------------------------------------------
 Reporter:  simplyworks  |      Owner:  nacin
     Type:  theme        |     Status:  assigned
Resolution:               |   Keywords:  theme-simply-works-core,
--------------------------+-------------------------------------------------

Comment(by nacin):

 In terms of security: Better than most, but still not great. Please read
 http://codex.wordpress.org/Data_Validation.

 - User input into text options fields are not being sanitized. Seeing a
 lot of REQUEST on the same line as update_option. That's no good.

 - check_admin_referer() isn't enough. You need current_user_can checks
 there too.

 - Seeing a lot of form fields whose data/values/names etc isn't being
 escaped properly.

 - bloginfo('name'); can't be used in the RSS feed title or any attribute,
 as there are a few instances of it, without getting esc_attr(). Otherwise
 it may escape out of the attribute.

 - get_stylesheet_directory() exposes the path. You don't want that. You
 want the URL.

 Non-security notes:

 - Custom header should be used instead of swc_logo.

 - in index.php, you have an is_attachment() inside of an is_single().
 That will never be true. single, page, and attachment are all mutually
 exclusive. They do however all mean is_singular() == true.

 - comment.php doesn't need the SCRIPT_FILENAME stuff.

 - Not a fan of the direct query in list_most_commented(), or the non-
 prefixed custom functions -- register_my_menus() isn't good enough IMO.

 - In addition to the non-prefixed functions, there's for example a global
 called "$options" in dashboard.php. That needs to be prefixed, or
 preferably even not just sitting there as a global. Same with themename
 and shortname -- avoid globals where possible.

 - Some strings aren't translated. Many are also not translatable, and you
 should instead use sprintf syntax instead of having dangling strings such
 as "By: ". Also, avoid escaping in translated strings, it annoys
 translators. Also, some strings such as "ADD COMMENT" should be "Add
 Comment", then text-transform:uppercase via CSS. This is better for all
 involved, including accessibility reasons.

 - Don't delete options that store yes/no values. You're causing
 unnecessary queries as then those values are not autoloaded. Instead,
 store the literal value.

 - add_theme_page() should not use `__FILE__`. Be more descriptive. Also,
 edit_themes should be edit_theme_options. (Or switch_themes in pre-3.0.)
 edit_themes controls *ONLY* the theme editor and may very well be denied
 to everyone in many environments, like multisite.

 - "The CSS, XHTML and design is released under GPL" -- and the PHP?

--
Ticket URL: <http://themes.trac.wordpress.org/ticket/1596#comment:3>
WordPress Themes <http://themes.trac.wordpress.org/>
WordPress.org Theme Directory Reviews
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wordpress.org/pipermail/theme-reviewers/attachments/20101020/e92db2d3/attachment.htm>