From 7ec490b6009965920fea35e971b29f11df6e6bff Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 11 Sep 2009 11:04:35 -0700 Subject: rawurlencode() path components in relative_path_cache and relative_url_cache so that they're safe for browser use. --- modules/gallery/tests/Item_Model_Test.php | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index 585e247c..84210e4c 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -150,4 +150,14 @@ class Item_Model_Test extends Unit_Test_Case { $this->assert_same("ORIGINAL_VALUE", $item->original()->title); $this->assert_same("NEW_VALUE", $item->title); } + + public function urls_are_rawurlencoded_test() { + $item = self::_create_random_item(); + $item->slug = "foo bar"; + $item->name = "foo bar.jpg"; + $item->save(); + + $this->assert_equal("foo%20bar", $item->relative_url()); + $this->assert_equal("foo%20bar.jpg", $item->relative_path()); + } } -- cgit v1.2.3 From 823fa2fc8339a6638ef4f0fcdae7f96e99a4f0bd Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 12 Sep 2009 10:33:46 -0700 Subject: Updated for url format changes applied in 2aad580f53dbc06bb170c710467b47a5a532c6c8. --- modules/gallery/tests/xss_data.txt | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/xss_data.txt b/modules/gallery/tests/xss_data.txt index 8c71740e..193d2ca1 100644 --- a/modules/gallery/tests/xss_data.txt +++ b/modules/gallery/tests/xss_data.txt @@ -48,6 +48,7 @@ modules/gallery/views/admin_block_log_entries.html.php 8 DIRTY $entry modules/gallery/views/admin_block_news.html.php 5 DIRTY_JS $entry["link"] modules/gallery/views/admin_block_news.html.php 5 DIRTY $entry["title"] modules/gallery/views/admin_block_news.html.php 7 DIRTY text::limit_words(strip_tags($entry["description"]),25); +modules/gallery/views/admin_block_photo_stream.html.php 5 DIRTY_JS $photo->url() modules/gallery/views/admin_block_photo_stream.html.php 6 DIRTY photo::img_dimensions($photo->width,$photo->height,72) modules/gallery/views/admin_block_photo_stream.html.php 7 DIRTY_ATTR $photo->thumb_url() modules/gallery/views/admin_dashboard.html.php 5 DIRTY_JS $csrf @@ -180,14 +181,14 @@ modules/image_block/views/image_block_block.html.php 3 DIRTY_JS $item- modules/image_block/views/image_block_block.html.php 4 DIRTY $item->thumb_img(array("class"=>"gThumbnail")) modules/info/views/info_block.html.php 22 DIRTY date("M j, Y H:i:s",$item->captured) modules/info/views/info_block.html.php 29 DIRTY_JS $item->owner->url -modules/notification/views/comment_published.html.php 28 DIRTY_JS $comment->item()->url(array(),true) -modules/notification/views/comment_published.html.php 29 DIRTY $comment->item()->url(array(),true) -modules/notification/views/item_added.html.php 16 DIRTY_JS $item->url(array(),true) -modules/notification/views/item_added.html.php 17 DIRTY $item->url(array(),true) -modules/notification/views/item_deleted.html.php 18 DIRTY_JS $item->parent()->url(array(),true) -modules/notification/views/item_deleted.html.php 19 DIRTY $item->parent()->url(array(),true) -modules/notification/views/item_updated.html.php 20 DIRTY_JS $item->url(array(),true) -modules/notification/views/item_updated.html.php 20 DIRTY $item->url(array(),true) +modules/notification/views/comment_published.html.php 28 DIRTY_JS $comment->item()->abs_url() +modules/notification/views/comment_published.html.php 29 DIRTY $comment->item()->abs_url() +modules/notification/views/item_added.html.php 16 DIRTY_JS $item->abs_url() +modules/notification/views/item_added.html.php 17 DIRTY $item->abs_url() +modules/notification/views/item_deleted.html.php 18 DIRTY_JS $item->parent()->abs_url() +modules/notification/views/item_deleted.html.php 19 DIRTY $item->parent()->abs_url() +modules/notification/views/item_updated.html.php 20 DIRTY_JS $item->abs_url() +modules/notification/views/item_updated.html.php 20 DIRTY $item->abs_url() modules/organize/views/organize_dialog.html.php 3 DIRTY_JS url::site("organize/move_to/__ALBUM_ID__?csrf=$csrf") modules/organize/views/organize_dialog.html.php 4 DIRTY_JS url::site("organize/rearrange/__TARGET_ID__/__BEFORE__?csrf=$csrf") modules/organize/views/organize_dialog.html.php 5 DIRTY_JS url::site("organize/sort_order/__ALBUM_ID__/__COL__/__DIR__?csrf=$csrf") @@ -246,6 +247,7 @@ modules/rss/views/feed.mrss.php 73 DIRTY_ATTR $chi modules/rss/views/feed.mrss.php 74 DIRTY_ATTR $child->mime_type modules/rss/views/rss_block.html.php 6 DIRTY_JS rss::url($url) modules/search/views/search.html.php 30 DIRTY_ATTR $item_class +modules/search/views/search.html.php 31 DIRTY_JS $item->url() modules/search/views/search.html.php 32 DIRTY $item->thumb_img() modules/server_add/views/admin_server_add.html.php 15 DIRTY_ATTR $id modules/server_add/views/admin_server_add.html.php 24 DIRTY $form @@ -285,6 +287,7 @@ themes/admin_default/views/admin.html.php 15 DIRTY_JS $theme themes/admin_default/views/admin.html.php 32 DIRTY $theme->admin_head() themes/admin_default/views/admin.html.php 36 DIRTY $theme->admin_page_top() themes/admin_default/views/admin.html.php 44 DIRTY $theme->admin_header_top() +themes/admin_default/views/admin.html.php 49 DIRTY_JS item::root()->url() themes/admin_default/views/admin.html.php 53 DIRTY $theme->admin_menu() themes/admin_default/views/admin.html.php 55 DIRTY $theme->admin_header_bottom() themes/admin_default/views/admin.html.php 62 DIRTY $content @@ -325,6 +328,8 @@ themes/default/views/page.html.php 41 DIRTY $new_w themes/default/views/page.html.php 42 DIRTY $new_height themes/default/views/page.html.php 43 DIRTY $thumb_proportion themes/default/views/page.html.php 82 DIRTY $header_text +themes/default/views/page.html.php 84 DIRTY_JS item::root()->url() +themes/default/views/page.html.php 98 DIRTY_JS $parent->url("show={$theme->item()->id}") themes/default/views/page.html.php 112 DIRTY $content themes/default/views/page.html.php 118 DIRTY newView("sidebar.html") themes/default/views/page.html.php 125 DIRTY $footer_text -- cgit v1.2.3 From 59eadacc67acb10d803ca7ef1bdc0635041a1d41 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 11:19:32 -0700 Subject: Improve language preference (Acccept-Language header matching): Boost same-language match over exact locale match for lower qvalue. --- modules/gallery/helpers/locales.php | 56 +++++++++++++++------------ modules/gallery/tests/Locales_Helper_Test.php | 10 ++++- 2 files changed, 41 insertions(+), 25 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/helpers/locales.php b/modules/gallery/helpers/locales.php index 16dda2d7..ab7f7526 100644 --- a/modules/gallery/helpers/locales.php +++ b/modules/gallery/helpers/locales.php @@ -165,50 +165,58 @@ class locales_Core { list ($ignored, $qvalue) = explode("=", $qvalue . "=="); $qvalue = floatval($qvalue); } - $locale_preferences[] = array($requested_locale, $qvalue); + // Group by language to boost inexact same-language matches + list ($language) = explode("_", $requested_locale . "_"); + if (!isset($locale_preferences[$language])) { + $locale_preferences[$language] = array(); + } + $locale_preferences[$language][$requested_locale] = $qvalue; } } // Compare and score requested locales with installed ones $scored_locales = array(); - foreach ($locale_preferences as $requested_value) { - $scored_locale_match = self::_locale_match_score($requested_value); - if ($scored_locale_match) { - $scored_locales[] = $scored_locale_match; + foreach ($locale_preferences as $language => $requested_locales) { + // Inexact match adjustment (same language, different region) + $fallback_adjustment_factor = 0.95; + if (count($requested_locales) > 1) { + // Sort by qvalue, descending + $qvalues = array_values($requested_locales); + rsort($qvalues); + // Ensure inexact match scores worse than 2nd preference in same language. + $fallback_adjustment_factor *= $qvalues[1]; + } + foreach ($requested_locales as $requested_locale => $qvalue) { + list ($matched_locale, $match_score) = + self::_locale_match_score($requested_locale, $qvalue, $fallback_adjustment_factor); + if ($matched_locale && + (!isset($scored_locales[$matched_locale]) || + $match_score > $scored_locales[$matched_locale])) { + $scored_locales[$matched_locale] = $match_score; + } } } - usort($scored_locales, array("locales", "_compare_locale_by_qvalue")); + arsort($scored_locales); - $best_match = array_shift($scored_locales); - if ($best_match) { - return $best_match[0]; - } + list ($locale) = each($scored_locales); + return $locale; } return null; } - static function _compare_locale_by_qvalue($a, $b) { - $a = $a[1]; - $b = $b[1]; - if ($a == $b) { - return 0; - } - return $a < $b ? 1 : -1; - } - - private static function _locale_match_score($requested_locale_and_qvalue) { - list ($requested_locale, $qvalue) = $requested_locale_and_qvalue; + private static function _locale_match_score($requested_locale, $qvalue, $adjustment_factor) { $installed = self::installed(); if (isset($installed[$requested_locale])) { - return $requested_locale_and_qvalue; + return array($requested_locale, $qvalue); } list ($language) = explode("_", $requested_locale . "_"); if (isset(self::$language_subtag_to_locale[$language]) && isset($installed[self::$language_subtag_to_locale[$language]])) { - return array(self::$language_subtag_to_locale[$language], $qvalue * 0.66); + $score = $adjustment_factor * $qvalue; + return array(self::$language_subtag_to_locale[$language], $score); } - return null; + return array(null, 0); } } \ No newline at end of file diff --git a/modules/gallery/tests/Locales_Helper_Test.php b/modules/gallery/tests/Locales_Helper_Test.php index 85b8e206..4c03d8d4 100644 --- a/modules/gallery/tests/Locales_Helper_Test.php +++ b/modules/gallery/tests/Locales_Helper_Test.php @@ -67,7 +67,7 @@ class Locales_Helper_Test extends Unit_Test_Case { locales::update_installed(array("no_NO", "pt_PT", "ja_JP")); $_SERVER["HTTP_ACCEPT_LANGUAGE"] = "en,en-us,ja_JP;q=0.7,no-fr;q=0.9"; $locale = locales::locale_from_http_request(); - $this->assert_equal("ja_JP", $locale); + $this->assert_equal("no_NO", $locale); } public function locale_from_http_request_best_match_vs_installed_2_test() { @@ -83,4 +83,12 @@ class Locales_Helper_Test extends Unit_Test_Case { $locale = locales::locale_from_http_request(); $this->assert_equal(null, $locale); } + + public function locale_from_http_request_prefer_inexact_same_language_match_over_exact_other_language_match_test() { + locales::update_installed(array("de_DE", "ar_AR", "fa_IR", "he_IL", "en_US")); + // Accept-Language header from Firefox 3.5/Ubuntu + $_SERVER["HTTP_ACCEPT_LANGUAGE"] = "he,en-us;q=0.9,de-ch;q=0.5,en;q=0.3"; + $locale = locales::locale_from_http_request(); + $this->assert_equal("he_IL", $locale); + } } \ No newline at end of file -- cgit v1.2.3 From 17254799d1069e9f67de14460264cda76395746f Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 20:27:04 -0700 Subject: Initial skeleton of Controller_Auth code audit test (non functional). --- modules/gallery/tests/Controller_Auth_Test.php | 211 +++++++++++++++++++++++++ modules/gallery/tests/controller_auth_data.txt | 0 2 files changed, 211 insertions(+) create mode 100644 modules/gallery/tests/Controller_Auth_Test.php create mode 100644 modules/gallery/tests/controller_auth_data.txt (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php new file mode 100644 index 00000000..9927859b --- /dev/null +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -0,0 +1,211 @@ +input, Forge] + // Require: ->validate() or access::verify_csrf\( + if ($function && $open_braces >= 2) { + if ($token[0] == T_STRING) { + if ($token[1] == "access" && + self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) && + self::_token_matches(array(T_STRING, "require"), $tokens, $token_number + 2) && + self::_token_matches("(", $tokens, $token_number + 3)) { + $token_number += 3; + $function->checks_authorization(true); + } else if ($token[1] == "access" && + self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) && + self::_token_matches(array(T_STRING, "verify_csrf"), $tokens, $token_number + 2) && + self::_token_matches("(", $tokens, $token_number + 3)) { + $token_number += 3; + $function->checks_csrf(true); + } else if (in_array($token[1], array("Input", "Forge")) && + self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1)) { + $token_number++; + $function->uses_input(true); + } + } else if ($token == T_VARIABLE) { + if ($token[1] == '$this' && + self::_token_matches(array(T_OBJECT_OPERATOR), $tokens, $token_number + 1) && + self::_token_matches(array(T_STRING, "input"), $tokens, $token_number + 2)) { + $token_number += 2; + $function->uses_input(true); + } + } else if ($token[0] == T_OBJECT_OPERATOR) { + if (self::_token_matches(array(T_STRING), "validate", $token_number + 1) && + self::_token_matches("(", $tokens, $token_number + 2)) { + $token_number += 2; + $function->checks_csrf(true); + } + } + } + } + } + } + + // Generate the report + $new = TMPPATH . "controller_auth_data.txt"; + $fd = fopen($new, "wb"); + ksort($found); + foreach ($found as $controller => $frames) { + foreach ($functions as $function) { + $flags = array(); + if ($function->uses_input() && !$function->checks_csrf()) { + $flags[] = "DIRTY_CSRF"; + } + if ($function->checks_authorization()) { + $flags[] = "DIRTY_AUTH"; + } + + if (!$flags) { + // Don't print CLEAN instances + continue; + } + + fprintf($fd, "%-60s %-20s %-21s\n", + $controller, $function->name, implode("|", $flags)); + } + } + fclose($fd); + + // Compare with the expected report from our golden file. + $canonical = MODPATH . "gallery/tests/controller_auth_data.txt"; + exec("diff $canonical $new", $output, $return_value); + $this->assert_false( + $return_value, "Controller auth golden file mismatch. Output:\n" . implode("\n", $output) ); + } + + private static function _token_matches($expected_token, &$tokens, $token_number) { + if (!isset($tokens[$token_number])) { + return false; + } + + $token = $tokens[$token_number]; + + if (is_array($expected_token)) { + for ($i = 0; $i < count($expected_token); $i++) { + if ($expected_token[$i] != $token[$i]) { + return false; + } + } + return true; + } else { + return $expected_token == $token; + } + } + + static function _function($name, $line) { + return new Controller_Auth_Test_Function($name, $line); + } +} + +class Controller_Auth_Test_Function { + public $name; + public $line; + private $_uses_input = false; + private $_checks_authorization = false; + private $_checks_csrf = false; + + function __construct($name, $line) { + $this->name = $name; + $this->line = $line; + } + + function uses_input($val=null) { + if ($val !== null) { + $this->_uses_input = $val; + } + return $this->_uses_input; + } + + function checks_authorization($val) { + if ($val !== null) { + $this->_checks_authorization = $val; + } + return $this->_checks_authorization; + } + + function checks_csrf($val) { + if ($val !== null) { + $this->_checks_csrf = $val; + } + return $this->_checks_csrf; + } +} \ No newline at end of file diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt new file mode 100644 index 00000000..e69de29b -- cgit v1.2.3 From 61bbe1d78c409dbc2d4af771146878f8f720959a Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 21:03:23 -0700 Subject: First functional version of Controller_Auth_Test --- modules/gallery/tests/Controller_Auth_Test.php | 48 ++++++++++++++++++-------- 1 file changed, 33 insertions(+), 15 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index 9927859b..e3eb4eaf 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -29,6 +29,8 @@ class Controller_Auth_Test extends Unit_Test_Case { } } + $is_admin_controller = false; + $open_braces = 0; $function = null; for ($token_number = 0; $token_number < count($tokens); $token_number++) { @@ -38,10 +40,12 @@ class Controller_Auth_Test extends Unit_Test_Case { // 1 open brace = in class context. // 2 open braces = in function. if (!is_array($token)) { - if ($token == "{") { + if ($token == "}") { $open_braces--; - if ($function) { + if ($open_braces == 1 && $function) { $found[$controller][] = $function; + } else if ($open_braces == 0) { + $is_admin_controller = false; } $function = null; } else if ($token == "{") { @@ -50,7 +54,11 @@ class Controller_Auth_Test extends Unit_Test_Case { } else { // An array token - if ($open_braces == 1 && $token[0] == T_FUNCTION) { + if ($open_braces == 0 && $token[0] == T_EXTENDS) { + if (self::_token_matches(array(T_STRING, "Admin_Controller"), $tokens, $token_number + 1)) { + $is_admin_controller = true; + } + } else if ($open_braces == 1 && $token[0] == T_FUNCTION) { $line = $token[2]; $name = ""; // Search backwards to check visibility, @@ -63,15 +71,15 @@ class Controller_Auth_Test extends Unit_Test_Case { // Search forward to get function name do { $token_number++; - if (self_::token_matches(array(T_STRING), $tokens, $token_number)) { + if (self::_token_matches(array(T_STRING), $tokens, $token_number)) { $token = $tokens[$token_number]; - $name = $tokens[1]; + $name = $token[1]; break; } } while ($token_number < count($tokens)); if (!$is_private) { - $function = self::_function($name, $line); + $function = self::_function($name, $line, $is_admin_controller); } } @@ -86,7 +94,7 @@ class Controller_Auth_Test extends Unit_Test_Case { if ($token[0] == T_STRING) { if ($token[1] == "access" && self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) && - self::_token_matches(array(T_STRING, "require"), $tokens, $token_number + 2) && + self::_token_matches(array(T_STRING, "required"), $tokens, $token_number + 2) && self::_token_matches("(", $tokens, $token_number + 3)) { $token_number += 3; $function->checks_authorization(true); @@ -109,7 +117,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $function->uses_input(true); } } else if ($token[0] == T_OBJECT_OPERATOR) { - if (self::_token_matches(array(T_STRING), "validate", $token_number + 1) && + if (self::_token_matches(array(T_STRING, "validate"), $tokens, $token_number + 1) && self::_token_matches("(", $tokens, $token_number + 2)) { $token_number += 2; $function->checks_csrf(true); @@ -124,13 +132,16 @@ class Controller_Auth_Test extends Unit_Test_Case { $new = TMPPATH . "controller_auth_data.txt"; $fd = fopen($new, "wb"); ksort($found); - foreach ($found as $controller => $frames) { + foreach ($found as $controller => $functions) { + $is_admin_controller = true; foreach ($functions as $function) { + $is_admin_controller &= $function->is_admin_controller; + $flags = array(); if ($function->uses_input() && !$function->checks_csrf()) { $flags[] = "DIRTY_CSRF"; } - if ($function->checks_authorization()) { + if (!$function->is_admin_controller && !$function->checks_authorization()) { $flags[] = "DIRTY_AUTH"; } @@ -142,6 +153,11 @@ class Controller_Auth_Test extends Unit_Test_Case { fprintf($fd, "%-60s %-20s %-21s\n", $controller, $function->name, implode("|", $flags)); } + + if (strpos(basename($controller), "admin_") === 0 && !$is_admin_controller) { + fprintf($fd, "%-60s %-20s %-21s\n", + $controller, basename($controller), "NO_ADMIN_CONTROLLER"); + } } fclose($fd); @@ -171,21 +187,23 @@ class Controller_Auth_Test extends Unit_Test_Case { } } - static function _function($name, $line) { - return new Controller_Auth_Test_Function($name, $line); + static function _function($name, $line, $is_admin_controller) { + return new Controller_Auth_Test_Function($name, $line, $is_admin_controller); } } class Controller_Auth_Test_Function { public $name; public $line; + public $is_admin_controller = false; private $_uses_input = false; private $_checks_authorization = false; private $_checks_csrf = false; - function __construct($name, $line) { + function __construct($name, $line, $is_admin_controller) { $this->name = $name; $this->line = $line; + $this->is_admin_controller = $is_admin_controller; } function uses_input($val=null) { @@ -195,14 +213,14 @@ class Controller_Auth_Test_Function { return $this->_uses_input; } - function checks_authorization($val) { + function checks_authorization($val=null) { if ($val !== null) { $this->_checks_authorization = $val; } return $this->_checks_authorization; } - function checks_csrf($val) { + function checks_csrf($val=null) { if ($val !== null) { $this->_checks_csrf = $val; } -- cgit v1.2.3 From e168e0dfae28bb56289b4debae8825c104ee69f9 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 21:50:48 -0700 Subject: CSRF / auth fixes, golden data file checkpoint --- modules/gallery/controllers/permissions.php | 2 +- modules/gallery/controllers/simple_uploader.php | 1 + modules/gallery/tests/Controller_Auth_Test.php | 8 ++++++-- modules/gallery/tests/controller_auth_data.txt | 17 +++++++++++++++++ modules/tag/controllers/tags.php | 2 ++ modules/user/controllers/admin_users.php | 2 +- 6 files changed, 28 insertions(+), 4 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/controllers/permissions.php b/modules/gallery/controllers/permissions.php index 5f4620b2..8d75862e 100644 --- a/modules/gallery/controllers/permissions.php +++ b/modules/gallery/controllers/permissions.php @@ -81,7 +81,7 @@ class Permissions_Controller extends Controller { } } - function _get_form($item) { + private function _get_form($item) { $view = new View("permissions_form.html"); $view->item = $item; $view->groups = ORM::factory("group")->find_all(); diff --git a/modules/gallery/controllers/simple_uploader.php b/modules/gallery/controllers/simple_uploader.php index 156d18ac..bc508319 100644 --- a/modules/gallery/controllers/simple_uploader.php +++ b/modules/gallery/controllers/simple_uploader.php @@ -32,6 +32,7 @@ class Simple_Uploader_Controller extends Controller { } public function start() { + access::verify_csrf(); batch::start(); } diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index e3eb4eaf..50afae8f 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -21,6 +21,10 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); foreach (glob("*/*/controllers/*.php") as $controller) { + if (strpos($controller, "modules/unit_test/") !== false) { + continue; + } + // List of all tokens without whitespace, simplifying parsing. $tokens = array(); foreach (token_get_all(file_get_contents($controller)) as $token) { @@ -150,12 +154,12 @@ class Controller_Auth_Test extends Unit_Test_Case { continue; } - fprintf($fd, "%-60s %-20s %-21s\n", + fprintf($fd, "%-60s %-20s %s\n", $controller, $function->name, implode("|", $flags)); } if (strpos(basename($controller), "admin_") === 0 && !$is_admin_controller) { - fprintf($fd, "%-60s %-20s %-21s\n", + fprintf($fd, "%-60s %-20s %s\n", $controller, basename($controller), "NO_ADMIN_CONTROLLER"); } } diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index e69de29b..aabd2863 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -0,0 +1,17 @@ +modules/comment/controllers/admin_comments.php queue DIRTY_CSRF +modules/digibug/controllers/digibug.php close_window DIRTY_AUTH +modules/gallery/controllers/combined.php javascript DIRTY_AUTH +modules/gallery/controllers/combined.php css DIRTY_AUTH +modules/gallery/controllers/maintenance.php index DIRTY_AUTH +modules/gallery/controllers/rest.php form_add DIRTY_AUTH +modules/gallery/controllers/rest.php _index DIRTY_AUTH +modules/gallery/controllers/rest.php _create DIRTY_AUTH +modules/gallery/controllers/rest.php _show DIRTY_AUTH +modules/gallery/controllers/rest.php _update DIRTY_AUTH +modules/gallery/controllers/rest.php _delete DIRTY_AUTH +modules/gallery/controllers/rest.php _form_add DIRTY_AUTH +modules/gallery/controllers/rest.php _form_edit DIRTY_AUTH +modules/gallery/controllers/simple_uploader.php start DIRTY_AUTH +modules/gallery/controllers/simple_uploader.php finish DIRTY_AUTH +modules/user/controllers/login.php ajax DIRTY_AUTH +modules/user/controllers/login.php html DIRTY_AUTH diff --git a/modules/tag/controllers/tags.php b/modules/tag/controllers/tags.php index c993e374..f4f98090 100644 --- a/modules/tag/controllers/tags.php +++ b/modules/tag/controllers/tags.php @@ -43,6 +43,8 @@ class Tags_Controller extends REST_Controller { } public function _index() { + // Far from perfection, but at least require view permission for the root album + access::required("view", 1); print tag::cloud(30); } diff --git a/modules/user/controllers/admin_users.php b/modules/user/controllers/admin_users.php index 521f82fa..0b748955 100644 --- a/modules/user/controllers/admin_users.php +++ b/modules/user/controllers/admin_users.php @@ -17,7 +17,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ -class Admin_Users_Controller extends Controller { +class Admin_Users_Controller extends Admin_Controller { public function index() { $view = new Admin_View("admin.html"); $view->content = new View("admin_users.html"); -- cgit v1.2.3 From dc3d45e7607acce91253e44c29998b8797131f93 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 22:01:59 -0700 Subject: Add exception for REST controllers (no fixes necessary). --- modules/gallery/tests/Controller_Auth_Test.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index 50afae8f..c4dc915b 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -18,6 +18,11 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class Controller_Auth_Test extends Unit_Test_Case { + static $rest_methods = array("_index", "_show", "_form_edit", "_form_add", "_create", + "_update", "_delete"); + + static $rest_methods_with_csrf_check = array("_update", "_delete", "_create"); + public function find_missing_auth_test() { $found = array(); foreach (glob("*/*/controllers/*.php") as $controller) { @@ -34,6 +39,7 @@ class Controller_Auth_Test extends Unit_Test_Case { } $is_admin_controller = false; + $is_rest_controller = false; $open_braces = 0; $function = null; @@ -50,6 +56,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $found[$controller][] = $function; } else if ($open_braces == 0) { $is_admin_controller = false; + $is_rest_controller = false; } $function = null; } else if ($token == "{") { @@ -61,6 +68,8 @@ class Controller_Auth_Test extends Unit_Test_Case { if ($open_braces == 0 && $token[0] == T_EXTENDS) { if (self::_token_matches(array(T_STRING, "Admin_Controller"), $tokens, $token_number + 1)) { $is_admin_controller = true; + } else if (self::_token_matches(array(T_STRING, "REST_Controller"), $tokens, $token_number + 1)) { + $is_rest_controller = true; } } else if ($open_braces == 1 && $token[0] == T_FUNCTION) { $line = $token[2]; @@ -82,8 +91,11 @@ class Controller_Auth_Test extends Unit_Test_Case { } } while ($token_number < count($tokens)); - if (!$is_private) { + if (!$is_private || ($is_rest_controller && in_array($name, self::$rest_methods))) { $function = self::_function($name, $line, $is_admin_controller); + if ($is_rest_controller && in_array($name, self::$rest_methods_with_csrf_check)) { + $function->checks_csrf(true); + } } } -- cgit v1.2.3 From 7608870537503ec571f45a175c8486d7945e7c63 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 22:51:49 -0700 Subject: Controller auth / CSRF fixes --- modules/gallery/tests/Controller_Auth_Test.php | 19 +++++++++++-------- modules/gallery/tests/controller_auth_data.txt | 22 ++++++++++++++++++++++ modules/organize/controllers/organize.php | 4 ++++ modules/rss/controllers/rss.php | 4 ++-- modules/tag/controllers/tags.php | 3 ++- 5 files changed, 41 insertions(+), 11 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index c4dc915b..cd4abe07 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -26,7 +26,7 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); foreach (glob("*/*/controllers/*.php") as $controller) { - if (strpos($controller, "modules/unit_test/") !== false) { + if (preg_match("{modules/(gallery_)?unit_test/}", $controller)) { continue; } @@ -54,11 +54,11 @@ class Controller_Auth_Test extends Unit_Test_Case { $open_braces--; if ($open_braces == 1 && $function) { $found[$controller][] = $function; + $function = null; } else if ($open_braces == 0) { $is_admin_controller = false; $is_rest_controller = false; } - $function = null; } else if ($token == "{") { $open_braces++; } @@ -80,6 +80,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $previous_2 = $tokens[$token_number - 2][0]; $is_private = in_array($previous, array(T_PRIVATE, T_PROTECTED)) || in_array($previous_2, array(T_PRIVATE, T_PROTECTED)); + $is_static = $previous == T_STATIC || $previous_2 == T_STATIC; // Search forward to get function name do { @@ -91,7 +92,9 @@ class Controller_Auth_Test extends Unit_Test_Case { } } while ($token_number < count($tokens)); - if (!$is_private || ($is_rest_controller && in_array($name, self::$rest_methods))) { + if (!$is_static && + (!$is_private || + ($is_rest_controller && in_array($name, self::$rest_methods)))) { $function = self::_function($name, $line, $is_admin_controller); if ($is_rest_controller && in_array($name, self::$rest_methods_with_csrf_check)) { $function->checks_csrf(true); @@ -110,7 +113,8 @@ class Controller_Auth_Test extends Unit_Test_Case { if ($token[0] == T_STRING) { if ($token[1] == "access" && self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) && - self::_token_matches(array(T_STRING, "required"), $tokens, $token_number + 2) && + self::_token_matches(array(T_STRING), $tokens, $token_number + 2) && + in_array($tokens[$token_number + 2][1], array("forbidden", "required")) && self::_token_matches("(", $tokens, $token_number + 3)) { $token_number += 3; $function->checks_authorization(true); @@ -125,7 +129,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $token_number++; $function->uses_input(true); } - } else if ($token == T_VARIABLE) { + } else if ($token[0] == T_VARIABLE) { if ($token[1] == '$this' && self::_token_matches(array(T_OBJECT_OPERATOR), $tokens, $token_number + 1) && self::_token_matches(array(T_STRING, "input"), $tokens, $token_number + 2)) { @@ -152,7 +156,6 @@ class Controller_Auth_Test extends Unit_Test_Case { $is_admin_controller = true; foreach ($functions as $function) { $is_admin_controller &= $function->is_admin_controller; - $flags = array(); if ($function->uses_input() && !$function->checks_csrf()) { $flags[] = "DIRTY_CSRF"; @@ -224,14 +227,14 @@ class Controller_Auth_Test_Function { function uses_input($val=null) { if ($val !== null) { - $this->_uses_input = $val; + $this->_uses_input = (bool) $val; } return $this->_uses_input; } function checks_authorization($val=null) { if ($val !== null) { - $this->_checks_authorization = $val; + $this->_checks_authorization = (bool) $val; } return $this->_checks_authorization; } diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index aabd2863..fcb977e4 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -1,8 +1,17 @@ modules/comment/controllers/admin_comments.php queue DIRTY_CSRF +modules/comment/controllers/comments.php _index DIRTY_CSRF +modules/digibug/controllers/digibug.php print_proxy DIRTY_CSRF|DIRTY_AUTH modules/digibug/controllers/digibug.php close_window DIRTY_AUTH +modules/gallery/controllers/admin.php __call DIRTY_AUTH +modules/gallery/controllers/albums.php _show DIRTY_CSRF +modules/gallery/controllers/albums.php _form_add DIRTY_CSRF modules/gallery/controllers/combined.php javascript DIRTY_AUTH modules/gallery/controllers/combined.php css DIRTY_AUTH +modules/gallery/controllers/file_proxy.php __call DIRTY_CSRF|DIRTY_AUTH modules/gallery/controllers/maintenance.php index DIRTY_AUTH +modules/gallery/controllers/rest.php __construct DIRTY_AUTH +modules/gallery/controllers/rest.php __call DIRTY_AUTH +modules/gallery/controllers/rest.php form_edit DIRTY_AUTH modules/gallery/controllers/rest.php form_add DIRTY_AUTH modules/gallery/controllers/rest.php _index DIRTY_AUTH modules/gallery/controllers/rest.php _create DIRTY_AUTH @@ -13,5 +22,18 @@ modules/gallery/controllers/rest.php _form_add modules/gallery/controllers/rest.php _form_edit DIRTY_AUTH modules/gallery/controllers/simple_uploader.php start DIRTY_AUTH modules/gallery/controllers/simple_uploader.php finish DIRTY_AUTH +modules/gallery/controllers/upgrader.php index DIRTY_AUTH +modules/gallery/controllers/welcome_message.php index DIRTY_AUTH +modules/rss/controllers/rss.php feed DIRTY_CSRF|DIRTY_AUTH +modules/search/controllers/search.php index DIRTY_CSRF|DIRTY_AUTH +modules/server_add/controllers/admin_server_add.php autocomplete DIRTY_CSRF +modules/server_add/controllers/server_add.php children DIRTY_CSRF +modules/tag/controllers/admin_tags.php index DIRTY_CSRF +modules/tag/controllers/tags.php _show DIRTY_CSRF|DIRTY_AUTH modules/user/controllers/login.php ajax DIRTY_AUTH +modules/user/controllers/login.php auth_ajax DIRTY_AUTH modules/user/controllers/login.php html DIRTY_AUTH +modules/user/controllers/login.php auth_html DIRTY_AUTH +modules/user/controllers/logout.php index DIRTY_CSRF|DIRTY_AUTH +modules/user/controllers/password.php reset DIRTY_AUTH +modules/user/controllers/password.php do_reset DIRTY_CSRF|DIRTY_AUTH diff --git a/modules/organize/controllers/organize.php b/modules/organize/controllers/organize.php index 259c94e7..08c80de3 100644 --- a/modules/organize/controllers/organize.php +++ b/modules/organize/controllers/organize.php @@ -45,9 +45,13 @@ class Organize_Controller extends Controller { access::verify_csrf(); $target_album = ORM::factory("item", $target_album_id); + access::required("view", $target_album); + access::required("add", $target_album); + foreach ($this->input->post("source_ids") as $source_id) { $source = ORM::factory("item", $source_id); if (!$source->contains($target_album)) { + access::required("edit", $source); item::move($source, $target_album); } } diff --git a/modules/rss/controllers/rss.php b/modules/rss/controllers/rss.php index b89bed40..015d6032 100644 --- a/modules/rss/controllers/rss.php +++ b/modules/rss/controllers/rss.php @@ -21,13 +21,13 @@ class Rss_Controller extends Controller { public static $page_size = 20; public function feed($module_id, $feed_id, $id=null) { - $page = $this->input->get("page", 1); + $page = (int) $this->input->get("page", 1); if ($page < 1) { url::redirect(url::merge(array("page" => 1))); } // Configurable page size between 1 and 100, default 20 - $page_size = max(1, min(100, $this->input->get("page_size", self::$page_size))); + $page_size = max(1, min(100, (int) $this->input->get("page_size", self::$page_size))); // Run the appropriate feed callback if (module::is_active($module_id)) { diff --git a/modules/tag/controllers/tags.php b/modules/tag/controllers/tags.php index f4f98090..b9f2c61c 100644 --- a/modules/tag/controllers/tags.php +++ b/modules/tag/controllers/tags.php @@ -44,7 +44,8 @@ class Tags_Controller extends REST_Controller { public function _index() { // Far from perfection, but at least require view permission for the root album - access::required("view", 1); + $album = ORM::factory("item", 1); + access::required("view", $album); print tag::cloud(30); } -- cgit v1.2.3 From 39632c4689842b3e3bb0715c0e9be757149c257d Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 23:01:26 -0700 Subject: Also check for rss feeds in controller auth check --- modules/gallery/tests/Controller_Auth_Test.php | 8 ++++++-- modules/gallery/tests/controller_auth_data.txt | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index cd4abe07..caf6d8f2 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -25,7 +25,9 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); - foreach (glob("*/*/controllers/*.php") as $controller) { + $controllers = glob("*/*/controllers/*.php"); + $feeds = glob("*/*/helpers/*_rss.php"); + foreach (array_merge($controllers, $feeds) as $controller) { if (preg_match("{modules/(gallery_)?unit_test/}", $controller)) { continue; } @@ -92,7 +94,9 @@ class Controller_Auth_Test extends Unit_Test_Case { } } while ($token_number < count($tokens)); - if (!$is_static && + $is_rss_feed = $name == "feed" && strpos(basename($controller), "_rss.php"); + + if ((!$is_static || $is_rss_feed) && (!$is_private || ($is_rest_controller && in_array($name, self::$rest_methods)))) { $function = self::_function($name, $line, $is_admin_controller); diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index fcb977e4..fdf00c5e 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -1,5 +1,6 @@ modules/comment/controllers/admin_comments.php queue DIRTY_CSRF modules/comment/controllers/comments.php _index DIRTY_CSRF +modules/comment/helpers/comment_rss.php feed DIRTY_AUTH modules/digibug/controllers/digibug.php print_proxy DIRTY_CSRF|DIRTY_AUTH modules/digibug/controllers/digibug.php close_window DIRTY_AUTH modules/gallery/controllers/admin.php __call DIRTY_AUTH -- cgit v1.2.3 From 2e23ae98c43ae099a0b7b18f3c65fae21401aa43 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Thu, 17 Sep 2009 14:12:43 -0700 Subject: - Add theme->movie_menu() to whitelisted methods. - xss_data checkpoint --- modules/gallery/tests/Xss_Security_Test.php | 3 ++- modules/gallery/tests/xss_data.txt | 25 +++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Xss_Security_Test.php b/modules/gallery/tests/Xss_Security_Test.php index 85624517..16541017 100644 --- a/modules/gallery/tests/Xss_Security_Test.php +++ b/modules/gallery/tests/Xss_Security_Test.php @@ -144,7 +144,8 @@ class Xss_Security_Test extends Unit_Test_Case { "dynamic_bottom", "dynamic_top", "footer", "head", "header_bottom", "header_top", "page_bottom", "page_top", "photo_blocks", "photo_bottom", "photo_top", "resize_bottom", "resize_top", "sidebar_blocks", "sidebar_bottom", - "sidebar_top", "thumb_bottom", "thumb_info", "thumb_top")) && + "sidebar_top", "thumb_bottom", "thumb_info", "thumb_top", + "movie_menu")) && self::_token_matches("(", $tokens, $token_number + 3)) { $method = $tokens[$token_number + 2][1]; diff --git a/modules/gallery/tests/xss_data.txt b/modules/gallery/tests/xss_data.txt index 193d2ca1..57da8730 100644 --- a/modules/gallery/tests/xss_data.txt +++ b/modules/gallery/tests/xss_data.txt @@ -295,10 +295,11 @@ themes/admin_default/views/admin.html.php 68 DIRTY $sideb themes/admin_default/views/admin.html.php 73 DIRTY $theme->admin_footer() themes/admin_default/views/admin.html.php 75 DIRTY $theme->admin_credits() themes/admin_default/views/admin.html.php 79 DIRTY $theme->admin_page_bottom() -themes/admin_default/views/block.html.php 2 DIRTY $id -themes/admin_default/views/block.html.php 2 DIRTY_ATTR $css_id -themes/admin_default/views/block.html.php 10 DIRTY $title -themes/admin_default/views/block.html.php 13 DIRTY $content +themes/admin_default/views/block.html.php 3 DIRTY_ATTR $anchor +themes/admin_default/views/block.html.php 5 DIRTY $id +themes/admin_default/views/block.html.php 5 DIRTY_ATTR $css_id +themes/admin_default/views/block.html.php 13 DIRTY $title +themes/admin_default/views/block.html.php 16 DIRTY $content themes/admin_default/views/pager.html.php 13 DIRTY_JS str_replace('{page}',1,$url) themes/admin_default/views/pager.html.php 20 DIRTY_JS str_replace('{page}',$previous_page,$url) themes/admin_default/views/pager.html.php 27 DIRTY $from_to_msg @@ -309,10 +310,10 @@ themes/default/views/album.html.php 16 DIRTY_ATTR $ite themes/default/views/album.html.php 18 DIRTY_JS $child->url() themes/default/views/album.html.php 19 DIRTY $child->thumb_img(array("class"=>"gThumbnail")) themes/default/views/album.html.php 23 DIRTY_JS $child->url() -themes/default/views/block.html.php 2 DIRTY_ATTR $anchor -themes/default/views/block.html.php 3 DIRTY_ATTR $css_id -themes/default/views/block.html.php 4 DIRTY $title -themes/default/views/block.html.php 6 DIRTY $content +themes/default/views/block.html.php 3 DIRTY_ATTR $anchor +themes/default/views/block.html.php 5 DIRTY_ATTR $css_id +themes/default/views/block.html.php 6 DIRTY $title +themes/default/views/block.html.php 8 DIRTY $content themes/default/views/dynamic.html.php 11 DIRTY_ATTR $child->is_album()?"gAlbum":"" themes/default/views/dynamic.html.php 13 DIRTY_JS $child->url() themes/default/views/dynamic.html.php 14 DIRTY_ATTR $child->id @@ -329,10 +330,10 @@ themes/default/views/page.html.php 42 DIRTY $new_h themes/default/views/page.html.php 43 DIRTY $thumb_proportion themes/default/views/page.html.php 82 DIRTY $header_text themes/default/views/page.html.php 84 DIRTY_JS item::root()->url() -themes/default/views/page.html.php 98 DIRTY_JS $parent->url("show={$theme->item()->id}") -themes/default/views/page.html.php 112 DIRTY $content -themes/default/views/page.html.php 118 DIRTY newView("sidebar.html") -themes/default/views/page.html.php 125 DIRTY $footer_text +themes/default/views/page.html.php 102 DIRTY_JS $parent->url($parent==$theme->item()->parent()?"show={$theme->item()->id}":null) +themes/default/views/page.html.php 117 DIRTY $content +themes/default/views/page.html.php 123 DIRTY newView("sidebar.html") +themes/default/views/page.html.php 130 DIRTY $footer_text themes/default/views/pager.html.php 13 DIRTY_JS str_replace('{page}',1,$url) themes/default/views/pager.html.php 20 DIRTY_JS str_replace('{page}',$previous_page,$url) themes/default/views/pager.html.php 27 DIRTY $from_to_msg -- cgit v1.2.3