Let a stored FOS report be longer than the notification it triggers

packages/web/lib/reg-task/taskerror.class.php

@@ -54,15 +54,43 @@
 class TaskError extends FOGBase
 {
     /**
-     * How much of the reported text is kept.
+     * How much of the report reaches a NOTIFICATION, in characters.
      *
-     * The text is written by whatever called this endpoint, and it ends up in
-     * a Slack/ntfy/pushbullet message. Bounded so a caller cannot use an
-     * admin's notification channel as a paste bin.
+     * The text is written by whatever called this endpoint, and this half of
+     * it ends up in a Slack/ntfy/pushbullet message. Bounded so a caller
+     * cannot use an admin's notification channel as a paste bin.
+     *
+     * Characters, not bytes, and deliberately: nothing downstream of here has
+     * a byte budget, and cutting a multibyte reason by bytes would silently
+     * make it a third as long for anyone not writing in ASCII.
      *
      * @var int
      */
     const MAX_REASON = 500;
+    /**
+     * How much of the report is STORED and logged, in bytes.
+     *
+     * Split from MAX_REASON because the two have opposite pressures. A push
+     * notification wants to stay short enough to read on a phone; a stored
+     * diagnostic wants the whole of what FOS had to say, and 500 characters
+     * is not a failure trace. `taskLog`.`logText` is TEXT, so the row can
+     * hold 65535 bytes and this could have been that.
+     *
+     * It is not, because of what this endpoint is: unauthenticated, matched
+     * to a host by MAC (see the class docblock). Taking the column's whole
+     * capacity would multiply what one unauthenticated request can write by
+     * 130 for no diagnostic gain -- a `fog.download` trace with its context
+     * runs to a few KB, not 64. So: generous against any real report,
+     * bounded against a caller with something else in mind.
+     *
+     * Bytes rather than characters because the limit that actually exists is
+     * the column's, and that one is in bytes: sql_mode carries
+     * STRICT_TRANS_TABLES, so an oversized value fails the INSERT rather
+     * than truncating, and the report would be lost entirely.
+     *
+     * @var int
+     */
+    const MAX_TEXT = 8192;
     /**
      * The report types a caller may send, mapped to the TaskLog type.
      *
@@ -111,7 +139,13 @@ public function __construct()
             $type = self::_reportedType();
             $script = self::_reported('script');
             if ('' !== $script) {
-                $text = sprintf('%s (%s)', $text, $script);
+                // Re-bounded after composing: both halves arrive already cut
+                // to MAX_TEXT, so joining them could otherwise hand the
+                // column twice what it was promised.
+                $text = self::_limit(
+                    sprintf('%s (%s)', $text, $script),
+                    self::MAX_TEXT
+                );
             }
             self::getHostItem(false);
             $Task = self::$Host->get('task');
@@ -166,7 +200,11 @@ public function __construct()
                         ''
                     ),
                     'TaskType' => $Task->getTaskTypeText(),
-                    'Reason' => $text
+                    // The short half. The row and the log line above keep the
+                    // whole report; a phone notification gets the opening of
+                    // it. Cut here rather than at the top so that widening
+                    // what is stored never widens what is pushed.
+                    'Reason' => mb_substr($text, 0, self::MAX_REASON)
                 ]
             );
         } catch (\Exception $e) {
@@ -304,12 +342,37 @@ private static function _sanitize($raw)
         if ('' === $clean) {
             return '';
         }
-        // mb_substr on invalid UTF-8 can return '', which would throw the
-        // report away; strlen-bound the bytes in that case instead.
-        $cut = mb_substr($clean, 0, self::MAX_REASON);
+
+        return self::_limit($clean, self::MAX_TEXT);
+    }
+    /**
+     * Cuts a string to a byte budget without splitting a character.
+     *
+     * mb_strcut, not mb_substr: the budget being spent is the column's, and
+     * that is counted in bytes. mb_substr counts characters, so a cut at
+     * 8192 characters can be 24576 bytes in utf8mb3 -- three times what was
+     * promised, and under STRICT_TRANS_TABLES that is a failed INSERT and a
+     * lost report rather than a truncated one.
+     *
+     * @param string $str the string to bound
+     * @param int    $max the budget, in bytes
+     *
+     * @return string
+     */
+    private static function _limit($str, $max)
+    {
+        if (strlen($str) <= $max) {
+            return $str;
+        }
+        // mb_strcut on invalid UTF-8 can return '', which would throw the
+        // report away; byte-cut it in that case instead. Never let a
+        // malformed string become an empty one -- the text is the whole
+        // point of the report.
+        $cut = mb_strcut($str, 0, $max);
         if ('' === $cut) {
-            $cut = substr($clean, 0, self::MAX_REASON);
+            $cut = substr($str, 0, $max);
         }
+
         return $cut;
     }
     /**

tests/task-error-report.test.php

@@ -58,7 +58,10 @@ public function processEvent($event, $data = [])
 $clean = function ($raw) use ($sanitize) {
     return $sanitize->invoke(null, $raw);
 };
-$max = (new \ReflectionClass('FOG\TaskError'))->getConstant('MAX_REASON');
+$refl = new \ReflectionClass('FOG\TaskError');
+$maxReason = $refl->getConstant('MAX_REASON');
+$maxText = $refl->getConstant('MAX_TEXT');
+$max = $maxText;
 
 // ------------------------------------------------------------- one line
 
@@ -82,24 +85,46 @@ public function processEvent($event, $data = [])
 
 // ---------------------------------------------------------------- bounds
 
-if (!is_int($max) || $max < 1) {
-    $fails[] = 'MAX_REASON is not a positive integer, so the report is unbounded';
+// Two bounds, not one, and they must not collapse back into each other. The
+// stored row and the push notification pull in opposite directions: a phone
+// message wants to stay readable, a diagnostic wants the whole trace. If
+// MAX_TEXT ever stops being the larger of the two, the split has been undone
+// and widening storage has quietly started widening what gets pushed.
+foreach (['MAX_REASON' => $maxReason, 'MAX_TEXT' => $maxText] as $name => $val) {
+    if (!is_int($val) || $val < 1) {
+        $fails[] = "$name is not a positive integer, so the report is unbounded";
+    }
+}
+if ($maxText <= $maxReason) {
+    $fails[] = 'MAX_TEXT is not larger than MAX_REASON, so splitting them'
+        . ' bought nothing -- the stored report is still notification-sized';
+}
+// The column is TEXT: 65535 BYTES. Bounding above that would mean an
+// oversized report fails its INSERT under STRICT_TRANS_TABLES and is lost
+// entirely, rather than being stored short.
+if ($maxText > 65535) {
+    $fails[] = 'MAX_TEXT exceeds what taskLog.logText can hold, so a large'
+        . ' report fails the INSERT instead of being truncated';
 }
-$long = str_repeat('A', $max * 3);
-if (mb_strlen($clean($long)) > $max) {
+
+// Bytes, not characters -- that is the budget the column actually spends.
+$long = str_repeat('A', $maxText * 2);
+if (strlen($clean($long)) > $maxText) {
     $fails[] = 'a long report is not truncated, so an unauthenticated caller'
-        . ' can use a notification channel as a paste bin';
+        . ' can write whatever it likes into the task log';
 }
 
-// A multibyte string must not be cut into an invalid sequence, and must not
-// be thrown away either.
-$mb = str_repeat('é', $max * 2);
+// A multibyte string must not be cut into an invalid sequence, must not be
+// thrown away, and must not be cut by CHARACTERS: 8192 utf8mb3 characters is
+// 24576 bytes, three times the budget, which the column would reject.
+$mb = str_repeat('é', $maxText);
 $cut = $clean($mb);
 if ('' === $cut) {
     $fails[] = 'a multibyte report is discarded entirely';
 }
-if (mb_strlen($cut) > $max) {
-    $fails[] = 'a multibyte report is not truncated';
+if (strlen($cut) > $maxText) {
+    $fails[] = 'a multibyte report is bounded by characters rather than bytes,'
+        . ' so it can exceed the column and fail the INSERT';
 }
 if ($cut !== mb_convert_encoding($cut, 'UTF-8', 'UTF-8')) {
     $fails[] = 'truncation split a multibyte character, so the message is no'
@@ -123,6 +148,38 @@ public function processEvent($event, $data = [])
 
 $src = file_get_contents($web . '/lib/reg-task/taskerror.class.php');
 
+// The split only exists if the SHORT bound is applied at the notification and
+// nowhere earlier. Cut at the top instead and both halves shrink together,
+// which is the state this change was undoing.
+if (false === strpos(
+    $src,
+    "'Reason' => mb_substr(\$text, 0, self::MAX_REASON)"
+)) {
+    $fails[] = 'the notification payload is not cut to MAX_REASON at the'
+        . ' notify() call, so widening what is stored also widens what is'
+        . ' pushed to an administrator\'s phone';
+}
+// _logRow gets the WHOLE text. If MAX_REASON reaches it, the stored row is
+// notification-sized again and the extra capacity is unreachable.
+// `self::` and the semicolon on purpose: without them this also matches the
+// method's own SIGNATURE, `_logRow($Task, $type, $text)`, so narrowing the
+// argument at the call site passed clean.
+if (false === strpos($src, 'self::_logRow($Task, $type, $text);')) {
+    $fails[] = 'the stored row is no longer written from the full report text';
+}
+// text and script arrive bounded separately, so the join has to be re-bounded
+// or the column is handed twice what it was promised.
+if (!preg_match(
+    '#sprintf\(\'%s \(%s\)\', \$text, \$script\)#s',
+    $src
+) || !preg_match(
+    '#\$text = self::_limit\(\s*sprintf#s',
+    $src
+)) {
+    $fails[] = 'the composed text+script is not re-bounded, so a report with a'
+        . ' script name can be twice MAX_TEXT';
+}
+
 if (false === strpos($src, '$Task->isImagingTask()')) {
     $fails[] = 'the endpoint no longer checks the task is an imaging one, so a'
         . ' failed wipe fires HOST_IMAGE_FAIL';