diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index afb722f3..97d65767 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -60,13 +60,25 @@ repos: language: system files: \.php$ args: [-l] - - id: assert-no-die-exit - name: Assert no die()/exit() - entry: ./test/assert-no-die-exit.bash + - id: assert-UnityHTTPD-used + name: Assert UnityHTTPD functions are used + entry: bash ./test/assert-UnityHTTPD-used.bash language: system files: \.php$ exclude: | (?x)^( - resources/lib/UnityHTTPD\.php$| + resources/lib/UnityHTTPD\.php| workers/.*| )$ + - id: assert-utils-used + name: Assert utils are used + entry: bash ./test/assert-utils-used.bash + language: system + files: \.php$ + exclude: ^resources/lib/utils\.php$ + - id: assert-exceptions-used + name: Assert exceptions are used + entry: bash ./test/assert-exceptions-used.bash + language: system + files: ^resources/.*\.php$ + exclude: ^resources/lib/UnityHTTPD\.php$ diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ea7c19ec..6f437e3a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,8 +7,20 @@ * The maximum line length for any PHP file is 100 characters, instead of PSR-12's 120 characters. * Comments should be used sparingly. * Empty lines should be used sparingly. -* No code should call `die()` or `exit()`, instead `UnityHTTPD::die()`. This will avoid the premature death of our automated testing processes. -* Instead of `assert`, use `\ensure`. This will enforce conditions even in production. +* No code should fail quietly, instead exceptions should be thrown. + PHP builtin functions that fail quietly (ex: `json_encode`) should be replaced with a wrapper in `resources/utils.php`. +* No code should call `die()` or `exit()`, instead `UnityHTTPD::die()`. + This will avoid the premature death of our automated testing processes. +* No code should call `assert()`, instead `\ensure()`. + This will enforce conditions even in production. +* No code should call `json_encode()`, instead `\jsonEncode()`. + This will throw errors and escape slashes by default. +* No code should call `mb_convert_encoding()`, instead `\mbConvertEncoding()`. + This will throw an exception rather than returning `false`. +* No code should call `mb_detect_encoding()`, instead `\mbDetectEncoding()`. + This will enable strict mode and throw an exception rather than returning `false`. +* `UnityHTTPD`'s user-facing error functionality (ex: `badRequest`) should only be called from `webroot/**/*.php`. + `resources/**/*.php` should throw exceptions instead. This repository will automatically check PRs for linting compliance. diff --git a/resources/autoload.php b/resources/autoload.php index a4326b05..6ec01691 100644 --- a/resources/autoload.php +++ b/resources/autoload.php @@ -29,6 +29,8 @@ require_once __DIR__ . "/lib/exceptions/SSOException.php"; require_once __DIR__ . "/lib/exceptions/ArrayKeyException.php"; require_once __DIR__ . "/lib/exceptions/EnsureException.php"; +require_once __DIR__ . "/lib/exceptions/EncodingUnknownException.php"; +require_once __DIR__ . "/lib/exceptions/EncodingConversionException.php"; require_once __DIR__ . "/config.php"; require __DIR__ . "/init.php"; diff --git a/resources/lib/UnityHTTPD.php b/resources/lib/UnityHTTPD.php index 42e826e5..542ce41d 100644 --- a/resources/lib/UnityHTTPD.php +++ b/resources/lib/UnityHTTPD.php @@ -58,9 +58,14 @@ public static function errorLog( $output["trace"] = explode("\n", (new \Exception())->getTraceAsString()); } if (!is_null($data)) { - $output["data"] = $data; + try { + \jsonEncode($data); + $output["data"] = $data; + } catch (\JsonException $e) { + $output["data"] = "data could not be JSON encoded: " . $e->getMessage(); + } } - error_log("$title: " . json_encode($output, JSON_UNESCAPED_SLASHES)); + error_log("$title: " . \jsonEncode($output)); } // recursive on $t->getPrevious() @@ -147,8 +152,11 @@ public static function getPostData(...$keys) } } - public static function getUploadedFileContents($filename, $do_delete_tmpfile_after_read = true) - { + public static function getUploadedFileContents( + $filename, + $do_delete_tmpfile_after_read = true, + $encoding = "UTF-8", + ) { try { $tmpfile_path = \arrayGet($_FILES, $filename, "tmp_name"); } catch (ArrayKeyException $e) { @@ -161,14 +169,15 @@ public static function getUploadedFileContents($filename, $do_delete_tmpfile_aft if ($do_delete_tmpfile_after_read) { unlink($tmpfile_path); } - return $contents; + $old_encoding = mbDetectEncoding($contents); + return mbConvertEncoding($contents, $encoding, $old_encoding); } // in firefox, the user can disable alert/confirm/prompt after the 2nd or 3rd popup // after I disable alerts, if I quit and reopen my browser, the alerts come back public static function alert(string $message) { - // json_encode escapes quotes - echo ""; + // jsonEncode escapes quotes + echo ""; } } diff --git a/resources/lib/UnityLDAP.php b/resources/lib/UnityLDAP.php index 7d1ae60d..74e30c08 100644 --- a/resources/lib/UnityLDAP.php +++ b/resources/lib/UnityLDAP.php @@ -332,7 +332,7 @@ public function getAllPIGroupOwnerAttributes($attributes) if (count($owners_not_found) > 0) { UnityHTTPD::errorLog( "warning", - "PI group owners not found: " . json_encode($owners_not_found) . "\n" + "PI group owners not found: " . \jsonEncode($owners_not_found) . "\n" ); } return $owner_attributes; diff --git a/resources/lib/UnityWebhook.php b/resources/lib/UnityWebhook.php index 3f479eb2..61c9dd1d 100644 --- a/resources/lib/UnityWebhook.php +++ b/resources/lib/UnityWebhook.php @@ -54,7 +54,7 @@ public function sendWebhook($template = null, $data = null) curl_setopt($ch, CURLOPT_POST, 1); curl_setopt($ch, CURLOPT_HTTPHEADER, array('Content-Type: application/json')); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); - curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode(array('text' => $message))); + curl_setopt($ch, CURLOPT_POSTFIELDS, \jsonEncode(array('text' => $message))); $result = curl_exec($ch); curl_close($ch); return $result; diff --git a/resources/lib/exceptions/EncodingConversionException.php b/resources/lib/exceptions/EncodingConversionException.php new file mode 100644 index 00000000..ac316d89 --- /dev/null +++ b/resources/lib/exceptions/EncodingConversionException.php @@ -0,0 +1,7 @@ + [1][2]["foo"] - implode("", array_map(fn($x) => json_encode([$x]), $keysTraversed)) + implode("", array_map(fn($x) => jsonEncode([$x]), $keysTraversed)) ); } $cursor = $cursor[$key]; @@ -53,3 +55,31 @@ function testValidSSHKey($key_str) return false; } } + +function jsonEncode($value, $flags = 0, $depth = 512) +{ + $flags |= JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES; + return json_encode($value, $flags, $depth); +} + +function mbConvertEncoding($string, $to_encoding, $from_encoding = null) +{ + $output = mb_convert_encoding($string, $to_encoding, $from_encoding); + if ($output === false) { + throw new EncodingConversionException( + jsonEncode( + ["to" => $to_encoding, "from" => $from_encoding, "base64" => base64_encode($string)] + ) + ); + } + return $output; +} + +function mbDetectEncoding($string, $encodings = null, $_ = null) +{ + $output = mb_detect_encoding($string, $encodings, strict: true); + if ($output === false) { + throw new EncodingUnknownException(base64_encode($string)); + } + return $output; +} diff --git a/test/assert-UnityHTTPD-used.bash b/test/assert-UnityHTTPD-used.bash new file mode 100644 index 00000000..a5b92dee --- /dev/null +++ b/test/assert-UnityHTTPD-used.bash @@ -0,0 +1,22 @@ +set -euo pipefail +trap 's=$?; echo "$0: Error on line "$LINENO": $BASH_COMMAND"; exit $s' ERR +if [[ $# -lt 1 ]]; then + echo "at least one argument required" + exit 1 +fi + +funcs=(die exit) +rc=0 +for func in "${funcs[@]}"; do + # --color=never because magit git output log doesn't support it + grep_rc=0; grep -H --color=never --line-number -P '\b'"$func"'\s*[\(;]' "$@" | grep -v -E 'UnityHTTPD::'"$func" || grep_rc=$? + case "$grep_rc" in + 0) + echo "$func() are not allowed! use UnityHTTPD::die() instead."; rc=1 ;; + 1) + : ;; # code is good, do nothing + *) + echo "grep failed!"; rc=1 ;; + esac +done +exit "$rc" diff --git a/test/assert-exceptions-used.bash b/test/assert-exceptions-used.bash new file mode 100644 index 00000000..90177c74 --- /dev/null +++ b/test/assert-exceptions-used.bash @@ -0,0 +1,22 @@ +set -euo pipefail +trap 's=$?; echo "$0: Error on line "$LINENO": $BASH_COMMAND"; exit $s' ERR +if [[ $# -lt 1 ]]; then + echo "at least one argument required" + exit 1 +fi + +funcs=(badRequest forbidden internalServerError) +rc=0 +for func in "${funcs[@]}"; do + # --color=never because magit git output log doesn't support it + grep_rc=0; grep -H --color=never --line-number -P '\b'"$func"'\s*[\(;]' "$@" || grep_rc=$? + case "$grep_rc" in + 0) + echo "$func() is not allowed! use an exception instead."; rc=1 ;; + 1) + : ;; # code is good, do nothing + *) + echo "grep failed!"; rc=1 ;; + esac +done +exit "$rc" diff --git a/test/assert-no-die-exit.bash b/test/assert-no-die-exit.bash deleted file mode 100755 index 638c2e6b..00000000 --- a/test/assert-no-die-exit.bash +++ /dev/null @@ -1,28 +0,0 @@ -#!/bin/bash -set -euo pipefail -if [[ $# -lt 1 ]]; then - echo "at least one argument required" - exit 1 -fi - -rc=0 - -# --color=never because magit git output log doesn't support it -die_occurrences="$( - grep -H --color=never --line-number -P '\bdie\s*[\(;]' "$@" | grep -v -P 'UnityHTTPD::die' -)" || true -if [ -n "$die_occurrences" ]; then - echo "die is not allowed! use UnityHTTPD::die() instead." - echo "$die_occurrences" - rc=1 -fi - -# --color=never because magit git output log doesn't support it -exit_occurrences="$(grep -H --color=never --line-number -P '\bexit\s*[\(;]' "$@")" || true -if [ -n "$exit_occurrences" ]; then - echo "exit is not allowed! use UnityHTTPD::die() instead." - echo "$exit_occurrences" - rc=1 -fi - -exit "$rc" diff --git a/test/assert-utils-used.bash b/test/assert-utils-used.bash new file mode 100644 index 00000000..51e95e31 --- /dev/null +++ b/test/assert-utils-used.bash @@ -0,0 +1,29 @@ +set -euo pipefail +trap 's=$?; echo "$0: Error on line "$LINENO": $BASH_COMMAND"; exit $s' ERR +if [[ $# -lt 1 ]]; then + echo "at least one argument required" + exit 1 +fi + +declare -A utils=( + ["assert"]="ensure" + ["json_encode"]="jsonEncode" + ["mb_detect_encoding"]="mbDetectEncoding" + ["mb_convert_encoding"]="mbConvertEncoding" +) + +rc=0 +for replaced in "${!utils[@]}"; do + replacement="${utils[$replaced]}" + # --color=never because magit git output log doesn't support it + grep_rc=0; grep -H --color=never --line-number -P '\b'"$replaced"'\s*[\(;]' "$@" || grep_rc=$? + case "$grep_rc" in + 0) + echo "$replaced() are not allowed! use $replacement() instead."; rc=1 ;; + 1) + : ;; # code is good, do nothing + *) + echo "grep failed!"; rc=1 ;; + esac +done +exit "$rc" diff --git a/test/functional/NewUserTest.php b/test/functional/NewUserTest.php index 268d5190..e9254ec1 100644 --- a/test/functional/NewUserTest.php +++ b/test/functional/NewUserTest.php @@ -108,14 +108,14 @@ private function ensureUserDoesNotExist() $org = $USER->getOrgGroup(); if ($org->exists() and $org->inOrg($USER)) { $org->removeUser($USER); - assert(!$org->inOrg($USER)); + ensure(!$org->inOrg($USER)); } $LDAP->getUserEntry($USER->uid)->delete(); - assert(!$USER->exists()); + ensure(!$USER->exists()); } if ($USER->getGroupEntry()->exists()) { $USER->getGroupEntry()->delete(); - assert(!$USER->getGroupEntry()->exists()); + ensure(!$USER->getGroupEntry()->exists()); } $all_users_group = $LDAP->getUserGroup(); $all_member_uids = $all_users_group->getAttribute("memberuid"); @@ -127,7 +127,7 @@ private function ensureUserDoesNotExist() array_values(array_diff($all_member_uids, [$USER->uid])) ); $all_users_group->write(); - assert(!in_array($USER->uid, $all_users_group->getAttribute("memberuid"))); + ensure(!in_array($USER->uid, $all_users_group->getAttribute("memberuid"))); } $REDIS->removeCacheArray("sorted_users", "", $USER->uid); } @@ -138,7 +138,7 @@ private function ensureOrgGroupDoesNotExist() $org_group = $LDAP->getOrgGroupEntry($SSO["org"]); if ($org_group->exists()) { $org_group->delete(); - assert(!$org_group->exists()); + ensure(!$org_group->exists()); } $REDIS->removeCacheArray("sorted_orgs", "", $SSO["org"]); } @@ -148,7 +148,7 @@ private function ensureUserNotInPIGroup(UnityGroup $pi_group) global $USER, $REDIS; if ($pi_group->memberExists($USER)) { $pi_group->removeUser($USER); - assert(!$pi_group->memberExists($USER)); + ensure(!$pi_group->memberExists($USER)); } $REDIS->removeCacheArray($pi_group->gid, "members", $USER->uid); } @@ -159,7 +159,7 @@ private function ensurePIGroupDoesNotExist() $gid = $USER->getPIGroup()->gid; if ($USER->getPIGroup()->exists()) { $LDAP->getPIGroupEntry($gid)->delete(); - assert(!$USER->getPIGroup()->exists()); + ensure(!$USER->getPIGroup()->exists()); } $REDIS->removeCacheArray("sorted_groups", "", $gid); } diff --git a/test/phpunit-bootstrap.php b/test/phpunit-bootstrap.php index 1bd49bc3..34b370d9 100644 --- a/test/phpunit-bootstrap.php +++ b/test/phpunit-bootstrap.php @@ -20,6 +20,8 @@ require_once __DIR__ . "/../resources/lib/exceptions/SSOException.php"; require_once __DIR__ . "/../resources/lib/exceptions/ArrayKeyException.php"; require_once __DIR__ . "/../resources/lib/exceptions/EnsureException.php"; +require_once __DIR__ . "/../resources/lib/exceptions/EncodingUnknownException.php"; +require_once __DIR__ . "/../resources/lib/exceptions/EncodingConversionException.php"; $_SERVER["HTTP_HOST"] = "phpunit"; // used for config override require_once __DIR__ . "/../resources/config.php"; @@ -48,7 +50,7 @@ '{"key": "value"}', 'SGVsbG8sIFdvcmxkIQ==', "Hello\x00World", - mb_convert_encoding("Hello, World!", "UTF-16") + mbConvertEncoding("Hello, World!", "UTF-16") ]; function arraysAreEqualUnOrdered(array $a, array $b): bool @@ -79,7 +81,7 @@ function switchUser( $_SERVER["givenName"] = $given_name; $_SERVER["sn"] = $sn; include __DIR__ . "/../resources/autoload.php"; - assert(!is_null($USER)); + ensure(!is_null($USER)); } function http_post(string $phpfile, array $post_data): void @@ -102,7 +104,7 @@ function http_post(string $phpfile, array $post_data): void $_SERVER = $_PREVIOUS_SERVER; } // https://en.wikipedia.org/wiki/Post/Redirect/Get - assert($post_did_redirect_or_die, "post did not redirect or die!"); + ensure($post_did_redirect_or_die, "post did not redirect or die!"); } function http_get(string $phpfile, array $get_data = array()): void diff --git a/webroot/api/notices/index.php b/webroot/api/notices/index.php index 9c4eaca9..94076c8f 100644 --- a/webroot/api/notices/index.php +++ b/webroot/api/notices/index.php @@ -15,5 +15,5 @@ $jsonArray[] = $formattedNotice; } -$jsonOutput = json_encode($jsonArray, JSON_PRETTY_PRINT); +$jsonOutput = jsonEncode($jsonArray, JSON_PRETTY_PRINT); echo $jsonOutput; diff --git a/webroot/panel/account.php b/webroot/panel/account.php index f97a84b6..fa2c5d6f 100644 --- a/webroot/panel/account.php +++ b/webroot/panel/account.php @@ -3,6 +3,8 @@ require_once __DIR__ . "/../../resources/autoload.php"; use UnityWebPortal\lib\UnityHTTPD; +use UnityWebPortal\lib\exceptions\EncodingUnknownException; +use UnityWebPortal\lib\exceptions\EncodingConversionException; $hasGroups = count($USER->getPIGroupGIDs()) > 0; @@ -15,7 +17,11 @@ array_push($keys, UnityHTTPD::getPostData("key")); break; case "import": - $key = UnityHTTPD::getUploadedFileContents("keyfile"); + try { + $key = UnityHTTPD::getUploadedFileContents("keyfile"); + } catch (EncodingUnknownException | EncodingConversionException $e) { + UnityHTTPD::badRequest("uploaded key has bad encoding", error: $e); + } array_push($keys, $key); break; case "generate":