From e28fcd8b03127a8ddc91245d143c3d2689189e06 Mon Sep 17 00:00:00 2001
From: Aaron van Geffen <aaron@aaronweb.net>
Date: Sat, 11 Nov 2023 15:29:32 +0100
Subject: [PATCH] Move photo deletion from ViewPhoto to EditAsset

Removes the intermediate confirmation page, instead using JavaScript for confirmation.

Fixes an XSS issue, in that the previous method was not passing or checking the session (!)
---
 controllers/EditAsset.php   | 10 ++++++++--
 controllers/ViewPhoto.php   | 38 +++++--------------------------------
 models/Asset.php            | 10 ++++++++++
 templates/EditAssetForm.php |  5 ++++-
 templates/PhotoPage.php     | 16 ++++++++++------
 5 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/controllers/EditAsset.php b/controllers/EditAsset.php
index 98e22690..a057529c 100644
--- a/controllers/EditAsset.php
+++ b/controllers/EditAsset.php
@@ -22,8 +22,14 @@ class EditAsset extends HTMLController
 		if (!($user->isAdmin() || $asset->isOwnedBy($user)))
 			throw new NotAllowedException();
 
-		if (isset($_REQUEST['delete']))
-			throw new Exception('Not implemented.');
+		if (isset($_REQUEST['delete']) && Session::validateSession('get'))
+		{
+			$redirectUrl = BASEURL . '/' . $asset->getSubdir();
+			$asset->delete();
+
+			header('Location: ' . $redirectUrl);
+			exit;
+		}
 
 		if (!empty($_POST))
 		{
diff --git a/controllers/ViewPhoto.php b/controllers/ViewPhoto.php
index 5e29ea8d..81b909a7 100644
--- a/controllers/ViewPhoto.php
+++ b/controllers/ViewPhoto.php
@@ -21,39 +21,14 @@ class ViewPhoto extends HTMLController
 
 		parent::__construct($photo->getTitle() . ' - ' . SITE_TITLE);
 
-		$author = $photo->getAuthor();
-
-		if (isset($_REQUEST['confirm_delete']) || isset($_REQUEST['delete_confirmed']))
-			$this->handleConfirmDelete($user, $author, $photo);
-		else
-			$this->handleViewPhoto($user, $author, $photo);
-	}
-
-	private function handleConfirmDelete(User $user, User $author, Asset $photo)
-	{
-		if (!($user->isAdmin() || $user->getUserId() === $author->getUserId()))
-			throw new NotAllowedException();
-
-		if (isset($_REQUEST['confirm_delete']))
-		{
-			$page = new ConfirmDeletePage($photo->getImage());
-			$this->page->adopt($page);
-		}
-		elseif (isset($_REQUEST['delete_confirmed']))
-		{
-			$album_url = $photo->getSubdir();
-			$photo->delete();
-
-			header('Location: ' . BASEURL . '/' . $album_url);
-			exit;
-		}
-	}
-
-	private function handleViewPhoto(User $user, User $author, Asset $photo)
-	{
 		if (!empty($_POST))
 			$this->handleTagging($photo->getImage());
+		else
+			$this->handleViewPhoto($photo);
+	}
 
+	private function handleViewPhoto(Asset $photo)
+	{
 		$page = new PhotoPage($photo->getImage());
 
 		// Exif data?
@@ -75,9 +50,6 @@ class ViewPhoto extends HTMLController
 		if ($next_url)
 			$page->setNextPhotoUrl($next_url);
 
-		if ($user->isAdmin() || $user->getUserId() === $author->getUserId())
-			$page->setIsAssetOwner(true);
-
 		$this->page->adopt($page);
 		$this->page->setCanonicalUrl($photo->getPageUrl());
 	}
diff --git a/models/Asset.php b/models/Asset.php
index 4a799f7e..31041b38 100644
--- a/models/Asset.php
+++ b/models/Asset.php
@@ -289,6 +289,16 @@ class Asset
 		return $this->date_captured;
 	}
 
+	public function getDeleteUrl()
+	{
+		return BASEURL . '/editasset/?id=' . $this->id_asset . '&delete';
+	}
+
+	public function getEditUrl()
+	{
+		return BASEURL . '/editasset/?id=' . $this->id_asset;
+	}
+
 	public function getFilename()
 	{
 		return $this->filename;
diff --git a/templates/EditAssetForm.php b/templates/EditAssetForm.php
index d791dcd1..70288426 100644
--- a/templates/EditAssetForm.php
+++ b/templates/EditAssetForm.php
@@ -23,7 +23,10 @@ class EditAssetForm extends Template
 			<form id="asset_form" action="" method="post" enctype="multipart/form-data">
 				<div class="content-box">
 					<div class="float-end">
-						<a class="btn btn-danger" href="', BASEURL, '/', $this->asset->getSlug(), '?delete_confirmed">Delete asset</a>
+						<a class="btn btn-danger" href="', $this->asset->getDeleteUrl(), '&',
+							Session::getSessionTokenKey(), '=', Session::getSessionToken(),
+						'" onclick="return confirm(\'Are you sure you want to delete this asset?\');">',
+						'Delete asset</a>
 						<button class="btn btn-primary" type="submit">Save asset data</button>
 					</div>
 					<h2>Edit asset \'', $this->asset->getTitle(), '\' (', $this->asset->getFilename(), ')</h2>
diff --git a/templates/PhotoPage.php b/templates/PhotoPage.php
index a4bcbb2f..bd9d6582 100644
--- a/templates/PhotoPage.php
+++ b/templates/PhotoPage.php
@@ -54,9 +54,7 @@ class PhotoPage extends Template
 					<div class="col-lg-4">';
 
 		$this->photoMeta();
-
-		if ($this->is_asset_owner)
-			$this->addUserActions();
+		$this->userActions();
 
 		echo '
 					</div>
@@ -259,13 +257,19 @@ class PhotoPage extends Template
 		$this->exif = $exif;
 	}
 
-	public function addUserActions()
+	public function userActions()
 	{
+		if (!$this->photo->isOwnedBy(Registry::get('user')))
+			return;
+
 		echo '
 				<div id="user_actions_box" class="content-box">
 					<h3>Actions</h3>
-					<a class="btn btn-primary" href="', BASEURL, '/editasset/?id=', $this->photo->getId(), '">Edit photo</a>
-					<a class="btn btn-danger" href="', BASEURL, '/', $this->photo->getSlug(), '/?confirm_delete">Delete photo</a>
+					<a class="btn btn-primary" href="', $this->photo->getEditUrl(), '">Edit photo</a>
+					<a class="btn btn-danger" href="', $this->photo->getDeleteUrl(), '&',
+						Session::getSessionTokenKey(), '=', Session::getSessionToken(),
+						'" onclick="return confirm(\'Are you sure you want to delete this photo?\');"',
+						'">Delete photo</a>
 				</div>';
 	}
 }