From bc07ff6e4672ad1c1ce2781fb82ebb6d5e05b10c Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Tue, 15 Sep 2026 08:24:47 -0300 Subject: [PATCH] Pin the bundled font paths so PDF export is not blank (#371, #251, #287) font_selection.bin stores an absolute path to every bundled font, written by whichever machine ran allfontsgen - the build machine, for an app store install. When one of those paths does not resolve, x2t's PDF writer prints "Can't load fontfile" on stdout, writes a PDF with no glyphs in it, and exits 0. Nothing downstream can tell that from a good conversion, which is why exporting to PDF has been blank since 2022 with no diagnostic anywhere. The build already pinned AllFonts.js to paths relative to x2t's working directory and left font_selection.bin alone, on the belief - written into the Makefile - that its paths were not used for loading. They are, by the PDF writer. x2t accepts relative paths in that file, so it is now pinned the same way: same input, same binary, 1 937 bytes with no text before, 12 804 bytes with the text after. font_selection.bin is not sed-able - its strings are length-prefixed with a little-endian int32 - so the rewriter lives in FontPaths, shared between tools/pin-font-paths.php at build time and FontManager at runtime, and only touches paths under core-fonts. Custom fonts live in appdata, outside the app directory, and keep the absolute paths they are found at. The rest is the silence around it: - finishFontList() checked only that some AllFonts.js existed containing "/core-fonts/", which a stale file from the build satisfies, so a rebuild that wrote nothing at all reported success. It now checks that this run wrote both files, that file_put_contents succeeded, and that every pinned path resolves - and refuses up front, naming the file and the user, when it cannot write. This is the shape of #145 and #278. - ConverterBinary logs the fonts x2t says it could not load, rather than discarding the one stream that mentions them. - documentserver:fonts returned 1 on success and 0 on failure, so anything scripting it was told the opposite of what happened. - The upgrade repair step puts the actual error in its warning. tests/rig/pdf_test.py covers all three: a converted PDF has the text in it, a font that cannot be loaded reaches the log, and a rebuild that cannot write fails loudly. FontPathsTest covers the binary rewriter. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Fabrice Meyer --- .github/workflows/integration.yml | 2 + .github/workflows/lint.yml | 2 +- .nextcloudignore | 1 + Makefile | 25 +++- README.md | 6 + lib/Command/Fonts.php | 7 +- lib/Document/ConverterBinary.php | 36 +++++ lib/Document/FontManager.php | 146 +++++++++++++++--- lib/Document/FontPaths.php | 164 +++++++++++++++++++++ lib/Migration/RebuildFonts.php | 8 +- tests/Document/FontPathsTest.php | 152 +++++++++++++++++++ tests/rig/README.md | 5 +- tests/rig/lib/config.py | 6 + tests/rig/lib/harness.py | 32 ++++ tests/rig/pdf_test.py | 236 ++++++++++++++++++++++++++++++ tests/rig/run_all.py | 3 + tools/pin-font-paths.php | 81 ++++++++++ 17 files changed, 880 insertions(+), 32 deletions(-) create mode 100644 lib/Document/FontPaths.php create mode 100644 tests/Document/FontPathsTest.php create mode 100755 tests/rig/pdf_test.py create mode 100644 tools/pin-font-paths.php 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";