diff --git a/packages/web/lib/reg-task/taskerror.class.php b/packages/web/lib/reg-task/taskerror.class.php index 1e81d4c90a..3e9088ed69 100644 --- a/packages/web/lib/reg-task/taskerror.class.php +++ b/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; } /** diff --git a/tests/task-error-report.test.php b/tests/task-error-report.test.php index 06b95a3db7..ec03017199 100644 --- a/tests/task-error-report.test.php +++ b/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';