From 9a07c351c05bf8bb49eb0394a221ef224d04a8ac Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Fri, 12 Dec 2025 11:04:26 -0500 Subject: [PATCH 1/2] copilot 1st draft remove form helpers remove constants remove magic array_key_exists WIP single use tokens remove normal token in favor of single use tokens rename function better logging fixup require refactor add inputs to all forms validate in all pages remove file remove from init remove from ajax fixup tests init csrf_tokens array unique sessions for test cases automatically generate csrf tokens for http_post in testing add missing imports don't overwrite tokens validate in POST in header.php remove bad test set csrf_tokens array even if unauthenticated add session cleanup write session data to filesystem immediately on cleanup rename config option clear should leave an empty array must session_start before accessing $_SESSION change test dont validate twice in header.php update test case to check for double verification --- defaults/config.ini.default | 1 + resources/autoload.php | 1 + resources/init.php | 16 ++++- resources/lib/CSRFToken.php | 67 +++++++++++++++++++ resources/lib/UnityHTTPD.php | 14 ++++ resources/templates/header.php | 4 ++ test/functional/ViewAsUserTest.php | 2 +- test/phpunit-bootstrap.php | 7 +- test/unit/CSRFTokenTest.php | 83 ++++++++++++++++++++++++ webroot/admin/ajax/get_group_members.php | 4 ++ webroot/admin/content.php | 2 + webroot/admin/notices.php | 8 ++- webroot/admin/pi-mgmt.php | 3 + webroot/admin/user-mgmt.php | 3 + webroot/panel/account.php | 27 +++++--- webroot/panel/groups.php | 5 ++ webroot/panel/modal/new_key.php | 2 + webroot/panel/modal/new_pi.php | 2 + webroot/panel/new_account.php | 2 + webroot/panel/pi.php | 5 ++ 20 files changed, 243 insertions(+), 15 deletions(-) create mode 100644 resources/lib/CSRFToken.php create mode 100644 test/unit/CSRFTokenTest.php diff --git a/defaults/config.ini.default b/defaults/config.ini.default index a505140f..11d6140b 100644 --- a/defaults/config.ini.default +++ b/defaults/config.ini.default @@ -18,6 +18,7 @@ enable_verbose_error_log = true ; internal use only enable_redirect_message = true ; internal use only enable_exception_handler = true ; internal use only enable_error_handler = true ; internal use only +session_cleanup_idle_seconds = 1800 ; how long a session must be idle before messages and CSRF tokens are cleared [ldap] uri = "ldap://identity" ; URI of remote LDAP server diff --git a/resources/autoload.php b/resources/autoload.php index 158db4bc..8affc966 100644 --- a/resources/autoload.php +++ b/resources/autoload.php @@ -24,6 +24,7 @@ require_once __DIR__ . "/lib/UnityWebhook.php"; require_once __DIR__ . "/lib/UnityGithub.php"; require_once __DIR__ . "/lib/utils.php"; +require_once __DIR__ . "/lib/CSRFToken.php"; require_once __DIR__ . "/lib/exceptions/NoDieException.php"; require_once __DIR__ . "/lib/exceptions/SSOException.php"; require_once __DIR__ . "/lib/exceptions/ArrayKeyException.php"; diff --git a/resources/init.php b/resources/init.php index f23f2c2d..b4c2f20d 100644 --- a/resources/init.php +++ b/resources/init.php @@ -21,8 +21,6 @@ set_error_handler(["UnityWebPortal\lib\UnityHTTPD", "errorHandler"]); } -session_start(); - if (isset($GLOBALS["ldapconn"])) { $LDAP = $GLOBALS["ldapconn"]; } else { @@ -34,10 +32,24 @@ $WEBHOOK = new UnityWebhook(); $GITHUB = new UnityGithub(); +session_start(); +// https://stackoverflow.com/a/1270960/18696276 +if (time() - ($_SESSION["LAST_ACTIVITY"] ?? 0) > CONFIG["site"]["session_cleanup_idle_seconds"]) { + $_SESSION["csrf_tokens"] = []; + $_SESSION["messages"] = []; + session_write_close(); + session_start(); +} +$_SESSION["LAST_ACTIVITY"] = time(); + if (!array_key_exists("messages", $_SESSION)) { $_SESSION["messages"] = []; } +if (!array_key_exists("csrf_tokens", $_SESSION)) { + $_SESSION["csrf_tokens"] = []; +} + if (isset($_SERVER["REMOTE_USER"])) { // Check if SSO is enabled on this page $SSO = UnitySSO::getSSO(); diff --git a/resources/lib/CSRFToken.php b/resources/lib/CSRFToken.php new file mode 100644 index 00000000..730539fb --- /dev/null +++ b/resources/lib/CSRFToken.php @@ -0,0 +1,67 @@ + $_SESSION], + ); + $_SESSION["csrf_tokens"] = []; + } + if (!is_array($_SESSION["csrf_tokens"])) { + UnityHTTPD::errorLog( + "invalid session", + '$_SESSION["csrf_tokens"] is not an array', + data: ['$_SESSION' => $_SESSION], + ); + $_SESSION["csrf_tokens"] = []; + } + } + + public static function generate(): string + { + self::ensureSessionCSRFTokensSanity(); + $token = bin2hex(random_bytes(32)); + $_SESSION["csrf_tokens"][$token] = false; + return $token; + } + + public static function validate(string $token): bool + { + self::ensureSessionCSRFTokensSanity(); + if ($token === "") { + UnityHTTPD::errorLog("empty CSRF token", ""); + return false; + } + if (!array_key_exists($token, $_SESSION["csrf_tokens"])) { + UnityHTTPD::errorLog("unknown CSRF token", $token); + return false; + } + $entry = $_SESSION["csrf_tokens"][$token]; + if ($entry === true) { + UnityHTTPD::errorLog("reused CSRF token", $token); + return false; + } + $_SESSION["csrf_tokens"][$token] = true; + return true; + } + + public static function clear(): void + { + if (!isset($_SESSION)) { + return; + } + if (array_key_exists("csrf_tokens", $_SESSION)) { + unset($_SESSION["csrf_tokens"]); + } + $_SESSION["csrf_tokens"] = []; + } +} diff --git a/resources/lib/UnityHTTPD.php b/resources/lib/UnityHTTPD.php index 016bedda..7952a176 100644 --- a/resources/lib/UnityHTTPD.php +++ b/resources/lib/UnityHTTPD.php @@ -390,4 +390,18 @@ public static function deleteMessage(UnityHTTPDMessageLevel $level, string $titl unset($_SESSION["messages"][$index]); $_SESSION["messages"] = array_values($_SESSION["messages"]); } + + public static function validatePostCSRFToken(): void + { + $token = self::getPostData("csrf_token"); + if (!CSRFToken::validate($token)) { + self::badRequest("CSRF token validation failed", data: ["token" => $token]); + } + } + + public static function getCSRFTokenHiddenFormInput(): string + { + $token = htmlspecialchars(CSRFToken::generate()); + return ""; + } } diff --git a/resources/templates/header.php b/resources/templates/header.php index bc055b1f..3ab4d742 100644 --- a/resources/templates/header.php +++ b/resources/templates/header.php @@ -3,6 +3,8 @@ use UnityWebPortal\lib\UnityHTTPD; if ($_SERVER["REQUEST_METHOD"] == "POST") { + // another page should have already validated and we can't validate the same token twice + // UnityHTTPD::validatePostCSRFToken(); if ( ($_SESSION["is_admin"] ?? false) == true && ($_POST["form_type"] ?? null) == "clearView" @@ -179,10 +181,12 @@ && isset($_SESSION["viewUser"]) ) { $viewUser = $_SESSION["viewUser"]; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo "
You are accessing the web portal as the user $viewUser
+ $CSRFTokenHiddenFormInput diff --git a/test/functional/ViewAsUserTest.php b/test/functional/ViewAsUserTest.php index 99383c1c..43b1caad 100644 --- a/test/functional/ViewAsUserTest.php +++ b/test/functional/ViewAsUserTest.php @@ -25,7 +25,7 @@ public function _testViewAsUser(array $beforeUser, array $afterUser) // now we should be new user $this->assertEquals($afterUid, $USER->uid); // $this->assertTrue($_SESSION["user_exists"]); - http_post(__DIR__ . "/../../resources/templates/header.php", [ + http_post(__DIR__ . "/../../webroot/panel/account.php", [ "form_type" => "clearView", ]); $this->assertArrayNotHasKey("viewUser", $_SESSION); diff --git a/test/phpunit-bootstrap.php b/test/phpunit-bootstrap.php index c613602f..306046bd 100644 --- a/test/phpunit-bootstrap.php +++ b/test/phpunit-bootstrap.php @@ -15,6 +15,7 @@ require_once __DIR__ . "/../resources/lib/UnityWebhook.php"; require_once __DIR__ . "/../resources/lib/UnityGithub.php"; require_once __DIR__ . "/../resources/lib/utils.php"; +require_once __DIR__ . "/../resources/lib/CSRFToken.php"; require_once __DIR__ . "/../resources/lib/exceptions/NoDieException.php"; require_once __DIR__ . "/../resources/lib/exceptions/SSOException.php"; require_once __DIR__ . "/../resources/lib/exceptions/ArrayKeyException.php"; @@ -25,6 +26,7 @@ require_once __DIR__ . "/../resources/lib/exceptions/EncodingConversionException.php"; require_once __DIR__ . "/../resources/lib/exceptions/UnityHTTPDMessageNotFoundException.php"; +use UnityWebPortal\lib\CSRFToken; use UnityWebPortal\lib\UnityGroup; use UnityWebPortal\lib\UnityHTTPD; use UnityWebPortal\lib\UnitySQL; @@ -97,7 +99,7 @@ function switchUser( ensure(!is_null($USER)); } -function http_post(string $phpfile, array $post_data, bool $enforce_PRG = true): void +function http_post(string $phpfile, array $post_data, bool $enforce_PRG = true, bool $do_generate_csrf_token = true): void { global $LDAP, $SQL, @@ -115,6 +117,9 @@ function http_post(string $phpfile, array $post_data, bool $enforce_PRG = true): $_SERVER["REQUEST_METHOD"] = "POST"; $_SERVER["PHP_SELF"] = preg_replace("/.*webroot\//", "/", $phpfile); $_SERVER["REQUEST_URI"] = preg_replace("/.*webroot\//", "/", $phpfile); // Slightly imprecise because it doesn't include get parameters + if (!array_key_exists("csrf_token", $post_data) && $do_generate_csrf_token) { + $post_data["csrf_token"] = CSRFToken::generate(); + } $_POST = $post_data; ob_start(); $post_did_redirect_or_die = false; diff --git a/test/unit/CSRFTokenTest.php b/test/unit/CSRFTokenTest.php new file mode 100644 index 00000000..1b1efd1f --- /dev/null +++ b/test/unit/CSRFTokenTest.php @@ -0,0 +1,83 @@ +assertIsString($token); + $this->assertEquals(64, strlen($token)); + $this->assertMatchesRegularExpression('/^[0-9a-f]{64}$/', $token); + } + + public function testGenerateStoresTokenInSession(): void + { + $token = CSRFToken::generate(); + $this->assertArrayHasKey("csrf_tokens", $_SESSION); + $this->assertArrayHasKey($token, $_SESSION["csrf_tokens"]); + $this->assertFalse($_SESSION["csrf_tokens"][$token]); + } + + public function testValidateWithValidToken(): void + { + $token = CSRFToken::generate(); + $this->assertTrue(CSRFToken::validate($token)); + $this->assertTrue($_SESSION["csrf_tokens"][$token]); + } + + public function testValidateWithInvalidToken(): void + { + CSRFToken::generate(); + $this->assertFalse(CSRFToken::validate("invalid_token")); + } + + public function testValidateWithEmptyToken(): void + { + CSRFToken::generate(); + $this->assertFalse(CSRFToken::validate("")); + } + + public function testValidateWithoutSessionToken(): void + { + $this->assertFalse(CSRFToken::validate("any_token")); + } + + public function testClearRemovesToken(): void + { + CSRFToken::generate(); + $this->assertNotEmpty($_SESSION["csrf_tokens"]); + CSRFToken::clear(); + $this->assertEmpty($_SESSION["csrf_tokens"]); + } + + public function testMultipleTokenGenerations(): void + { + $token1 = CSRFToken::generate(); + $token2 = CSRFToken::generate(); + $this->assertNotEquals($token1, $token2); + } + + public function testTokenIsSingleUse(): void + { + $token = CSRFToken::generate(); + $this->assertTrue(CSRFToken::validate($token)); + $this->assertFalse(CSRFToken::validate($token)); + $this->assertTrue($_SESSION["csrf_tokens"][$token]); + } +} diff --git a/webroot/admin/ajax/get_group_members.php b/webroot/admin/ajax/get_group_members.php index 7bd4c618..e9076043 100644 --- a/webroot/admin/ajax/get_group_members.php +++ b/webroot/admin/ajax/get_group_members.php @@ -31,6 +31,7 @@ echo "$uid"; echo "$mail"; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput @@ -62,9 +64,11 @@ echo "$user->uid"; echo "$email"; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput diff --git a/webroot/admin/content.php b/webroot/admin/content.php index 620a8fac..0336e380 100644 --- a/webroot/admin/content.php +++ b/webroot/admin/content.php @@ -9,6 +9,7 @@ } if ($_SERVER["REQUEST_METHOD"] == "POST") { + UnityHTTPD::validatePostCSRFToken(); if (!empty($_POST["pageSel"])) { $SQL->editPage($_POST["pageSel"], $_POST["content"], $USER); } @@ -21,6 +22,7 @@
+ @@ -62,8 +64,10 @@ echo "" . date('Y-m-d', strtotime($notice["date"])) . ""; echo "
" . $notice["message"] . "
"; echo ""; - echo - " + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); + echo " + + $CSRFTokenHiddenFormInput diff --git a/webroot/admin/pi-mgmt.php b/webroot/admin/pi-mgmt.php index 7b161a4b..b4e82554 100644 --- a/webroot/admin/pi-mgmt.php +++ b/webroot/admin/pi-mgmt.php @@ -12,6 +12,7 @@ } if ($_SERVER["REQUEST_METHOD"] == "POST") { + UnityHTTPD::validatePostCSRFToken(); if (isset($_POST["uid"])) { $form_user = new UnityUser($_POST["uid"], $LDAP, $SQL, $MAILER, $WEBHOOK); } @@ -76,8 +77,10 @@ echo "$email"; echo "" . date("jS F, Y", strtotime($request['timestamp'])) . ""; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput "; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput diff --git a/webroot/panel/account.php b/webroot/panel/account.php index 8c2cf1a2..ee1b8246 100644 --- a/webroot/panel/account.php +++ b/webroot/panel/account.php @@ -10,6 +10,7 @@ $hasGroups = count($USER->getPIGroupGIDs()) > 0; if ($_SERVER['REQUEST_METHOD'] == "POST") { + UnityHTTPD::validatePostCSRFToken(); switch (UnityHTTPD::getPostData("form_type")) { case "addKey": $keys = array(); @@ -165,6 +166,7 @@ id='piReq' > "; + echo UnityHTTPD::getCSRFTokenHiddenFormInput(); if ($SQL->accDeletionRequestExists($USER->uid)) { echo ""; echo " @@ -214,6 +216,7 @@ } for ($i = 0; $sshPubKeys != null && $i < count($sshPubKeys); $i++) { + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo "
@@ -222,6 +225,7 @@ onsubmit='return confirm(\"Are you sure you want to delete this SSH key?\");' method='POST' > + $CSRFTokenHiddenFormInput @@ -229,21 +233,23 @@
"; } -echo ' - +$CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); +echo " +
Login Shell
- - - + -
- + +
+
Account Deletion
@@ -252,6 +258,7 @@ if ($hasGroups) { echo "

You cannot request to delete your account while you are in a PI group.

"; } else { + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); if ($SQL->accDeletionRequestExists($USER->uid)) { echo "

Your request has been submitted and is currently pending.

@@ -264,6 +271,7 @@ ) ' > + $CSRFTokenHiddenFormInput @@ -275,6 +283,7 @@ method='POST' onsubmit='return confirm(\"Are you sure you want to request an account deletion?\")' > + $CSRFTokenHiddenFormInput diff --git a/webroot/panel/groups.php b/webroot/panel/groups.php index a04030d1..e4359e01 100644 --- a/webroot/panel/groups.php +++ b/webroot/panel/groups.php @@ -7,6 +7,7 @@ use UnityWebPortal\lib\UnityHTTPD; if ($_SERVER["REQUEST_METHOD"] == "POST") { + UnityHTTPD::validatePostCSRFToken(); if (isset($_POST["form_type"])) { if (isset($_POST["pi"])) { $pi_groupname = $_POST["pi"]; @@ -102,7 +103,9 @@ echo "$mail"; echo "" . date("jS F, Y", strtotime($request['timestamp'])) . ""; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo "
+ $CSRFTokenHiddenFormInput @@ -148,10 +151,12 @@ echo "$full_name"; echo "" . $group->gid . ""; echo "" . $owner->getMail() . ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput diff --git a/webroot/panel/modal/new_key.php b/webroot/panel/modal/new_key.php index 55d0df12..0421aeb3 100644 --- a/webroot/panel/modal/new_key.php +++ b/webroot/panel/modal/new_key.php @@ -1,10 +1,12 @@ /panel/account.php"> +
diff --git a/webroot/panel/modal/new_pi.php b/webroot/panel/modal/new_pi.php index 14d486f6..d48277a2 100644 --- a/webroot/panel/modal/new_pi.php +++ b/webroot/panel/modal/new_pi.php @@ -1,6 +1,7 @@ /panel/groups.php" > +
diff --git a/webroot/panel/new_account.php b/webroot/panel/new_account.php index c1dc05c8..e3c13799 100644 --- a/webroot/panel/new_account.php +++ b/webroot/panel/new_account.php @@ -9,6 +9,7 @@ UnityHTTPD::redirect(CONFIG["site"]["prefix"] . "/panel/account.php"); } if ($_SERVER["REQUEST_METHOD"] == "POST") { + UnityHTTPD::validatePostCSRFToken(); $user = new UnityUser($SSO["user"], $LDAP, $SQL, $MAILER, $WEBHOOK); $user->init($SSO["firstname"], $SSO["lastname"], $SSO["mail"], $SSO["org"]); // header.php will redirect to this same page again and then this page will redirect to account @@ -29,6 +30,7 @@

Your unity cluster username will be


+ $email"; echo "$date"; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo "
+ $CSRFTokenHiddenFormInput "; echo ""; + $CSRFTokenHiddenFormInput = UnityHTTPD::getCSRFTokenHiddenFormInput(); echo " + $CSRFTokenHiddenFormInput Date: Mon, 15 Dec 2025 11:27:48 -0500 Subject: [PATCH 2/2] prettier --- test/phpunit-bootstrap.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/phpunit-bootstrap.php b/test/phpunit-bootstrap.php index 306046bd..4fecb4b0 100644 --- a/test/phpunit-bootstrap.php +++ b/test/phpunit-bootstrap.php @@ -99,8 +99,12 @@ function switchUser( ensure(!is_null($USER)); } -function http_post(string $phpfile, array $post_data, bool $enforce_PRG = true, bool $do_generate_csrf_token = true): void -{ +function http_post( + string $phpfile, + array $post_data, + bool $enforce_PRG = true, + bool $do_generate_csrf_token = true, +): void { global $LDAP, $SQL, $MAILER,