feat: add support for font management

[DmySyz]

Added routing for the admin panel.

Details

REST API mounted at /admin/api/v1/fonts:

GET @ / - List installed custom fonts
POST @ / - Upload a font (application/octet-stream, X-Font-Name header)
DELETE @ /:name - Delete a font by filename
POST @ /regenerate - Start async font cache regeneration

Files

AdminPanel/server/sources/routes/fonts/router.js - new file - the fonts router
AdminPanel/server/sources/server.js - mounts the fonts router at /admin/api/v1/fonts

Notes

Euro-Office/document-server-package#14 is to be merged first.

Assisted-by: Claude Code (Opus-4.8)

[moodyjmz]

TL;DR

The CRUD parts (list/upload/delete fonts) are solid — proper magic-byte validation, extension whitelist, symlink-safe listing, and filename sanitisation that genuinely blocks path traversal. That part's fine.

The /regenerate endpoint — the actually novel part of this PR — is broken end to end, verified empirically against the real sudoers/supervisor/nginx contracts in document-server-package / Docker-DocumentServer / DocumentServer, not just read from the diff. There's also a real security gap in the bearer-auth path. Details below.

Findings, ranked

1. Blocker — the service-restart step can never succeed. router.js runs spawn('supervisorctl', ['restart', 'docservice', 'converter'], ...) as the ds user (per the AdminPanel supervisor config). Supervisord's control socket ships chmod=0700, root-owned, and nothing in the org loosens that. Tested empirically: ds gets PermissionError: [Errno 13] Permission denied from supervisorctl every time, regardless of process naming (bare vs. ds:-grouped — bare is actually correct for the current standalone build, that's not the bug). /regenerate will always land on status: 'error', "Service restart failed (supervisorctl exit 1)".

2. Blocker — the upstream failure detection is dead code, so "done" can't be trusted either. documentserver-generate-allfonts.sh.m4 has no set -e/|| guards; its last statement is an if gated on $1 != "true". Since the router passes 'true', that branch is false with no else — the script exits 0 unconditionally on that path. allfontsgen/allthemesgen/x2t -create-js-cache can all fail internally and it still reports success. So real generation failures get reported as done, and the (also-broken) restart step then reports a spurious error on top of that. The status machine can't be trusted in either direction.

3. High — sudo /usr/sbin/nginx -s reload is both unauthorized and functionally inert. The sudoers grant (document-server-package:common/documentserver/sudoers/documentserver) only covers 3 named documentserver-*.sh scripts, not nginx. Tested: ds$ sudo nginx -s reload fails immediately — sudo: a password is required. Even authorized, it wouldn't help: font assets are served Cache-Control: public, max-age=31536000, immutable, invalidated only by documentserver-flush-cache.sh rotating a $cache_tag — which is in the sudoers allowlist and was bypassed in favour of a command that can't run and wouldn't do anything for this cache anyway.

4. High — security: bearer-auth reuses the shared integration JWT secret with no tenant scoping, and has no producer or consumer anywhere in the ecosystem. Verifies bearer tokens against services.CoAuthoring.secret.browser.string — the same secret handed to every WOPI/Nextcloud integrator. Anyone holding that integration secret can mint {sub:'font-api'} and get font-directory write plus service-restart triggering. It also skips the tenant-aware path every other Browser-secret consumer uses (tenantManager.getTenantSecret), setting req.ctx = operationContext.global with no ctx.init(tenant)/initTenantCache() — in multitenant mode, one global secret governs a font directory that isn't tenant-partitioned at all. jwt.verify doesn't require exp either, so an exp-less token is eternal. And nothing in the org currently issues or consumes a sub:'font-api' token — this branch is speculative attack surface with no legitimate caller yet.

5. Medium — missing sudo, works today only by accident. documentserver-generate-allfonts.sh is invoked directly rather than via the sudoers grant that exists specifically for it to run as root. Its chown -R ds:ds step happens to no-op today only because the tree is already ds:ds-owned — an unstated invariant, not a guarantee.

6. Medium — EO_ROOT override desyncs upload target from scan source. The router resolves the fonts dir from EO_ROOT at runtime; the script it spawns hardcodes its own path at package-build time via m4. Defaults agree today, but overriding the documented EO_ROOT env var at runtime sends uploads somewhere the regen script will never read. The LD_LIBRARY_PATH override in the spawn env is also dead weight — the script exports its own.

7. Low — the route-ordering comment is fabricated reasoning. "Registered BEFORE DELETE /:name so Express doesn't treat 'regenerate' as a name param" — verified empirically with both orderings: identical behaviour, because DELETE is a different verb and can never shadow a POST/GET handler. Harmless, but confidently-wrong — worth deleting rather than leaving to mislead the next reader.

8. Low — cleanup. OUTPUT_CAP = 65536 is pointless (only output.slice(0, 500) is ever read), it's a soft cap that can overshoot by a chunk, and per-chunk .toString() can mangle multi-byte UTF-8 across chunk boundaries. Upload limit '20mb' is hardcoded instead of pulled from services.CoAuthoring.server.limits_tempfile_upload like the sibling config router does. err.message (can contain filesystem paths) is leaked in 500 responses. Wrong Content-Type on upload silently yields req.body = {} and a misleading "Empty or missing font body" error.

Already fine — not re-raising: path traversal is genuinely blocked (path.basename + character whitelist + magic-byte/extension cross-check); CSRF on the cookie path is covered by the existing sameSite: 'strict' cookie config; the EO_ROOT default value is correct; passing 'true' does correctly skip the script's internal restart branch (that reasoning is right, just moot given #1–#3).

Bottom line: the CRUD half is solid. The regeneration half guessed at infrastructure behaviour (privilege model, process management, cache invalidation) instead of checking the sudoers file, supervisor config, and nginx cache setup that actually define it — and got each one wrong in a different way. Please rework that part against document-server-package/DocumentServer directly, and hold off on the bearer-token branch until there's an actual caller and a tenant-scoped secret for it.

[moodyjmz]

Re-review of the force-push (db1c9d9 → 5771075)

Re-verified finding by finding against the packaging repos, not just the diff. Short version: the regenerate pipeline is now architecturally correct — generate via the sudoers grant, restart via the new documentserver-restart.sh, cache flush via documentserver-flush-cache.sh — and most of the small stuff is cleaned up properly. Two things gate the merge: this now hard-depends on Euro-Office/document-server-package#14 shipping first, and #39 is still an open competing implementation of the same API.

Finding-by-finding status

1. Restart step — fixed, with a shipping dependency. The supervisorctl-as-ds call is replaced by sudo /usr/bin/documentserver-restart.sh. Verified document-server-package#14: bare docservice/converter names match the standalone image's supervisor config ([program:docservice]/[program:converter]), ds-docservice/ds-converter matches the deb systemd units, and the sudoers entry is fixed-argv with no glob — exactly the hardening shape the existing entries should also have. Both package manifests pick the script up by wildcard (deb package.install: bin/*.sh; rpm spec: documentserver-*.sh), so #14 is complete as filed. But the router comment "scripts are in the ds sudoers allowlist" is only true after #14 merges and a rebuilt package/image ships. On any deployment running the current package, the restart step dies on sudo: a password is required — reported cleanly as status: 'error' now, but the feature is dead. Merge order: #14 → package/image rebuild → this PR.

2. Upstream failure detection — documented, not fixed. The status comment now says plainly that 'done' only means the script exited 0 and internal allfontsgen/allthemesgen/x2t failures aren't surfaced. That's an honest limitation statement rather than a silent lie, which is the part that mattered. Fine as a deliberate call; just don't build UI copy that promises more than "script finished".

3. nginx reload — fixed properly. documentserver-flush-cache.sh via the sudoers grant: rotates $cache_tag and reloads nginx, which is the actual invalidation mechanism for the year-cached immutable assets. Correct sequence too (generate → restart services → flush).

4. Bearer auth — partially fixed. A finite exp is now required (kills eternal tokens), and the caller now exists: eurooffice-nextcloud#142 mints sub: 'font-api' tokens with a 60 s TTL against this exact contract. The shared browser secret and missing tenant scoping remain, now documented as "single-tenant deployments only" — but nothing enforces that. Cheap improvement: reject the bearer path when tenantManager.isMultitenantMode(ctx) is true, so the documented limitation is a guard rather than a comment.

5. Missing sudo — fixed. Now invoked through the grant. (This does mean uploaded-font parsing runs as root again — same as the documented manual procedure, so not a regression introduced here, but it stays on the radar.)

6. EO_ROOT / LD_LIBRARY_PATH — fixed. Dead env override removed; each script resolves its own paths.

7. Fabricated route-ordering comments — removed.

8. Cleanup — mostly fixed. Output capping is now actually correct (buffer accumulation, single UTF-8 decode, real 4 KB cap), and err.message no longer leaks into any 500 response. Still open, all minor: '20mb' remains hardcoded instead of services.CoAuthoring.server.limits_tempfile_upload (the config router pulls it from there); a wrong Content-Type on upload still yields the misleading "Empty or missing font body"; and the three-step pipeline has no timeout, so a hung script leaves status: 'running' forever and 409-blocks every future regen until the AdminPanel restarts.

Not previously raised, worth a check before merge:

• feat: Add AdminPanel API endpoints for adding and deleting custom fonts. #39 duplication is still unresolved. Open since 11 July, same endpoints, incompatible API shape (multipart upload, .ttc instead of .woff/.woff2, /regenerate/status, cookie-only auth). eurooffice-nextcloud#142 is coded against this PR's shape. One of the two server implementations has to be closed — decide which before merging either.
• WOFF/WOFF2 support is unverified. This PR accepts .woff/.woff2; feat: Add AdminPanel API endpoints for adding and deleting custom fonts. #39 deliberately restricts to ttf/ttc/otf. WOFF2 in particular needs FreeType built with brotli. If the shipped allfontsgen toolchain skips them, uploads "succeed" but the font never appears in the editor. One empirical upload of each format settles it.

Bottom line: with #14 merged and shipped first, the regeneration design is sound. Remaining asks before merge: resolve the #39/#41 duplication, and preferably turn the single-tenant comment into an actual multitenant guard on the bearer path.

[moodyjmz]

Addendum — findings from the adversarial verification pass

An independent cold review of the current head completed after the re-review above; it confirmed all of its conclusions and surfaced the following new items, each re-verified against source before posting. None block the architecture — the merge-order and #39 asks above stand unchanged — but several are worth fixing while the file is open.

New findings, ranked

1. The #14 dependency fails half-applied, not cleanly. Sharpening the point above: when regenerate runs against a package without #14, step 1 (sudo generate-allfonts.sh) succeeds as root before step 2 is denied. Result: new fonts are baked into AllFonts.js on disk, but docservice/converter hold the old font list and every browser keeps the year-cached assets — status: 'error' with no rollback and a half-applied state. Same end-state if any later step fails. Worth stating in the PR description that #14 must ship first.

2. Upload write follows symlinks, isn't atomic, and silently overwrites. fs.writeFileSync(dest, buf) with default flags: a pre-planted symlink in custom-fonts/ gets followed and its target clobbered — while the list handler deliberately lstats symlinks out of the listing, so the planted link is invisible in the UI. The directory shares a volume and owner with Data/.private (secure-link secret) and the WOPI keys. Needs local write access to plant, so defence-in-depth rather than a remote primitive — but the fix is one line: write to a temp name and rename(), or open with 'wx' + O_NOFOLLOW. That also fixes the other two problems for free: a crash/ENOSPC mid-write currently leaves a truncated "valid" font, and re-uploading an existing name silently destroys the old file with a 201.

3. Non-ASCII font names are mangled into colliding underscores. Node decodes header bytes as latin1, so Ärial.ttf arrives as two latin1 chars per UTF-8 byte and the sanitiser turns both Ärial.ttf and Örial.ttf into __rial.ttf — the second upload silently destroys the first (see 2). There is no way for a client to express a non-ASCII filename at all, and for a font-management feature, accented/Cyrillic/CJK names are the normal case, not the edge. Percent-encode the header value (decode server-side) and reject collisions instead of mangling. Related: duplicate X-Font-Name headers are joined by Node into "a.ttf, b.otf", which sanitises to an accepted garbage filename — take the first value or reject on comma.

4. Oversized uploads return 500, not 413. body-parser throws with status: 413, but the global error handler (server.js, res.sendStatus(500)) ignores err.status. The client can't distinguish "font too big" from "server broken". Either map err.status in the global handler or catch it in the route.

5. 'done' can mean "nothing restarted" — fix belongs in #14. The new restart script is if pgrep systemd … elif pgrep supervisord … fi with no else: if neither matches, it falls through and exits 0, this router treats that as success, flush-cache runs, and status reports done while the services still hold the old fonts — exactly the silent-success class the status comment warns about. #14 should exit non-zero when it finds no init to talk to.

6. Listing 500s on a concurrent delete. The filter guards lstatSync with try/catch, but the statSync in the map below is bare — a DELETE landing between readdir and stat throws ENOENT and the outer catch turns one missing file into a 500 for the whole list. Reuse the lstat result (or withFileTypes). Same class in DELETE itself: existsSync then unlinkSync → concurrent delete → 500 instead of 404; just unlink and map ENOENT → 404.

7. Output capping applied to one child, forgotten for two. proc output is capped at OUTPUT_CAP; restartChunks and flushChunks accumulate unbounded. Small scripts today, but the cap was evidently deliberate — apply it to all three.

8. Bearer secret doesn't follow runtime config rotation. cfgDsJwtSecret is read once at module load, while the wopi router reads secrets per-request via ctx.getCfg(path, fallback) precisely so PATCH /admin/api/v1/config takes effect live. Rotate the browser secret and this router keeps accepting tokens signed with the old one until the AdminPanel restarts. (The bearer path has no tenant ctx to call getCfg on — which is itself another argument that the shared-secret design wants a dedicated, properly-plumbed secret.)

9. Hardening: no Origin check anywhere on the API. server.js mounts nothing between the listener and these routers — no CSRF token, no Origin/Referer validation. The only guard on POST /regenerate (a CORS-simple request that restarts the document service and drops every editing session) is sameSite: 'strict' on the cookie — which is site-scoped, not origin-scoped, so anything served on the same registrable domain (including DS-served content) is inside the boundary. An Origin allowlist on the state-changing routes is cheap. Related, pre-existing: dev-mode CORS (devProxy.getDevCors()) reflects any origin with credentials and explicitly allowlists Authorization, making the bearer path cross-origin reachable in development builds — this PR is the first filesystem-mutating surface behind it.

10. Small stuff. ensureFontsDir's mkdirSync(recursive: true) will happily recreate the whole Data chain if the volume didn't mount, sending uploads to container-local storage that evaporates on restart — check the parent exists and fail loudly. The deb control doesn't declare sudo in Depends (images install it explicitly; bare-metal deb installs may not have it). No tests accompany the router (#39 has them).

[DmySyz]

WOFF/WOFF2 are supported by allfontsgen

[moodyjmz]

TL;DR: Prior review's asks (atomicity, non-ASCII filenames, CSRF/Origin, output capping, timeouts, race conditions) are genuinely fixed — nice work. Two new findings before merge, both in explanatory comments rather than logic, plus one still-open cross-PR item:

1. The multitenant bearer-block's advice to "use the cookie path instead" doesn't hold up — traced it, the cookie path gives no tenant isolation at all.
2. The "secret is read per-request, rotation needs no restart" comment is false — config is frozen at process start.
3. server#39 is still open, untouched, zero comments — duplicates this API incompatibly. Needs resolving before either merges.

Detail

1. Multitenant "fallback" doesn't restore tenant isolation (AdminPanel/server/sources/routes/fonts/router.js:50-53,64-66)

The comment blocks bearer auth in multitenant mode and tells operators to use the cookie path (admin browser login) instead, on the basis that the bearer path's shared secret + unpartitioned font directory would allow cross-tenant access. But the cookie path doesn't fix that: adminpanel/router.js:255 hardcodes jwt.sign({tenant: 'localhost', isAdmin: true}) for every admin login regardless of actual tenant, and getCustomFontsDir() (router.js:107-109) is one global path with no tenant component. So in multitenant mode, any admin with that cookie gets the same unpartitioned directory the bearer-block was meant to prevent — gated by a login password instead of the DS secret. Net effect: NC-driven font management (which only calls via bearer, per eurooffice-nextcloud#142) is simply unavailable in multitenant deployments, not "less secure" — but the code implies a safe alternative exists when it doesn't.

Suggested fix: either drop the "use cookie path instead" claim and document that multitenant + NC-admin font management isn't supported yet, or actually tenant-scope getCustomFontsDir() and the adminpanel login.

2. Bearer-secret-rotation comment is incorrect (router.js:53-54)

Claims the secret is "read per-request so runtime config rotation (PATCH /admin/api/v1/config) is reflected without an AdminPanel restart." Traced: server.js:29 calls moduleReloader.requireConfigWithRuntime() once, at module load. config@3.3.12's Config.prototype.get freezes the config tree on first access (makeImmutable), which this router triggers at router.js:36. There's no watcher/periodic reload of that singleton anywhere in the repo. The actual hot-reload path in this codebase is ctx.getCfg() (used correctly by the wopi router), not the direct config.get(...) this router uses at router.js:67. Rotating the browser secret via the AdminPanel UI won't revoke acceptance of old-secret tokens here until a process restart, contrary to the comment.

Suggested fix: delete the false claim, or actually wire this router through ctx.getCfg().

3. server#39 duplication still unresolved

Verified via gh api repos/Euro-Office/server/pulls/39: open, zero review comments, untouched since 2026-07-11. Incompatible API shape (multipart upload, .ttc support, cookie-only auth, different status endpoint path) vs. this PR. eurooffice-nextcloud#142 is coded against this PR's shape specifically. Not a defect in #41, but needs a decision (close #39 or reconcile) before either merges cleanly.

[moodyjmz]

TL;DR: Both previously-flagged comments are fixed — the false secret-rotation claim is replaced with an honest note, and the misleading "use the cookie path instead" advice is gone. On re-verifying against the current head (238f1a91), one residual gap: the cookie-login path itself still has no multitenant guard at all, only the bearer path does. A couple of new minor findings below too.

Detail

Fixed — confirmed against source, not just the comment text:

• router.js:47-48 now correctly documents that config.get() is frozen at process start and hot-rotation needs ctx.getCfg().
• The false "cookie path is a safe fallback" claim is gone, replaced with an accurate statement that NC-driven font management isn't supported in multitenant mode.

Still open — the residual gap the new comment doesn't cover:

requireAuth (router.js:47-81) only calls tenantManager.isMultitenantMode() inside the Bearer branch (:58-59). If the request isn't Bearer, it falls straight to validateJWT (:81, the AdminPanel cookie-login path) with zero multitenant check. getCustomFontsDir() (:107-109) is still one global, unpartitioned path, and the AdminPanel cookie login still hardcodes tenant: 'localhost' regardless of actual tenant (adminpanel/router.js:255). So in multitenant mode, any admin who's logged into the AdminPanel via cookie — independent of NC/bearer at all — can still list/upload/delete/regenerate fonts against the one shared directory. The new comment is accurate about the bearer path but doesn't mention this.

Suggested fix: either extend the isMultitenantMode() check to gate the whole router (both branches), or explicitly document that the cookie-login path is knowingly out of scope for tenant isolation too.

Minor, carried forward unfixed:

• DELETE /:name (router.js:383) double-decodes the filename — Express already decodes route params once, so a font with a literal % in its name (uploadable) becomes undeletable via the API.
• ensureFontsDir (router.js:149-151) still silently mkdirSync(recursive: true)s the whole chain if the data volume isn't mounted.

New minor findings:

• regenerate's timeout can't actually kill the child on expiry: proc.kill('SIGTERM') targets a process spawned via sudo, which an unprivileged sender can't signal (EPERM, silently swallowed as an 'error' event). The API reports the step as errored and releases its lock while the root-owned script keeps running — a second regenerate call can overlap the first.
• The filename character whitelist got dropped when percent-encoding was introduced for X-Font-Name. Extension + magic-byte checks still hold so this isn't currently exploitable, but worth tightening back up.

[moodyjmz]

Multitenant cookie-path gap is fixed — the check moved to a single router-level router.use() gate that runs before either auth branch, closing the hole where cookie-login sessions bypassed tenant isolation. DELETE double-decode and the unmounted-volume mkdir issue are both fixed too. Remaining two items (SIGTERM can't kill a sudo child on timeout; dropped filename charset whitelist) are now honestly documented as accepted trade-offs rather than silently wrong — not blocking, worth a follow-up issue if regen usage patterns change. Approving.

[moodyjmz]

All three PRs in this chain are approved now — #14, #41, and this one. Every substantive finding across the rounds got a real fix rather than a cosmetic one (the multitenant gate, the demo-mode guard, the font-name encoding contract), which isn't the default outcome for a fast-turnaround fix cycle, so — well done. #39 stays out of scope per your note; that's a separate call for whoever owns that decision, not a blocker on any of these three.