Repository navigation
feat(os): report operating system concerns where admins look - #1217
Conversation
27b6c04 to
6a62a2d
Compare
3620c9c to
dde9895
Compare
6a62a2d to
a7044bb
Compare
dde9895 to
3f78919
Compare
a7044bb to
f0d69b6
Compare
AndyScherzinger
left a comment
AndyScherzinger
left a comment
There was a problem hiding this comment.
I reviewed this for correctness, security and performance, and checked it live on the nextcloud-docker-dev instance with the whole stack deployed (Docker application container, system cron). Inline comments carry the details; this is the summary.
Security
The concern from my #1214 review is addressed. Live, api/v1/info?skipOs=false returns only counts, flags and dates: no package names, versions or advisories. The default skipOs=true leaves the response unchanged.
Access control:
- The API keeps the existing NC-Token/admin check and brute-force throttling.
- The setup checks, the widget and the notifier each re-check
isAdmin. - The setup-check description escapes its parameters before core renders it with
v-html, and notification subjects and widget items are plain text.
One gap: the API ignores restricted_mode (inline).
Correctness
The pattern to fix is that unknown keeps turning into green. Live in the Docker container:
- The dashboard widget says "The system is up to date", although apt in that container listed 35 pending security updates.
- The two setup checks show "This could not be checked on this system" as info rows, which never go away there.
In the code, the updates setup check can show success from a secondary concern while the updates themselves are unknown. The PHP restart verdict uses a sample that only setup-check runs refresh.
Performance
No command runs in a web request. The PHP sample reads /proc/self/maps (capped) and writes app config at most every five minutes or on change. The notifier loads and evaluates the stored status on each notification fetch by an admin with a live OS notification. That is bounded, so I have no change to ask for there.
| return SetupResult::info($this->l10n->t('Only administrators can see the operating system status.')); | ||
| } | ||
|
|
||
| $this->service->samplePhpProcess(); |
There was a problem hiding this comment.
In this PR this is the only place the PHP process is sampled; the cron job can't, because it runs as CLI. OsVerdict::restart() puts no age limit on the sample. So after an admin restarts php-fpm, the restart concern, the notification, the widget and the API's phpRestartRequired keep saying PHP runs outdated libraries, escalating by days, until someone opens Settings > Overview. The reverse also happens: a library update after the last sample is never noticed. #1218 adds a second sample point, which narrows the gap but does not close it. Suggest treating a sample older than a limit, or older than the last package-database change, as unknown.
| $verdict = $this->service->evaluate($status); | ||
| $concerns = array_values(array_filter( | ||
| array_map(static fn (string $id): ?Concern => $verdict?->get($id), $this->concernIds()), | ||
| static fn (?Concern $concern): bool => $concern !== null && $concern->level !== Concern::LEVEL_UNKNOWN, |
There was a problem hiding this comment.
Unknown concerns are filtered out here, and the worst remaining one is shown. With UPDATES unknown (e.g. apt-get -s or dnf check-update failed) and PACKAGE_LISTS OK, "Operating system updates" shows success with the package-lists text. On dnf, zypper, pacman and apk, packageLists() has days => null, so the text even reads "0 days ago". The reboot check does the same with REBOOT unknown and RESTART OK. Suggest returning "could not be determined" whenever the check's primary concern (concernIds()[0]) is unknown, instead of falling back to a secondary concern's OK.
| if ($items === []) { | ||
| $support = $verdict?->get(Concern::SUPPORT); | ||
| $items[] = new WidgetItem( | ||
| $this->l10n->t('The system is up to date'), |
There was a problem hiding this comment.
Reproduced. In the Docker container the widget returns "title": "The system is up to date", but apt in that container listed 71 pending updates, 35 from Debian-Security. atLeast(LEVEL_INFO) drops unknown concerns, and in an application container there is no UPDATES concern at all. Suggest claiming "up to date" only when UPDATES exists and is OK, and otherwise showing the unknown or not-assessed title.
| static fn (?Concern $concern): bool => $concern !== null && $concern->level !== Concern::LEVEL_UNKNOWN, | ||
| )); | ||
| if ($concerns === []) { | ||
| return SetupResult::info($this->l10n->t('This could not be checked on this system. {link}'), null, $this->linkParameter()); |
There was a problem hiding this comment.
In the Docker container this produces two permanent info rows on Overview that say the same thing ("This could not be checked on this system" for both updates and reboot). The same happens on installs where no status is ever collected, e.g. AJAX or webcron: there all three checks repeat "has not been checked yet". Without system cron the freshness warning is also appended to all three, on top of core's own cron warning. Suggest returning success early where the check's concerns do not exist in this environment (absent is not the same as unknown), and reporting "not collected yet" and freshness from one check only.
| public function monitoringSummary(): array { | ||
| $status = $this->getStatus(); | ||
| if ($status === null) { | ||
| return ['state' => 'pending', 'level' => Concern::LEVEL_UNKNOWN]; |
There was a problem hiding this comment.
Nit: for pending, restricted and unsupported, server.os has two or three keys instead of the full set. A monitoring parser written against the README example hits missing keys rather than the documented nulls. Suggest always emitting the full key set.
| * not become a list of unpatched software. Unknown values are null, never 0 | ||
| * or false, so a monitoring system cannot mistake "unknown" for "healthy". | ||
| */ | ||
| public function monitoringSummary(): array { |
There was a problem hiding this comment.
monitoringSummary() does not check isApplicable(). After an admin turns on restricted_mode, token holders keep receiving the last stored host status until the job runs again, and without system cron it never does. Suggest returning the restricted shape here when restricted mode is on.
| public function run(): SetupResult { | ||
| $user = $this->userSession->getUser(); | ||
| if ($user !== null && !$this->groupManager->isAdmin($user->getUID())) { | ||
| return SetupResult::info($this->l10n->t('Only administrators can see the operating system status.')); |
There was a problem hiding this comment.
Nit: the gate is correct, but users with Overview delegation get "Only administrators can see the operating system status." three times. Returning it from one check only, or as success, would be quieter.
Three setup checks on Settings > Overview, a notification to admins when a concern escalates (withdrawn once it is resolved), an os block with counts and flags only in the monitoring API, and a Server health dashboard widget. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
The support titles used {name}-style placeholders with a keyed parameter
array, but IL10N::t() in PHP only runs vsprintf(). Every install showed
"{name} receives security updates until {date}" in the setup check, the
notification and the dashboard widget. They now use positional %1$s
placeholders.
The tests did not notice because their IL10N mocks replaced {key}
placeholders, which the real class never does. A shared FakeL10N now
formats like the server, %n and vsprintf() only, and the setup check,
notifier and widget tests use it too.
Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Frank Karlitschek <[email protected]>
ConcernTexts read list facts such as the failed units or reboot kinds as they were stored and passed them to implode() and count(). A stored status is read back from JSON, so a nested array or a number in such a list raised "Array to string conversion" or miscounted. Only string items are used now. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
Setup check results are also read by occ setupchecks and the API, where a path without the host is useless. The notifier and the widget already use linkToRouteAbsolute(); the setup checks now do too. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
The monitoring API added server.os to every response, while app and server updates are opt-in with skipApps and skipUpdate. Existing token holders, often third-party monitoring services, would suddenly have received the distribution, security counts and the kernel version. skipOs now defaults to true, so the block is sent with skipOs=false. The kernel string is no longer part of the block at all: its exact version is what someone matching CVEs needs, and the README promised no versions. The README now says how to request the block. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
One Overview load ran the three setup checks, and each read up to 8 MB of /proc/self/maps for its PHP sample and decoded and evaluated the stored status again, about six times in all. The notifier did the same for every notification it rendered. The store already reads the status once per request. OsStatusService now also reuses the verdict for the same status object and samples the PHP process at most once per instance; a new sample clears the reused verdict, since the verdict depends on it. With that, IPreloadableNotifier would add nothing: every notification in a request already shares one verdict. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
OsNotifier threw AlreadyProcessedException whenever the concern was not a problem at render time, including when it was unknown, missing from the verdict, or when the stored status could not be read, for example after an upgrade changed its format. The notification manager then deleted that admin's notification, while os_notified_levels still remembered the level, so it was never sent again. It is now only dropped when the concern is known and no longer a problem (Concern::isResolved()), or when the sender has withdrawn it. Otherwise it renders a generic "The operating system needs attention" with the link to the page. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
Three gaps in OsNotificationSender::sync(): - Admins added to the admin group while a concern was live never got its notification, because a concern notified only on escalation. Every run now notifies enabled admins who were not notified since the last escalation and have no notification of the concern. Who was notified is kept in os_notified_users, so an admin who dismissed the notification does not get it again until the concern escalates. - The job and occ serverinfo:os-status --refresh could sync at the same time and notify twice. sync() now holds an exclusive lock from ILockingProvider and skips when another process holds it. Each admin is also checked with IManager::getCount() before a notification, and the subject is now the level, so an escalation withdraws only the notifications of the other level instead of all of them; a process with outdated notified levels therefore re-sends nothing. - markProcessed() failures escaped and failed the job. Withdrawals are now caught and logged, and a concern whose withdrawal failed stays remembered so the next run tries again. A stored status that cannot be read no longer withdraws anything, for the same reason the notifier keeps such notifications. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
On systems other than Linux and in restricted mode the widget said "The operating system has not been checked yet." forever, since the status there is never more than "unsupported" or "restricted". ServerHealthWidget::isEnabled() now also asks the new OsStatusService::isApplicable(), which reads restricted_mode and the OS family but no stored status, so the dashboard stays cheap. Should the widget still be asked for items, it shows the sentence the setup checks use; both now come from ConcernTexts::unavailable(). Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
The stored status is only renewed by the background job under system cron. When cron stops, or background jobs run by AJAX or webcron, the Overview and the dashboard kept presenting a weeks-old status as the current one. The freshness concern now has its own headline and sentence: how long ago the system was checked, and either "switch background jobs to Cron" or "check that the cron job runs". Each setup check appends it and is at least a warning while the status is old, since all three judge the same stored status. The widget lists it like the other concerns. It sends no notification: the job that would send one is the one that stopped. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
ConcernTexts::title() only told the concerns' known levels apart, so a concern that could not be determined got the headline of a problem: "Reboot required" when the reboot state was unknown, "The system clock is not synchronized" when time sync could not be checked. Setup checks and the widget skip unknown concerns, but the page shows every concern with its headline. Unknown concerns now say what could not be determined, in the words the page already uses. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
The setup checks are the first to sample the PHP process that serves Nextcloud, so this is where the README can promise that check and name the process maps it reads. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
Core serves GET /settings/setupcheck with #[NoAdminRequired], and the settings app's sub-admin middleware lets sub-admins through. They got the full OS setup checks: pending security updates and their age, the reboot state and the end-of-life status. The widget and notifications are for admins only, so the endpoint undercut the app's own boundary. The checks now answer a session user who is not an admin with the same neutral info result, whatever the system's state, and do not sample the PHP process for them. occ setupchecks and cron run without a session user and still get the full result. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
Settings → Overview renders a setup check's description with v-html
and escapes only the rich object parameters. The OS checks build their
description from ConcernTexts, which puts strings from the system into
it, such as PRETTY_NAME from /etc/os-release, which is free text. Markup
there ran in an admin's browser.
The checks now escape the text with htmlspecialchars() before adding
the {link} placeholder, which core fills with an escaped link, and on
the success path, which has no link. Quotes are left alone: the text is
never put into an attribute, and occ setupchecks prints the description
without decoding entities, so translations with apostrophes stay
readable there. Only <, > and & show as entities in occ.
Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Frank Karlitschek <[email protected]>
…er admins OsNotifier builds the text from the current status every time the notification is fetched, and the sender withdraws a notification only when its concern resolves. An admin who was removed from the admin group kept getting live security update counts, reboot and end-of-life state through notifications sent while they were an admin. prepare() now throws AlreadyProcessedException when the recipient is not an admin, so the notification is deleted before anything from the status is read. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
PhpProcessProbe::canSample() only looks at the SAPI, so the setup checks still read /proc/self/maps, /proc/self/exe and the confinement files, and stored a sample, with restricted_mode=yes, although the collector honours the setting and the README promises that restricted mode stops reads outside the app directory. samplePhpProcess() now returns before any read or store access when isApplicable() is false: in restricted mode or on a system other than Linux. FakeHost records the paths it is asked to read, so the test can show that nothing was read. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Frank Karlitschek <[email protected]>
f0d69b6 to
e9df3f3
Compare
3f78919 to
4cdd1b9
Compare
Part 4 of 5, stacked on #1216:
AI disclosure: this change was written with Claude Code (claude-opus-5-5) and reviewed by the author; every commit carries an
Assisted-bytrailer.🤖 Generated with Claude Code