A lot of follow-up reading to be done there, and some learning curves to be straightened out, too.<br><br>Interesting last note about the PHP being released under the GPL, that should be added to the license declarations ... especially for the WP3.1 Theme Review guidelines.<br>
<br><br>Cais.<br><br><div class="gmail_quote">On Wed, Oct 20, 2010 at 10:06 AM, Chip Bennett <span dir="ltr"><<a href="mailto:chip@chipbennett.net">chip@chipbennett.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
All,<div><br></div><div>Please see Nacin's security review comments below.</div><div><br></div><div>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.)</div>
<div><br></div><div>And, thanks to Nacin for taking the time to perform the security review!</div><div><br></div><div>Chip<br><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">WordPress Themes</b> <span dir="ltr"><<a href="mailto:theme-reviewers@lists.wordpress.org" target="_blank">theme-reviewers@lists.wordpress.org</a>></span><br>
Date: Wed, Oct 20, 2010 at 8:53 AM<br>Subject: Re: [WordPress Themes] #1596: THEME: Simply Works Core - 1.3.3<br>To: <br><br><br><div>#1596: THEME: Simply Works Core - 1.3.3<br>
--------------------------+-------------------------------------------------<br>
Reporter: simplyworks | Owner: nacin<br>
Type: theme | Status: assigned<br>
Resolution: | Keywords: theme-simply-works-core,<br>
--------------------------+-------------------------------------------------<br>
<br>
</div>Comment(by nacin):<br>
<br>
In terms of security: Better than most, but still not great. Please read<br>
<a href="http://codex.wordpress.org/Data_Validation" target="_blank">http://codex.wordpress.org/Data_Validation</a>.<br>
<br>
- User input into text options fields are not being sanitized. Seeing a<br>
lot of REQUEST on the same line as update_option. That's no good.<br>
<br>
- check_admin_referer() isn't enough. You need current_user_can checks<br>
there too.<br>
<br>
- Seeing a lot of form fields whose data/values/names etc isn't being<br>
escaped properly.<br>
<br>
- bloginfo('name'); can't be used in the RSS feed title or any attribute,<br>
as there are a few instances of it, without getting esc_attr(). Otherwise<br>
it may escape out of the attribute.<br>
<br>
- get_stylesheet_directory() exposes the path. You don't want that. You<br>
want the URL.<br>
<br>
Non-security notes:<br>
<br>
- Custom header should be used instead of swc_logo.<br>
<br>
- in index.php, you have an is_attachment() inside of an is_single().<br>
That will never be true. single, page, and attachment are all mutually<br>
exclusive. They do however all mean is_singular() == true.<br>
<br>
- comment.php doesn't need the SCRIPT_FILENAME stuff.<br>
<br>
- Not a fan of the direct query in list_most_commented(), or the non-<br>
prefixed custom functions -- register_my_menus() isn't good enough IMO.<br>
<br>
- In addition to the non-prefixed functions, there's for example a global<br>
called "$options" in dashboard.php. That needs to be prefixed, or<br>
preferably even not just sitting there as a global. Same with themename<br>
and shortname -- avoid globals where possible.<br>
<br>
- Some strings aren't translated. Many are also not translatable, and you<br>
should instead use sprintf syntax instead of having dangling strings such<br>
as "By: ". Also, avoid escaping in translated strings, it annoys<br>
translators. Also, some strings such as "ADD COMMENT" should be "Add<br>
Comment", then text-transform:uppercase via CSS. This is better for all<br>
involved, including accessibility reasons.<br>
<br>
- Don't delete options that store yes/no values. You're causing<br>
unnecessary queries as then those values are not autoloaded. Instead,<br>
store the literal value.<br>
<br>
- add_theme_page() should not use `__FILE__`. Be more descriptive. Also,<br>
edit_themes should be edit_theme_options. (Or switch_themes in pre-3.0.)<br>
edit_themes controls *ONLY* the theme editor and may very well be denied<br>
to everyone in many environments, like multisite.<br>
<br>
- "The CSS, XHTML and design is released under GPL" -- and the PHP?<br>
<font color="#888888"><br>
--<br>
Ticket URL: <<a href="http://themes.trac.wordpress.org/ticket/1596#comment:3" target="_blank">http://themes.trac.wordpress.org/ticket/1596#comment:3</a>><br>
</font><div><div></div><div>WordPress Themes <<a href="http://themes.trac.wordpress.org/" target="_blank">http://themes.trac.wordpress.org/</a>><br>
WordPress.org Theme Directory Reviews<br>
</div></div></div><br></div>
<br>_______________________________________________<br>
theme-reviewers mailing list<br>
<a href="mailto:theme-reviewers@lists.wordpress.org">theme-reviewers@lists.wordpress.org</a><br>
<a href="http://lists.wordpress.org/mailman/listinfo/theme-reviewers" target="_blank">http://lists.wordpress.org/mailman/listinfo/theme-reviewers</a><br>
<br></blockquote></div><br>