Data From Theme Reviews Shows Authors Need More Education on Developing Secure WordPress Themes

Last week, we highlighted the progress being made by the Theme Review Team in clearing out a 1K+ review backlog. In an effort to determine common problems with themes discovered by reviewers, Carolina Nymark, a member of the Theme Review Team, reviewed 100 tickets from 531 themes that were closed and marked not approved between December and February. Nymark cautions that the data does not assure statistical accuracy and is not representative of the entire directory.

Her assessment shows that the most common problems discovered by reviewers were:

  • Missing escaping or using the wrong functions: 23 themes
  • Text that is not translation ready: 21 themes
  • Missing prefix: 20 themes
  • Scripts or styles are not enqueued: 18 themes
  • PHP notices, errors or warnings: 12 themes
  • Style tags does not correspond with theme functionality, or are deprecated: 10 themes

Nymark also reviewed 100 out of 177 new themes that went live between December and February. Out of these themes, the most common problems were:

  • Missing escaping or using the wrong functions: 51 Themes
  • Text that is not translation ready: 44 Themes
  • Missing prefix: 39 Themes
  • Missing license or copyright information for included assets: 34 Themes
  • Unused code or files: 25 Themes
  • PHP notices, errors or warnings: 20 Themes
  • Missing sanitization, or using the wrong functions: 18 Themes
  • Options in the customizer that are not working: 18 Themes

Last Friday, Jose Castaneda, Ulrich Pogson, and Nymark participated in a voice chat with Matt Mullenweg, co-creator of the WordPress project, to discuss the future of the theme directory. The team discussed ideas around automation, improving the theme preview experience, and content portability. One of the experiments Mullenweg proposed is to remove the manual review process and rely more on user feedback. Feedback could include, tags, reviews, and other meta data.

“As we are not sure if the process will function without manual reviews, we will start working on getting better user feedback on themes,” Pogson said. “Once we have a good infrastructure in place we can experiment with how the repository reacts with no manual reviews.

“We discussed the process we would go about making decisions on changes to the theme repository and came to the consensus that a direct democracy is too fragile and representative democracy would be a better solution.”

Security, code errors, and prefixing, were also mentioned in the conversation as the most common issues encountered with themes. The team was given a series of tasks to complete and will report the results to Mullenweg at a later date.

New Theme Check Plugin Will Detect Common Security Issues

The Theme Handbook doesn’t have a chapter on security but it does link to a series of articles on writing secure themes in the resources section. Justin Tadlock, Key Reviewer, says work is underway on a new Theme Check plugin that will automatically detect security issues commonly seen during the manual review process. These include escaping and data sanitization.

“If we could get the greater theme developer community to pitch in and help get this finished, it would be awesome,” Tadlock said. “Even outside of WordPress.org, ThemeForest and commercial theme shops could really use this.”

Members of the TRT are testing the plugin behind the scenes and are working to eliminate false-positives. The best way to get involved in the project is to view the Issue tracker and submit pull requests. Once the new theme check plugin is live, it will give authors another tool at their disposal for developing more secure WordPress themes.

27 Comments


  1. As a WP developer that often build theme from scratch, I’m very surprised at those stats. PHP warning left out? Are they not developing in WP_DEBUG?

    I believe the cause is WP suggesting (in the doc) to use twentysomething as benchmark. They should use things like _underscore where everything is still in WP vanilla.

    Report


  2. I am usually very harsh on the core developers, the plugin review and theme review teams, even though they are obviously overwhelmed and do the best they can. That said, things should be done correctly or not at all.

    If I understand correctly these stats are form themes that were not approved, thus I want to congratulate and fullheartedly thank the theme review team for a job well done. I just hope no theme or plugin that has security holes do not fall through the cracks. And again, I’m strictly referring to security issues and not things not working, like customizer options.

    Thanks again, please keep up the good work.

    Report


  3. What means the part “missing prefix” ?

    Honestly don’t know what prefix should i check into a theme or where i can find the guidelines that points me to a proper prefix usage.

    Maybe is realted to prefix all the custom code, anyway don’t know where security is affected by this.

    Report


    1. I think they’re referring to the fact that all functions created by a theme should have a unique prefix.

      Good: mytheme_get_logo()
      Bad: get_logo()

      Report


      1. Even better:
        if( !function_exists( 'my_theme_prefix_get_something' ) ) :
        ...
        endif;

        Makes it child-theme ready and enhancement not a total PITA. Having more grief with this nowadays than ever before.

        cu, w0lf.

        Report


    2. From the Theme Review Requirements:

      Provide a unique prefix for everything the Theme defines in the public namespace, including options, functions, global variables, constants, post meta, etc.

      It’s not about security, it’s about themes clashing with WordPress core or plugins. If they use the same name for one of those things as something else, there will be problems. By adding a prefix, it greatly reduces the chance of a clash.

      Report


      1. That has more sense, compatibility and security are two diferent things.

        Report


  4. One of the experiments Mullenweg proposed is to remove the manual review process and rely more on user feedback

    Deep in the WP Tavern archives are conversations where a large number of people suggested this originally. There was quite a deep divide between those of us who felt this was the only way forward due to us feeling it was not viable to manually review every theme without creating bottlenecks, and those who felt that the way forward was to get more people involved with the review process until it was running smoothly.

    Report


      1. No, I’m thinking closer to 10 years ago ;) In the forum, not blog comments. It would have been back before the theme review theme process was implemented.

        Report


      2. I found it. It’s a forum thread created by Justin Tadlock 08-24-2010 called Theme repository reviews. It has 118 posts in the thread. You have a good memory :)

        Report


      3. That one is relevant, but I’m thinking of something from before when the theme review system was implemented. Or maybe my memory is scrambled :P

        Report


      4. I’m fairly sure Justin was involved in that other discussion too. I felt his views were very similar to mine.

        Report


    1. Yep, I remember that. That’s when Matt suggested that I join the review team (the whole “take a walk in their shoes” idea).

      Report


      1. The theme review process was the polar opposite of how I would have implemented it. For this reason I’ve avoided being involved with the theme review process.

        My only involvement would have been to say everything is wrong and instructed them to do it my way. This was not going to be productive, so I’ve always stood back and hoped the manual review process was ditched eventually. To be honest, I didn’t think it would last a year. I assumed when the wait times got longer than a month that they’d just throw in the towel and give up. It seems the reviewers are more stubborn than I had anticipated :P

        Report


      2. The team has discussed so many variations over the years it’s hard to keep track.

        The question is are we brave enough to “let things go” or would that just be foolish?
        -When you see some of the crazy things people try to submit it is much more difficult to move past the “trust no one” barrier.
        (Everything from exe files to the more common commercial versions of other peoples themes and plugins.)

        There are things that should not be on WordPress.org, like the legitimate looking theme that had Isis propaganda in a .pdf file. As the saying goes, this is why we can’t have nice things.

        We probably can’t automate everything, for example licensing is a blocker at the moment, themes still need to be GPL compatible.

        Report


      3. If the team are not brave enough to just let things go, then I think a new team is the solution to the problem.

        Licences and .exe files shouldn’t be any more of a problem in the themes repository than they are in the plugins repository, and I don’t see any major problems with those there.

        Report


  5. I’m appreciative of the theme review team. (Which is why I’ve never submitted one of my half baked themes.)

    Report


  6. It’s interesting what Matt is wanting to find out about the theme directory and the reviewing. I can also agree that if the security issues is a problem with submitted themes, this is really the most important element for any theme; having a secure theme is absolute.

    However, what I would like to see happen is the comment Matt has mentioned in the past on a few occasions about the theme rules and guidelines being (I believe he called it) draconian. The review process, rules and guidelines are too restrictive and prevents theme authors from getting too creative.

    ….this is one of the reasons I walked away last year from submitting themes, the other being the queue wait times. Personally, I would rather have long wait periods if the theme guidelines were not so restricting and eased off to just reviewing of security only.

    Report


    1. So many people are saying this but are not able to say exactly which requirement they would like to remove or change… and I’m sorry but that’s not as helpful.

      Report


      1. First, I would like to thank WPTRT for their reviews of my themes, because of which I’m a better developer today.

        Some rules felt like you were unnecessarily holding back the theme.
        I came to understand the significance and reasons behind some rules later in my career.

        With the new reviewers it felt like they were trying to impress someone by finding faults in the themes with a magnifying glass.

        1. My theme used to have a slider, it had Steve Jobs picture in one of the slide. Some reviewer complained about it.
        https://www.flickr.com/photos/10678910@N07/6215552915/
        2. Some one had to dig through the TOS (which even our paying customers barely check, even after prominently linking to it) listed on our website to find fault with the submission.

        Whether the above two cases were right or wrong is not the point here. The general attitude of the reviewers (the new joines) is not right towards the theme authors and the themes.

        And titles like “WPTRT Cracking down on so and so” doesn’t make things any better.

        Also most of the rules are mostly written in rock. At least that is how they are treated by new reviewers.

        I recently developed a theme using Hybrid framework, and bundled functionality not in hybrid into a second framework.
        This resulted in two name spaces for the frameworks and one for the actual theme code.

        When I if this will be allowed, the answer is no.

        Report


      2. A few comes to mind, such as not allowed to modify any core WP functions, even when it’s at the theme level for style purposes; Not allowed to remove the archive title label such as Category: Your Category Name; not allowed to modify the pagination with my own; WPTitle restrictions; not allowed inline styles (situations where it’s required); screenshot restrictions; no default logos, required to use core-bundled scripts rather than including their own….In many cases, we’ve experienced requirements in reviews that are listed that are not even in the actual development guidelines…almost like the reviewer added them on their own. I guess I should correct myself to mention that most of the issues are during the review process where you get some cowboy/girl type reviews happening. Like Satish mentioned, it’s often reviewer attitudes and a feeling of I’m the reviewer and what I say goes. And when they start adding in their own rules and requirements, or they misinterpret a real requirement, it’s a bad experience for the theme author.

        Not all reviewers are like this, by all means, I’ve encountered a few really amazing ones, but there are many over the years that come across like they have something to prove or impress someone.

        The whole review process is a cringing experience and often made me think, what’s the point. I know many authors have walked away and scares new ones who were contemplating theme submissions.

        To become a reviewer, I believe it should be like a job application you go through. You need to also be a people person because you are working in an environment that involves many people.

        Report


      3. Ping me (@greenshady) on Slack if you have issues in your review.

        Some of those things don’t look like requirements (archive label, pagination, title, and inline styles).

        The other things such as screenshot (must be a real screenshot of your theme at the correct size for our system) and using core scripts (so that you’re not breaking plugins) are pretty important.

        Without context, it’s hard to talk about anything definitively though. Just get in touch so that we can walk through any issues together.

        Report


  7. Concerning the prefix, what about setting one in the header section of themes (and plugins) and automatically compare the functions, options and all to that ?

    Report

Comments are closed.