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

Thu Jan 30 17:07:37 UTC 2014

> 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