fix(auth,jobs): 13 undeclared admin endpoints, a 500 on an unknown id, and two dead seams - #438
Merged
Merged
Conversation
…, and two dead seams
Measured at gate package 365fa31 with NODE_PATH set (require.resolve('ajv')
→ .../node_modules/ajv/dist/ajv.js). Every run had a 0-byte .err and
verdicts were read from stdout, never the exit byte.
gate-3 stub-scan 1 -> 0
gate-5 route-auth 13 -> 0
gate-9 semantic-auth 1 -> 0
gate-18 notification-dialect WARNING -> PASS
gate-49 controller-exception-translation 1 -> 0
gate-57 orphaned-write-capability 2 -> 0
full-repo failing gates 19 -> 14
**Thirteen admin endpoints declared nothing** (gate-5). Audit-log query,
governance reports, multilingual reconciliation and regulator export all
call requireAdmin() in the body but carried no auth attribute at all.
Nextcloud treats "no attribute" as admin-required, so they worked — but
"deliberately admin-only" and "someone forgot" look identical in the
source, and AuditLogController's own class docblock had already noticed,
warning that the absence "is silently bypassed in some test setups".
They now carry #[AuthorizedAdminSetting(AdminSettings::class)], which is
this repo's existing, live idiom for an admin-only REST endpoint
(MemberImportController, SettingsController) — not a new invention. It
also lets an admin DELEGATE these to a group rather than hardcoding "is a
server admin". Body guards are kept as defence in depth.
`#[NoCSRFRequired]` would also have satisfied gate-5. It was not used:
it says nothing about admin and would have weakened CSRF protection to
quiet a checker.
**DecisionController::publish said the opposite of what it did**
(gate-9). It carried #[NoAdminRequired] — "any authenticated user" —
while the next statements refuse non-administrators and its own docblock
said "Requires Nextcloud admin role". The attribute is what a reader, an
auditor and the middleware all see first.
**getStats 500'd on an unknown participant** (gate-49). OpenRegister's
find() THROWS DoesNotExistException; it does not return null. The
`!== null` branch was unreachable for the case it was written for, and
the exception escaped as a 500 on an ordinary "no such participant"
request — the same defect class as ParticipantResolver, fixed in #425.
An absent participant now answers 403, deliberately fail-CLOSED: a 404
would let any authenticated user enumerate participant UUIDs. The
service call translates DoesNotExist to 404 and InvalidArgument to 400.
The catches are narrowed, and a third test asserts a RuntimeException
still PROPAGATES — narrowing is only a fix if the narrowing is real; a
blanket catch would turn an OpenRegister outage into a tidy 403 and hide
it from monitoring.
**Nobody was ever told a decision went public** (gate-57 + gate-18).
DecisionNotificationService::notifyOnPublish had zero callers, zero
tests and no DI registration, and `isPublished` was the one Decision
event with NO declarative rule — so the notification simply never
happened. ADR-031 keeps notifications declarative and gate-18 flagged
the class by name, so the rule lands in the register as
`decisionPublished` (mirroring its six siblings) and the dead imperative
class is removed rather than wired up.
**createPreference was a second name for one write** (gate-57). A
self-described "(alias)" delegating to updatePreference, which is itself
an upsert. No callers in lib/, src/ or tests/ — nothing wrote through
it, so nothing can be orphaned by its removal.
**A retired job that could never retire** (gate-3). OverdueActionItemsJob
was emptied to a no-op with a comment saying it was kept "so the
registered oc_jobs row reaps cleanly" — while still declared in
appinfo/info.xml, so Nextcloud re-registered it on every app update and
cron ran an empty job every 24 hours, forever. Both halves are removed.
No repair step is needed: read from the running server's own
lib/private/BackgroundJob/JobList.php::buildJob(), an unresolvable job
class is logged and then dropped via removeById() — "Remove job from
disabled app or old version of an app". Measured, not assumed; a repair
step was written first and deleted once the source said it was redundant.
Can-fail proof — one item reverted at a time, each gate naming it:
gate-5 AuditLogController.php:126 method=verify rule=missing-auth-attribute
gate-9 DecisionController.php:225 method=publish rule=no-admin-required-annotation-with-admin-body
gate-57 NotificationPreferenceService.php:194 method=createPreference
gate-49 removing the catch makes the new test error with the escaping
DoesNotExistException — the live 500, reproduced
All restored; all back to PASS.
Measured: phpunit 826 tests / 3009 assertions / 0 failures (823 + 3 new,
minus the 5 belonging to the deleted job); phpcs 0 errors / 98 warnings;
phpstan 0; psalm 0; phpmd 0 on both rulesets.
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
August 9, 2026 20:32
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 549/549 | |||
| PHPUnit | ✅ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-09 21:08 UTC
Download the full PDF report from the workflow artifacts.
…ch gate-53 needs)
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 549/549 | |||
| PHPUnit | ✅ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-09 21:46 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 9, 2026
Measured at gate package 365fa31, NODE_PATH set, 0-byte .err, 0 gates SKIPPED(wiring): gate-46 spec-anchor-existence 18 -> 4, and the last 4 live in lib/BackgroundJob/OverdueActionItemsJob.php, which #438 deletes — so the pair takes this gate to 0. Three distinct causes, none of them "the tag was missing": **A trailing full stop swallowed into the path — 6 tags.** `@spec openspec/specs/decision-management/spec.md.` resolves to a filename ending in a dot. All three targets exist; only the sentence punctuation was wrong. This is the cheapest possible defect and it had made three real, current, canonical specs unreachable. **Tags pointing INTO archived changes — 8 tags.** `@spec` targets the canonical `openspec/specs/`, never a change directory: a change is archived once it lands, and the tag then dangles. Each was retargeted to the canonical spec that survived its change, verified to exist on disk: publish-decisions-via-opencatalogi -> specs/public-publication meeting-transcription-ai-minutes -> specs/meeting-transcription citizen-participation -> specs/citizen-participation authorizedadminsetting-fix-fleet x2 -> specs/admin-settings board-meeting-resolutions -> specs/decision-management retrofit-2026-05-26-preferences-api -> specs/user-settings (x2) pluggable-integration-registry -> specs/nextcloud-integration **A path that was never valid and a placeholder anchor — 4 tags.** MeetingService carried `openspec/changes/spec/tasks.md#task-1` three times — a directory literally named "spec" that has never existed, so this tag could not have resolved on any commit. Retargeted to specs/meeting-management. ActionItemController and actionItemApi.js carried `#task-2.x`, an anchor with a literal "x" placeholder in it; both now point at specs/action-item-board-via-deck-leaf, whose requirements they implement. No spec file was created to make an anchor resolve. Every target was checked to exist before the tag was pointed at it, and where a canonical spec genuinely did not exist the tag was moved to the one that describes the code rather than to an invented file. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
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.
Measured at gate package
365fa31withNODE_PATHset —require.resolve('ajv')→/home/rubenlinde/nextcloud-docker-dev/workspace/server/node_modules/ajv/dist/ajv.js. Every run had a 0-byte.err; verdicts read from stdout, never the exit byte.Thirteen admin endpoints that declared nothing (gate-5)
Audit-log query, governance reports, multilingual reconciliation and regulator export all call
requireAdmin()in the body but carried no auth attribute at all. Nextcloud treats "no attribute" as admin-required, so they worked — but "deliberately admin-only" and "someone forgot" look identical in the source.AuditLogController's own class docblock had already noticed, warning that the absence "is silently bypassed in some test setups".They now carry
#[AuthorizedAdminSetting(AdminSettings::class)]— this repo's existing, live idiom for an admin-only REST endpoint (MemberImportController,SettingsController), not a new invention. It additionally lets an admin delegate these to a group rather than hardcoding "is a server admin". Body guards kept as defence in depth.#[NoCSRFRequired]would also have satisfied gate-5. It was not used: it says nothing about admin and would have weakened CSRF protection to quiet a checker.publish()said the opposite of what it did (gate-9)It carried
#[NoAdminRequired]— "any authenticated user" — while the next statements refuse non-administrators and its own docblock said "Requires Nextcloud admin role". The attribute is what a reader, an auditor and the middleware all see first.getStats500'd on an unknown participant (gate-49)OpenRegister's
find()throwsDoesNotExistException; it does not return null. The!== nullbranch was unreachable for the case it was written for, and the exception escaped as a 500 on an ordinary "no such participant" request — the same defect class asParticipantResolver, fixed in #425.An absent participant now answers 403, deliberately fail-closed: a 404 would let any authenticated user enumerate participant UUIDs. The service call translates
DoesNotExist→ 404 andInvalidArgument→ 400.A third test asserts a
RuntimeExceptionstill propagates. Narrowing an exception type is only a fix if the narrowing is real — a blanket catch would turn an OpenRegister outage into a tidy 403 and hide it from monitoring.Nobody was ever told a decision went public (gate-57 + gate-18)
DecisionNotificationService::notifyOnPublishhad zero callers, zero tests and no DI registration, andisPublishedwas the one Decision event with no declarative rule — so the notification simply never happened. ADR-031 keeps notifications declarative and gate-18 flagged the class by name, so the rule lands in the register asdecisionPublished(mirroring its six siblings) and the dead imperative class is removed rather than wired up.createPreferencewas a second name for one write (gate-57)A self-described "(alias)" delegating to
updatePreference, which is itself an upsert. No callers inlib/,src/ortests/— nothing wrote through it, so nothing can be orphaned by its removal.A retired job that could never retire (gate-3)
OverdueActionItemsJobwas emptied to a no-op with a comment saying it was kept "so the registered oc_jobs row reaps cleanly" — while still declared inappinfo/info.xml, so Nextcloud re-registered it on every app update and cron ran an empty job every 24 hours, forever. Both halves are removed.No repair step is needed. Read from the running server's own
lib/private/BackgroundJob/JobList.php::buildJob(): an unresolvable job class is logged and then dropped viaremoveById()— "Remove job from disabled app or old version of an app". Measured, not assumed; a repair step was written first and deleted once the source said it was redundant.Can-fail proof
One item reverted at a time, each gate required to name it:
AuditLogController.php:126 method=verify rule=missing-auth-attributeDecisionController.php:225 method=publish rule=no-admin-required-annotation-with-admin-bodyNotificationPreferenceService.php:194 method=createPreferenceDoesNotExistException— the live 500, reproducedAll restored; all back to PASS.
Other checks
phpunit 826 / 3009 assertions / 0 failures (823 + 3 new, minus the 5 belonging to the deleted job) · phpcs 0 errors, 98 warnings · phpstan 0 · psalm 0 · phpmd 0 on both rulesets.
🤖 Generated with Claude Code