diff --git a/packages/web/lib/fog/system.class.php b/packages/web/lib/fog/system.class.php
index d0975f6356..6d17253fd4 100644
--- a/packages/web/lib/fog/system.class.php
+++ b/packages/web/lib/fog/system.class.php
@@ -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
diff --git a/packages/web/lib/reg-task/taskerror.class.php b/packages/web/lib/reg-task/taskerror.class.php
index 3e9088ed69..d808114dc5 100644
--- a/packages/web/lib/reg-task/taskerror.class.php
+++ b/packages/web/lib/reg-task/taskerror.class.php
@@ -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) {
@@ -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) {
@@ -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) {
@@ -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.
*
diff --git a/packages/web/management/js/fog/task/fog.task.list.js b/packages/web/management/js/fog/task/fog.task.list.js
index 85348fe030..8813e89b20 100644
--- a/packages/web/management/js/fog/task/fog.task.list.js
+++ b/packages/web/management/js/fog/task/fog.task.list.js
@@ -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 . 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
}
],
@@ -538,7 +553,11 @@
// rather than an empty box.
html += 'Message'
+ (row.logtext ?
- '' + $.escapeHtml(row.logtext) + '
' :
+ // NOT .text-wrap: Bootstrap defines that as
+ // `white-space: normal !important`, which overrides the and
+ // collapses exactly the line breaks the report is now stored with.
+ // pre-wrap keeps them and still wraps long lines inside the modal.
+ '' + $.escapeHtml(row.logtext) + '
' :
'none')
+ '
';
$dl.html(html);
diff --git a/tests/task-error-report.test.php b/tests/task-error-report.test.php
index ec03017199..d3275f6208 100644
--- a/tests/task-error-report.test.php
+++ b/tests/task-error-report.test.php
@@ -65,17 +65,19 @@ 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"
@@ -83,6 +85,44 @@ public function processEvent($event, $data = [])
}
}
+$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
@@ -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.
diff --git a/tests/task-log-view.test.php b/tests/task-log-view.test.php
index 33a8b1ce99..3d6c6b042b 100644
--- a/tests/task-log-view.test.php
+++ b/tests/task-log-view.test.php
@@ -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
+// and collapses them -- so its absence here is load-bearing.
+if (!preg_match('#]*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('#]*text-wrap#', $js)) {
+ $fails[] = 'the modal still carries .text-wrap, whose'
+ . ' `white-space: normal !important` overrides the ';
+}
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';