[theme-reviewers] Why Rigorous Review of Theme Functional Files is Important

Thu Jan 30 16:48:26 UTC 2014

Let's discuss when we post the discussion thread. I think there's a case to
be made for a Theme option for Theme-specific CSS.

On Thu, Jan 30, 2014 at 11:46 AM, Konstantin Kovshenin
<kovshenin at gmail.com>wrote:

> > Sneak peek: when we begin discussing Guidelines Revisions (next week),
> I'm going to propose that options for arbitrary header/footer scripts are
> Plugin Territory.
>
> Thank you! Can we add CSS to that? :)
>
> On Thu, Jan 30, 2014 at 8:44 PM, Chip Bennett <chip at chipbennett.net>
> wrote:
> > Sneak peek: when we begin discussing Guidelines Revisions (next week),
> I'm
> > going to propose that options for arbitrary header/footer scripts are
> Plugin
> > Territory.
> >
> >
> > On Thu, Jan 30, 2014 at 11:40 AM, Konstantin Kovshenin <
> kovshenin at gmail.com>
> > wrote:
> >>
> >> > So, if I make the The option of Custom Js available only to users with
> >> > unfiltered_html capability, then the themes are good to go, right?
> >>
> >> That would make the theme more secure, yes, but ultimately it's up to
> >> the reviewers and the guidelines on whether it's good to go or not.
> >>
> >> Again, my opinion is that Custom CSS and Custom JS are plugin
> >> territory, which would stop theme authors from reinventing the wheel.
> >> It would also mean that reviewers will spend less time auditing theme
> >> code. And it would also mean that there's less risk of insecure code
> >> ending up in the themes directory. Right, let's dump it all in the
> >> plugins directory :)
> >>
> >> On Thu, Jan 30, 2014 at 8:34 PM, Rohit Tripathi <rohitink at live.com>
> wrote:
> >> > Thanks Koveshenin.
> >> >
> >> > So, if I make the The option of Custom Js available only to users with
> >> > unfiltered_html capability, then the themes are good to go, right?
> >> >
> >> >
> >> >
> >> >> Date: Thu, 30 Jan 2014 20:19:39 +0400
> >> >> From: kovshenin at gmail.com
> >> >
> >> >> To: theme-reviewers at lists.wordpress.org
> >> >> Subject: Re: [theme-reviewers] Why Rigorous Review of Theme
> Functional
> >> >> Files is Important
> >> >>
> >> >> > I have just allowed the <script> tag in the text area. Is the
> script
> >> >> > tag
> >> >> > not acceptable at all? Or should I create a New Field, derivate of
> >> >> > Textfield, and allow <script> in that?
> >> >>
> >> >> As Justin pointed out earlier, you should be checking whether the
> >> >> current user can publish unfiltered html, and only then show your
> >> >> custom js fields that allow script tags. Note that an some setups,
> >> >> neither admins nor super admins have the unfiltered_html capability
> >> >> for security reasons.
> >> >>
> >> >> Also, in my opinion, Custom CSS and especially Custom JS should not
> be
> >> >> allowed in themes.
> >> >>
> >> >> On Thu, Jan 30, 2014 at 8:14 PM, Chip Bennett <chip at chipbennett.net>
> >> >> wrote:
> >> >> > Speaking in general terms, any Theme option must be properly
> >> >> > sanitized/validated on input, and escaped on output, as
> appropriate.
> >> >> > Specific sanitization/validation/escaping methods depend on the
> >> >> > specific
> >> >> > data type.
> >> >> >
> >> >> >
> >> >> > On Thu, Jan 30, 2014 at 11:12 AM, Rohit Tripathi <
> rohitink at live.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> I Use options framework for my theme options. I have just allowed
> >> >> >> the
> >> >> >> <script> tag in the text area. Is the script tag not acceptable at
> >> >> >> all?
> >> >> >> Or
> >> >> >> should I create a New Field, derivate of Textfield, and allow
> >> >> >> <script>
> >> >> >> in
> >> >> >> that?
> >> >> >>
> >> >> >> Regards
> >> >> >>
> >> >> >> ________________________________
> >> >> >> Date: Thu, 30 Jan 2014 11:01:22 -0500
> >> >> >>
> >> >> >> From: chip at chipbennett.net
> >> >> >> To: theme-reviewers at lists.wordpress.org
> >> >> >> Subject: Re: [theme-reviewers] Why Rigorous Review of Theme
> >> >> >> Functional
> >> >> >> Files is Important
> >> >> >>
> >> >> >> It's example code, to show that an arbitrary script can be
> executed.
> >> >> >> You
> >> >> >> didn't really expect me to put actually dangerous code there, did
> >> >> >> you?
> >> >> >> :)
> >> >> >>
> >> >> >>
> >> >> >> On Thu, Jan 30, 2014 at 10:58 AM, Rohit Tripathi <
> rohitink at live.com>
> >> >> >> wrote:
> >> >> >>
> >> >> >> I am not sure, if asking this is lame. But, why is the entering
> >> >> >> alert('text') in the header/footer codes area, being considered as
> >> >> >> an
> >> >> >> issue?
> >> >> >>
> >> >> >> Regards
> >> >> >>
> >> >> >> ________________________________
> >> >> >> Date: Thu, 30 Jan 2014 10:40:22 -0500
> >> >> >> From: chip at chipbennett.net
> >> >> >> To: theme-reviewers at lists.wordpress.org
> >> >> >> Subject: Re: [theme-reviewers] Why Rigorous Review of Theme
> >> >> >> Functional
> >> >> >> Files is Important
> >> >> >>
> >> >> >>
> >> >> >> In many cases, the issue is the lack of inherent sanitization when
> >> >> >> using
> >> >> >> the Theme Mods API with the Theme Customizer:
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> http://make.wordpress.org/themes/2014/01/30/using-the-theme-customizer-with-the-theme-mods-api/
> >> >> >>
> >> >> >>
> >> >> >> On Thu, Jan 30, 2014 at 10:21 AM, Justin Tadlock
> >> >> >> <justin at justintadlock.com> wrote:
> >> >> >>
> >> >> >> if ( !current_user_can( 'unfiltered_html' ) ) {
> >> >> >> /* Sanitize. */
> >> >> >> }
> >> >> >>
> >> >> >> All theme reviewers should be intimately familiar with this page:
> >> >> >> http://codex.wordpress.org/Data_Validation
> >> >> >>
> >> >> >>
> >> >> >> On 1/30/2014 7:00 AM, Chip Bennett wrote:
> >> >> >>
> >> >> >> Good morning, all,
> >> >> >>
> >> >> >> Just as a reminder why it is imperative that our reviews are
> >> >> >> thorough
> >> >> >> and
> >> >> >> complete, including a review of the Theme code and not merely a
> >> >> >> Theme-Check/front-end review, I woke up this morning to several
> >> >> >> emails
> >> >> >> reporting various Theme security vulnerabilities. Here's a
> sampling:
> >> >> >>
> >> >> >> To reproduce:
> >> >> >>
> >> >> >> 1. Add define( 'DISALLOW_UNFILTERED_HTML', true ); to
> wp-config.php
> >> >> >> 2. Activate the theme, navigate to Theme Options, add an image
> logo
> >> >> >> 3. In General Options - Logo Text, enter (as is, with quotes): "
> >> >> >> onclick="javascript:alert(1);"
> >> >> >> 4. Visit the homepage, click on the logo, boom.
> >> >> >>
> >> >> >> 5. In Slider Options, add a slider image and use the following for
> >> >> >> the
> >> >> >> slider text: Foo bar <script>alert('baz');</script>
> >> >> >> 6. Visit the home page, boom.
> >> >> >>
> >> >> >>
> >> >> >> To reproduce:
> >> >> >>
> >> >> >> 1. Add define( 'DISALLOW_UNFILTERED_HTML', true ); to
> wp-config.php
> >> >> >> 2. Activate the theme, go to Appearance - Theme Settings
> >> >> >> 3. In More Text enter: <script>alert('xss');</script>
> >> >> >> 4. Visit the home page.
> >> >> >>
> >> >> >> (you will have to have at least one post with a <!--more--> tag
> >> >> >>
> >> >> >> To reproduce:
> >> >> >>
> >> >> >> 1. Add define( 'DISALLOW_UNFILTERED_HTML', true ); to
> wp-config.php
> >> >> >> 2. Activate the Theme, navigate to Appearance - Theme
> >> >> >> Options - Social Netowrks Configuration
> >> >> >> 3. In Twitter URL enter: http://twitter.com/kovshenin'
> >> >> >> onclick='alert(1);'
> >> >> >> 4. Visit the home page and click the Twitter icon on the top
> right,
> >> >> >> ouch. Other URL fields affected too.
> >> >> >>
> >> >> >> 5. In Layout Settings - Footer enter: <script>alert(123)</script>
> >> >> >> 6. Visit the front page, ouch
> >> >> >>
> >> >> >> 7. In Advertise Settings, Header Banner Alternative: '
> >> >> >> onclick='alert(1)'
> >> >> >> 8. Visit the front page and click the header banner, ouch
> >> >> >>
> >> >> >> 9. In Advertise Settings, Header Banner Link: http://foo.com'
> >> >> >> onclick='alert("bar")
> >> >> >> 10. Visit the front page and click the banner
> >> >> >>
> >> >> >> To reproduce:
> >> >> >>
> >> >> >> 11. In Theme Options - Integration
> >> >> >> 12. For header code: <script>alert('wow');</script>
> >> >> >> 13. Body code: <script>alert('seriously?')</script>
> >> >> >> 14. Visit the front page
> >> >> >>
> >> >> >> To reproduce:
> >> >> >>
> >> >> >> 15. in Theme Options - Colors, go to your browser JS console and
> >> >> >> enter: jQuery('#cwp_templates_topbar_colorid_color').val('blue;"
> >> >> >> onclick="javascript:alert(123);')
> >> >> >> 16. Hit save changes, visit the front page
> >> >> >> 17. The top bar is blue, try and click it. Probably all the color
> >> >> >> fields in this theme are vulnerable to this.
> >> >> >>
> >> >> >>
> >> >> >> That these issues are appearing is approved/live Themes is exactly
> >> >> >> the
> >> >> >> reason that it takes so long to get through the approved-Theme
> >> >> >> queue.
> >> >> >> We
> >> >> >> have to audit for these things, and the audits are turning into
> >> >> >> complete
> >> >> >> re-reviews in several cases.
> >> >> >>
> >> >> >> If you are uncomfortable with performing this level of review -
> >> >> >> first:
> >> >> >> don't worry. We've all been there. But the important thing is to
> ask
> >> >> >> for
> >> >> >> help. We have a team of 100 people, most/all of whom would be more
> >> >> >> than
> >> >> >> happy to lend a hand. We've all learned from each other. Post a
> >> >> >> comment
> >> >> >> in-ticket, or post to the mail-list, and ask for guidance.
> >> >> >> Especially
> >> >> >> when
> >> >> >> it comes to Theme options, Theme code can get quite complex and
> >> >> >> often
> >> >> >> difficult to follow. Understanding how the Settings API works
> >> >> >> sometimes
> >> >> >> seems like it requires a master's degree. And developers all have
> >> >> >> different
> >> >> >> coding styles. It's completely understandable if someone needs a
> >> >> >> second
> >> >> >> pair
> >> >> >> of eyes when reviewing a given Theme. So please: ask for help if
> you
> >> >> >> need it
> >> >> >> when reviewing.
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> theme-reviewers mailing list
> >> >> >> theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> theme-reviewers mailing list
> >> >> >> theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________ theme-reviewers
> >> >> >> mailing
> >> >> >> list theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> theme-reviewers mailing list
> >> >> >> theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________ theme-reviewers
> >> >> >> mailing
> >> >> >> list theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> theme-reviewers mailing list
> >> >> >> theme-reviewers at lists.wordpress.org
> >> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >>
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > theme-reviewers mailing list
> >> >> > theme-reviewers at lists.wordpress.org
> >> >> > http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >> >
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> Konstantin
> >> >> _______________________________________________
> >> >> theme-reviewers mailing list
> >> >> theme-reviewers at lists.wordpress.org
> >> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >
> >> > _______________________________________________
> >> > theme-reviewers mailing list
> >> > theme-reviewers at lists.wordpress.org
> >> > http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >> >
> >>
> >>
> >>
> >> --
> >> Konstantin
> >> _______________________________________________
> >> theme-reviewers mailing list
> >> theme-reviewers at lists.wordpress.org
> >> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >
> >
> >
> > _______________________________________________
> > theme-reviewers mailing list
> > theme-reviewers at lists.wordpress.org
> > http://lists.wordpress.org/mailman/listinfo/theme-reviewers
> >
>
>
>
> --
> Konstantin
> _______________________________________________
> 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/20140130/5e0ae58c/attachment-0001.html>