diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 18454d5..bad3fab 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -85,6 +85,8 @@ jobs: - name: install what drives the browser run: | python3 -m pip install --quiet websockets requests + # pdftotext, for reading the text back out of a converted PDF + sudo apt-get install -y -qq poppler-utils # the runner image ships Chrome; the rig will take either echo "RIG_CHROMIUM=$(command -v google-chrome || command -v chromium || command -v chromium-browser)" >> "$GITHUB_ENV" diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index abb9361..5cbbb1c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -35,7 +35,7 @@ jobs: - name: lint run: | - find lib appinfo tests -name '*.php' -not -path './3rdparty/*' -print0 \ + find lib appinfo tests tools -name '*.php' -not -path './3rdparty/*' -print0 \ | xargs -0 -n1 -P4 php -l javascript: diff --git a/.nextcloudignore b/.nextcloudignore index c64de07..c851e4c 100644 --- a/.nextcloudignore +++ b/.nextcloudignore @@ -10,3 +10,4 @@ screenshots .nextcloudignore Makefile tests +tools diff --git a/Makefile b/Makefile index 73bf287..6d34165 100644 --- a/Makefile +++ b/Makefile @@ -46,6 +46,7 @@ appstore: --exclude="../$(app_name)/3rdparty/onlyoffice/documentserver/fonts" \ --exclude="../$(app_name)/build" \ --exclude="../$(app_name)/tests" \ + --exclude="../$(app_name)/tools" \ --exclude="../$(app_name)/vendor" \ --exclude="../$(app_name)/composer.json" \ --exclude="../$(app_name)/composer.lock" \ @@ -141,13 +142,23 @@ appstore: # Pin the generated font paths to x2t's working directory # (server/FileConverter/bin). allfontsgen writes them out prefixed with its # own working directory, which is neither where x2t runs nor the same path - # on the machine that installs the app; x2t then hands a null buffer to - # CFontFileLoader.LoadFontFromData and any change replay that has to measure - # text - saving a spreadsheet, for one - dies with "Cannot read property - # 'length' of null". The paths in font_selection.bin alongside it are not - # used for loading, so they can stay as generated. - sed -i 's|"[^"]*/core-fonts/|"../../../core-fonts/|g' \ - 3rdparty/onlyoffice/documentserver/server/FileConverter/bin/AllFonts.js + # on the machine that installs the app - and this tarball ships to every + # installation, so what is generated here is what they all get. + # + # Both of the files it writes have to be pinned. sdkjs reads AllFonts.js + # during change replay and hands the null buffer to + # CFontFileLoader.LoadFontFromData, so saving a spreadsheet dies with + # "Cannot read property 'length' of null"; x2t's PDF writer reads + # font_selection.bin and, when the path does not resolve, writes a PDF with + # no glyphs in it and exits 0 (#371, #251, #287). This used to pin only + # AllFonts.js, on the belief that font_selection.bin was not used for + # loading, which is why PDF export was blank for four years. + # + # font_selection.bin is not sed-able - its strings are length-prefixed - + # hence the php, which shares its rewriter with the runtime rebuild. + command -v php >/dev/null || { echo 'php is needed to pin the font paths' >&2; exit 1; } + php tools/pin-font-paths.php \ + 3rdparty/onlyoffice/documentserver/server/FileConverter/bin # Build the presentation design themes. Another job the DocService we do not # ship normally does: the package only carries their sources under # sdkjs/slide/themes/src, so without this the presentation editor's design diff --git a/README.md b/README.md index c33dd8b..7b04b89 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,12 @@ You can add custom fonts to the document server using the following occ commands - Add font by path `occ documentserver:fonts --add /usr/share/fonts/myfont.ttf` - List added fonts `occ documentserver:fonts` - Remove an added font `occ documentserver:fonts --remove myfont.ttf` +- Rebuild the font library `occ documentserver:fonts --rebuild` + +Each of these rebuilds the document server's font library, which needs the app +directory to be writable by the web server user. If it is not, the command says +so and exits non-zero — the bundled fonts keep working, but fonts you add will +not be picked up. ## Self signed certificates diff --git a/lib/Command/Fonts.php b/lib/Command/Fonts.php index 4eb7565..ed98582 100644 --- a/lib/Command/Fonts.php +++ b/lib/Command/Fonts.php @@ -77,8 +77,11 @@ protected function execute(InputInterface $input, OutputInterface $output) { } catch (\Exception $e) { $error = $e->getMessage(); $output->writeln("$error"); - return 0; + // These two were the wrong way round, so a rebuild that failed + // exited 0 and one that worked exited 1: anything scripting this + // was told the opposite of what happened. + return 1; } - return 1; + return 0; } } diff --git a/lib/Document/ConverterBinary.php b/lib/Document/ConverterBinary.php index a6cae54..9efe660 100644 --- a/lib/Document/ConverterBinary.php +++ b/lib/Document/ConverterBinary.php @@ -28,6 +28,9 @@ class ConverterBinary { public const BINARY_DIRECTORY = __DIR__ . '/../../3rdparty/onlyoffice/documentserver/server/FileConverter/bin'; + /** What x2t prints on stdout, and only on stdout, for a font it cannot open. */ + private const MISSING_FONT = "Can't load fontfile "; + private $logger; public function __construct(LoggerInterface $logger) { @@ -84,9 +87,42 @@ public function run(string $param, ?string $password = null): string { throw new DocumentConversionException("x2t exited with status $status"); } + $this->reportMissingFonts($output); + return $output; } + /** + * A font x2t could not load is a line on stdout and nothing else. + * + * It carries on and exits 0, and what comes out is a document with no + * glyphs in it - the whole of #371, #251 and #287, which went four years + * without a diagnostic because a blank PDF and a good one are the same + * successful conversion from here. Not fatal: one missing custom font + * should not fail a conversion that is otherwise fine. But it goes in the + * log, because nothing else in the system can tell the difference. + */ + private function reportMissingFonts(string $output): void { + if (strpos($output, self::MISSING_FONT) === false) { + return; + } + + $missing = []; + foreach (explode("\n", $output) as $line) { + $at = strpos($line, self::MISSING_FONT); + if ($at !== false) { + $missing[trim(substr($line, $at + strlen(self::MISSING_FONT)))] = true; + } + } + + $this->logger->error( + 'x2t could not load ' . count($missing) . ' font(s); anything set in them converts ' + . 'with no glyphs at all. Run `occ documentserver:fonts --rebuild`. Missing: ' + . implode(', ', array_slice(array_keys($missing), 0, 5)), + ['app' => 'documentserver_community'] + ); + } + public function test(): bool { try { $output = $this->run(''); diff --git a/lib/Document/FontManager.php b/lib/Document/FontManager.php index 265cfa4..265c9fe 100644 --- a/lib/Document/FontManager.php +++ b/lib/Document/FontManager.php @@ -56,9 +56,39 @@ public function rebuildFonts() { // Absolute paths throughout: since 9.x allfontsgen no longer // resolves relative arguments against the working directory, and - // with relative ones it just writes out an empty font list. - $binDir = ConverterBinary::BINARY_DIRECTORY; - $documentServer = $binDir . '/../../..'; + // with relative ones it just writes out an empty font list. And + // resolved rather than left with the '/../..' this file reaches + // them by, so that what ends up in the font lists, and in the + // errors below, is a path an admin can act on. + $binDir = realpath(ConverterBinary::BINARY_DIRECTORY) ?: ConverterBinary::BINARY_DIRECTORY; + $documentServer = realpath($binDir . '/../../..') ?: $binDir . '/../../..'; + + // Before anything runs: a rebuild that cannot write its output is + // the failure this whole method exists to make visible, and + // allfontsgen signals it by exiting 0 with nothing on either pipe. + // Saying so here names the file, which "no usable font list" below + // cannot. + $this->assertWritable([ + $binDir . '/AllFonts.js', + $binDir . '/font_selection.bin', + $documentServer . '/sdkjs/common/AllFonts.js', + $documentServer . '/fonts', + ]); + + // How a run that wrote nothing at all is told apart from one that + // rewrote both files with identical content, which is the ordinary + // case: allfontsgen rewrites them either way, so the mtime moves + // either way. Two readings because neither is enough on its own - + // a second rebuild inside the same second leaves the mtime where it + // was, and a filesystem clock that runs behind this one (NFS) never + // reaches $startedAt. + $startedAt = time(); + $before = []; + foreach ([$binDir . '/AllFonts.js', $binDir . '/font_selection.bin'] as $path) { + clearstatcache(true, $path); + $before[$path] = @filemtime($path); + } + $cmd = $binDir . '/../../tools/allfontsgen \ --input="' . $documentServer . '/core-fonts" \ --input="' . $fontsDir . '" \ @@ -95,26 +125,78 @@ public function rebuildFonts() { throw new \Exception($error); } - $this->finishFontList($binDir . '/AllFonts.js', $status, $output); + $this->finishFontList($binDir, $status, $output, $startedAt, $before); }); } /** - * Check what allfontsgen produced, then pin the bundled font paths to - * x2t's working directory (server/FileConverter/bin). + * Fail unless every path is writable, or can be created. + * + * @param string[] $paths + */ + private function assertWritable(array $paths): void { + foreach ($paths as $path) { + $target = file_exists($path) ? $path : dirname($path); + if (!is_writable($target)) { + $user = function_exists('posix_geteuid') + ? (posix_getpwuid(posix_geteuid())['name'] ?? (string)posix_geteuid()) + : 'the web server user'; + throw new \Exception( + "Cannot rebuild the font library: $target is not writable by $user. " + . "Custom fonts will not be picked up, and on an install made before the " + . "bundled font paths were pinned at build time, PDF export stays blank." + ); + } + } + } + + /** + * Check that this run of allfontsgen produced both font lists, then pin the + * bundled font paths in them to x2t's working directory + * (server/FileConverter/bin). * - * Two problems are handled here, both of which allfontsgen reports by - * exiting successfully with nothing on stderr. It writes no font list at - * all when it cannot create the file, and it writes an empty one when it - * cannot find the fonts. It also prefixes every path it does write with its - * own working directory, which stops resolving as soon as the app directory - * moves. In each case x2t ends up handing a null buffer to - * CFontFileLoader.LoadFontFromData, and any change replay that has to + * Everything allfontsgen gets wrong here it reports by exiting successfully + * with nothing on stderr: it writes no font list at all when it cannot + * create the file, an empty one when it cannot find the fonts, and it + * prefixes every path it does write with its own working directory, which + * stops resolving as soon as the app directory moves - or, for an app store + * install, the moment it leaves the machine that built it. + * + * Nothing downstream notices any of that. sdkjs hands the null buffer to + * CFontFileLoader.LoadFontFromData and any change replay that has to * measure text - saving a spreadsheet, for one - fails with "Cannot read - * property 'length' of null". Paths to custom fonts live outside the app - * directory and stay absolute. + * property 'length' of null"; x2t's PDF writer prints "Can't load fontfile" + * on a stream nobody reads and writes a PDF with no glyphs in it (#371, + * #251, #287). So the check has to happen here, and it has to be loud: the + * repair step turns it into a warning at `occ upgrade`, which is four years + * earlier than the alternative. + * + * Paths to custom fonts live outside the app directory and stay absolute. */ - private function finishFontList(string $allFontsJs, int $status, string $output): void { + private function finishFontList(string $binDir, int $status, string $output, int $startedAt, array $before): void { + $allFontsJs = $binDir . '/AllFonts.js'; + $selection = $binDir . '/font_selection.bin'; + + foreach ([$allFontsJs, $selection] as $path) { + clearstatcache(true, $path); + $mtime = @filemtime($path); + + if ($mtime === false || @filesize($path) === 0) { + throw new \Exception( + "allfontsgen wrote no $path (exit status $status) " . trim($output) + ); + } + // A stale file left over from the build satisfies every check on + // its contents, so the one thing worth knowing about it is that + // this run did not write it. + if ($mtime === $before[$path] && $mtime < $startedAt) { + throw new \Exception( + "allfontsgen left $path untouched (exit status $status), so the font " + . "paths in it are still the ones it was built with " . trim($output) + ); + } + } + $content = @file_get_contents($allFontsJs); if ($content === false || strpos($content, '/core-fonts/') === false) { throw new \Exception( @@ -122,10 +204,36 @@ private function finishFontList(string $allFontsJs, int $status, string $output) . trim($output) ); } + $this->write($allFontsJs, FontPaths::pinAllFontsJs($content)); + + $data = @file_get_contents($selection); + if ($data === false) { + throw new \Exception("Cannot read $selection"); + } + $pinned = FontPaths::pinFontSelection($data); + $this->write($selection, $pinned); + + // The pinning is the part with nothing else watching it: a path form + // the rewriter does not recognise would leave x2t loading no fonts + // exactly as before, and exiting 0 about it. + $paths = FontPaths::fontSelectionPaths($pinned); + if (!$paths) { + throw new \Exception( + "No bundled font paths in $selection (exit status $status) " . trim($output) + ); + } + foreach ($paths as $path) { + if (!is_file($binDir . '/' . $path)) { + throw new \Exception( + "Font path in $selection does not resolve from $binDir: $path" + ); + } + } + } - $pinned = preg_replace('|"[^"]*/core-fonts/|', '"../../../core-fonts/', $content); - if ($pinned !== null && $pinned !== $content) { - file_put_contents($allFontsJs, $pinned); + private function write(string $path, string $content): void { + if (file_put_contents($path, $content) === false) { + throw new \Exception("Cannot write $path"); } } diff --git a/lib/Document/FontPaths.php b/lib/Document/FontPaths.php new file mode 100644 index 0000000..81e03f1 --- /dev/null +++ b/lib/Document/FontPaths.php @@ -0,0 +1,164 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\DocumentServer\Document; + +/** + * Pins the font paths allfontsgen writes to x2t's working directory. + * + * allfontsgen prefixes every path it emits with its own working directory, so + * the two files it produces carry the absolute paths of whichever machine ran + * it. That is the build machine for an app store install, which is why the + * paths have to be rewritten: relative to server/FileConverter/bin, which is + * the working directory x2t, allfontsgen and allthemesgen are all given. + * + * Both consumers of those files load fonts by path and neither of them + * complains when the path does not resolve: + * + * - sdkjs reads AllFonts.js during change replay and hands the null buffer to + * CFontFileLoader.LoadFontFromData, so saving a spreadsheet dies with + * "Cannot read property 'length' of null". + * - x2t's PDF writer reads font_selection.bin, prints "Can't load fontfile" on + * stdout, writes a PDF with no glyphs in it and exits 0 (#371, #251, #287). + * + * Only the bundled fonts are pinned. Custom fonts live in appdata, outside the + * app directory, and keep the absolute paths they are found at. + * + * This class is used both at build time (tools/pin-font-paths.php) and at + * runtime (FontManager), so it must not depend on the server. + */ +class FontPaths { + /** + * Where core-fonts sits as seen from server/FileConverter/bin. + */ + public const BUNDLED_PREFIX = '../../../core-fonts/'; + + private const NEEDLE = '/core-fonts/'; + + /** + * Longest path we will believe a length prefix describes. Keeps a wild + * int32 read out of a string that happens to contain the needle from + * swallowing the rest of the file. + */ + private const MAX_PATH = 4096; + + /** + * AllFonts.js is JavaScript, so its paths are plain quoted strings. + */ + public static function pinAllFontsJs(string $content): string { + $pinned = preg_replace('|"[^"]*' . preg_quote(self::NEEDLE, '|') . '|', '"' . self::BUNDLED_PREFIX, $content); + return $pinned === null ? $content : $pinned; + } + + /** + * font_selection.bin is not sed-able: its strings are length-prefixed with + * a little-endian int32, so shortening one means rewriting its prefix. The + * two counts in the header count entries rather than bytes and no offset in + * the file is absolute, so the rest can be copied through untouched. + */ + public static function pinFontSelection(string $data): string { + $out = ''; + $pos = 0; + + foreach (self::eachBundledPath($data) as [$start, $length, $path]) { + $pinned = self::BUNDLED_PREFIX . substr($path, strpos($path, self::NEEDLE) + strlen(self::NEEDLE)); + $out .= substr($data, $pos, $start - 4 - $pos) . pack('V', strlen($pinned)) . $pinned; + $pos = $start + $length; + } + + return $out . substr($data, $pos); + } + + /** + * The bundled font paths font_selection.bin holds, in the order they appear. + * + * @return string[] + */ + public static function fontSelectionPaths(string $data): array { + $paths = []; + foreach (self::eachBundledPath($data) as [, , $path]) { + $paths[] = $path; + } + return $paths; + } + + /** + * @return \Generator offset, length, path + */ + private static function eachBundledPath(string $data): \Generator { + $pos = 0; + while (($needleAt = strpos($data, self::NEEDLE, $pos)) !== false) { + $start = self::stringStart($data, $needleAt, $pos); + if ($start === null) { + $pos = $needleAt + strlen(self::NEEDLE); + continue; + } + + $length = unpack('V', substr($data, $start - 4, 4))[1]; + yield [$start, $length, substr($data, $start, $length)]; + $pos = $start + $length; + } + } + + /** + * Walk back from the needle to the start of the string that contains it. + * + * The file gives us no index to look that up in, but the length prefix is a + * tight enough constraint on its own: the four bytes before any other + * offset inside the path are path characters, which read as an int32 far + * larger than any path. + * + * @param int $after offset before which nothing may be read, because it has + * already been copied to the output + */ + private static function stringStart(string $data, int $needleAt, int $after): ?int { + $floor = max($after + 4, $needleAt - self::MAX_PATH); + + for ($i = $needleAt; $i >= $floor; $i--) { + $length = unpack('V', substr($data, $i - 4, 4))[1]; + + // must reach past the needle, and stay inside the file + if ($length < $needleAt - $i + strlen(self::NEEDLE) || $length > self::MAX_PATH) { + continue; + } + if ($i + $length > strlen($data)) { + continue; + } + + $path = substr($data, $i, $length); + if (strpos($path, self::NEEDLE) === false) { + continue; + } + // a path holds no control bytes; the padding and the numeric + // fields around it are full of them, so this is what tells a real + // string apart from a length read out of the middle of one + if (preg_match('/[\x00-\x1f\x7f]/', $path)) { + continue; + } + + return $i; + } + + return null; + } +} diff --git a/lib/Migration/RebuildFonts.php b/lib/Migration/RebuildFonts.php index dec6933..1b4befc 100644 --- a/lib/Migration/RebuildFonts.php +++ b/lib/Migration/RebuildFonts.php @@ -52,7 +52,13 @@ public function run(IOutput $output) { $this->fontManager->rebuildFonts(); } catch (\Exception $e) { $this->logger->warning('An exception occurred trying to rebuild fonts', ['exception' => $e]); - $output->warning("Error while trying to rebuild fonts, if you had any custom fonts configured you'll need to run `occ documentserver:fonts --rebuild`"); + // The message, not just the fact: what goes wrong here is a font + // list that was not written, and the only useful thing anyone can + // be told is which file and why. Without it this reads as a + // formality and an admin has to go to the log to find out that + // their fonts are the ones the app was built with. + $output->warning('Error while trying to rebuild fonts: ' . $e->getMessage()); + $output->warning("Custom fonts will not be available until this is fixed and `occ documentserver:fonts --rebuild` succeeds"); } } } diff --git a/tests/Document/FontPathsTest.php b/tests/Document/FontPathsTest.php new file mode 100644 index 0000000..41c77ae --- /dev/null +++ b/tests/Document/FontPathsTest.php @@ -0,0 +1,152 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\DocumentServer\Tests\Document; + +use OCA\DocumentServer\Document\FontPaths; +use Test\TestCase; + +/** + * Rewriting the font paths allfontsgen leaves behind. + * + * font_selection.bin is where blank PDF export lived (#371, #251, #287), and it + * is not a text file: its strings are length-prefixed with a little-endian + * int32, so a path that is replaced without its prefix being updated corrupts + * everything after it - and x2t reports a corrupt font list the same way it + * reports a missing font, by exiting 0 having loaded none. There is nothing + * downstream to catch a mistake here, hence these. + */ +class FontPathsTest extends TestCase { + /** + * Where the first length prefix starts in the real file, and so in this + * stand-in for it: two counts, the length of the magic, the magic, and one + * more int32. + */ + private const HEADER = 21; + + /** + * A stand-in for the real thing: the header above, then length-prefixed + * strings with numeric fields between them. + */ + private function selection(array $strings): string { + $data = pack('V', count($strings)) . pack('V', 215) . pack('V', 5) . 'ASCW3' . pack('V', 0); + foreach ($strings as $string) { + $data .= pack('V', strlen($string)) . $string . pack('V', 0) . "\x00\x00"; + } + return $data; + } + + /** @return string[] */ + private function stringsOf(string $data, array $original): array { + // Read the file back the way it is written, to prove the prefixes still + // describe the strings that follow them. + $out = []; + $offset = self::HEADER; + for ($i = 0; $i < count($original); $i++) { + $length = unpack('V', substr($data, $offset, 4))[1]; + $out[] = substr($data, $offset + 4, $length); + $offset += 4 + $length + 6; + } + return $out; + } + + public function testPinsBundledPathsAndUpdatesTheirLengthPrefix(): void { + $strings = [ + '/build/machine/server/tools/../../core-fonts/ASC.ttf', + 'Liberation Serif', + '/build/machine/server/tools/../../core-fonts/liberation/LiberationSerif-Regular.ttf', + ]; + + $pinned = FontPaths::pinFontSelection($this->selection($strings)); + + $this->assertSame([ + '../../../core-fonts/ASC.ttf', + 'Liberation Serif', + '../../../core-fonts/liberation/LiberationSerif-Regular.ttf', + ], $this->stringsOf($pinned, $strings)); + } + + public function testLeavesCustomFontPathsAlone(): void { + // Custom fonts live in appdata, outside the app directory, so there is + // no relative path that reaches them. + $custom = '/var/www/html/data/appdata_abc/documentserver_community/fonts/Yrsa.ttf'; + $strings = ['/build/machine/server/tools/../../core-fonts/ASC.ttf', $custom]; + + $pinned = FontPaths::pinFontSelection($this->selection($strings)); + + $this->assertSame(['../../../core-fonts/ASC.ttf', $custom], + $this->stringsOf($pinned, $strings)); + } + + public function testIsIdempotent(): void { + $data = $this->selection(['/build/machine/server/tools/../../core-fonts/ASC.ttf']); + + $once = FontPaths::pinFontSelection($data); + $this->assertSame($once, FontPaths::pinFontSelection($once)); + } + + public function testFindsEveryBundledPath(): void { + $data = $this->selection([ + '/build/machine/server/tools/../../core-fonts/ASC.ttf', + 'not a path at all', + '/build/machine/server/tools/../../core-fonts/liberation/LiberationSans-Bold.ttf', + '/var/lib/appdata/fonts/Custom.ttf', + ]); + + $this->assertSame([ + '/build/machine/server/tools/../../core-fonts/ASC.ttf', + '/build/machine/server/tools/../../core-fonts/liberation/LiberationSans-Bold.ttf', + ], FontPaths::fontSelectionPaths($data)); + } + + public function testLeavesEverythingElseInTheFileWhereItWas(): void { + $strings = ['/b/server/tools/../../core-fonts/ASC.ttf']; + $data = $this->selection($strings); + + $pinned = FontPaths::pinFontSelection($data); + + // The header counts entries, not bytes, so it is copied through; and + // the file shrinks by exactly what came off the front of the path. + $this->assertSame(substr($data, 0, self::HEADER), substr($pinned, 0, self::HEADER)); + $this->assertSame( + strlen($data) - (strlen($strings[0]) - strlen('../../../core-fonts/ASC.ttf')), + strlen($pinned) + ); + } + + public function testPinsAllFontsJs(): void { + $content = '["/build/machine/core-fonts/ASC.ttf","/data/appdata/fonts/Custom.ttf"]'; + + $this->assertSame( + '["../../../core-fonts/ASC.ttf","/data/appdata/fonts/Custom.ttf"]', + FontPaths::pinAllFontsJs($content) + ); + } + + public function testHandlesAFileWithNoBundledPaths(): void { + $data = $this->selection(['/var/lib/appdata/fonts/Custom.ttf']); + + $this->assertSame($data, FontPaths::pinFontSelection($data)); + $this->assertSame([], FontPaths::fontSelectionPaths($data)); + } +} diff --git a/tests/rig/README.md b/tests/rig/README.md index 682f677..9ddcee6 100644 --- a/tests/rig/README.md +++ b/tests/rig/README.md @@ -14,8 +14,8 @@ never settles for "the request returned 200". ## Running it -Needs docker, python3 with `websockets` and `requests`, chromium, and the -document server tree in place: +Needs docker, python3 with `websockets` and `requests`, chromium, `pdftotext` +(`poppler-utils`, for the PDF test), and the document server tree in place: make # in the app directory, once: downloads the document server pip install websockets requests @@ -47,6 +47,7 @@ nothing about a particular instance is hardcoded in a test. | `coedit` | two users in one document each see what the other types, in a text document and in a spreadsheet, with one socket session per browser — a second one means the transport reconnected mid-edit | | `forcesave` | the Save button the "Keep intermediate versions when editing" setting puts in the editor: the two-part reply the editor accepts, the text in the file with no cron and no flush, and a session opened afterwards still seeing everything — the button must snapshot, not flush | | `chat` | the chat panel carries messages both ways, and a late joiner reads the backlog | +| `pdf` | a document converted to PDF has its text in it. The bundled font paths are stored in `font_selection.bin` as absolute paths of whichever machine ran `allfontsgen` — the build machine, for an app store install — and when one does not resolve x2t writes a PDF with no glyphs and exits 0, which is why exporting to PDF was blank for four years (#371, #251, #287). Also checks the two things that kept it silent: a font x2t cannot load now reaches the log, and a font rebuild that cannot write its output fails instead of printing `rebuilding` and exiting 0 | ## Things that will mislead you diff --git a/tests/rig/lib/config.py b/tests/rig/lib/config.py index 43410d1..28a1093 100644 --- a/tests/rig/lib/config.py +++ b/tests/rig/lib/config.py @@ -47,6 +47,12 @@ def _env(name, default): CHROMIUM = _env('RIG_CHROMIUM', '/usr/bin/chromium') +# The working copy the rig mounts, and where x2t's files are inside it. The PDF +# test reads and writes the document server tree directly, because the font list +# it covers is a file in the tree rather than anything the app stores. +APP_SOURCE = _env('APP_SOURCE', os.path.dirname(os.path.dirname(HERE))) +CONVERTER_BIN = '3rdparty/onlyoffice/documentserver/server/FileConverter/bin' + # The documents provisioning uploaded, and where the text typed into each of # them ends up inside the saved file. DOCUMENTS = { diff --git a/tests/rig/lib/harness.py b/tests/rig/lib/harness.py index 7c54281..0131102 100644 --- a/tests/rig/lib/harness.py +++ b/tests/rig/lib/harness.py @@ -266,3 +266,35 @@ def app_log_errors(): "grep -o '\"level\":[34][^\\n]*documentserver[^\\n]*' " "/var/www/html/data/nextcloud.log | tail -20 || true") return [line for line in out.split('\n') if line.strip()] + + +# Booting Nextcloud the way occ does, so a test can call app classes with the +# real container behind them. The rig's image ships no tests/ and no phpunit, +# and there is no occ command that converts a document, so this is how the +# converter gets driven from outside a browser. +NC_EVAL = r"""get(\OCP\ISession::class)->close(); +require $argv[1]; +""" + + +def php_eval(source): + """Run PHP inside the container with the server booted. Returns its output.""" + with tempfile.TemporaryDirectory() as tmp: + runner, script = os.path.join(tmp, 'nc-eval.php'), os.path.join(tmp, 'script.php') + with open(runner, 'w') as f: + f.write(NC_EVAL) + with open(script, 'w') as f: + f.write(source) + for path in (runner, script): + os.chmod(path, 0o644) + subprocess.run(['docker', 'cp', path, f'{config.APP_CONTAINER}:/tmp/'], + capture_output=True) + r = subprocess.run( + ['docker', 'exec', '-u', 'www-data', '-w', '/var/www/html', config.APP_CONTAINER, + 'php', '/tmp/nc-eval.php', '/tmp/script.php'], + capture_output=True, text=True) + return (r.stdout + r.stderr).strip() diff --git a/tests/rig/pdf_test.py b/tests/rig/pdf_test.py new file mode 100755 index 0000000..cbaccb9 --- /dev/null +++ b/tests/rig/pdf_test.py @@ -0,0 +1,236 @@ +#!/usr/bin/env python3 +"""Converting to PDF puts the text in the PDF, and a font rebuild that did not +rebuild says so. + +Exporting to PDF was blank for four years (#371, #251, #287) because +`font_selection.bin` holds the absolute path of every bundled font as they were +on the machine that ran allfontsgen - the build machine, for an app store +install. When a path does not resolve x2t prints "Can't load fontfile" on a +stream nobody reads, writes a PDF with no glyphs in it, and exits 0. From +everywhere else in the system that is a successful conversion, which is why +nothing caught it: same exit code, same absence of errors, 1.9 KB instead of +12.7 KB. + +So the assertions here are the ones nothing else makes: + +- a converted PDF contains the text that went in, and embeds a font +- a font whose path does not resolve reaches the log instead of nowhere +- a rebuild that cannot write its output fails loudly, rather than printing + "rebuilding" and exiting 0 having changed nothing - the shape of #145 and + #278, and of the rig's own bind mount +""" +import argparse +import hashlib +import os +import shutil +import subprocess +import sys +import tempfile +import zipfile + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness + +MARKER = 'PdfFontProbe' +# What ConverterBinary looks for on x2t's stdout, and the start of what it logs. +MISSING_FONT_LOG = 'could not load' + +SELECTION = os.path.join(config.APP_SOURCE, config.CONVERTER_BIN, 'font_selection.bin') +WORK = '/tmp/pdfrig' + + +def make_docx(path, marker): + """A document with real typed text in it, in a bundled font.""" + content_types = ( + '' + '' + '' + '' + '') + rels = ( + '' + '' + '') + document = ( + '' + '' + '' + f'{marker}') + + with zipfile.ZipFile(path, 'w', zipfile.ZIP_DEFLATED) as z: + z.writestr('[Content_Types].xml', content_types) + z.writestr('_rels/.rels', rels) + z.writestr('word/document.xml', document) + + +def convert_to_pdf(marker): + """Convert a docx to PDF through the app's own converter, and bring it back. + + Through DocumentConverter rather than by calling x2t directly: the point is + that what the app does with a document produces a readable PDF, and it is + also what puts a missing font in the log. + """ + local = os.path.join(tempfile.gettempdir(), f'rig-{marker}.docx') + make_docx(local, marker) + + harness.app_exec(f'mkdir -p {WORK} && chmod 777 {WORK}') + subprocess.run(['docker', 'cp', local, f'{config.APP_CONTAINER}:{WORK}/in.docx'], + capture_output=True) + harness.app_exec(f'rm -f {WORK}/out.pdf') + + out = harness.php_eval(f"""get(\\OCA\\DocumentServer\\DocumentConverter::class); +try {{ + $converter->convertFiles('{WORK}/in.docx', '{WORK}/out.pdf', + DocumentFormat::AVS_OFFICESTUDIO_FILE_CROSSPLATFORM_PDF); + echo "converted\\n"; +}} catch (\\Throwable $e) {{ + echo "FAILED: ", $e->getMessage(), "\\n"; +}} +""") + + pdf = os.path.join(tempfile.gettempdir(), f'rig-{marker}.pdf') + with open(pdf, 'wb') as f: + r = subprocess.run(['docker', 'exec', config.APP_CONTAINER, 'cat', f'{WORK}/out.pdf'], + stdout=f, stderr=subprocess.DEVNULL) + return out, (pdf if r.returncode == 0 and os.path.getsize(pdf) else None) + + +def pdf_text(path): + r = subprocess.run(['pdftotext', path, '-'], capture_output=True, text=True) + return r.stdout + + +def pdf_embeds_a_font(path): + """Whether the PDF carries a font program at all. + + The backstop for the text check: a PDF x2t wrote with no fonts loaded has + no /FontFile2 in it, and this needs nothing installed to see that. + """ + with open(path, 'rb') as f: + return b'/FontFile2' in f.read() + + +def check_conversion(): + print('== a converted PDF has the text in it') + before = harness.app_log(MISSING_FONT_LOG) + out, pdf = convert_to_pdf(MARKER) + + if pdf is None: + print(f' FAIL - no PDF came out: {out}') + return False + + ok = True + size = os.path.getsize(pdf) + text = pdf_text(pdf) + if MARKER not in text: + ok = False + print(f' FAIL - "{MARKER}" is not in the {size} byte PDF; ' + f'extracted {len(text.split())} word(s)') + else: + print(f' ok - "{MARKER}" is in the {size} byte PDF') + + if not pdf_embeds_a_font(pdf): + ok = False + print(' FAIL - the PDF embeds no font program, so x2t loaded no fonts') + + after = harness.app_log(MISSING_FONT_LOG) + if after > before: + ok = False + print(f' FAIL - x2t reported {after - before} font(s) it could not load') + + return ok + + +def check_missing_font_is_reported(): + """The other half: when a path really does not resolve, somebody says so. + + Without this the log line added for it is untested, and a blank PDF stays + indistinguishable from a good one. + """ + print('== a font path that does not resolve reaches the log') + original = open(SELECTION, 'rb').read() + digest = hashlib.sha256(original).hexdigest() + + try: + # Same length, so every length prefix in the file stays correct - only + # the path stops existing. + broken = original.replace(b'../../../core-fonts/', b'../../../core-f0nts/') + if broken == original: + print(' FAIL - no pinned font paths in font_selection.bin to break') + return False + with open(SELECTION, 'wb') as f: + f.write(broken) + + before = harness.app_log(MISSING_FONT_LOG) + _, pdf = convert_to_pdf(MARKER + 'Broken') + after = harness.app_log(MISSING_FONT_LOG) + + if after <= before: + print(' FAIL - x2t loaded no fonts and nothing was logged') + return False + if pdf and MARKER in pdf_text(pdf): + print(' FAIL - the text survived a font that cannot be loaded, ' + 'so this check proves nothing') + return False + print(' ok - logged, and the PDF came out blank as expected') + return True + finally: + with open(SELECTION, 'wb') as f: + f.write(original) + restored = hashlib.sha256(open(SELECTION, 'rb').read()).hexdigest() + if restored != digest: + print(f' !! font_selection.bin was NOT restored ({restored} != {digest})') + + +def check_rebuild_that_cannot_write_fails(): + print('== a font rebuild that cannot write says so') + mode = os.stat(SELECTION).st_mode + try: + os.chmod(SELECTION, 0o444) + r = subprocess.run( + ['docker', 'exec', '-u', 'www-data', config.APP_CONTAINER, + 'php', 'occ', 'documentserver:fonts', '--rebuild'], + capture_output=True, text=True) + out = (r.stdout + r.stderr).strip() + + if r.returncode == 0: + print(f' FAIL - exited 0 with the font list read-only: {out}') + return False + if 'not writable' not in out: + print(f' FAIL - exited {r.returncode} but did not say what was wrong: {out}') + return False + print(f' ok - exit {r.returncode}, and it named the file') + return True + finally: + os.chmod(SELECTION, mode) + + +def main(): + p = argparse.ArgumentParser() + p.parse_args() + + if not shutil.which('pdftotext'): + print('pdftotext is needed to read the converted PDF (apt install poppler-utils)') + return 2 + if not os.path.isfile(SELECTION): + print(f'{SELECTION} is missing: run make in the app directory first') + return 2 + + results = { + 'conversion': check_conversion(), + 'reported': check_missing_font_is_reported(), + 'rebuild': check_rebuild_that_cannot_write_fails(), + } + harness.app_exec(f'rm -rf {WORK}') + + print('\n VERDICT:', ' '.join(f'{k}={"ok" if v else "FAIL"}' for k, v in results.items())) + return 0 if all(results.values()) else 1 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/tests/rig/run_all.py b/tests/rig/run_all.py index 846bea2..c9464ed 100755 --- a/tests/rig/run_all.py +++ b/tests/rig/run_all.py @@ -41,6 +41,9 @@ "the editor's Save button writes the file without ending the session"), ('chat', 'chat_test.py', [], 'the chat panel, including a late joiner reading the backlog'), + ('pdf', 'pdf_test.py', [], + 'a converted PDF has the text in it, a font that cannot be loaded is ' + 'logged, and a rebuild that cannot write fails loudly'), ] diff --git a/tools/pin-font-paths.php b/tools/pin-font-paths.php new file mode 100644 index 0000000..adf6842 --- /dev/null +++ b/tools/pin-font-paths.php @@ -0,0 +1,81 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +/** + * Build-time half of FontPaths: rewrites the paths allfontsgen just wrote into + * server/FileConverter/bin so they resolve wherever the app is installed, + * rather than only on the machine that built it. + * + * Usage: php tools/pin-font-paths.php + * + * Not shipped - the Makefile runs it, and FontManager does the same thing on + * an install that rebuilds its fonts. + */ + +require __DIR__ . '/../lib/Document/FontPaths.php'; + +use OCA\DocumentServer\Document\FontPaths; + +$binDir = $argv[1] ?? null; +if ($binDir === null || !is_dir($binDir)) { + fwrite(STDERR, "usage: php tools/pin-font-paths.php \n"); + exit(1); +} + +$files = [ + 'AllFonts.js' => [FontPaths::class, 'pinAllFontsJs'], + 'font_selection.bin' => [FontPaths::class, 'pinFontSelection'], +]; + +foreach ($files as $name => $pin) { + $path = $binDir . '/' . $name; + + $content = @file_get_contents($path); + if ($content === false || $content === '') { + fwrite(STDERR, "!! allfontsgen produced no $path; refusing to ship a build that cannot load fonts\n"); + exit(1); + } + + $pinned = $pin($content); + if (file_put_contents($path, $pinned) === false) { + fwrite(STDERR, "!! cannot write $path\n"); + exit(1); + } + + echo "pinned $name (" . strlen($content) . " -> " . strlen($pinned) . " bytes)\n"; +} + +// Both files name the same fonts, so one resolving and the other not means the +// rewrite missed a form of path rather than that a font is missing. +$paths = FontPaths::fontSelectionPaths((string)file_get_contents($binDir . '/font_selection.bin')); +if (!$paths) { + fwrite(STDERR, "!! no bundled font paths in font_selection.bin\n"); + exit(1); +} +foreach ($paths as $path) { + if (!is_file($binDir . '/' . $path)) { + fwrite(STDERR, "!! pinned font path does not resolve from $binDir: $path\n"); + exit(1); + } +} +echo "checked " . count($paths) . " bundled font paths resolve from the converter directory\n";