Skip to content

Keep the line breaks in a stored FOS report #1224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mastacontrola merged 1 commit into from
Aug 19, 2026
Merged

Keep the line breaks in a stored FOS report #1224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 59 additions & 10 deletions packages/web/lib/reg-task/taskerror.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,16 @@ public function __construct()
// deploy that dies on a bad image, and this is the half of the
// report that carries no notification consequences.
self::_logRow($Task, $type, $text);
// Flattened: _record() writes one timestamped line per report and
// `tail`ing this file depends on that holding. The row written
// just above is where the trace keeps its line breaks.
self::_record(
sprintf(
'FOG: %s reported by host %s (task %s): %s',
$type,
self::$Host->get('name'),
$Task->get('id'),
$text
self::_flatten($text)
)
);
if (TaskLog::TYPE_ERROR !== $type) {
Expand Down Expand Up @@ -208,7 +211,11 @@ public function __construct()
// 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)
'Reason' => mb_substr(
self::_flatten($text),
0,
self::MAX_REASON
)
)
);
} catch (Exception $e) {
Expand Down Expand Up @@ -335,16 +342,29 @@ private static function _reported($field)
*/
private static function _sanitize($raw)
{
// \p{C} is every Unicode control and format character, which covers
// CR, LF, NUL and the terminal escapes a console-facing error string
// can easily contain.
$clean = preg_replace('#\p{C}+#u', ' ', $raw);
// Line breaks SURVIVE here; every other control character does not.
// A stored report is up to MAX_TEXT bytes of trace, and 8K of trace
// on a single line is barely more readable than the 500 characters
// it replaced. The reason this guard existed -- an embedded newline
// lets a caller forge a second message -- is true of a chat
// notification and of a log file whose entries are one line each,
// and is not true of a database row. So it moved to _flatten(),
// which those two destinations call and the stored row does not.
//
// Normalised first so only one line ending has to survive the class
// below, and so a CRLF report does not store stray carriage returns.
$raw = preg_replace('#\r\n?#', "\n", (string)$raw);
// [^\P{C}\n] is "in \p{C} but not a newline": every Unicode control
// and format character -- NUL, the terminal escapes a console-facing
// error string easily carries -- except the one being kept.
$clean = preg_replace('#[^\P{C}\n]+#u', ' ', $raw);
if (null === $clean) {
// Invalid UTF-8 makes preg_replace return null rather than throw,
// so fall back to the byte-wise class. Never let a malformed
// string become an empty one silently -- the text is the whole
// point of the report.
$clean = preg_replace('#[[:cntrl:]]+#', ' ', $raw);
// so fall back to an explicit byte range: the same set minus LF,
// written out because [[:cntrl:]] would take the newline back.
// Never let a malformed string become an empty one silently --
// the text is the whole point of the report.
$clean = preg_replace('#[\x00-\x09\x0B-\x1F\x7F]+#', ' ', $raw);
}
$clean = trim((string)$clean);
if ('' === $clean) {
Expand All @@ -353,6 +373,35 @@ private static function _sanitize($raw)

return self::_limit($clean, self::MAX_TEXT);
}
/**
* Collapses a report onto one line.
*
* For the two destinations where a line break is a forgery risk rather
* than formatting: a chat notification, where it lets a caller fake a
* second message under an administrator's eyes, and fosreports.log,
* where every entry is one timestamped line and `tail` depends on that
* staying true.
*
* The stored row deliberately does NOT come through here -- see
* _sanitize(). That is the whole point of the split: the row keeps the
* shape of the trace, the two places a forged line would do damage do
* not.
*
* @param string $str the sanitized report text
*
* @return string
*/
private static function _flatten($str)
{
$flat = preg_replace('#\s+#u', ' ', $str);
if (null === $flat) {
// Same invalid-UTF-8 fallback as _sanitize(): a report from a
// machine with the wrong locale still has to reach somebody.
$flat = preg_replace('#\s+#', ' ', $str);
}

return trim((string)$flat);
}
/**
* Cuts a string to a byte budget without splitting a character.
*
Expand Down
69 changes: 58 additions & 11 deletions tests/task-error-report.test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,24 +58,64 @@ class TaskLog

// ------------------------------------------------------------- one line

// Line breaks are the ONE control character the stored row keeps -- a trace
// is worth having in the shape FOS wrote it. Everything else still goes,
// including the carriage return, so a CRLF report does not store stray \r.
foreach (array(
"Failed to mount\nHost imaging completed successfully" => 'a newline',
"Failed to mount\r\nsecond line" => 'a CRLF',
"Failed\tto mount" => 'a tab',
"Failed to mount\x00truncated" => 'a NUL',
"Failed \x1b[31mto\x1b[0m mount" => 'a terminal escape',
"Failed to mount\r\nsecond line" => 'a carriage return',
) as $raw => $what) {
$out = $clean($raw);
if (preg_match('#[\r\n\t\x00\x1b]#', $out)) {
$fails[] = "$what survives sanitizing, so a caller can forge a second"
. ' line in an administrator\'s notification';
if (preg_match('#[\r\t\x00\x1b]#', $out)) {
$fails[] = "$what survives sanitizing, so it reaches a log file and a"
. ' notification that are both single-line by contract';
}
if ('' === $out) {
$fails[] = "$what makes the whole report empty, which throws away the"
. ' only thing it carries';
}
}

$multi = $clean("Failed to mount\nArgs: -i 2\r\nexit 32");
if (2 !== substr_count($multi, "\n")) {
$fails[] = 'the stored report does not keep its line breaks, so 8K of'
. ' trace arrives as one unreadable line -- the reason MAX_TEXT was'
. ' widened in the first place';
}
// Exact, because stripping the CR without NORMALISING it first turns every
// CRLF into "space + newline": a trailing space on every line of a report
// from a machine that ends lines the DOS way, which is most of them.
if ("a\nb" !== $clean("a\r\nb")) {
$fails[] = 'a CRLF report is not normalised to a bare newline, so every'
. ' line of it is stored with trailing whitespace';
}
// The invalid-UTF-8 fallback has to keep the newline too. [[:cntrl:]] --
// the obvious class, and what this used to be -- includes it, so a report
// from a machine with the wrong locale would silently lose its shape while
// a well-formed one kept it.
if (false === strpos($clean("Failed \xC3\x28 to mount\nsecond line"), "\n")) {
$fails[] = 'the invalid-UTF-8 fallback strips line breaks, so a report'
. ' from a machine with the wrong locale is flattened while every'
. ' other report keeps its shape';
}

// ...and the two destinations that ARE single-line by contract flatten it
// themselves. A newline in a chat message lets a caller forge a second one;
// a newline in fosreports.log breaks the one-entry-per-line property `tail`
// depends on.
$flatten = new ReflectionMethod('TaskError', '_flatten');
$flatten->setAccessible(true);
$flat = $flatten->invoke(null, $multi);
if (false !== strpos($flat, "\n")) {
$fails[] = '_flatten() leaves line breaks in, so a caller can forge a'
. ' second line in an administrator\'s notification';
}
if ('' === $flat) {
$fails[] = '_flatten() empties the report';
}

// ---------------------------------------------------------------- bounds

// Two bounds, not one, and they must not collapse back into each other. The
Expand Down Expand Up @@ -161,13 +201,20 @@ class TaskLog
// 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)"
if (!preg_match(
'#\'Reason\' => mb_substr\(\s*self::_flatten\(\$text\),\s*0,\s*self::MAX_REASON\s*\)#s',
$src
)) {
$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';
$fails[] = 'the notification payload is not flattened and cut to'
. ' MAX_REASON at the notify() call, so widening what is stored also'
. ' widens -- or breaks the line discipline of -- what is pushed to'
. ' an administrator\'s phone';
}
// The log file is the other single-line contract. Its entries are one
// timestamped line each, written by _record().
if (!preg_match('#self::_record\(\s*sprintf\((?:[^;]*?)self::_flatten\(\$text\)#s', $src)) {
$fails[] = 'the fosreports.log line is not flattened, so one report can'
. ' span several lines and forge entries around itself';
}
// _logRow gets the WHOLE text. `self::` and the semicolon on purpose: without
// them this also matches the method's own SIGNATURE, so narrowing the
Expand Down