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/tag/controllers/tags.php | 2 ++ 1 file changed, 2 insertions(+) (limited to 'modules/tag/controllers') 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); } -- 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/tag/controllers') 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 f1887422f8b4ba68dc273fe6f7d3f1123681e89a Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 23:07:41 -0700 Subject: Stricter input handling (cast to int) --- modules/tag/controllers/tags.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/tag/controllers') diff --git a/modules/tag/controllers/tags.php b/modules/tag/controllers/tags.php index b9f2c61c..1bd6b3cc 100644 --- a/modules/tag/controllers/tags.php +++ b/modules/tag/controllers/tags.php @@ -22,7 +22,7 @@ class Tags_Controller extends REST_Controller { public function _show($tag) { $page_size = module::get_var("gallery", "page_size", 9); - $page = $this->input->get("page", "1"); + $page = (int) $this->input->get("page", "1"); $children_count = $tag->items_count(); $offset = ($page-1) * $page_size; -- cgit v1.2.3 From 5490057480f17e5810cf8b9e558769ebd74d4b27 Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Wed, 16 Sep 2009 12:27:13 -0700 Subject: When editing tags in place, and there is a validation error, highlight the tag with a red border and show a statust message. This fixes ticket: #779 --- modules/tag/controllers/admin_tags.php | 15 ++++++++++----- modules/tag/js/tag.js | 6 ++++++ 2 files changed, 16 insertions(+), 5 deletions(-) (limited to 'modules/tag/controllers') diff --git a/modules/tag/controllers/admin_tags.php b/modules/tag/controllers/admin_tags.php index 8b8dde21..3301566b 100644 --- a/modules/tag/controllers/admin_tags.php +++ b/modules/tag/controllers/admin_tags.php @@ -81,15 +81,20 @@ class Admin_Tags_Controller extends Admin_Controller { kohana::show_404(); } - $form = tag::get_rename_form($tag); - $valid = $form->validate(); + //Don't use a form as the form is dynamically created in the js + $post = new Validation($_POST); + $post->add_rules("name", "required", "length[1,64]"); + $valid = $post->validate(); if ($valid) { - $new_name = $form->rename_tag->inputs["name"]->value; + $new_name = $this->input->post("name"); $new_tag = ORM::factory("tag")->where("name", $new_name)->find(); if ($new_tag->loaded) { - $form->rename_tag->inputs["name"]->add_error("in_use", 1); + $error_msg = "There is already a tag with that name"; $valid = false; } + } else { + $error_msg = $post->errors(); + $error_msg = $error_msg[0]; } if ($valid) { @@ -110,7 +115,7 @@ class Admin_Tags_Controller extends Admin_Controller { } else { print json_encode( array("result" => "error", - "form" => $form->__toString())); + "message" => $error_msg)); } } } diff --git a/modules/tag/js/tag.js b/modules/tag/js/tag.js index aaae9e72..d656da36 100644 --- a/modules/tag/js/tag.js +++ b/modules/tag/js/tag.js @@ -19,6 +19,7 @@ function ajaxify_tag_form() { function closeEditInPlaceForms() { // closes currently open inplace edit forms if ($("#gRenameTagForm").length) { + $("#gEditErrorMessage").remove(); var li = $("#gRenameTagForm").parent(); $("#gRenameTagForm").parent().html($("#gRenameTagForm").parent().data("revert")); li.height(""); @@ -66,6 +67,11 @@ function editInPlace(element) { $("#gTag-" + data.tag_id).text(data.new_tagname); // update tagname console.log(data); window.location.reload(); + } else if (data.result == "error") { + $("#gRenameTagForm #name") + .css("border", "2px solid red") + .focus(); + $("#gTagAdmin").before("

" + data.message + "

"); } } }); -- cgit v1.2.3 From 48326ad01708fcfa020283e2ad8b2cae4ede1600 Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Thu, 17 Sep 2009 12:11:00 -0700 Subject: Cleanup issues with the original fix for #779 --- modules/tag/controllers/admin_tags.php | 4 ++-- modules/tag/js/tag.js | 2 +- themes/admin_default/css/screen.css | 5 +++++ 3 files changed, 8 insertions(+), 3 deletions(-) (limited to 'modules/tag/controllers') diff --git a/modules/tag/controllers/admin_tags.php b/modules/tag/controllers/admin_tags.php index 3301566b..ced73d65 100644 --- a/modules/tag/controllers/admin_tags.php +++ b/modules/tag/controllers/admin_tags.php @@ -81,7 +81,7 @@ class Admin_Tags_Controller extends Admin_Controller { kohana::show_404(); } - //Don't use a form as the form is dynamically created in the js + // Don't use a form as the form is dynamically created in the js $post = new Validation($_POST); $post->add_rules("name", "required", "length[1,64]"); $valid = $post->validate(); @@ -89,7 +89,7 @@ class Admin_Tags_Controller extends Admin_Controller { $new_name = $this->input->post("name"); $new_tag = ORM::factory("tag")->where("name", $new_name)->find(); if ($new_tag->loaded) { - $error_msg = "There is already a tag with that name"; + $error_msg = t("There is already a tag with that name")->__toString(); $valid = false; } } else { diff --git a/modules/tag/js/tag.js b/modules/tag/js/tag.js index d656da36..52c695c6 100644 --- a/modules/tag/js/tag.js +++ b/modules/tag/js/tag.js @@ -69,7 +69,7 @@ function editInPlace(element) { window.location.reload(); } else if (data.result == "error") { $("#gRenameTagForm #name") - .css("border", "2px solid red") + .addClass("gError") .focus(); $("#gTagAdmin").before("

" + data.message + "

"); } diff --git a/themes/admin_default/css/screen.css b/themes/admin_default/css/screen.css index 33cc6733..de6d436e 100644 --- a/themes/admin_default/css/screen.css +++ b/themes/admin_default/css/screen.css @@ -1034,6 +1034,11 @@ li.gDefaultGroup h4, li.gDefaultGroup .gUser { float: right; } +#gRenameTagForm input[type="text"].gError { + border: 2px solid red; + background: none; +} + #gRenameTagForm input[type="submit"] { height: 25px; } -- cgit v1.2.3 From 2eeacd2656083739a588738b28d578e616d46c9c Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Thu, 17 Sep 2009 13:55:11 -0700 Subject: use an implicit cast to convert the translated error message to a string as it is encoded by the json routines and will be treated as an object otherwise --- modules/tag/controllers/admin_tags.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/tag/controllers') diff --git a/modules/tag/controllers/admin_tags.php b/modules/tag/controllers/admin_tags.php index ced73d65..b42e3748 100644 --- a/modules/tag/controllers/admin_tags.php +++ b/modules/tag/controllers/admin_tags.php @@ -89,7 +89,7 @@ class Admin_Tags_Controller extends Admin_Controller { $new_name = $this->input->post("name"); $new_tag = ORM::factory("tag")->where("name", $new_name)->find(); if ($new_tag->loaded) { - $error_msg = t("There is already a tag with that name")->__toString(); + $error_msg = (string)t("There is already a tag with that name"); $valid = false; } } else { -- cgit v1.2.3 From d050f0a2466fedfe96c3bbc072374d01b17951c5 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Thu, 17 Sep 2009 14:04:13 -0700 Subject: Minor style fix: (string) $var, not (string)$var, and move the explicit cast down where it's needed. --- modules/tag/controllers/admin_tags.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'modules/tag/controllers') diff --git a/modules/tag/controllers/admin_tags.php b/modules/tag/controllers/admin_tags.php index b42e3748..63f7957c 100644 --- a/modules/tag/controllers/admin_tags.php +++ b/modules/tag/controllers/admin_tags.php @@ -89,7 +89,7 @@ class Admin_Tags_Controller extends Admin_Controller { $new_name = $this->input->post("name"); $new_tag = ORM::factory("tag")->where("name", $new_name)->find(); if ($new_tag->loaded) { - $error_msg = (string)t("There is already a tag with that name"); + $error_msg = t("There is already a tag with that name"); $valid = false; } } else { @@ -115,7 +115,7 @@ class Admin_Tags_Controller extends Admin_Controller { } else { print json_encode( array("result" => "error", - "message" => $error_msg)); + "message" => (string) $error_msg)); } } } -- cgit v1.2.3