Security pass over all controller code. Mostly adding CSRF checking

and verifying user permissions, but there are several above-the-bar
changes:

1) Server add is now only available to admins.  This is a hard
   requirement because we have to limit server access (eg:
   server_add::children) to a user subset and the current permission
   model doesn't include that.  Easiest fix is to restrict to admins.
   Got rid of the server_add permission.

2) We now know check permissions at every level, which means in
   controllers AND in helpers.  This "belt and suspenders" approach will
   give us defense in depth in case we overlook it in one area.

3) We now do CSRF checking in every controller method that changes the
   code, in addition to the Forge auto-check.  Again, defense in depth
   and it makes scanning the code for security much simpler.

4) Moved Simple_Uploader_Controller::convert_filename_to_title to
   item:convert_filename_to_title

5) Fixed a bug in sending notification emails.

6) Fixed the Organize code to verify that you only have access to your
   own tasks.  In general, added permission checks to organize which had
   pretty much no validation code.

I did my best to verify every feature that I touched.

modules/akismet/controllers/admin_akismet.php

@@ -22,6 +22,9 @@ class Admin_Akismet_Controller extends Admin_Controller {
     $form = akismet::get_configure_form();
 
     if (request::method() == "post") {
+      // @todo move the "post" handler part of this code into a separate function
+      access::verify_csrf();
+
       $valid = $form->validate();
 
       if ($valid) {

modules/comment/controllers/admin_comments.php

@@ -107,6 +107,7 @@ class Admin_Comments_Controller extends Admin_Controller {
 
   public function set_state($id, $state) {
     access::verify_csrf();
+
     $comment = ORM::factory("comment", $id);
     $orig = clone $comment;
     if ($comment->loaded) {
@@ -121,6 +122,7 @@ class Admin_Comments_Controller extends Admin_Controller {
 
   public function delete_all_spam() {
     access::verify_csrf();
+
     ORM::factory("comment")
       ->where("state", "spam")
       ->delete_all();

modules/comment/controllers/comments.php

@@ -134,6 +134,7 @@ class Comments_Controller extends REST_Controller {
    */
   public function _update($comment) {
     $item = ORM::factory("item", $comment->item_id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     $form = comment::get_edit_form($comment);
@@ -161,6 +162,7 @@ class Comments_Controller extends REST_Controller {
    */
   public function _delete($comment) {
     $item = ORM::factory("item", $comment->item_id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     $comment->delete();
@@ -183,6 +185,9 @@ class Comments_Controller extends REST_Controller {
    *  @see REST_Controller::form_edit($resource)
    */
   public function _form_edit($comment) {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
     print comment::get_edit_form($comment);
   }
 }

modules/g2_import/controllers/admin_g2_import.php

@@ -39,6 +39,8 @@ class Admin_g2_import_Controller extends Admin_Controller {
   }
 
   public function save() {
+    access::verify_csrf();
+
     $form = $this->_get_import_form();
     if ($form->validate()) {
       $embed_path = $form->configure_g2_import->embed_path->value;

modules/gallery/controllers/admin.php

@@ -22,8 +22,9 @@ class Admin_Controller extends Controller {
 
   public function __construct($theme=null) {
     if (!(user::active()->admin)) {
-      throw new Exception("@todo UNAUTHORIZED", 401);
+      access::forbidden();
     }
+
     parent::__construct();
   }
 

modules/gallery/controllers/admin_dashboard.php

@@ -29,6 +29,8 @@ class Admin_Dashboard_Controller extends Admin_Controller {
   }
 
   public function add_block() {
+    access::verify_csrf();
+
     $form = gallery_block::get_add_block_form();
     if ($form->validate()) {
       list ($module_name, $id) = explode(":", $form->add_block->id->value);
@@ -51,6 +53,7 @@ class Admin_Dashboard_Controller extends Admin_Controller {
 
   public function remove_block($id) {
     access::verify_csrf();
+
     $blocks_center = block_manager::get_active("dashboard_center");
     $blocks_sidebar = block_manager::get_active("dashboard_sidebar");
 
@@ -73,6 +76,7 @@ class Admin_Dashboard_Controller extends Admin_Controller {
 
   public function reorder() {
     access::verify_csrf();
+
     $active_set = array();
     foreach (array("dashboard_sidebar", "dashboard_center") as $location) {
       foreach (block_manager::get_active($location) as $id => $info) {

modules/gallery/controllers/admin_graphics.php

@@ -43,6 +43,7 @@ class Admin_Graphics_Controller extends Admin_Controller {
 
   public function choose($toolkit) {
     access::verify_csrf();
+
     if ($toolkit != module::get_var("gallery", "graphics_toolkit")) {
       module::set_var("gallery", "graphics_toolkit", $toolkit);
 

modules/gallery/controllers/admin_languages.php

@@ -31,6 +31,8 @@ class Admin_Languages_Controller extends Admin_Controller {
   }
 
   public function save() {
+    access::verify_csrf();
+
     $form = $this->_languages_form();
     if ($form->validate()) {
       module::set_var("gallery", "default_locale", $form->choose_language->locale->value);
@@ -41,6 +43,8 @@ class Admin_Languages_Controller extends Admin_Controller {
   }
 
   public function share() {
+    access::verify_csrf();
+
     $form = $this->_share_translations_form();
     if (!$form->validate()) {
       // Show the page with form errors

modules/gallery/controllers/admin_theme_details.php

@@ -26,6 +26,8 @@ class Admin_Theme_Details_Controller extends Admin_Controller {
   }
 
   public function save() {
+    access::verify_csrf();
+
     $form = theme::get_edit_form_admin();
     if ($form->validate()) {
       module::set_var("gallery", "page_size", $form->edit_theme->page_size->value);

modules/gallery/controllers/albums.php

@@ -24,11 +24,11 @@ class Albums_Controller extends Items_Controller {
    */
   public function _show($album) {
     if (!access::can("view", $album)) {
-      if ($album->id != 1) {
-        access::forbidden();
-      } else {
+      if ($album->id == 1) {
         print new Theme_View("login_page.html", "album");
         return;
+      } else {
+        access::forbidden();
       }
     }
 
@@ -77,6 +77,8 @@ class Albums_Controller extends Items_Controller {
    * @see REST_Controller::_create($resource)
    */
   public function _create($album) {
+    access::verify_csrf();
+    access::required("view", $album);
     access::required("add", $album);
 
     switch ($this->input->post("type")) {
@@ -92,6 +94,7 @@ class Albums_Controller extends Items_Controller {
   }
 
   private function _create_album($album) {
+    access::required("view", $album);
     access::required("add", $album);
 
     $form = album::get_add_form($album);
@@ -120,6 +123,7 @@ class Albums_Controller extends Items_Controller {
   }
 
   private function _create_photo($album) {
+    access::required("view", $album);
     access::required("add", $album);
 
     // If we set the content type as JSON, it triggers saving the result as
@@ -153,6 +157,8 @@ class Albums_Controller extends Items_Controller {
    * @see REST_Controller::_update($resource)
    */
   public function _update($album) {
+    access::verify_csrf();
+    access::required("view", $album);
     access::required("edit", $album);
 
     $form = album::get_edit_form($album);
@@ -202,6 +208,7 @@ class Albums_Controller extends Items_Controller {
    */
   public function _form_add($album_id) {
     $album = ORM::factory("item", $album_id);
+    access::required("view", $album);
     access::required("add", $album);
 
     switch ($this->input->get("type")) {
@@ -223,6 +230,7 @@ class Albums_Controller extends Items_Controller {
    *  @see REST_Controller::_form_add($parameters)
    */
   public function _form_edit($album) {
+    access::required("view", $album);
     access::required("edit", $album);
 
     print album::get_edit_form($album);

modules/gallery/controllers/l10n_client.php

@@ -20,7 +20,9 @@
 class L10n_Client_Controller extends Controller {
   public function save() {
     access::verify_csrf();
-    user::active()->admin or access::forbidden();
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
     $input = Input::instance();
     $message = $input->post("l10n-message-source");
@@ -58,6 +60,9 @@ class L10n_Client_Controller extends Controller {
 
   public function toggle_l10n_mode() {
     access::verify_csrf();
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
     $session = Session::instance();
     $session->set("l10n_mode",
@@ -89,6 +94,10 @@ class L10n_Client_Controller extends Controller {
   }
 
   public static function l10n_form() {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
+
     $calls = I18n::instance()->call_log();
 
     if ($calls) {

modules/gallery/controllers/move.php

@@ -20,6 +20,7 @@
 class Move_Controller extends Controller {
   public function browse($source_id) {
     $source = ORM::factory("item", $source_id);
+    access::required("view", $source);
     access::required("edit", $source);
 
     $view = new View("move_browse.html");
@@ -33,6 +34,11 @@ class Move_Controller extends Controller {
     $source = ORM::factory("item", $source_id);
     $target = ORM::factory("item", $this->input->post("target_id"));
 
+    access::required("view", $source);
+    access::required("edit", $source);
+    access::required("view", $target);
+    access::required("edit", $target);
+
     item::move($source, $target);
 
     print json_encode(
@@ -43,8 +49,11 @@ class Move_Controller extends Controller {
   public function show_sub_tree($source_id, $target_id) {
     $source = ORM::factory("item", $source_id);
     $target = ORM::factory("item", $target_id);
+    access::required("view", $source);
     access::required("edit", $source);
     access::required("view", $target);
+    // show targets even if they're not editable because they may contain children which *are*
+    // editable
 
     print $this->_get_tree_html($source, $target);
   }

modules/gallery/controllers/movies.php

@@ -66,6 +66,8 @@ class Movies_Controller extends Items_Controller {
    * @see REST_Controller::_update($resource)
    */
   public function _update($photo) {
+    access::verify_csrf();
+    access::required("view", $photo);
     access::required("edit", $photo);
 
     $form = photo::get_edit_form($photo);
@@ -108,6 +110,7 @@ class Movies_Controller extends Items_Controller {
    *  @see REST_Controller::_form_edit($resource)
    */
   public function _form_edit($photo) {
+    access::required("view", $photo);
     access::required("edit", $photo);
     print photo::get_edit_form($photo);
   }

modules/gallery/controllers/permissions.php

@@ -20,6 +20,7 @@
 class Permissions_Controller extends Controller {
   function browse($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if (!$item->is_album()) {
@@ -37,6 +38,7 @@ class Permissions_Controller extends Controller {
 
   function form($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if (!$item->is_album()) {
@@ -48,9 +50,11 @@ class Permissions_Controller extends Controller {
 
   function change($command, $group_id, $perm_id, $item_id) {
     access::verify_csrf();
+
     $group = ORM::factory("group", $group_id);
     $perm = ORM::factory("permission", $perm_id);
     $item = ORM::factory("item", $item_id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if ($group->loaded && $perm->loaded && $item->loaded) {

modules/gallery/controllers/photos.php

@@ -62,10 +62,13 @@ class Photos_Controller extends Items_Controller {
     print $template;
   }
 
+
   /**
    * @see REST_Controller::_update($resource)
    */
   public function _update($photo) {
+    access::verify_csrf();
+    access::required("view", $photo);
     access::required("edit", $photo);
 
     $form = photo::get_edit_form($photo);
@@ -110,7 +113,9 @@ class Photos_Controller extends Items_Controller {
    *  @see REST_Controller::_form_edit($resource)
    */
   public function _form_edit($photo) {
+    access::required("view", $photo);
     access::required("edit", $photo);
+
     print photo::get_edit_form($photo);
   }
 }

modules/gallery/controllers/quick.php

@@ -19,8 +19,8 @@
  */
 class Quick_Controller extends Controller {
   public function pane($id) {
-    $item = ORM::factory("item", $id);
-    if (!$item->loaded) {
+    $item = model_cache::get("item", $id);
+    if (!access::can("view", $item) || !access::can("edit", $item)) {
       return "";
     }
 
@@ -32,10 +32,9 @@ class Quick_Controller extends Controller {
 
   public function rotate($id, $dir) {
     access::verify_csrf();
-    $item = ORM::factory("item", $id);
-    if (!$item->loaded) {
-      return "";
-    }
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
+    access::required("edit", $item);
 
     $degrees = 0;
     switch($dir) {
@@ -82,14 +81,21 @@ class Quick_Controller extends Controller {
 
   public function make_album_cover($id) {
     access::verify_csrf();
-    item::make_album_cover(ORM::factory("item", $id));
+
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
+    access::required("view", $item->parent());
+    access::required("edit", $item->parent());
+
+    item::make_album_cover($item);
 
     print json_encode(array("result" => "success"));
   }
 
   public function delete($id) {
     access::verify_csrf();
-    $item = ORM::factory("item", $id);
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if ($item->is_album()) {
@@ -110,8 +116,10 @@ class Quick_Controller extends Controller {
   }
 
   public function form_edit($id) {
-    $item = ORM::factory("item", $id);
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
+
     if ($item->is_album()) {
       $form = album::get_edit_form($item);
     } else {

modules/gallery/controllers/rest.php

@@ -86,21 +86,20 @@ class REST_Controller extends Controller {
       return Kohana::show_404();
     }
 
-    if ($request_method != "get") {
-      access::verify_csrf();
-    }
-
     switch ($request_method) {
     case "get":
       return $this->_show($resource);
 
     case "put":
+      access::verify_csrf();
       return $this->_update($resource);
 
     case "delete":
+      access::verify_csrf();
       return $this->_delete($resource);
 
     case "post":
+      access::verify_csrf();
       return $this->_create($resource);
     }
   }
@@ -111,17 +110,18 @@ class REST_Controller extends Controller {
       throw new Exception("@todo ERROR_MISSING_RESOURCE_TYPE");
     }
 
-    // @todo this needs security checks
     $resource = ORM::factory($this->resource_type, $resource_id);
     if (!$resource->loaded) {
       return Kohana::show_404();
     }
 
+    // Security checks must be performed in _form_edit
     return $this->_form_edit($resource);
   }
 
   /* We're adding a new item, pass along any additional parameters. */
   public function form_add($parameters) {
+    // Security checks must be performed in _form_add
     return $this->_form_add($parameters);
   }
 

modules/gallery/controllers/simple_uploader.php

@@ -20,6 +20,7 @@
 class Simple_Uploader_Controller extends Controller {
   public function app($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("add", $item);
 
     $v = new View("simple_uploader.html");
@@ -33,13 +34,13 @@ class Simple_Uploader_Controller extends Controller {
 
   public function add_photo($id) {
     $album = ORM::factory("item", $id);
+    access::required("view", $album);
     access::required("add", $album);
     access::verify_csrf();
 
     $file_validation = new Validation($_FILES);
     $file_validation->add_rules("Filedata", "upload::valid", "upload::type[gif,jpg,png,flv,mp4]");
     if ($file_validation->validate()) {
-
       // SimpleUploader.swf does not yet call /start directly, so simulate it here for now.
       if (!batch::in_progress()) {
         batch::start();
@@ -48,7 +49,7 @@ class Simple_Uploader_Controller extends Controller {
       $temp_filename = upload::save("Filedata");
       try {
         $name = substr(basename($temp_filename), 10);  // Skip unique identifier Kohana adds
-        $title = $this->convert_filename_to_title($name);
+        $title = item::convert_filename_to_title($name);
         $path_info = pathinfo($temp_filename);
         if (array_key_exists("extension", $path_info) &&
             in_array(strtolower($path_info["extension"]), array("flv", "mp4"))) {
@@ -69,18 +70,11 @@ class Simple_Uploader_Controller extends Controller {
     print "File Received";
   }
 
-  /**
-   * We should move this into a helper somewhere.. but where is appropriate?
-   */
-  private function convert_filename_to_title($filename) {
-    $title = strtr($filename, "_", " ");
-    $title = preg_replace("/\..*?$/", "", $title);
-    $title = preg_replace("/ +/", " ", $title);
-    return $title;
-  }
-
   public function finish() {
+    access::verify_csrf();
+
     batch::stop();
     print json_encode(array("result" => "success"));
   }
+
 }

modules/gallery/helpers/item.php

@@ -19,6 +19,8 @@
  */
 class item_Core {
   static function move($source, $target) {
+    access::required("view", $source);
+    access::required("view", $target);
     access::required("edit", $source);
     access::required("edit", $target);
 
@@ -47,6 +49,8 @@ class item_Core {
 
   static function make_album_cover($item) {
     $parent = $item->parent();
+    access::required("view", $item);
+    access::required("view", $parent);
     access::required("edit", $parent);
 
     model_cache::clear("item", $parent->album_cover_item_id);
@@ -61,6 +65,7 @@ class item_Core {
   }
 
   static function remove_album_cover($album) {
+    access::required("view", $album);
     access::required("edit", $album);
     @unlink($album->thumb_path());
 
@@ -102,4 +107,16 @@ class item_Core {
       $input->add_error("conflict", 1);
     }
   }
+
+  /**
+   * Sanitize a filename into something presentable as an item title
+   * @param string $filename
+   * @return string title
+   */
+  static function convert_filename_to_title($filename) {
+    $title = strtr($filename, "_", " ");
+    $title = preg_replace("/\..*?$/", "", $title);
+    $title = preg_replace("/ +/", " ", $title);
+    return $title;
+  }
 }

modules/gallery/libraries/MY_Forge.php

@@ -26,6 +26,7 @@ class Forge extends Forge_Core {
     parent::__construct($action, $title, $method, $attr);
     $this->hidden("csrf")->value("");
   }
+
   /**
    * Use our own template
    */

modules/gallery/views/simple_uploader.html.php

@@ -3,7 +3,7 @@
 <script type="text/javascript" src="<?= url::file("lib/swfupload/swfupload.queue.js") ?>"></script>
 
 <!-- hack to set the title for the dialog -->
-<form id="gAddPhotosForm" action="<?= url::site("simple_uploader/finish") ?>">
+<form id="gAddPhotosForm" action="<?= url::site("simple_uploader/finish?csrf=$csrf") ?>">
   <fieldset>
     <legend> <?= t("Add photos to %album_title", array("album_title" => p::clean($item->title))) ?> </legend>
   </fieldset>

modules/notification/helpers/notification.php

@@ -149,7 +149,7 @@ class notification {
       $result = ORM::factory("pending_notification")
         ->where("email", $email)
         ->find_all();
-      if ($result->count == 1) {
+      if ($result->count() == 1) {
         $pending = $result->get();
         Sendmail::factory()
           ->to($email)

modules/organize/controllers/organize.php

@@ -24,19 +24,22 @@ class Organize_Controller extends Controller {
   function index($item_id=1) {
     $item = ORM::factory("item", $item_id);
     $root = ($item->id == 1) ? $item : ORM::factory("item", 1);
+    access::required("view", $item);
+    access::required("edit", $item);
 
     $v = new View("organize.html");
     $v->root = $root;
     $v->item = $item;
     $v->album_tree = $this->tree($item, $root);
-
     $v->button_pane = new View("organize_button_pane.html");
-
     print $v;
   }
 
   function content($item_id) {
     $item = ORM::factory("item", $item_id);
+    access::required("view", $item);
+    access::required("edit", $item);
+
     $width = $this->input->get("width");
     $height = $this->input->get("height");
     $offset = $this->input->get("offset", 0);
@@ -55,12 +58,17 @@ class Organize_Controller extends Controller {
 
   function header($item_id) {
     $item = ORM::factory("item", $item_id);
+    access::required("view", $item);
+    access::required("edit", $item);
 
     print json_encode(array("title" => $item->title,
                             "description" => empty($item->description) ? "" : $item->description));
   }
 
   function tree($item, $parent) {
+    access::required("view", $item);
+    access::required("edit", $item);
+
     $albums = ORM::factory("item")
       ->where(array("parent_id" => $parent->id, "type" => "album"))
       ->orderby(array("title" => "ASC"))
@@ -88,6 +96,8 @@ class Organize_Controller extends Controller {
     $items = $this->input->post("item");
 
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
+    access::required("edit", $item);
 
     $definition = $this->_getOperationDefinition($item, $operation);
 
@@ -101,22 +111,26 @@ class Organize_Controller extends Controller {
     // @todo If there is only one item then call task_run($task->id); Maybe even change js so
     // we can call finish as well.
     batch::start();
-    print json_encode(array("result" => "started",
-                            "runningMsg" => $definition["runningMsg"],
-                            "pauseMsg" => "<div class=\"gWarning\">{$definition['pauseMsg']}</div>",
-                            "resumeMsg" => "<div class=\"gWarning\">{$definition['resumeMsg']}</div>",
-                            "task" => array("id" => $task->id,
-                                            "percent_complete" => $task->percent_complete,
-                                            "type" => $task->get("type"),
-                                            "status" => $task->status,
-                                            "state" => $task->state,
-                                            "done" => $task->done)));
+    print json_encode(
+      array("result" => "started",
+            "runningMsg" => $definition["runningMsg"],
+            "pauseMsg" => "<div class=\"gWarning\">{$definition['pauseMsg']}</div>",
+            "resumeMsg" => "<div class=\"gWarning\">{$definition['resumeMsg']}</div>",
+            "task" => array("id" => $task->id,
+                            "percent_complete" => $task->percent_complete,
+                            "type" => $task->get("type"),
+                            "status" => $task->status,
+                            "state" => $task->state,
+                            "done" => $task->done)));
   }
 
   function runTask($task_id) {
     access::verify_csrf();
 
     $task = task::run($task_id);
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
 
     print json_encode(array("result" => $task->done ? $task->state : "in_progress",
                             "task" => array("id" => $task->id,
@@ -132,6 +146,9 @@ class Organize_Controller extends Controller {
     access::verify_csrf();
 
     $task = ORM::factory("task", $task_id);
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
 
     if ($task->done) {
       $item = ORM::factory("item", (int)$task->get("target"));
@@ -178,6 +195,9 @@ class Organize_Controller extends Controller {
     access::verify_csrf();
 
     $task = ORM::factory("task", $task_id);
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
 
     if (!$task->done) {
       $task->done = 1;
@@ -210,7 +230,7 @@ class Organize_Controller extends Controller {
   function editForm() {
     $event_parms = new stdClass();
     $event_parms->panes = array();
-    $event_parms->itemids = $this->input->get("item");;
+    $event_parms->itemids = $this->input->get("item");
 
     // The following code should be done more dynamically i.e. use the event mechanism
     if (count($event_parms->itemids) == 1) {
@@ -218,8 +238,12 @@ class Organize_Controller extends Controller {
         ->in("id", $event_parms->itemids[0])
         ->find();
 
-      $event_parms->panes[] = array("label" => $item->is_album() ? t("Edit Album") : t("Edit Photo"),
-                                    "content" => organize::get_general_edit_form($item));
+      access::required("view", $item);
+      access::required("edit", $item);
+
+      $event_parms->panes[] = array(
+        "label" => $item->is_album() ? t("Edit Album") : t("Edit Photo"),
+        "content" => organize::get_general_edit_form($item));
 
       if ($item->is_album()) {
         $event_parms->panes[] = array("label" => t("Sort Order"),
@@ -243,6 +267,7 @@ class Organize_Controller extends Controller {
     $item = ORM::factory("item")
       ->in("id", $itemids[0])
       ->find();
+    access::required("view", $item);
     access::required("edit", $item);
 
     $form = organize::get_general_edit_form($item);
@@ -273,6 +298,7 @@ class Organize_Controller extends Controller {
     $item = ORM::factory("item")
       ->in("id", $itemids[0])
       ->find();
+    access::required("view", $item);
     access::required("edit", $item);
 
     print organize::get_general_edit_form($item);
@@ -285,6 +311,7 @@ class Organize_Controller extends Controller {
     $item = ORM::factory("item")
       ->in("id", $itemids[0])
       ->find();
+    access::required("view", $item);
     access::required("edit", $item);
 
     $form = organize::get_sort_edit_form($item);
@@ -309,6 +336,7 @@ class Organize_Controller extends Controller {
     $item = ORM::factory("item")
       ->in("id", $itemids[0])
       ->find();
+    access::required("view", $item);
     access::required("edit", $item);
 
     print organize::get_sort_edit_form($item);
@@ -373,6 +401,13 @@ class Organize_Controller extends Controller {
   }
 
   private function _add_tag($new_tag, $itemids) {
+    // Super lame security stopgap.  This code is going to get rewritten anyway.
+    foreach ($itemids as $item_id) {
+      $item = ORM::factory("item", $item_id);
+      access::required("view", $item);
+      access::required("edit", $item);
+    }
+
     $tag = ORM::factory("tag")
       ->where("name", $new_tag)
       ->find();
@@ -391,6 +426,13 @@ class Organize_Controller extends Controller {
   }
 
   private function _delete_tag($new_tag, $itemids) {
+    // Super lame security stopgap.  This code is going to get rewritten anyway.
+    foreach ($itemids as $item_id) {
+      $item = ORM::factory("item", $item_id);
+      access::required("view", $item);
+      access::required("edit", $item);
+    }
+
     $tag = ORM::factory("tag")
       ->where("name", $new_tag)
       ->find();
@@ -407,6 +449,13 @@ class Organize_Controller extends Controller {
   }
 
   private function _update_tag($new_tag, $itemids) {
+    // Super lame security stopgap.  This code is going to get rewritten anyway.
+    foreach ($itemids as $item_id) {
+      $item = ORM::factory("item", $item_id);
+      access::required("view", $item);
+      access::required("edit", $item);
+    }
+
     $tag = ORM::factory("tag")
       ->where("name", $new_tag)
       ->find();
@@ -441,6 +490,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("The move operation was paused"),
                    "resumeMsg" => t("The move operation was resumed"));
       break;
+
     case "rearrange":
       return array("description" => t("Rearrange the order of albums and photos"),
                    "name" => t("Rearrange: %name", array("name" => $item->title)),
@@ -449,6 +499,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("The rearrange operation was paused"),
                    "resumeMsg" => t("The rearrange operation was resumed"));
       break;
+
     case "rotateCcw":
       return array("description" => t("Rotate the selected photos counter clockwise"),
                    "name" => t("Rotate images in %name", array("name" => $item->title)),
@@ -457,6 +508,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("The rotate operation was paused"),
                    "resumeMsg" => t("The rotate operation was resumed"));
       break;
+
     case "rotateCw":
       return array("description" => t("Rotate the selected photos clockwise"),
                    "name" => t("Rotate images in %name", array("name" => $item->title)),
@@ -465,6 +517,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("The rotate operation was paused"),
                    "resumeMsg" => t("The rotate operation was resumed"));
       break;
+
     case "delete":
       return array("description" => t("Delete selected photos and albums"),
                    "name" => t("Delete images in %name", array("name" => $item->title)),
@@ -473,6 +526,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("The delete operation was paused"),
                    "resumeMsg" => t("The delete operation was resumed"));
       break;
+
     case "albumCover":
       return array("description" => t("Reset Album Cover"),
                    "name" => t("Reset Album cover for %name", array("name" => $item->title)),
@@ -481,6 +535,7 @@ class Organize_Controller extends Controller {
                    "pauseMsg" => t("Reset album cover was paused"),
                    "resumeMsg" => t("Reset album cover was resumed"));
       break;
+
     default:
       throw new Exception("Operation '$operation' is not implmented");
     }

modules/organize/helpers/organize.php

@@ -66,6 +66,14 @@ class organize_Core {
     $tagPane->hidden("item")->value(implode("|", $itemids));
     $item_count = count($itemids);
     $ids = implode(", ", $itemids);
+
+    // Lame stopgap security check.  This code is going to get rewritten anyway.
+    foreach ($itemids as $id) {
+      $item = ORM::factory("item", $id);
+      access::required("view", $item);
+      access::required("edit", $item);
+    }
+
     $tags = Database::instance()->query(
       "SELECT t.name, COUNT(it.item_id) as count
          FROM {items_tags} it, {tags} t

modules/organize/helpers/organize_task.php

@@ -38,30 +38,55 @@ class organize_task_Core {
         switch ($taskType) {
         case "move":
           $source = ORM::factory("item", $id);
+          access::required("view", $source);
+          access::required("view", $target);
+          access::required("edit", $source);
+          access::required("edit", $target);
+
           item::move($source, $target);
           break;
+
         case "rearrange":
+          $item = ORM::factory("item", $id);
+          access::required("view", $item);
+          access::required("edit", $item);
+
           Database::instance()
             ->query("Update {items} set weight = {$context["position"]} where id=$id;");
           break;
+
         case "rotateCcw":
         case "rotateCw":
           $item = ORM::factory("item", $id);
+          access::required("view", $item);
+          access::required("edit", $item);
+
           if ($item->is_photo()) {
             $context["post_process"]["reload"][] =
               self::_do_rotation($item, $taskType == "rotateCcw" ? -90 : 90);
           }
           break;
+
         case "albumCover":
-          item::make_album_cover(ORM::factory("item", $id));
+          $item = ORM::factory("item", $id);
+          access::required("view", $item);
+          access::required("view", $item->parent());
+          access::required("edit", $item->parent());
+
+          item::make_album_cover($item);
           break;
+
         case "delete":
           $item = ORM::factory("item", $id);
+          access::required("view", $item);
+          access::required("edit", $item);
+
           $item->delete();
           $context["post_process"]["remove"][] = array("id" => $id);
           break;
+
         default:
-          throw new Exception("Task '$taskType' is not implmented");
+          throw new Exception("Task '$taskType' is not implemented");
         }
       }
       $context["position"] += $stop;

modules/recaptcha/controllers/admin_recaptcha.php

@@ -21,6 +21,8 @@ class Admin_Recaptcha_Controller extends Admin_Controller {
   public function index() {
     $form = recaptcha::get_configure_form();
     if (request::method() == "post") {
+      // @todo move the "save" part of this into a separate controller function
+      access::verify_csrf();
       $old_public_key = module::get_var("recaptcha", "public_key");
       $old_private_key = module::get_var("recaptcha", "private_key");
       if ($form->validate()) {
@@ -55,10 +57,4 @@ class Admin_Recaptcha_Controller extends Admin_Controller {
     $view->content->form = $form;
     print $view;
   }
-
-  public function test() {
-    $view = new View("admin_recaptcha_test.html");
-    $view->public_key = module::get_var("recaptcha", "public_key");
-    print $view;
-  }
 }

modules/server_add/controllers/server_add.php

@@ -21,10 +21,11 @@ class Server_Add_Controller extends Controller {
   public function index($id) {
     $paths = unserialize(module::get_var("server_add", "authorized_paths"));
 
-    $item = ORM::factory("item", $id);
-    access::required("server_add", $item);
-    access::required("add", $item);
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
+    $item = ORM::factory("item", $id);
     $view = new View("server_add_tree_dialog.html");
     $view->action = url::abs_site("__ARGS__/{$id}__TASK_ID__?csrf=" . access::csrf_token());
     $view->parents = $item->parents();
@@ -41,8 +42,11 @@ class Server_Add_Controller extends Controller {
   }
 
   public function children() {
-    $paths = unserialize(module::get_var("server_add", "authorized_paths"));
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
+    $paths = unserialize(module::get_var("server_add", "authorized_paths"));
     $path_valid = false;
     $path = $this->input->post("path");
 
@@ -66,7 +70,12 @@ class Server_Add_Controller extends Controller {
   }
 
   function start($id) {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
     access::verify_csrf();
+
+    $item = ORM::factory("item", $id);
     $paths = unserialize(module::get_var("server_add", "authorized_paths"));
     $input_files = $this->input->post("path");
     $files = array();
@@ -114,9 +123,15 @@ class Server_Add_Controller extends Controller {
   }
 
   function add_photo($task_id) {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
     access::verify_csrf();
 
     $task = task::run($task_id);
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
 
     if ($task->done) {
       switch ($task->state) {
@@ -146,10 +161,16 @@ class Server_Add_Controller extends Controller {
   }
 
   public function finish($id, $task_id) {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
     access::verify_csrf();
-
     $task = ORM::factory("task", $task_id);
 
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
+
     if (!$task->done) {
       message::warning(t("Add from server was cancelled prior to completion"));
     }
@@ -159,9 +180,14 @@ class Server_Add_Controller extends Controller {
   }
 
   public function pause($id, $task_id) {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
     access::verify_csrf();
-
     $task = ORM::factory("task", $task_id);
+    if (!$task->loaded || $task->owner_id != user::active()->id) {
+      access::forbidden();
+    }
 
     message::warning(t("Add from server was cancelled prior to completion"));
     batch::stop();

modules/server_add/helpers/server_add_installer.php

@@ -22,7 +22,6 @@ class server_add_installer {
     $db = Database::instance();
     $version = module::get_version("server_add");
     if ($version == 0) {
-      access::register_permission("server_add", t("Add files from server"));
       module::set_version("server_add", 1);
     }
     server_add::check_config();
@@ -31,8 +30,4 @@ class server_add_installer {
   static function deactivate() {
     site_status::clear("server_add_configuration");
   }
-
-  static function uninstall() {
-    access::delete_permission("server_add");
-  }
 }

modules/server_add/helpers/server_add_menu.php

@@ -28,11 +28,9 @@ class server_add_menu_Core {
 
   static function site($menu, $theme) {
     $item = $theme->item();
-
     $paths = unserialize(module::get_var("server_add", "authorized_paths"));
 
-    if ($item && access::can("edit", $item) && access::can("server_add", $item) &&
-        $item->is_album() && !empty($paths)) {
+    if (user::active()->admin && $item->is_album() && !empty($paths)) {
       $options_menu = $menu->get("options_menu")
         ->append(Menu::factory("dialog")
                  ->id("server_add")

modules/server_add/helpers/server_add_task.php

@@ -31,7 +31,6 @@ class server_add_task_Core {
       if (!empty($context["files"][$path])) {
         $file = $context["files"][$path][$context["position"]];
         $parent = ORM::factory("item", $file["parent_id"]);
-        access::required("server_add", $parent);
         access::required("add", $parent);
         if (!$parent->is_album()) {
           throw new Exception("@todo BAD_ALBUM");

modules/tag/controllers/admin_tags.php

@@ -42,6 +42,7 @@ class Admin_Tags_Controller extends Admin_Controller {
 
   public function delete($id) {
     access::verify_csrf();
+
     $tag = ORM::factory("tag", $id);
     if (!$tag->loaded) {
       kohana::show_404();

modules/tag/controllers/tags.php

@@ -48,6 +48,7 @@ class Tags_Controller extends REST_Controller {
 
   public function _create($tag) {
     $item = ORM::factory("item", $this->input->post("item_id"));
+    access::required("view", $item);
     access::required("edit", $item);
 
     $form = tag::get_add_form($item);
@@ -73,6 +74,7 @@ class Tags_Controller extends REST_Controller {
   public function _form_add($item_id) {
     $item = ORM::factory("item", $item_id);
     access::required("view", $item);
+    access::required("edit", $item);
 
     return tag::get_add_form($item);
   }

modules/user/controllers/admin_users.php

@@ -28,6 +28,7 @@ class Admin_Users_Controller extends Controller {
 
   public function add_user() {
     access::verify_csrf();
+
     $form = user::get_add_form_admin();
     $valid = $form->validate();
     $name = $form->add_user->inputs["name"]->value;
@@ -63,6 +64,7 @@ class Admin_Users_Controller extends Controller {
 
   public function delete_user($id) {
     access::verify_csrf();
+
     if ($id == user::active()->id || $id == user::guest()->id) {
       access::forbidden();
     }
@@ -97,6 +99,7 @@ class Admin_Users_Controller extends Controller {
 
   public function edit_user($id) {
     access::verify_csrf();
+
     $user = ORM::factory("user", $id);
     if (!$user->loaded) {
       kohana::show_404();
@@ -182,6 +185,7 @@ class Admin_Users_Controller extends Controller {
 
   public function add_group() {
     access::verify_csrf();
+
     $form = group::get_add_form_admin();
     $valid = $form->validate();
     if ($valid) {
@@ -210,6 +214,7 @@ class Admin_Users_Controller extends Controller {
 
   public function delete_group($id) {
     access::verify_csrf();
+
     $group = ORM::factory("group", $id);
     if (!$group->loaded) {
       kohana::show_404();
@@ -240,6 +245,7 @@ class Admin_Users_Controller extends Controller {
 
   public function edit_group($id) {
     access::verify_csrf();
+
     $group = ORM::factory("group", $id);
     if (!$group->loaded) {
       kohana::show_404();

modules/user/controllers/login.php

@@ -26,6 +26,8 @@ class Login_Controller extends Controller {
   }
 
   public function auth_ajax() {
+    access::verify_csrf();
+
     list ($valid, $form) = $this->_auth("login/auth_ajax");
     if ($valid) {
       print json_encode(
@@ -42,6 +44,8 @@ class Login_Controller extends Controller {
   }
 
   public function auth_html() {
+    access::verify_csrf();
+
     list ($valid, $form) = $this->_auth("login/auth_html");
     if ($valid) {
       url::redirect("albums/1");

modules/user/controllers/logout.php

@@ -19,6 +19,8 @@
  */
 class Logout_Controller extends Controller {
   public function index() {
+    access::verify_csrf();
+
     $user = user::active();
     user::logout();
     log::info("user", t("User %name logged out", array("name" => $user->name)),

modules/user/controllers/password.php

@@ -19,6 +19,8 @@
  */
 class Password_Controller extends Controller {
   public function reset() {
+    access::verify_csrf();
+
     if (request::method() == "post") {
       $this->_send_reset();
     } else {
@@ -27,6 +29,8 @@ class Password_Controller extends Controller {
   }
 
   public function do_reset() {
+    access::verify_csrf();
+
     if (request::method() == "post") {
       $this->_change_password();
     } else {

modules/user/views/login.html.php

@@ -12,7 +12,7 @@
                 '" title="' . t("Edit Your Profile") .
                 '" id="gUserProfileLink" class="gDialogLink">' .
                 p::clean(empty($user->full_name) ? $user->name : $user->full_name) . '</a>')) ?></li>
-  <li><a href="<?= url::site("logout?continue=" . url::current(true)) ?>"
+  <li><a href="<?= url::site("logout?csrf=$csrf&continue=" . url::current(true)) ?>"
       id="gLogoutLink"><?= t("Logout") ?></a></li>
   <? endif; ?>
 </ul>

modules/watermark/controllers/admin_watermarks.php

@@ -38,6 +38,8 @@ class Admin_Watermarks_Controller extends Admin_Controller {
   }
 
   public function edit() {
+    access::verify_csrf();
+
     $form = watermark::get_edit_form();
     if ($form->validate()) {
       module::set_var("watermark", "position", $form->edit_watermark->position->value);
@@ -61,6 +63,8 @@ class Admin_Watermarks_Controller extends Admin_Controller {
   }
 
   public function delete() {
+    access::verify_csrf();
+
     $form = watermark::get_delete_form();
     if ($form->validate()) {
       if ($name = module::get_var("watermark", "name")) {
@@ -91,6 +95,8 @@ class Admin_Watermarks_Controller extends Admin_Controller {
   }
 
   public function add() {
+    access::verify_csrf();
+
     $form = watermark::get_add_form();
     if ($form->validate()) {
       $file = $_POST["file"];