feat(advisories): configurable check interval and a weekly digest - #171
Merged
Conversation
added 5 commits
August 21, 2026 07:28
…eal corpus
Nextcloud advisories describe several parallel maintenance branches in one
record, and the version-range field cannot express that. Measured over the
161 vulnerability entries in the live nextcloud/security-advisories feed
(captured as tests/fixtures/nextcloud-advisories.json):
66.5% several lower bounds, no upper bound
23.6% a single lower bound, no upper bound
5.0% a single upper bound
1.2% several upper bounds
Neither boolean reading of that comma is correct:
AND — the CURRENT behaviour of isAffected() — collapses Mail's
'>= 3.5.0, >= 3.7.0, >= 4.1.0, >= 4.3.0' to '>= 4.3.0', so an instance on
3.6.0 is told it is SAFE. A false negative, the worst direction for a
security check, and it applies to two thirds of real advisories.
OR turns Talk's '< 21.1.10, < 22.0.11, < 23.0.3' into '< 23.0.3', so a
correctly-patched 22.0.11 is reported VULNERABLE.
The structure the data actually has is one patch per release branch, so the
branch decides: branch is major.minor; a branch with a listed patch is judged
only against that patch; a branch with none falls through to the nearest
higher patch on the same major; and a version below EVERY published patch is
affected even when the only exit is a major upgrade.
That last rule is a correction the corpus forced. The first draft refused to
cross a major on the grounds that it recommends a migration — which reports
User OIDC 2.0.0 (patches 3.0.0/4.0.0/5.0.0, no 2.x fix) as safe. The test
that asserted the old rule now asserts the opposite and says why.
The two properties are swept over the entire corpus rather than spot-checked:
- no instance sitting ON a published patch is ever reported affected
(458 probes)
- every instance one patch level below a patch is reported affected
(412 probes, skipping constructed versions that are themselves patches)
Controls: a naive "nearest greater patch, ignore branches" implementation
fails the first sweep; the sweeps assert a non-trivial probe count so an
emptied fixture cannot pass by checking nothing.
No behaviour change yet — nothing constructs BranchAwareRange. Wiring it into
AdvisoryService lands with the source that actually supplies patched-version
lists, so the semantics change and the data arrive in one reviewable step.
The feed endpoint ignores `?page=` — page 1 and page 2 return identical bodies — and paginates by an opaque cursor in the Link header instead. The original fixture was built with a page-number loop, so it captured only the first 100 advisories and I described it as the corpus. Following the cursor reaches 277 advisories: 389 vulnerability entries with patched versions across 53 distinct packages, against 161 entries and 27 packages before. The validation sweeps now run over 2.4x the data. Both properties still hold unchanged: no instance sitting on a published patch is reported affected, and every instance one patch level below a patch is reported affected.
Third of three following up #160/#166, and the one that gives the feature real data. Builds on #168 (branch-aware evaluation). THE GAP. The App Store publishes no advisory information at all — measured, garm3.nextcloud.com/api/v1/apps.json returns 755 entries and 31.7 MB with no `securityAdvisories` field and nothing advisory-shaped. So correlation asked 87 of 88 apps a question their source cannot answer and recorded the silence as `error => null`, i.e. indistinguishable from "checked, clean" (#166). The real data is published centrally as GHSA records on nextcloud/security-advisories: 277 advisories, 389 vulnerability entries, 53 distinct packages. WHAT LANDS. NextcloudAdvisoryFeed reads that feed ONCE per sweep and indexes it by target. It follows the Link-header CURSOR rather than `?page=`, because that endpoint ignores the page parameter — pages 1 and 2 return identical bodies — so a page-number loop silently truncates the feed to its first 100 records. That is the same ignored-parameter trap as the App Store's `?filter=`, in a second API. A partial read keeps what it got AND reports the error, because discarding it would turn a feed that failed on page three into "no advisories". AdvisoryPackageMap resolves published package names to app ids. This is not cosmetic: the feed says `Talk` for `spreed`, `Team Folders` for `groupfolders`, and carries BOTH `User OIDC` and `user_oidc` for the same app. Matching normalised ids AND display names resolves 19 of 27 packages from the catalogue alone; indexing installed apps as well is what catches bundled apps like Photos and Flow, which are absent from the App Store catalogue entirely. A name that resolves to nothing is dropped, never guessed — a wrong match attaches a real advisory to the wrong app and leaves the affected one looking clean. AdvisoryService now merges feed advisories into each app's correlation, and routes any record carrying `patchedVersions` through BranchAwareRange. An app whose source has no advisory capability is no longer a dead end: the feed may still cover it, and for App Store apps it is the only thing that does. The server gets its own row, keyed distinctly so nothing mistakes it for an app. It is 95 of the 277 advisories — the largest single subject in the feed. Desktop and mobile client advisories are filtered out: an administrator cannot act on those from here. ServerVersionProvider exists so that row is TESTABLE. OCP\ServerVersion is readonly (PHPUnit cannot double it) and its constructor requires the server's own version.php, so depending on it directly would leave the server path — the largest slice of the feed — with no test at all. NOT TOUCHED, deliberately: three psalm errors in lib/Service/Installer. They appear locally and NOT in CI, because CI resolves OCP types against a real Nextcloud tree while a local run uses the app's stubs. "Fixing" them from local output — by pruning the baseline entries psalm calls unused — would have turned a green CI job red. 545 unit tests, 1099 assertions, no failure outside tests/unit/Command (19 errors there are a missing symfony/console in the local vendor copy). psalm clean on every file this change adds or touches; gate-16 count=0.
Completes the advisory work behind #160/#166. Two decisions taken by the maintainer: 6-hour default with 1-24 configurable, and urgent notifications immediately plus a weekly digest for everything else. INTERVAL. AdvisorySettingsStore holds it, AdvisoryRefreshJob reads it at construction (TimedJob fixes its interval there), and GET/PUT /api/advisory/settings expose it alongside the supported bounds — a client that hardcodes the range drifts from the server the first time it changes. The store CLAMPS out-of-range values; the endpoint REJECTS them. That is deliberate rather than inconsistent. A UI that asks for 48 hours and is answered "200 OK" while the server stored 24 has been lied to, so the API says no. But a value that arrives another way — `occ config:app:set`, or a future release narrowing the range — must still produce a working schedule: refusing to run because a stored number is out of bounds would silently stop security checks, which is worse than checking at a neighbouring frequency. DIGEST. AdvisoryDigestNotifier summarises the informational advisories — apps with a security history whose installed version is already safe — once a week. Urgent advisories keep their own immediate path and are excluded here, so nobody is told twice. Three behaviours worth naming, each of which is a way this could have gone quietly wrong: - a week with nothing informational sends NOTHING and does NOT advance the clock, so the first week with something to report sends immediately rather than waiting out a window consumed by silence; - a dispatch that reached nobody does not advance the clock either, so one transient failure does not suppress a second week as well; - the digest rate-limits itself, so the job may call it on every sweep — up to 24 times a day — and it still sends once a week. It defaults ON: the urgent path fires regardless, and the digest is what carries everything else. Defaulting it off would hide that material behind a setting nobody knows exists. UI in the Settings panel, with the bounds read from the server. The frontend sends '1'/'0' rather than a JSON boolean, because PHP casts a JSON `false` to '' and the server would read that as "unspecified" — the same trap already documented on the auto-update kill switch. Verification: 564 unit tests, 1125 assertions, no failure outside tests/unit/Command (19 errors there are a missing symfony/console in the local vendor copy). psalm clean on every file added or touched; gate-16 count=0; openapi regenerated (22 routes); frontend builds and the new strings were confirmed present in the built bundle. Three psalm errors in AdvisoryNotifier.php are NOT touched: they appear locally and not in CI, which resolves OCP types against a real Nextcloud tree rather than the app's stubs.
rubenvdlinde
changed the base branch from
feat/nextcloud-advisory-feed
to
development
August 21, 2026 07:58
added 2 commits
August 21, 2026 09:58
…settings-and-digest
Two additions: - the interval control renders with the bounds the SERVER reports, rather than a range hardcoded in the test as well. A test that pins its own copy of the range stops catching a server-side change to it. - an out-of-range interval is REFUSED (400) and leaves the stored value untouched. That is the half of the clamp/reject split which is easy to regress into a silent clamp, and a 200 that stored something else is exactly the lie the endpoint exists to avoid. Also removes the unused `page` fixture from the job-registration test added in #164 — it drives occ, not the browser, and eslint was right about it.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-21 08:19 UTC
Download the full PDF report from the workflow artifacts.
psalm in CI: UnusedProperty — $settings is read only in the constructor, where TimedJob's interval is fixed. Keeping a reference implied the job could re-read the setting during its life, which it cannot: a changed interval takes effect because the job is constructed afresh on the next run, not because anything re-reads it. Local psalm did not flag this. CI resolves OCP types against a real Nextcloud tree and sees the whole class; the local stub tree does not.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-21 08:34 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the advisory work behind #160/#166. Stacked on #169 — merge that first and this retargets cleanly.
Implements two maintainer decisions: 6-hour default, 1–24 configurable, and urgent immediately + a weekly digest for everything else.
Interval
AdvisorySettingsStoreholds it,AdvisoryRefreshJobreads it at construction (TimedJobfixes its interval there), andGET/PUT /api/advisory/settingsexpose it alongside the supported bounds — a client that hardcodes the range drifts from the server the first time it changes.The store clamps; the endpoint rejects. Deliberate, not inconsistent:
200 OKwhile the server stored 24 has been lied to → the API returns 400.occ config:app:set, or a future release narrowing the range — must still produce a working schedule. Refusing to run because a stored number is out of bounds would silently stop security checks, which is worse than checking at a neighbouring frequency.Digest
AdvisoryDigestNotifiersummarises the informational advisories — apps with a security history whose installed version is already safe — once a week. Urgent advisories keep their own immediate path and are excluded here, so nobody is told twice.Three behaviours worth naming, each a way this could have gone quietly wrong:
Defaults on: the urgent path fires regardless, and the digest carries everything else. Defaulting it off would hide that material behind a setting nobody knows exists.
UI
A control in the Settings panel, with the bounds read from the server. It sends
'1'/'0'rather than a JSON boolean, because PHP casts a JSONfalseto''and the server would read that as unspecified — the same trap already documented on the auto-update kill switch.Verification
tests/unit/Command(19 errors there are a missingsymfony/consolein my local vendor copy)count=0openapi.jsonregenerated — 22 routes, both new operations presentNot touched: three psalm errors in
AdvisoryNotifier.php. They appear locally and not in CI, which resolvesOCPtypes against a real Nextcloud tree rather than the app's stubs. "Fixing" them from local output would risk turning a green job red.🤖 Generated with Claude Code