fix(coverage-guard): scope the ratchet to the files a change touches - #2524
fix(coverage-guard): scope the ratchet to the files a change touches#2524rubenvdlinde wants to merge 2 commits into
Conversation
…L-2)
`NamesController` exposed three endpoints with no Nextcloud session required.
gate-7 flagged one of them; the other two it could not see, because they take
no object id.
GET /api/names/{id} is the serious one. It resolved the name of ANY object
through `CacheHandler::getSingleObjectName()`, whose database fallback calls
findAcrossAllSources(identifier: $id, _rbac: false, _multitenancy: false)
and tries organisations first. Both access controls are switched off by name,
so an anonymous caller holding a UUID could read that object's name in any
register, any schema, any tenant — organisation names included. Names are
frequently the sensitive part: a person, a case title, a document subject.
`AnonRateLimit(120/60)` raises the cost of blind enumeration and does nothing
against a targeted lookup, and UUIDs are not secrets — they travel in URLs,
exports and relations.
POST /api/names/warmup let an anonymous caller clear and rebuild the entire
name cache. GET /api/names/stats exposed cache internals.
Worth noting how this survived: `index()` was already hardened under this same
SEC-CTRL-2 heading — "Dropped @publicpage", plus a 401 preamble — and carries a
TODO recording that name resolution is still not RBAC/tenant-aware. The fix was
applied to one method of the controller and not to its three siblings.
What replaces them:
GET /api/names/{id} -> POST /api/names {"ids":["<id>"]}, session required.
Same {"names": {...}} response shape.
POST /api/names/warmup -> POST /api/settings/cache/warmup-names, which has no
#[NoAdminRequired] and is therefore admin-only.
GET /api/names/stats -> nothing; cache metrics belong in admin settings.
`RegistersIndex.vue` called the public warmup route from an admin screen and now
calls the admin one — which is what a maintenance action should have been doing.
`getSingleObjectName()` is KEPT. It has substantial direct test coverage and two
BackgroundJobs depend on its sibling `warmupNameCache()`; deleting it would have
removed working tests to no benefit. It gains a docblock stating plainly that it
is unscoped and must not be exposed from a controller, so the next person to
reach for it sees why the route is gone.
The seven tests that exercised the removed methods are replaced rather than
deleted. Deleting them alone would have left nothing to notice a revert. The
replacements assert the methods are absent AND that the two survivors carry no
`#[PublicPage]` attribute — the second half matters because a regression could
also take the form of re-decorating index()/create() instead of re-adding show().
Verified:
- full suite 16455 tests, 36835 assertions, 0 failures
- positive control: re-adding #[PublicPage] to index() makes the new tripwire
FAIL with its intended message; reverted, and the file diffs clean
- phpcs clean on both changed PHP files
- eslint 0 errors, prettier clean, frontend build OK
This closes the anonymous hole. It does NOT make name resolution
permission-aware — `getMultipleObjectNames()` still returns names across all
organisations to any authenticated caller. That remains open under the existing
TODO in `index()`, and the docs now say so.
Refs #2518
Adopts the canonical script from ConductionNL/.github (quality-config/coverage-guard.php). The whole-project comparison fires on measurement noise. doriath#240 was a PR whose entire diff was `webpack.config.js` — no PHP at all — and the guard failed it: identical denominator (13723), both runs reporting exactly `Tests: 948, Assertions: 3051, Skipped: 1`, and six covered statements of run-to-run xdebug variance between them. The measured `--against` floor cancels driver variance (xdebug vs pcov), as its header says. It does not cancel run-to-run variance within one driver, and the ratchet has no tolerance. Scoping the comparison to the PHP a change actually touches keeps full strength where a regression matters and makes the noise unreachable by construction — a diff with no PHP cannot fail. New `changed-files` capability; the shared workflow PROBES for it rather than assuming, so an un-updated copy keeps the previous behaviour instead of silently accepting and ignoring the flag. Script only — no behaviour change until the workflow passes `--changed-files`. Byte-identical to the canonical copy (md5 5be122aad209da030c79b22a133232fb).
[CREW-B] Measured: this PR is a no-op — superseded by #2523, which landed at 23:53ZNot merging it, and not because anything is wrong with it. Both of its commits are already on
Measured, not inferred —
The parity read on it was a false settle — worth recording
The whole 🔑 A RecommendClose as superseded. Nothing is lost: the security fix and the canonical guard are both live on |
|
Closing as fully superseded by #2523, verified file-by-file rather than by title. All 7 files this PR touches are byte-identical between its head and
Nothing is lost by closing: both the SEC-CTRL-2 change and the changed-files scoping of the coverage ratchet are already on Worth recording separately — this PR was also the case that exposed a defect in our own merge gate. Its head carried 7 check-runs against the base's 47, with |
1 participant
Adopts the canonical script from ConductionNL/.github (quality-config/coverage-guard.php).
The whole-project comparison fires on measurement noise. doriath#240 was a PR
whose entire diff was
webpack.config.js— no PHP at all — and the guard failedit: identical denominator (13723), both runs reporting exactly
Tests: 948, Assertions: 3051, Skipped: 1, and six covered statements ofrun-to-run xdebug variance between them.
The measured
--againstfloor cancels driver variance (xdebug vs pcov), as itsheader says. It does not cancel run-to-run variance within one driver, and the
ratchet has no tolerance. Scoping the comparison to the PHP a change actually
touches keeps full strength where a regression matters and makes the noise
unreachable by construction — a diff with no PHP cannot fail.
New
changed-filescapability; the shared workflow PROBES for it rather thanassuming, so an un-updated copy keeps the previous behaviour instead of silently
accepting and ignoring the flag.
Script only — no behaviour change until the workflow passes
--changed-files.Byte-identical to the canonical copy (md5 5be122aad209da030c79b22a133232fb).
Pairs with ConductionNL/.github#473, which teaches the shared workflow to pass
--changed-files.🤖 Generated with Claude Code