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

Thu Jan 30 17:20:45 UTC 2014

It is true that a good custom CSS Plugin will save settings on a per-Theme
basis. But the critical criterion isn't whether a Plugin *can* do
something; rather, it is whether the *something* is functional or
presentational. After all, technically speaking, a Plugin *can* add Theme
support for custom headers and custom backgrounds. Should we force Theme
developers then to use a custom header/background Plugin, simply because
one exists and can accomplish the task?

Custom CSS is presentational, and specific to the Theme. As such, I believe
it falls squarely within the purview of Themes, and if Themes want to
provide a custom CSS Theme option, such an option would be appropriate.

On Thu, Jan 30, 2014 at 12:10 PM, Chip Bennett <chip at chipbennett.net> wrote:

> Yes, because Theme-specific custom JS is an extreme edge case. The vast
> majority of uses for custom JS are site functionality independent of the
> active Theme.
>
>
> On Thu, Jan 30, 2014 at 12:07 PM, Konstantin Kovshenin <
> kovshenin at gmail.com> wrote:
>
>> > The existence of custom CSS plugins, IMHO, is not sufficient
>> justification to force Theme developers to remove custom CSS Theme Options.
>>
>> What would make Custom JS any different?
>>
>> On Thu, Jan 30, 2014 at 8:57 PM, Chip Bennett <chip at chipbennett.net>
>> wrote:
>> > The existence of custom CSS plugins, IMHO, is not sufficient
>> justification
>> > to force Theme developers to remove custom CSS Theme Options.
>> >
>> >
>> > On Thu, Jan 30, 2014 at 11:54 AM, Konstantin Kovshenin <
>> kovshenin at gmail.com>
>> > wrote:
>> >>
>> >> > I really won't recommened Adding Custom CSS to Plugins Territory.
>> >> > Because, In support Forums, sometimes I provide users with Options
>> to make
>> >> > modifications to css which are specific to the theme. So, a Custom
>> CSS
>> >> > Option is a Great Help.
>> >>
>> >> That's not a valid point because there are plenty of great and secure
>> >> Custom CSS plugins out there.
>> >>
>> >> On Thu, Jan 30, 2014 at 8:50 PM, Rohit Tripathi <rohitink at live.com>
>> wrote:
>> >> > I really won't recommened Adding Custom CSS to Plugins Territory.
>> >> > Because,
>> >> > In support Forums, sometimes I provide users with Options to make
>> >> > modifications to css which are specific to the theme. So, a Custom
>> CSS
>> >> > Option is a Great Help.
>> >> >
>> >> > Random Header/Footer Scripts Could be considered Plugin Territory.
>> >> >
>> >> >> Date: Thu, 30 Jan 2014 20:46:06 +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
>> >> >>
>> >> >> > 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
>> >> >
>> >> > _______________________________________________
>> >> > 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/c28a0545/attachment-0001.html>