Na blogu dev.WPZlecenia pojawił się wczoraj artykuł sponsorowany pokazujący, jak nauczyć WordPressa przechowywania adresów IP logujących się użytkowników. Ponieważ komentarz do niego może być długi, a będzie miał też całkiem fajną wartość merytoryczną, to postanowiłem opublikować go tutaj. Zobaczymy sobie przy okazji, jak zły kod można krok po kroku poprawić.
Przede wszystkim trudno byłoby mi zaufać hostingowi, który wsadza mnie na minę i zapomina wspomnieć o ważnych konsekwencjach. Po pierwsze, jeśli chcemy sobie zapisywać adres IP, to ze względu na RODO musimy poinformować o tym fakcie użytkowników, wpisać to w regulamin i politykę prywatności, a przede wszystkim – uzasadnić potrzebę przechowywania tej właśnie informacji (bo nie możemy zbierać i przetwarzać danych bez uzasadnienia).
Natomiast, jeśli już przebrniemy przez całe to prawnicze zamieszanie… Fajnie byłoby robić to kodem, który ma ręce i nogi.
Przyjrzyjmy się zatem tym fragmentom kodu i zobaczmy, jak wiele da się zepsuć w kilku linijkach kodu…
Na początek funkcja zapisująca adres IP
We wspomnianym artykule wygląda ona tak:
function save_users_ip ($login) {
global $user_id;
$user = get_userdatabylogin($login);
update_user_meta($user->ID,'user_ip',$_SERVER['REMOTE_ADDR']);}
add_action('wp_login','save_users_ip');
Wszystko wygląda tutaj OK, ale… Zauważmy, że na początku funkcji dostajemy się do globalnej zmiennej $user_id. Po co? Jeśli taka zmienna istnieje, to znaczy, że mamy ID użytkownika i następna linijka jest zbędna. A jeśli nie istnieje, to po co ją tworzyć, skoro nawet jej nie używamy?
Ale w funkcji tej zaszyty jest także inny błąd, trudniejszy do wyłapania, jeśli się nie wie, że akcja wp_login otrzymuje dwa argumenty, a nie jeden (trac):
do_action( 'wp_login', $user->user_login, $user );
I nagle okazuje się, że pobieranie użytkownika na bazie loginu jest zbędne, bo dostajemy go za darmo, a zatem funkcja ta może wyglądać tak:
function save_users_ip ( $login, $user ) {
update_user_meta( $user->ID, 'user_ip', $_SERVER['REMOTE_ADDR'] );
}
add_action( 'wp_login', 'save_users_ip', 10, 2 );
Trochę się uprościła, prawda? A do tego nie wykonuje już zbędnego zapytania do bazy danych.
Dodawanie kolumny…
Autor realizuje to takim kodem:
function add_users_ip_column($column) {
return array_merge( $column,
array('user_ip' => __('Adres IP')) );}
add_filter('manage_users_columns','add_users_ip_column');
Jednolinijkowiec. Cóż może być źle? A no może… Autor bardzo ładnie użył funkcji internacjonalizacji. Niestety zapomniał o wytycznych WordPressa mówiących, że kod piszemy zawsze po angielsku tak, aby każdy programista czy tłumacz, był w stanie go zrozumieć i przetłumaczyć na swój język. No i większe przewinienie – brak domeny tłumaczeń (textdomain), przez co przetłumaczenie tej frazy będzie praktycznie niemożliwe.
A jeśli miałbym się już bardzo czepiać, to zwróciłbym jeszcze uwagę na nazewnictwo zmiennych. Parametr $column nie ma sensu i wprowadza w błąd, że zawiera on informacje o jednej kolumnie. Tymczasem tak nie jest. $columns to znacznie rozsądniejsza nazwa w tym przypadku.
Wyświetlanie przechowywanego adresu IP
Zostało nam jeszcze pokazanie adresów IP w tej kolumnie:
function show_users_ip($val,$column,$user_id) {
$user = get_userdata($user_id);
switch ($column) {
case 'user_ip' : return $user->user_ip; break;
default: }
return $return; }
add_filter('manage_users_custom_column','show_users_ip',10,3);
I cóż miał tu autor na myśli? Zacznijmy od końca. Jeśli $column jest różne od 'user_ip’, to zwracana jest wartość zmiennej $return. Tyle, że taka zmienna nigdzie nie istnieje. Oznacza to, że nasza funkcja skutecznie zadba o to, aby wszystkie dodatkowe kolumny były puste.
Dodatkowo kod ten za każdym razem pobiera dane użytkownika. Przy czym przecież filtr ten jest uruchamiany dla każdej kolumny, a dane te potrzebne są tylko dla naszej.
Jeśli natomiast skupimy się nieco dłużej, do odkryjemy, że autor dość bezrefleksyjnie użył tu kalki, która ma sens, jeśli dodajemy wiele własnych kolumn. Cała ta konstrukcja switch może być sprowadzona do:
function show_users_ip( $output, $column_name, $user_id ) {
if ( 'user_ip' === $column_name ) {
$user = get_userdata( $user_id );
return $user->user_ip;
}
return $output;
}
add_filter( 'manage_users_custom_column', 'show_users_ip', 10, 3 );
Nieco czytelniej, prawda? Przy okazji zmieniłem nazwy parametrów na zgodne z wywołaniem w kodzie WordPressa. I teraz łatwo zauważyć jeszcze jeden, bardzo poważny błąd… Mamy zmienną $output, która nie jest w żaden sposób escape’owana. Jeśli spojrzymy w kod WordPressa, gdzie ten filtr jest wywoływany, to zauważymy, że wartość zwrócona przez naszą funkcję zostanie po prostu sklejona z wypisywaną treścią tabeli. Oznacza to, że to na nas spoczywa obowiązek poprawnego escape’owania wyniku tej funkcji. Zatem jej poprawna i bezpieczna wersja wygląda tak:
function show_users_ip( $output, $column_name, $user_id ) {
if ( 'user_ip' === $column_name ) {
$user = get_userdata( $user_id );
return esc_html($user->user_ip);
}
return $output;
}
add_filter( 'manage_users_custom_column', 'show_users_ip', 10, 3 );
Podsumowanie
Jak widać nawet pisząc kilkanaście linijek kodu, można popełnić wiele błędów. Niektóre będą niegroźne (jak wielokrotne i zbędne pobieranie tej samej informacji z bazy danych), a inne narażą stronę na ataki (brak escape’owania pozwala w tym przypadku na wprowadzenie do bazy danych szkodliwego kodu JS, a następnie wykonanie go w panelu z uprawnieniami administracyjnymi – nie jest to nawet trudne, bo wartość REMODTE_ADDR atakujący może dość łatwo przekłamać).
Na tym mniej więcej polega właśnie code review, czyli solidny i spokojny przegląd kodu. Po części pozwala sprawić, że kod jest czystszy, bardziej czytelny i tańszy w utrzymaniu (bo kto za dwa lata będzie pamiętał, co autor danego fragmentu spaghetti miał na myśli). Ten przykład jednak świetnie pokazuje, że przegląd kodu pozwala wyłapać błędy w kodzie, który „został przetestowany i działa”, bo to, że kod „działa”, wcale nie znaczy, że nie powoduje błędów lub, co gorsze, nie naraża strony na ataki. Róbcie code review, a jeśli sami nie umiecie – zlecajcie – zapraszam 😉.