throw JSON errors, ensure file encoding, handle bad data in errorLog

.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$

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.
 

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";

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 "<script type='text/javascript'>alert(" . json_encode($message) . ");</script>";
+        // jsonEncode escapes quotes
+        echo "<script type='text/javascript'>alert(" . \jsonEncode($message) . ");</script>";
     }
 }

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;

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;

resources/lib/exceptions/EncodingConversionException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace UnityWebPortal\lib\exceptions;
+
+class EncodingConversionException extends \Exception
+{
+}

resources/lib/exceptions/EncodingUnknownException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace UnityWebPortal\lib\exceptions;
+
+class EncodingUnknownException extends \Exception
+{
+}

resources/lib/utils.php

@@ -2,6 +2,8 @@
 
 use UnityWebPortal\lib\exceptions\ArrayKeyException;
 use UnityWebPortal\lib\exceptions\EnsureException;
+use UnityWebPortal\lib\exceptions\EncodingUnknownException;
+use UnityWebPortal\lib\exceptions\EncodingConversionException;
 use phpseclib3\Crypt\PublicKeyLoader;
 
 function arrayGet($array, ...$keys)
@@ -14,7 +16,7 @@ function arrayGet($array, ...$keys)
             throw new ArrayKeyException(
                 "key not found: \$array" .
                 // [1, 2, "foo"] => [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;
+}

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"

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"

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"

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);
     }

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