Skip to content

feat: add a script for restarting the docservice - #14

Merged
DmySyz merged 1 commit into
mainfrom
feat/font-management
Aug 24, 2026
Merged

feat: add a script for restarting the docservice#14
DmySyz merged 1 commit into
mainfrom
feat/font-management

Conversation

@DmySyz

@DmySyz DmySyz commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

needed for loading new fonts

Assisted-by: Claude Code (Opus-4.8)

@moodyjmz

Copy link
Copy Markdown
Member

Two notes from reviewing this together with Euro-Office/server#41 (details in the review addendum there):

1. The script can no-op and still exit 0. if pgrep systemd … elif pgrep supervisord … fi has no else branch — if neither matches (different init, supervisord under an unexpected argv[0]), the script falls through and exits 0. The #41 router treats that as success, runs flush-cache, and reports done while docservice/converter still hold the old font list — a silent no-op success. Add an else that exits non-zero:

if pgrep -x "systemd" > /dev/null; then
    systemctl restart ds-docservice ds-converter
elif pgrep -x "supervisord" > /dev/null; then
    supervisorctl restart docservice converter
else
    echo "documentserver-restart.sh: no supported init found (systemd/supervisord)" >&2
    exit 1
fi

2. What's right, for the record: the bare docservice/converter names are correct for the standalone image's supervisor config, ds-docservice/ds-converter matches the deb systemd units, the glob-free sudoers entry is the shape the three existing entries should also adopt, and the script rides the existing wildcard manifests (deb package.install, rpm spec, Makefile) so no manifest changes are needed.

One packaging nit: the deb control doesn't declare sudo in Depends — the images install it explicitly, but a bare-metal deb install without sudo makes every AdminPanel spawn fail with only a generic error in the UI.

Ordering: this must merge and ship in a rebuilt package/image before server#41, or its regenerate endpoint fails half-applied (fonts regenerated on disk, services and browser caches stale).

@DmySyz
DmySyz force-pushed the feat/font-management branch from b6ed1c5 to f8f5a65 Compare August 24, 2026 10:12
@moodyjmz

Copy link
Copy Markdown
Member

TL;DR: No privilege-escalation issue — script takes no args, no env/config injection surface, correct perms, no TOCTOU. The prior review comment (silent exit-0 on unrecognized init system) is genuinely fixed. Two packaging gaps worth closing before merge:

  1. sudo is only a Recommends, not a Depends — a minimal install silently ships a broken feature.
  2. The sudoers entry has no arg-lock, so it's actually the loosest entry in the file, not the tightest.
Detail

1. sudo should be Depends, not Recommends (deb/template/control.m4:37)

server#41's AdminPanel/server/sources/routes/fonts/router.js does spawn('sudo', [RESTART_SCRIPT], ...) unconditionally, with no existence check — failure only surfaces via a generic proc.on('error'). Recommends is skipped by apt-get install --no-install-recommends. The live standalone image only works today because its Dockerfile lists sudo explicitly on the install line, independent of this deb's metadata — a bare-metal/minimal install that doesn't do the same gets a broken font-regen feature with an unhelpful error.

Suggested fix: Depends: sudo.

2. Sudoers entry permits arbitrary arguments (common/documentserver/sudoers/documentserver)

The entry for /usr/bin/documentserver-restart.sh has no trailing args spec. Per sudoers(5): with no command-line arguments specified, the user may run the command with any arguments they choose. So this is currently the loosest entry in the file, not the tightest as previously assumed — no live exploit today since the script never reads $1/$@, but it's a silent trap if an arg-driven branch is ever added later without anyone revisiting the sudoers scope.

Suggested fix: /usr/bin/documentserver-restart.sh "" to lock to zero arguments, matching actual script behavior.

(Non-blocking, for awareness: orchestrated/k8s deployments have no supervisor at all, so this script correctly — and loudly — fails there; the font-regen feature is simply unavailable on that deployment target for now.)

needed for loading new fonts

Signed-off-by: dsyzov <dmytro.syzov@nextcloud.com>
@DmySyz
DmySyz force-pushed the feat/font-management branch from f8f5a65 to 27c599b Compare August 24, 2026 13:55
@moodyjmz

Copy link
Copy Markdown
Member

TL;DR: The sudoers arg-lock is fixed, exactly as suggested. The sudo dependency issue is still open — flagged twice now, no fix or response. Same gap exists on the RPM side too.

Detail

Fixed — confirmed:

common/documentserver/sudoers/documentserver now reads /usr/bin/documentserver-restart.sh "", correctly locking the entry to zero arguments per sudoers(5) semantics.

Still open:

deb/template/control.m4:37 still lists sudo only under Recommends:, not Depends:. This was flagged in the previous round and hasn't been addressed or responded to — a --no-install-recommends install (or any minimal image that skips Recommends) silently ships a broken font-regen feature, since the caller (spawn('sudo', ...) in server#41) has no existence check. One-line fix: move sudo into Depends.

New, same gap on the other packaging format:

Neither rpm/requires.spec nor apt-rpm/requires.spec declares a sudo requirement at all, and the sudoers file + restart script are installed on RPM too (rpm/common.spec:43,74). If sudo moves to Depends on the deb side, please add the equivalent Requires: sudo to both RPM specs in the same pass, or the same failure mode reappears there.

@DmySyz

DmySyz commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

TL;DR: The sudoers arg-lock is fixed, exactly as suggested. The sudo dependency issue is still open — flagged twice now, no fix or response. Same gap exists on the RPM side too.
Detail

The sudo dependency is intentional. Minimal installation users often do not want to add it.

@moodyjmz

Copy link
Copy Markdown
Member

Agreed on Recommends — apt installs recommends by default, the Docker image installs sudo explicitly (Dockerfile:69 in Docker-DocumentServer), and DS core itself is unaffected without it. That fits Debian Policy's definition of Recommends exactly. Withdrawing the Depends ask.

Correction to my earlier comment: the failure isn't actually opaque. spawn('sudo', ...)'s ENOENT surfaces as spawn sudo ENOENT in server#41's runScript, GET /status passes that through verbatim, and FontManager.vue renders it as "Regeneration error: allfontsgen spawn failed: spawn sudo ENOENT" — that names the binary and the failure, good enough to act on. Apologies for overstating it as generic.

@moodyjmz moodyjmz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sudoers arg-lock fixed, silent exit-0 fixed, sudo Depends/Recommends settled (Recommends is correct — apt installs it by default, the Docker image installs sudo explicitly). No open issues from my side. Approving.

@DmySyz
DmySyz merged commit 84907ab into main Aug 24, 2026
1 check passed
@DmySyz
DmySyz deleted the feat/font-management branch August 26, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants