Fix missing CSRF checks in admin/account_edit

Zotlabs/Module/Admin/Account_edit.php

@@ -8,6 +8,11 @@ class Account_edit {
 
 	function post() {
 
+		// Validate CSRF token
+		//
+		// We terminate with a 403 Forbidden status if the check fails.
+		check_form_security_token_ForbiddenOnErr('admin_account_edit', 'security');
+
 		$account_id = $_REQUEST['aid'];
 
 		if(! $account_id)
@@ -18,7 +23,7 @@ class Account_edit {
 		if($pass1 && $pass2 && ($pass1 === $pass2)) {
 			$salt = random_string(32);
 			$password_encoded = hash('whirlpool', $salt . $pass1);
-			$r = q("update account set account_salt = '%s', account_password = '%s', 
+			$r = q("update account set account_salt = '%s', account_password = '%s',
 				account_password_changed = '%s' where account_id = %d",
 				dbesc($salt),
 				dbesc($password_encoded),
@@ -34,7 +39,7 @@ class Account_edit {
 		$account_level = 5;
 		$account_language = trim($_REQUEST['account_language']);
 
-		$r = q("update account set account_service_class = '%s', account_level = %d, account_language = '%s' 
+		$r = q("update account set account_service_class = '%s', account_level = %d, account_language = '%s'
 			where account_id = %d",
 			dbesc($service_class),
 			intval($account_level),
@@ -62,8 +67,8 @@ class Account_edit {
 			return '';
 		}
 
-
 		$a = replace_macros(get_markup_template('admin_account_edit.tpl'), [
+			'$security' => get_form_security_token('admin_account_edit'),
 			'$account' => $x[0],
 			'$title' => t('Account Edit'),
 			'$pass1' => [ 'pass1', t('New Password'), ' ','' ],

tests/unit/Module/AdminAccountEditTest.php

@@ -0,0 +1,214 @@
+<?php
+/* Tests for Account_edit module
+ *
+ * SPDX-FileCopyrightText: 2024 Hubzilla Community
+ * SPDX-FileContributor: Harald Eilertsen
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+namespace Zotlabs\Tests\Unit\Module;
+
+use DateTimeImmutable;
+use PHPUnit\Framework\Attributes\{Before, After};
+use Zotlabs\Model\Account;
+
+class AdminAccountEditTest extends TestCase {
+
+	#[Before]
+	public function setup_mocks(): void {
+		/*
+		 * As we're testing pages that should only be reachable by the
+		 * site admin, it makes no sense to have it return anything else
+		 * than true.
+		 */
+		$this->stub_is_site_admin =
+			$this->getFunctionMock('Zotlabs\Module', 'is_site_admin')
+				->expects($this->once())
+				->willReturn(true);
+
+		$this->info = [];
+		$this->stub_info =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'info')
+				->expects($this->any())
+				->willReturnCallback(function (string $arg) {
+					$this->info[] = $arg;
+				});
+
+		$this->notice = [];
+		$this->stub_notice =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'notice')
+				->expects($this->any())
+				->willReturnCallback(function (string $arg) {
+					$this->notice[] = $arg;
+				});
+
+	}
+
+	#[After]
+	public function tear_down_mocks(): void {
+		$this->stub_is_site_admin = null;
+		$this->stub_info = null;
+		$this->stub_notice = null;
+		$this->stub_check_security = null;
+		$this->stub_get_form_security_token = null;
+	}
+
+	public function test_rendering_admin_account_edit_page(): void {
+		$this->stub_get_form_security_token =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'get_form_security_token')
+				->expects($this->once())
+				->willReturn('the-csrf-token');
+
+		$account = $this->fixtures['account'][0];
+
+		$this->get("admin/account_edit/{$account['account_id']}");
+
+		$this->assertPageContains("<form action=\"admin/account_edit/{$account['account_id']}\" method=\"post\"");
+		$this->assertPageContains($account['account_email']);
+
+		// Check that we generate a CSRF token for the form
+		$this->assertPageContains("<input type=\"hidden\" name=\"security\" value=\"the-csrf-token\"");
+	}
+
+	public function test_rendering_admin_account_edit_page_fails_if_id_is_not_found(): void {
+		$this->get("admin/account_edit/666");
+
+		$this->assertEquals('', \App::$page['content']);
+	}
+
+	public function test_rendering_admin_account_edit_page_fails_if_id_is_not_numeric(): void {
+		$this->get("admin/account_edit/66invalid");
+
+		$this->assertEquals('', \App::$page['content']);
+	}
+
+	public function test_post_empty_form_does_not_modify_account(): void {
+		$this->stub_goaway();
+		$this->stub_check_form_security(true);
+
+		$account = get_account_by_id($this->fixtures['account'][0]['account_id']);
+
+		try {
+			$this->post(
+				"admin/account_edit/{$account['account_id']}",
+				[],
+				[
+					'aid' => $account['account_id'],
+					'pass1' => '',
+					'pass2' => '',
+					'service_class' => $account['account_service_class'],
+					'account_language' => $account['account_language'],
+					'security' => 'The security token',
+				]
+			);
+		} catch (RedirectException $ex) {
+			$this->assertEquals(z_root() . '/admin/accounts', $ex->getMessage());
+		}
+
+		$reloaded = get_account_by_id($account['account_id']);
+
+		$this->assertEquals($account, $reloaded);
+
+		// Not sure if this is expected behaviour, but this is how it is today.
+		$this->assertContains('Account settings updated.' . EOL, $this->info);
+	}
+
+	public function test_post_form_changes_account(): void {
+		$this->stub_goaway();
+		$this->stub_check_form_security(true);
+
+		// clone account from fixture, to ensure it's not replaced with
+		// the reloaded one below.
+		$account = get_account_by_id($this->fixtures['account'][0]['account_id']);
+
+		try {
+			$this->post(
+				"admin/account_edit/{$account['account_id']}",
+				[],
+				[
+					'aid' => $account['account_id'],
+					'pass1' => 'hunter2',
+					'pass2' => 'hunter2',
+					'service_class' => 'Some other class',
+					'account_language' => 'nn',
+					'security' => 'The security token',
+				]
+			);
+		} catch (RedirectException $ex) {
+			$this->assertEquals(z_root() . '/admin/accounts', $ex->getMessage());
+		}
+
+		$reloaded = get_account_by_id($account['account_id']);
+
+		$this->assertNotEquals($account, $reloaded);
+		$this->assertEquals('Some other class', $reloaded['account_service_class']);
+		$this->assertEquals('nn', $reloaded['account_language']);
+
+		$now = new DateTimeImmutable('now');
+		$this->assertEquals($now->format('Y-m-d H:i:s'), $reloaded['account_password_changed']);
+
+		$this->assertContains('Account settings updated.' . EOL, $this->info);
+		$this->assertContains("Password changed for account {$account['account_id']}." . EOL, $this->info);
+	}
+
+	public function test_form_with_missing_or_incalid_csrf_token_is_rejected(): void {
+		$this->expectException(KillmeException::class);
+
+		// Emulate a failed CSRF check
+		$this->stub_check_form_security(false);
+
+		$account_id = $this->fixtures['account'][0]['account_id'];
+
+		$this->post(
+			"admin/account_edit/{$account_id}",
+			[],
+			[
+				'aid' => $account_id,
+				'pass1' => 'hunter2',
+				'pass2' => 'hunter2',
+				'service_class' => 'Some other class',
+				'account_language' => 'nn',
+				'security' => 'Invalid security token',
+			]
+		);
+	}
+
+	/*
+	 * Override the stub_goaway method because we need the stub to live in the
+	 * Admin namespace.
+	 */
+	protected function stub_goaway(): void {
+		$this->goaway_stub = $this->getFunctionMock('Zotlabs\Module\Admin', 'goaway')
+			->expects($this->once())
+			->willReturnCallback(
+				function (string $uri) {
+					throw new RedirectException($uri);
+				}
+			);
+	}
+
+	/**
+	 * Stub the check_form_security_token_ForbiddenOnErr.
+	 *
+	 * In these tests we're not really interested in _how_ the form security
+	 * tokens work, but that the code under test perform the checks. This stub
+	 * allows us to do that without having to worry if everything is set up so
+	 * that the real function would work or not.
+	 *
+	 * @param bool $valid		true if emulating a valid token, false otherwise.
+	 */
+	protected function stub_check_form_security(bool $valid): void {
+		$this->stub_check_security =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'check_form_security_token_ForbiddenOnErr')
+				->expects($this->once())
+				->with(
+					$this->identicalTo('admin_account_edit'),
+					$this->identicalTo('security'))
+				->willReturnCallback(function () use ($valid) {
+					if (! $valid) {
+						throw new KillmeException();
+					}
+				});
+	}
+}

tests/unit/Module/TestCase.php

@@ -10,6 +10,7 @@
 
 namespace Zotlabs\Tests\Unit\Module;
 
+use PHPUnit\Framework\Attributes\After;
 use Zotlabs\Tests\Unit\UnitTestCase;
 use App;
 
@@ -25,6 +26,31 @@ class TestCase extends UnitTestCase {
 	// Import PHPMock methods into this class
 	use \phpmock\phpunit\PHPMock;
 
+	#[After]
+	public function cleanup_stubs(): void {
+		$this->killme_stub = null;
+		$this->goaway_stub = null;
+	}
+
+	protected function do_request(string $method, string $uri, array $query = [], array $params = []): void {
+		$_GET['q'] = $uri;
+		$_GET = array_merge($_GET, $query);
+		$_POST = $params;
+
+		$_SERVER['REQUEST_METHOD'] = $method;
+		$_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
+		$_SERVER['QUERY_STRING'] = "q={$uri}";
+		// phpcs:disable Generic.PHP.DisallowRequestSuperglobal.Found
+		$_REQUEST = array_merge($_GET, $_POST);
+		// phpcs::enable
+
+		\App::init();
+		\App::$page['content'] = '';
+
+		$router = new \Zotlabs\Web\Router();
+		$router->Dispatch();
+	}
+
 	/**
 	 * Emulate a GET request.
 	 *
@@ -34,24 +60,21 @@ class TestCase extends UnitTestCase {
 	 *						as keys.
 	 */
 	protected function get(string $uri, array $query = []): void {
-		$_GET['q'] = $uri;
+		$this->do_request('GET', $uri, $query);
+	}
 
-		if (!empty($query)) {
-			$_GET = array_merge($_GET, $query);
-		}
-
-		$_SERVER['REQUEST_METHOD'] = 'GET';
-		$_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
-		$_SERVER['QUERY_STRING'] = "q={$uri}";
-		// phpcs:disable Generic.PHP.DisallowRequestSuperglobal.Found
-		$_REQUEST = $_GET;
-		// phpcs::enable
-
-		\App::init();
-		\App::$page['content'] = '';
-
-		$router = new \Zotlabs\Web\Router();
-		$router->Dispatch();
+	/**
+	 * Emulate a POST request.
+	 *
+	 * @param string $uri	The URI to request. Typically this will be the module
+	 *						name, followed by any req args separated by slashes.
+	 * @param array $query	Associative array of query args, with the parameters
+	 *						as keys.
+	 * @param array $params	Associative array of POST params, with the param names
+	 *						as keys.
+	 */
+	protected function post(string $uri, array $query = [], array $params = []): void {
+		$this->do_request('POST', $uri, $query, $params);
 	}
 
 	/**
@@ -100,8 +123,7 @@ class TestCase extends UnitTestCase {
 	 * @throws KillmeException
 	 */
 	protected function stub_killme(): void {
-		$killme_stub = $this->getFunctionMock('Zotlabs\Module', 'killme');
-		$killme_stub
+		$this->killme_stub = $this->getFunctionMock('Zotlabs\Module', 'killme')
 			->expects($this->once())
 			->willReturnCallback(
 				function () {
@@ -147,8 +169,7 @@ class TestCase extends UnitTestCase {
 	 * @throws RedirectException
 	 */
 	protected function stub_goaway(): void {
-		$goaway_stub = $this->getFunctionMock('Zotlabs\Module', 'goaway');
-		$goaway_stub
+		$this->goaway_stub = $this->getFunctionMock('Zotlabs\Module', 'goaway')
 			->expects($this->once())
 			->willReturnCallback(
 				function (string $uri) {

tests/unit/includes/dba/_files/account.yml

@@ -3,9 +3,11 @@ account:
     account_id: 42
     account_email: "hubzilla@example.com"
     account_language: "no"
+    account_level: 5
     account_flags: 0
   -
     account_id: 43
     account_email: "hubzilla@example.org"
     account_language: "de"
+    account_level: 5
     account_flags: 1

view/tpl/admin_account_edit.tpl

@@ -5,6 +5,7 @@
 
 <form action="admin/account_edit/{{$account.account_id}}" method="post" >
 <input type="hidden" name="aid" value="{{$account.account_id}}" />
+<input type="hidden" name="security" value="{{$security}}">
 
 {{include file="field_password.tpl" field=$pass1}}
 {{include file="field_password.tpl" field=$pass2}}

view/tpl/field_input.tpl

@@ -1,6 +1,6 @@
 	<div id="id_{{$field.0}}_wrapper" class="mb-3">
 		<label for="id_{{$field.0}}" id="label_{{$field.0}}">
-			{{$field.1}}{{if $field.4}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}}
+			{{$field.1}}{{if isset($field.4)}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}}
 		</label>
 		<input
 			class="form-control"
@@ -8,7 +8,7 @@
 			id="id_{{$field.0}}"
 			type="text"
 			value="{{$field.2|escape:'html':'UTF-8':FALSE}}"
-			{{if $field.5}}{{$field.5}}{{/if}}
+			{{if isset($field.5)}}{{$field.5}}{{/if}}
 			>
 		<small id="help_{{$field.0}}" class="form-text text-muted">
 			{{$field.3}}

view/tpl/field_password.tpl

@@ -1,5 +1,5 @@
 	<div class="mb-3">
 		<label for="id_{{$field.0}}">{{$field.1}}</label>
-		<input class="form-control" type="password" name="{{$field.0}}" id="id_{{$field.0}}" value="{{$field.2}}"{{if $field.5}} {{$field.5}}{{/if}}>{{if $field.4}} <span class="required">{{$field.4}}</span> {{/if}}
+		<input class="form-control" type="password" name="{{$field.0}}" id="id_{{$field.0}}" value="{{$field.2}}"{{if isset($field.5)}} {{$field.5}}{{/if}}>{{if isset($field.4)}} <span class="required">{{$field.4}}</span> {{/if}}
 		<small id="help_{{$field.0}}" class="form-text text-muted">{{$field.3}}</small>
 	</div>

view/tpl/field_select.tpl

@@ -1,11 +1,11 @@
 	<div id="id_{{$field.0}}_wrapper" class="mb-3">
-		<label for="id_{{$field.0}}">{{$field.1}}{{if $field.5}}<sup class="required zuiqmid"> {{$field.5}}</sup>{{/if}}</label>
+		<label for="id_{{$field.0}}">{{$field.1}}{{if isset($field.5)}}<sup class="required zuiqmid"> {{$field.5}}</sup>{{/if}}</label>
 		<select class="form-control" name="{{$field.0}}" id="id_{{$field.0}}">
 			{{foreach $field.4 as $opt=>$val}}<option value="{{$opt}}" {{if $opt==$field.2}}selected="selected"{{/if}}>{{$val}}</option>{{/foreach}}
 		</select>
 		<small class="form-text text-muted">{{$field.3}}</small	>
 	</div>
-{{* 
+{{*
 	COMMENTS for this template:
 	@author hilmar runge, 2020.01
 	$field array index: