Closed missjwo closed 10 years ago
Hey @missjwo, I've been promising you a high level review on your work with HM Time for a while, so here it is! This is fairly high level / sparadic, I don't want to go through and comment every small thing as there is nothing totally wrong etc. The quality of the code is good, and I think you did a great job with this trial project. We didn't give a large amount of direction and I think what you architected was well done.
The documentation you wrote in the form of the docs/server_configs.md
file and the api_design.md
was fantastic. Very coherently written, and easy to understand. Most projects I come into contact with suffer from not having this high level documenation explaining concepts etc, so it's great to see that done here. Please keep up this habbit of writing docs, it helps developer onboarding a huge amount!
I thought the code structure was generally good, though there was a few notes on making it a bit cleaner:
Class
definition in the same file as code that just "runs", for example api-users.php
includes add_action( 'wp_json_server_before_serve', 'hm_time_api_init' );
. This means including this file will have side effects if you just wanted access to the Class
. Always have you "bootstrapping" code, like add filters etc in either the main plugin file, or a method on a class which is invoked from the main plugin file.includes/user-profile-fields.php
ther is a lot of "missing spaces" which is making my fingers twitch just looking at it! Remember the WordPress mantra, "If in doubt, space it out!" and you should be good!It's obvious from reading the code that you are familiar with sanitizing, but it often could/should be taken further. The general rule is "sanitize early". This means if you have ever dealing with a superglobal
like $_POST
, you should pass your data to a sanitizing function as soon as you assign it to a variable. Take the following example:
...
} else {
$hm_tz_new_set_method = $_POST['hm_tz_set_method'];
$hm_tz_new_timezone = $_POST['hm_tz_timezone'];
$hm_tz_new_location = $_POST['hm_tz_location'];
$hm_tz_new_workhours = $_POST['hm_tz_workhours'];
}
// Validate and Sanitize
//Set method validation
$valid_set_methods = hm_tz_timezone_options();
if(!empty($hm_tz_new_set_method) && array_key_exists($hm_tz_new_set_method, $valid_set_methods)){
update_user_meta( $user_id, 'hm_tz_set_method', $hm_tz_new_set_method );
}
...
Here you are validating to make sure the $hm_tz_new_set_method
is a valid value, which is great. However, because the variable $hm_tz_new_set_method
still potentially is unsafe data (say the string: <script>alert( document.cookie )</script>
), it's going to be very easy to accidently use that variable in another place and forget it is unsafe. For this reason, this code would be rejected from WordPress.com VIP for example. To get around this, always sanitize on assigning a variable:
$hm_tz_new_set_method = sanitize_key( $_POST['hm_tz_set_method'] );
$hm_tz_new_timezone = sanitize_key( $_POST['hm_tz_timezone'] );
$hm_tz_new_location = sanitize_text_field( $_POST['hm_tz_location'] );
$hm_tz_new_workhours = absint( $_POST['hm_tz_workhours'] ); //maybe this isn't an int?
Always try to use the most strict validation function. If the data should be a number, absint
, if it's a "loose" string, sanitize_text_field
, if it's a URL esc_url_raw
. it takes a bit of learning to get all the sanitizing functions WordPress provides, and also you shold be writing your own sanitizing functions for complex data types.
I also noticed somewhere you were doing:
$hm_time_new_timezone = apply_filters ( 'hm_time_timezone_filter', $hm_time_new_timezone, $_POST );
When a function hooks into this filter, it would not typically expect to be sanitizing the data passed in the second argument, as it wouldn't be $_POST
in that context. Here you are potentially passing dangerous data to an unsuspecting function.
On the escaping
side I saw much less, the other side to "sanitize early", is "escape late". That means you should be using an escaping function 90% of the time you are outputting anything in PHP. For example:
$hm_tz_location_value = get_user_meta($user_id, 'hm_tz_location', true);
$hm_tz_location_input = '<input type="text" name="hm_tz_location" value="'.$hm_tz_location_value.'"/>';
printf($table_row, 'hm_tz_location', __('City/ Country'), $hm_tz_location_input , '');
Here you are "trusting" that the user_meta
hm_tz_location
does not contain anything nefarious. As crazy as it may sound, in WordPress Security you don't trust data that comes from the database. It's possible (via some crazy mechanisn, or lack of sanitizing on input) the user meta contains the string " onclick="alert( document.cookie)
which is going to allow JavaScript execution, steal administrators cookies and generally make the sky fall on your head.
For every "context" of outputting in PHP there is a WordPress escaping functions. In this case we'd use esc_attr
because the value is being outputted to a attribute
. esc_attr
is not going to allow any charecters such as "
or '
that would allow the string to "break out" the attribute. Others, for example: esc_url
to be used when you put a URL, esc_html
when you are outputting inside HTML elements, e.g. <h1><?php esc_html( $title ) ?></h1>
, and many more.
It may seem a pain to have to do these things on every input of data, and every output, but these oversights typically account for 90% of the WordPress security exploits out there. These functions are tried and tested to save you from unexpected situations with handling user input and output.
I noticed you used translation functions for all the string which is great! It's much nicer to do this as you go, rather than needing to run through and wrap all your strings in the appropriate translation function.
You didn't use a Text Domain for the functions which you techincally should have. This allows the correct translation to be loaded for a given string, see http://codex.wordpress.org/I18n_for_WordPress_Developers#Text_Domains for more information there.
There was quite a few translation strings not translated - I know I didn't say specifically to do this so it's not an problem. Typically if you ever find yourself typing apostrophes with a English word like "Food "
then an alarm bell should go off to add translation functions!
That's about all, again I wanted to stress this was a great peice of work and really the above notes something to take on board when continueing with development. I'd recomment looking at other plugins we have built (more modern the better!) to just get an idea of code layout, formatting etc. Great Stuff!
There's no like button, so... :+1: to @missjwo for the project and @joehoyle for the feedback :)
Nice work @missjwo and great review @joehoyle, I imagine a lot of people would find the review a really useful read.
@missjwo I'd love to share this internally on updates as I think it would be really valuable for others in HM to read through, is that cool?
:+1:
Only just found this whilst I was cleaning my inbox.
I totally missed the email during WordCamp Manchester weekend.
@willmot Feel free to share. @joehoyle Thanks for the feedback! It's really useful.
Thanks!
Thanks for sharing this. It was very useful to read the review to know what kind of things to look out for! Thanks @missjwo and @joehoyle
I came across this intro article about validation and sanitisation/escaping. Might be useful to have a read http://code.tutsplus.com/articles/data-sanitization-and-validation-with-wordpress--wp-25536
Hi @joehoyle ,
@noeltock seemed keen for me to submit what I had done so far so I have got the work to a suitable state and added some notes that can be found in [docs/server_configs.md]
If you have any questions just contact me.
Jenny