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

[Edward Caissie]

A lot of follow-up reading to be done there, and some learning curves to be
straightened out, too.

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.


Cais.

On Wed, Oct 20, 2010 at 10:06 AM, Chip Bennett <chip at chipbennett.net> wrote:

> 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
>
>
> _______________________________________________
> theme-reviewers mailing list
> theme-reviewers at lists.wordpress.org
> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wordpress.org/pipermail/theme-reviewers/attachments/20101020/7f3895f7/attachment.htm>