feat(advisories): read the advisories Nextcloud actually publishes - #169
Merged
Conversation
added 3 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.
rubenvdlinde
changed the base branch from
fix/advisory-branch-aware-evaluation
to
development
August 21, 2026 07:36
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 07:57 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
1 participant
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.
Third of three following up #160/#166, and the one that gives the feature real data.
The gap
The App Store publishes no advisory information at all. Measured against the endpoint the code calls:
So correlation asked 87 of 88 apps a question their source cannot answer, and the missing field took the
!is_array($raw)branch returningerror => null— indistinguishable from checked and clean. Real coverage was 1 app.The data exists, published centrally as GHSA records on
nextcloud/security-advisories: 277 advisories, 389 vulnerability entries, 53 distinct packages.What lands
NextcloudAdvisoryFeed— reads the feed once per sweep and indexes it by target. It follows theLinkheader cursor, not?page=, because that endpoint ignores the page parameter (pages 1 and 2 return identical bodies). A page-number loop truncates the feed to its first 100 records — which is exactly how my own first corpus capture went wrong. Same ignored-parameter trap as the App Store's?filter=in #167, in a second API.A partial read keeps what it got and reports the error. Discarding it would turn a feed that failed on page three into "no advisories".
AdvisoryPackageMap— resolves published package names to app ids. Not cosmetic:TalkspreedTeam FoldersgroupfoldersUser OIDCanduser_oidcuser_oidcTwofactor WebAuthntwofactor_webauthnMeasured: normalised id+name matching resolves 19 of 27 packages from the catalogue alone. Indexing installed apps as well is what catches bundled apps —
Photos,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— merges feed advisories into each app's correlation and routes records carryingpatchedVersionsthroughBranchAwareRange. An app whose source has no advisory capability is no longer a dead end.The server gets its own row, keyed distinctly. It is 95 of the 277 advisories, the largest single subject in the feed. Desktop and mobile client advisories are filtered out — an admin cannot act on those from here.
ServerVersionProviderexists so that row is testable:OCP\ServerVersionisreadonly(PHPUnit cannot double it) and its constructorrequires the server's ownversion.php. Depending on it directly would leave the largest slice of the feed with no test at all.Deliberately not touched
Three psalm errors in
lib/Service/Installer. They appear locally and not in CI, because CI resolvesOCPtypes against a real Nextcloud tree while a local run uses the app's stubs. "Fixing" them from local output — pruning the baseline entries psalm calls unused — would have turned a green CI job red.Verification
545 unit tests, 1099 assertions; no failure outside
tests/unit/Command(19 errors there are a missingsymfony/consolein my local vendor copy). psalm clean on every file added or touched. gate-16count=0.🤖 Generated with Claude Code