Skip to content

Keep the line breaks in a stored FOS report, and escape it on the way out #1223

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, and escape it on the way out #1223

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
2 changes: 1 addition & 1 deletion packages/web/lib/fog/system.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public function __construct()
// permanently "up to date" from the updater's point of view and will
// never run another indexed step, whatever this constant says.
define('FOG_SCHEMA', 340);
define('FOG_BCACHE_VER', 287);
define('FOG_BCACHE_VER', 288);
define('FOG_CLIENT_VERSION', '0.13.0');
// GH-959: iPXE lives in FOGProject/fog-ipxe and its binaries arrive as
// a release asset. Pinned here rather than tracked as "latest" so a
Expand Down
81 changes: 67 additions & 14 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 @@ -157,13 +157,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 %d): %s',
$type,
self::$Host->get('name'),
(int) $Task->get('id'),
$text
self::_flatten($text)
)
);
if (TaskLog::TYPE_ERROR !== $type) {
Expand Down Expand Up @@ -200,11 +203,18 @@ public function __construct()
''
),
'TaskType' => $Task->getTaskTypeText(),
// 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)
// The short, single-line half. The stored row keeps the
// whole report and its line breaks; a phone notification
// gets the opening of it, flattened so a caller cannot
// forge a second message inside one. Cut here rather
// than at the top so that widening what is stored never
// widens what is pushed. Flattened BEFORE the cut, so
// the 500 is spent on text rather than on whitespace.
'Reason' => mb_substr(
self::_flatten($text),
0,
self::MAX_REASON
)
]
);
} catch (\Exception $e) {
Expand Down Expand Up @@ -327,16 +337,30 @@ 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 rendered in a modal. 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 @@ -345,6 +369,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
21 changes: 20 additions & 1 deletion packages/web/management/js/fog/task/fog.task.list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,21 @@
},
{
responsivePriority: -1,
// Escaped, like every other column here. DataTables writes cell
// content with innerHTML, so a column with no render is an HTML
// sink -- and this is the one column fed by taskerror.class.php,
// an endpoint FOS reaches without authenticating. It was the only
// one left bare.
//
// Flattened as well: the stored report now keeps its line breaks,
// which the modal renders in a <pre>. In a one-line grid cell they
// are only whitespace, and a preview that starts with a blank line
// reads as an empty column.
render: function(data) {
return $.escapeHtml(
String(data || '').replace(/\s+/g, ' ').trim()
);
},
targets: 5
}
],
Expand Down Expand Up @@ -538,7 +553,11 @@
// rather than an empty box.
html += '<dt class="col-sm-3">Message</dt><dd class="col-sm-9">'
+ (row.logtext ?
'<pre class="mb-0 text-wrap">' + $.escapeHtml(row.logtext) + '</pre>' :
// NOT .text-wrap: Bootstrap defines that as
// `white-space: normal !important`, which overrides the <pre> and
// collapses exactly the line breaks the report is now stored with.
// pre-wrap keeps them and still wraps long lines inside the modal.
'<pre class="mb-0" style="white-space:pre-wrap;overflow-wrap:anywhere;">' + $.escapeHtml(row.logtext) + '</pre>' :
'<em>none</em>')
+ '</dd>';
$dl.html(html);
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 @@ -65,24 +65,64 @@ public function processEvent($event, $data = [])

// ------------------------------------------------------------- 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 ([
"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('FOG\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 @@ -151,13 +191,20 @@ public function processEvent($event, $data = [])
// 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. If MAX_REASON reaches it, the stored row is
// notification-sized again and the extra capacity is unreachable.
Expand Down
47 changes: 47 additions & 0 deletions tests/task-log-view.test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,53 @@
$fails[] = 'nothing opens the log detail modal from a row, so the markup'
. ' is emitted and unreachable';
}
// Every column in this grid renders through $.escapeHtml, and the message
// column is the one that must: DataTables writes cell content with
// innerHTML, and this column alone is fed by taskerror.class.php -- an
// endpoint FOS reaches without authenticating. A bare `{data: 'logtext'}`
// with no render is a stored-XSS sink an unauthenticated caller can fill.
// Comments stripped first: the prose above this columnDef names the very
// thing being looked for, and would satisfy the search on its own.
// Scoped to buildLogs() FIRST. This file builds four grids and `targets: 5`
// occurs in more than one of them -- searching the whole file found another
// pane's column and reported on that instead, which is a check that passes
// while looking at the wrong thing.
//
// Comments stripped too: the prose above this columnDef names the very thing
// being looked for, and would satisfy the search on its own.
$jsBare = preg_replace('#^\s*//.*$#m', '', $js);
preg_match('#function buildLogs\(.*?function showLogDetail\(#s', $jsBare, $lb);
$logsFn = $lb[0] ?? '';
$msgCol = '' === $logsFn ? false : strpos($logsFn, 'targets: 5');
// Bounded below by the PREVIOUS `targets:`, so the window cannot reach into
// the neighbouring columnDef. A fixed character count did: deleting the
// render outright still passed, on column 4's escapeHtml.
$prevCol = false === $msgCol
? false
: strrpos(substr($logsFn, 0, $msgCol), 'targets:');
if (false === $msgCol || false === $prevCol) {
$fails[] = 'buildLogs() has no column 5, so the message column this'
. ' checks has moved and the check no longer sees its subject';
} elseif (false === strpos(
substr($logsFn, $prevCol, $msgCol - $prevCol),
'$.escapeHtml'
)) {
$fails[] = 'the logs grid message column does not escape its data, so a'
. ' report from an unauthenticated caller renders as HTML in an'
. " administrator's browser";
}
// The modal is where the stored line breaks are meant to show. Bootstrap's
// .text-wrap is `white-space: normal !important`, which overrides a <pre>
// and collapses them -- so its absence here is load-bearing.
if (!preg_match('#<pre class="mb-0"[^>]*white-space:pre-wrap#', $js)) {
$fails[] = 'the modal does not render the message with pre-wrap, so the'
. ' line breaks the report is now stored with are collapsed and the'
. ' trace reads as one paragraph';
}
if (preg_match('#<pre[^>]*text-wrap#', $js)) {
$fails[] = 'the modal still carries .text-wrap, whose'
. ' `white-space: normal !important` overrides the <pre>';
}
if (false === strpos($js, "closest('a').length")) {
$fails[] = 'the row click does not defer to the links inside it, so'
. ' clicking through to a host opens the modal instead';
Expand Down