733420 - [One Mozilla Blog Theme] Review Universal Theme to be Deployed in Q1 to 14+ blogs

Status: VERIFIED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[mcbmoz]

What it does?

-Universal blog theme for all official Mozilla channels. Full list of blogs receiving this theme on Bug 733150

(more background: https://wiki.mozilla.org/One-Mozilla-Project/Documentation/Universal-Blog-Theme)

Where is the source code located?

-There is no staging environment, so Craig will need to link the final product here from his personal site.

Where would you like the bugs filed in bugzilla?

-Websites: component --> blog.mozilla.org

Will this application be collecting any personally identifiable information from users (email address, physical address, phone number, etc)?

-NO

Please describe if this app will be connecting to any internal or external services or if it is able to interact with the OS.

-NA


Does this app support logins or multiple roles? If so, we'll need test accounts created for each available role.

-NO
 

What is the worst case scenario that could happen with this system, data or connected systems? (This is used to help understand the criticality of this server.)

-NA

Does this website contain an administration page? If so, have the admin page blockers (listed here) all been addressed?

-Wordpress Admin connected with user LDAP

This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

-Must go live by March 20, 2012

Comment 1

[Craig Cook (:craigcook)]

> Where is the source code located?

https://github.com/craigcook/One-Mozilla-blog/tree/master/themes/OneMozilla

You can see it staged at http://wpstage.focalcurve.com/onemozilla/ and I can provide access there if you need it. But it's just a standard WP theme so you should be able to install it anywhere you need to for more thorough testing.

Comment 2

[Craig Cook (:craigcook)]

Update: the repo ownership has been moved to Mozilla's github account, so its official new home is https://github.com/mozilla/One-Mozilla-blog

Comment 3

[mcbmoz]

@tom - what is the date that this exact copy, as you've submitted in comment 2 (no changes), should go live? 

Notes: 

As mentioned in earlier conversations, we are freezing the current site for two weeks as we migrate to a new platform. As a result, the only release left for this to go live is Tuesday, March 13, 2012.

Otherwise, this can go into the April 3, 2012, release, when we resume our regular weekly publishing schedule.

Comment 4

[Adam Muntner [:adamm] (use NEEDINFO)]

I started taking a look today, and I think ptheriault is going to take a look, too.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I've looked through the code and it seems ok to me.

Only possible issue is the use of unsanitised user input. There are a number of places, for example, https://github.com/mozilla/One-Mozilla-blog/blob/master/themes/OneMozilla/archive.php#L22 where echo is used without an escape function:

<?php echo get_userdata(intval($author))->display_name; ?>

I am not sure if this is a concern of whether or not there is internal data validation which will prevent XSS in this case. From what I can understand from wordpress documentation there is a filtering layer which does just this, but it would be good for someone who knows it better to comfirm this. My main concern was that in other places functions like esc_attr are used, but not always. 

If this is an issue, then a search through the codebase is required as this is the case around half of the cases (which leans me more to thinking this isn't an issue) but I thought I would flag it to be on the safe side.

Apart from that I didnt find any other issues.

Comment 6

[Craig Cook (:craigcook)]

(In reply to Paul Theriault [:pauljt] from comment #5)

> Only possible issue is the use of unsanitised user input. There are a number
> of places, for example,
> https://github.com/mozilla/One-Mozilla-blog/blob/master/themes/OneMozilla/
> archive.php#L22 where echo is used without an escape function:
> 
> <?php echo get_userdata(intval($author))->display_name; ?>

Good catch. I believe such things get sanitized by WordPress on their way *in* (when the form is submitted) so it should be pretty clean as it lives in the database. But just to be extra careful we should sanitize those things wherever they're displayed as well (in case someone gains access to the DB and bypasses the forms). I mostly focused on comments but I'll go through and find all the places where author data is shown as well (names, URLs, bios, etc).

Comment 7

[Adam Muntner [:adamm] (use NEEDINFO)]

I'm still taking a look - can you please post to here when the change hits your repo? Thanks!

Comment 8

[Adam Muntner [:adamm] (use NEEDINFO)]

Not quite done yet, but wanting to update Craig on the ongoing test:

/wordpress/wp-content/themes/OneMozilla/inc/theme-options.php

194 <img src="<?php echo esc_url( $scheme['thumbnail'] ); ?>" width="140" height="140" alt="">
195  <?php echo $scheme['label']; ?>

Right not not a risk, since onemozilla_color_schemes() returns hardcoded values into the scheme[] array. However, I noticed that line 194 is properly escaped, but 195 is not, and should be, to "future-proof" the code. I can easily imagine this theme being changed in the future to make scheme[] array something other than hard-coded vals.

The same issue appears on line 211:

211 	<input type="checkbox" name="onemozilla_theme_options[hide_author]" value="1" <?php checked('1', $options['hide_author']); ?> />

Comment 9

[Adam Muntner [:adamm] (use NEEDINFO)]

Craig: I finished the review, and other than the already noted issues, it's in good shape!

Let me know when to re-sync from your git repo, so I can take a look at the changes.

Comment 10

[Craig Cook (:craigcook)]

(In reply to Adam Muntner :adamm from comment #9)
> Craig: I finished the review, and other than the already noted issues, it's
> in good shape!
> 
> Let me know when to re-sync from your git repo, so I can take a look at the
> changes.

I made a few updates in https://github.com/mozilla/One-Mozilla-blog/commit/d2f3070a1e1e86b255a419deaebbca5f29778208 escaping obvious places where author names are displayed. I think we should be good to go.

Comment 11

[Adam Muntner [:adamm] (use NEEDINFO)]

Excellent, I'll check it out this morning

Comment 12

[Adam Muntner [:adamm] (use NEEDINFO)]

I am looking at this view

https://github.com/mozilla/One-Mozilla-blog/commit/851b5ebaadc4f15b4a2d98c0020a4bfea06e7b78#themes/OneMozilla

It looks good, except I think there might be a functional bug: since the scripts are using printf instead of _e() in some places, which means not all the text hits the translate() function, which _e() is wrapping. 

Examples:

Archive.php

+  <?php elseif (is_author()) : ?><?php _e('Posts by ','onemozilla'); ?> <span><?php echo esc_html(get_userdata(intval($author))->display_name); ?></span>
+  <?php elseif (is_search()) : ?><?php printf( __('We found %1s results for &ldquo;%2s&ldquo;','onemozilla'), $total_results, esc_html(get_search_query()) ); ?> 

I see the same in search.php 13 +

where line 42 + in search.php uses _e()

and 45 + uses printf again, etc.

Comment 13

[Craig Cook (:craigcook)]

(In reply to Adam Muntner :adamm from comment #12)

> It looks good, except I think there might be a functional bug: since the
> scripts are using printf instead of _e() in some places, which means not all
> the text hits the translate() function, which _e() is wrapping. 

__() and _e() are roughly equivalent in WordPress, the difference is that __() simply "gets" a string and returns it to PHP, whereas _e() gets it and then echoes it immediately. We need to use the __() function inside a printf in some circumstances when the string includes a variable. In the example above the "%1s" and "%2s" are replaced variables so it has to be printf instead of a straight echo, and the rest of the string around it is still available for translation. Any string without a variable doesn't need to mess with printf so a simple _e() will echo the string right into the page.

See http://codex.wordpress.org/Translating_WordPress#Localization_Technology for more.

I have a few more minor updates to commit shortly but they're only stylistic things, no functional code. So I think we should be all done, security-wise?

Comment 14

[Adam Muntner [:adamm] (use NEEDINFO)]

Yup! And thank you for clearing that up.

Since the ticket is assigned to me, do I need to do something to it's status, now?

Comment 15

[mcbmoz]

@adamm - I think you approve it in the bug and close/resolved, which then let's Craig, me, and IT know that we can push to production.

We're super close right? Craig?

Comment 16

[Craig Cook (:craigcook)]

(In reply to Adam Muntner :adamm from comment #14)
> Since the ticket is assigned to me, do I need to do something to it's
> status, now?

If everything checks out, I think you just need to mark it Resolved - Fixed and that's it.