feat(advisories): branch-aware version evaluation, validated on the real corpus by rubenvdlinde · Pull Request #168 · ConductionNL/versioniq

rubenvdlinde · 2026-08-21T05:28:53Z

Second of three following up #160/#166. Pure addition — no behaviour change yet; nothing constructs this class. It lands separately because it changes correctness-critical semantics and deserves review on its own.

The problem, measured

Nextcloud advisories describe several parallel maintenance branches in a single record, and the version-range field cannot express that. Over the 161 vulnerability entries in the live nextcloud/security-advisories feed (committed as tests/fixtures/nextcloud-advisories.json):

shape	share	example
several lower bounds, no upper	66.5%	Mail `>= 3.5.0, >= 3.7.0, >= 4.1.0, >= 4.3.0` → patched `3.7.25, 5.5.16, 5.6.20, 5.7.13`
one lower bound, no upper	23.6%	Forms `>=4.3.0` → patched `5.2.7`
one upper bound	5.0%	Approval `< 2.7.2` → patched `2.7.2`
several upper bounds	1.2%	Talk `< 21.1.10, < 22.0.11, < 23.0.3`

Neither boolean reading of that comma is right:

AND — what isAffected() does today — collapses Mail's four clauses 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, applying to two thirds of real advisories.
OR turns Talk's clauses into < 23.0.3, so a correctly-patched 22.0.11 is reported vulnerable.

The rule

One patch per release branch, so the branch decides. Branch is major.minor:

If the installed version's own branch has a listed patch, that patch alone decides.
A branch with no patch falls through to the nearest higher patch on the same major.
A version below every published patch is affected, even when the only exit is a major upgrade.
Otherwise, fixed.

Rule 3 is a correction the corpus forced. My first draft refused to cross a major, reasoning that a higher-major patch is a migration rather than a security update. Sweeping the corpus disproved it: User OIDC 2.0.0 against patches 3.0.0/4.0.0/5.0.0 has no 2.x fix, so "never cross" reports a vulnerable instance as safe. The test that asserted the old rule now asserts the opposite, and says why in the docblock.

How it is tested

Both properties are swept over the entire corpus, not 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

The second sweep skips constructed versions that are themselves published patches: with patches 3.0.0/4.0.0/5.0.0, "one below 4.0.0" is 3.0.0, a fixed version, and the two properties would contradict each other on it. That was a flaw in the probe, not in the verdict.

Also handled, because the corpus contains it: records whose patched_versions field carries operators rather than bare versions (>= 28.0.0, >= 29.0.0).

Controls

A naive "nearest greater patch, ignore branches" implementation fails the false-positive sweep.
Both sweeps assert a non-trivial probe count, so an emptied fixture cannot pass by checking nothing.

Verification

515 unit tests, 1045 assertions; no failure outside tests/unit/Command (19 errors there are a missing symfony/console in my local vendor copy). psalm clean; gate-16 count=0.

🤖 Generated with Claude Code

…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.

github-actions · 2026-08-21T06:02:27Z

Quality Report — ConductionNL/app-versions @ `056a4fc`

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 06:02 UTC

Download the full PDF report from the workflow artifacts.

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.

rubenvdlinde · 2026-08-21T07:19:21Z

Correction: the fixture was one page of the corpus, not the corpus

I described the fixture as "the 161 vulnerability entries in the live feed". That was the first page, not the feed.

The endpoint ignores ?page= — page 1 and page 2 return identical bodies — and paginates by an opaque cursor in the Link header. My capture loop used page numbers, so it read the same 100 advisories repeatedly and I recorded the deduplicated result as the whole corpus.

Following the cursor:

page 1: +100   page 2: +100   page 3: +77   ->  277 advisories

The fixture is now the full set: 389 vulnerability entries with patched versions, across 53 distinct packages (was 161 entries / 27 packages). The validation sweeps run over 2.4× the data.

Both properties still hold unchanged — no instance on a published patch is reported affected, and every instance one patch level below a patch is reported affected. So the rule survives the larger corpus; only my description of the evidence was wrong.

This is the same ignored-parameter trap as the App Store's ?filter= in #167, in a second API. Worth noting for the follow-up that reads this feed at runtime: a page-number loop there would silently truncate coverage to the first 100 advisories.

github-actions · 2026-08-21T07:34:23Z

Quality Report — ConductionNL/app-versions @ `5055dbb`

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 07:34 UTC

Download the full PDF report from the workflow artifacts.

) * 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. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>

* 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 21, 2026

feat(advisories): read the advisories Nextcloud actually publishes #169

Merged

rubenvdlinde merged commit 257ffaf into development Aug 21, 2026
41 checks passed

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

rubenvdlinde deleted the fix/advisory-branch-aware-evaluation branch August 21, 2026 08:51

Navigation Menu

Uh oh!

feat(advisories): branch-aware version evaluation, validated on the real corpus - #168

feat(advisories): branch-aware version evaluation, validated on the real corpus#168
rubenvdlinde merged 2 commits into
developmentfrom
fix/advisory-branch-aware-evaluation

rubenvdlinde commented Aug 21, 2026

Uh oh!

github-actions Bot commented Aug 21, 2026

Uh oh!

rubenvdlinde commented Aug 21, 2026

Uh oh!

github-actions Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Aug 21, 2026

The problem, measured

The rule

How it is tested

Controls

Verification

Uh oh!

github-actions Bot commented Aug 21, 2026

Quality Report — ConductionNL/app-versions @ 056a4fc

Uh oh!

rubenvdlinde commented Aug 21, 2026

Correction: the fixture was one page of the corpus, not the corpus

Uh oh!

github-actions Bot commented Aug 21, 2026

Quality Report — ConductionNL/app-versions @ 5055dbb

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/app-versions @ `056a4fc`

Quality Report — ConductionNL/app-versions @ `5055dbb`