feat(advisories): correlate in the background job and serve a stored snapshot by rubenvdlinde · Pull Request #164 · ConductionNL/versioniq

rubenvdlinde · 2026-08-20T21:40:22Z

Closes the scope question left open on #160, taking option B: keep covering every enabled app, move the correlation off the request path.

Why

Correlation makes two external calls per app (listAdvisories + listVersions). On this instance's 88 enabled apps that is ~176 sequential external calls, which GET /api/advisories used to do inline. Measured on a live instance:

call 1: 120.02s  http=000
call 2: 120.01s  http=000

It could not even warm its own cache — the per-app payload cache is written on completion, and the request never completed. And because this loader is dispatched first of three, while it held the PHP session lock the sibling /api/pins request never ran at all, so pin badges silently never rendered and nothing anywhere said why.

The alternative was narrowing correlation to apps with an explicit source binding — exactly 1 of 88 here, so 176 calls become 2. Rejected: it makes a security feature quietly stop looking at 87 apps with nothing on screen saying so.

What changes

AdvisoryResultStore persists the snapshot and the time the sweep completed. A snapshot that fails to encode leaves the previous one intact — a stale answer whose age is displayed beats no answer.
AdvisoryRefreshJob (already registered, already 6-hourly) now stores what it sweeps. It persists before notifying, because notification is the more failure-prone half, and warns when some apps could not be correlated.
GET /api/advisories reads the snapshot and returns checkedAt.
App.vue renders the age. Without it, "swept and found nothing", "never swept because cron has not run", and "the fetch failed" all render as an identical empty badge set — an absence that reads as reassurance.

The budget interaction — the subtle part

#162 gave correlateAll() a 5s ceiling, sized for a request a user is waiting on. That default applied to the background job too, so the 6-hourly sweep, and the notifications derived from it, were already being clipped to the first handful of apps. The budget that made the endpoint answerable had quietly become the budget that made the coverage wrong.

The budget is now a parameter; the job passes 600s. A regression test asserts the job passes something larger than the request-path default, and I confirmed it fails when the job is reverted to the bare call:

Failed asserting that null is greater than 5.0.

Pre-existing debt fixed in passing

satisfiesClause() fed version_compare() an operator psalm could not prove was in its closed set, so the declared bool return was really bool|null. It now maps through the literal set. Also drops a redundant array_values. Those four entries leave psalm-baseline.xml — which is how they surfaced: fixing them turned the baseline stale.

Verification

505 unit tests, 1020 assertions, zero failures outside tests/unit/Command (19 errors there are a missing symfony/console in my local vendor copy — all 19 are Class "Symfony\Component\Console\..." not found, not code).
psalm: no issue in any touched file; AdvisoryService.php now carries no baseline entries at all.
Frontend builds, and the new strings were confirmed present in the built bundle rather than assumed.
New e2e spec asserts the freshness line always renders, that the endpoint answers promptly, that checkedAt is present, and that the job is registered — because an honest endpoint with no writer would report "not checked yet" forever.

🤖 Generated with Claude Code

…snapshot Closes the scope question left open on #160. The endpoint keeps covering EVERY enabled app and pays for that in staleness instead of in a request that cannot return. WHY. Correlation makes two external calls per app (listAdvisories + listVersions). On an 88-app instance that is ~176 sequential external calls, which GET /api/advisories used to do inline. Measured on a live instance it did not answer within 120s, twice, and because it was dispatched first and held the PHP session lock, the sibling /api/pins request never ran at all — so pin badges silently never rendered and nothing reported why. The alternative considered was narrowing correlation to apps with an explicit source binding: exactly 1 of the 88 here, so 176 calls become 2. That was rejected because it makes a SECURITY feature quietly stop looking at 87 apps with nothing on screen saying so. WHAT CHANGES. - AdvisoryResultStore persists the snapshot plus the time the sweep completed. A snapshot that fails to encode leaves the previous one intact: a stale answer whose age is shown beats no answer. - AdvisoryRefreshJob (already registered, already 6-hourly) now stores what it sweeps, persisting BEFORE notifying because notification is the more failure-prone half. It also warns when some apps could not be correlated. - GET /api/advisories reads the snapshot and returns `checkedAt`. - App.vue renders the age. "Swept and found nothing", "never swept because cron has not run" and "the fetch failed" otherwise render as an identical empty badge set — an absence that reads as reassurance. THE BUDGET INTERACTION, which is the subtle part. #162 gave correlateAll a 5s ceiling sized for a request someone is waiting on. That default applied to the background job too, so the 6-hourly sweep — and the notifications derived from it — were already being clipped to the first few apps. The budget is now a parameter: the job passes 600s. A regression test asserts the job passes something larger than the request-path default, and was confirmed to FAIL when the job is reverted to the bare call. PRE-EXISTING DEBT FIXED IN PASSING. satisfiesClause fed version_compare an operator psalm could not prove was in its closed set, so the declared bool return was really bool|null; it now maps through the literal set. Also drops a redundant array_values. Those four entries leave psalm-baseline.xml, which is what surfaced them: fixing them turned the baseline stale. Verification: 505 unit tests, 1020 assertions, 0 failures outside tests/unit/Command (19 errors there are a missing symfony/console in the local vendor copy, not code). psalm reports no issue in any touched file. Frontend builds; the new strings were confirmed present in the built bundle rather than assumed.

`GET /api/advisories` now returns `checkedAt` alongside `advisories`, so the committed spec was stale and the `openapi` job failed on the resulting git diff — which is the job working exactly as intended. Regenerating locally needs the PATCHED extractor: nextcloud/openapi-extractor rejects EUPL-1.2 as an SPDX identifier and dies with Uncaught Error: license: Unable to convert EUPL-1.2 to SPDX identifier The repo already carries the fix as a composer patch (vendor-bin/openapi-extractor/patches/0001-allow-eupl-spdx-identifiers.patch), but it only applies on a fresh install of that package — a vendor tree copied from another checkout is unpatched and fails this way.

github-actions · 2026-08-20T22:01:03Z

Quality Report — ConductionNL/app-versions @ `7da7522`

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-20 22:01 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-08-20T22:46:01Z

Quality Report — ConductionNL/app-versions @ `12e9768`

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-20 22:45 UTC

Download the full PDF report from the workflow artifacts.

gate-16 spec-coverage failed on this one method: `run()` is protected, this change touched it, and it carried no `@spec`. Reproduced locally against origin/development (count=1), then confirmed count=0 after. The gate is right to want it — the sweep is now the ONLY writer of the advisory snapshot the admin UI reads, so it is squarely part of the security-advisory-correlation spec rather than incidental plumbing.

github-actions · 2026-08-20T23:39:14Z

Quality Report — ConductionNL/app-versions @ `1a189b1`

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-20 23:39 UTC

Download the full PDF report from the workflow artifacts.

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.

* feat(advisories): branch-aware version evaluation, validated on the real 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. * test(advisories): use the FULL advisory corpus, not the first page of it 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. * feat(advisories): read the advisories Nextcloud actually publishes 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. * feat(advisories): configurable check interval and a weekly digest 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. * test(e2e): cover the advisory settings, and drop an unused fixture param 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. * fix(advisories): do not promote the settings store to a property 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. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>

rubenvdlinde mentioned this pull request Aug 20, 2026

The three non-blocking loaders at the end of onMounted never run (pins, advisories, policies) #160

Closed

rubenvdlinde merged commit f5f147e into development Aug 20, 2026
40 checks passed

rubenvdlinde deleted the feat/advisory-background-job branch August 20, 2026 23:41

rubenvdlinde mentioned this pull request Aug 21, 2026

Advisory correlation reports "no advisories" for 87 of 88 apps because the App Store publishes none #166

Closed

Navigation Menu

Uh oh!

feat(advisories): correlate in the background job and serve a stored snapshot - #164

feat(advisories): correlate in the background job and serve a stored snapshot#164
rubenvdlinde merged 3 commits into
developmentfrom
feat/advisory-background-job

rubenvdlinde commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Aug 20, 2026

Why

What changes

The budget interaction — the subtle part

Pre-existing debt fixed in passing

Verification

Uh oh!

github-actions Bot commented Aug 20, 2026

Quality Report — ConductionNL/app-versions @ 7da7522

Uh oh!

github-actions Bot commented Aug 20, 2026

Quality Report — ConductionNL/app-versions @ 12e9768

Uh oh!

github-actions Bot commented Aug 20, 2026

Quality Report — ConductionNL/app-versions @ 1a189b1

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/app-versions @ `7da7522`

Quality Report — ConductionNL/app-versions @ `12e9768`

Quality Report — ConductionNL/app-versions @ `1a189b1`