fix(security): wave-3 critical fixes - #669
Closed
rubenvdlinde wants to merge 2045 commits into
Closed
Conversation
Contributor
Author
|
Auto-merge blocked: unrelated histories between development and main. The security fixes are committed and pushed to Recommended resolution: Admin-merge or manually resolve by rebasing main from development, then merge this PR. Alternatively, a Nextcloud App release tag can be cut directly from |
…logi' (#202) from wip/woo-publication-via-opencatalogi into development
…hemas, zero write verbs' (#203) from wip/mcp-adoption into development
Municipalities must submit quarterly IV3 financial reports to CBS broken down by taakveld (BBV functional classification). Case types gain an optional iv3Taakveld code; cases gain a lightweight kosten record (leges income / handling cost, dated). Iv3ReportService aggregates per taakveld per quarter (case counts, totals, avg cost per case; cases without a taakveld are reported as uncategorized) via OpenRegister's ObjectService — no raw SQL, no new financial subsystem duplicating Pipelinq's fee ledger (ADR-003) or the Subsidie domain's settlement chain. New GET /api/reports/iv3 (JSON/CSV, gated to controllers/ beheerders/admin) and GET /api/reports/iv3/taakvelden endpoints, an Iv3ReportDashboard alongside the existing Termijn/Doorlooptijd reports, and a taakveld picker on case-type settings.
…aakveld case cost reporting' (#204) from wip/iv3-case-cost-reporting into development
…ad-intelligent-queue
…y scoring + coordinator workload' (#205) from wip/werkvoorraad-intelligent-queue into development
Procest ships authoritative BRP and KvK lookup seams but had no authoritative BAG seam for VTH/spatial cases. Adds BagAdapterInterface + BagApiAdapter (Kadaster BAG API Individuele Bevragingen v2, config-tier selection, dormant by default) + BagResponseMapper (pure normalization) mirroring the BRP/KvK adapter pattern exactly, plus a BagController exposing address/pand/ verblijfsobject lookups and a bagApi.js frontend shim. Distinct from the existing free/open PdokBagService (BAG WFS mirror) — see openspec/changes/archive/2026-07-13-bag-register-adapter/design.md for the PDOK-overlap analysis and the postcode+huisnummer search gap this closes.
…ress + pand/verblijfsobject lookup' (#206) from wip/bag-register-adapter into development
… publish path, keyboard a11y
Deletes the dead, unwired @vue-flow-based duplicate editor
(src/components/workflow/**, 6 files) — it can never build under this
app's Vue 2.7 stack (@vue-flow is Vue-3-only) and its own validator.js
modelled the graph on the wrong entity (step.isFinal instead of
statusType.isFinal). The canonical, already-wired WorkflowEditor.vue
(via WorkflowTab.vue) survives and gets the gaps closed:
- Real validation: new src/utils/workflowGraphValidation.js checks the
actual engine constraints (no/unreachable final status, dangling/
duplicate transitions, orphan nodes, cycles with no exit), replacing
the store's weak inline check.
- Publish now calls the canonical POST /api/workflow-definitions/{id}/publish
endpoint instead of writing isDraft/isActive flags directly, so
referential-integrity checking, role-authorization freezing, previous-
version deprecation and case-type pinning happen from the canvas too.
- Closes CRUD gaps: delete a status node (guarded — at least one final
status must remain) and delete a step (removeStep() existed but was
never wired to any control).
- Keyboard operability mirroring the kanban pattern: nodes are
focusable/Enter/Space-selectable, get a keyboard-reachable actions
menu (Connect to/Disconnect from/Add step/Delete status), and the
palette gets a visible "Add status node" button as a drag-and-drop
alternative.
- Tests: vitest for every validation rule + a serialization round-trip,
component smoke tests for WorkflowEditor.vue (renders a definition;
validate() blocks the exact gate WorkflowTab.vue::publish() calls),
and a non-destructive Playwright spec for canvas/keyboard reachability.
Adds a scoped jsdom component-test lane (@vitejs/plugin-vue2,
@vue/test-utils, jsdom) alongside the existing node-environment suite.
- i18n: 19 new strings added as English/Dutch pairs.
…ditor-integration # Conflicts: # l10n/en.json # l10n/nl.json
…ration — validation, publish path, keyboard a11y' (#207) from wip/workflow-editor-integration into development
Change merged in #207 — moves it to openspec/changes/archive/2026-07-13-workflow-editor-integration/ and syncs the visual-workflow-editor spec delta into openspec/specs/.
Adds ProcessMiningService (dwell-time stats, bottleneck ranking, transition matrix + rework-loop detection, weekly throughput trend) computed from the statusRecord chain StatusTransitionService already writes on every case transition, a coordinator-gated report endpoint (same auth shape as Iv3ReportController), and a dashboard using existing nc-vue leaves (CnKpiGrid, CnStatsBlock, CnChartWidget) — no new chart components. Also fixes a pre-existing bug in the shared FakeTermijnStore PHPUnit fixture: it didn't strip OpenRegister's _limit/_offset pagination keys before applying its equality filter, so any paginating service under test would silently get zero rows back.
… statusRecord history' (#209) from wip/process-mining-bottlenecks into development
…path validation
Closes the bag-register-adapter tasks.md item 4.1 follow-up: the location
schema's source: bag / nummeraanduidingId fields had zero enforcement.
LocationBagValidationListener hooks OpenRegister's pre-persist
ObjectCreatingEvent/ObjectUpdatingEvent (the same generic object-save event
pipeline every procest schema goes through) and rejects a location save
whose source=bag lacks a syntactically valid 16-digit nummeraanduidingId.
When BagAdapterInterface is non-dormant (test/live tier), it also attempts
a best-effort existence check via lookupObject('nummeraanduiding', ...),
extended onto BagApiAdapter's OBJECT_PATHS for this change, failing open
(accept-with-warning) on any inconclusive outcome.
…on-save-validation
…ion source=bag save-path validation' (#210) from wip/bag-location-save-validation into development
…ed views, version history Bumps @conduction/nextcloud-vue to 1.0.0-beta.211 (published fresh — beta.210 predated PR #215/#216) and enables/configures the four just-merged nc-vue leaf features per ADR-Leaf-First: - Saved views: allowSavedViews on Cases, Bezwaren, Tasks, Voorstellen, Advice, Beroepen. - Multi-column sort: rides CnIndexPage/CnDataTable automatically, no procest-side sort code exists to touch. - Notes @mention: new CaseNotesTab.vue sidebar tab (wraps the library's CnNotesTab via leafTab('notes')) forwards the mention event to a new POST /api/notes/mention endpoint, which dispatches real NC notifications via a new MentionNotificationService + Notifier. - Version history: new VersionHistoryLeafTab registry entry (leafTab('version-history')) wired as a sidebar tab beside "audit" on all 21 detail pages.
Moves the completed change to openspec/changes/archive/2026-07-14-ncvue-w2-leaves-adoption/ and syncs its spec into the canonical openspec/specs/ncvue-w2-leaves-adoption/.
… sort, mentions, saved views, version history' (#211) from wip/ncvue-w2-leaves-adoption into development
Third and final slice for procest's src/. Twelve Dutch source strings across
eight components move to English.
KEPT — the judgement calls
'College van B&W' statutory body name
'Grounds for Objection (Gronden van Bezwaar)' already English-first
'Inadmissible (niet-ontvankelijk)' already English-first
A COLLISION THIS SLICE EXPOSED, AND THE RULE IT GIVES
'Verplicht' translates to 'Required' — which ALREADY EXISTS as a separate source
string in every locale. Renaming onto it produced a duplicate key in all 37
files, silently collapsing two entries per file with no way to tell which value
survived. The integrity check caught it before commit.
Correct handling for a rename whose TARGET ALREADY EXISTS is DELETE, not rename:
the component now emits t('procest', 'Required'), which already has a
translation in every locale, so the old 'Verplicht' entry is simply removed. Its
translation is not lost — the surviving 'Required' entry already carried it.
That generalises: before retargeting a key, check whether the English target is
already a source string. If it is, the entry is deleted and the existing
translation takes over.
VERIFIED
- Source diff is 13 lines, every one a t('procest', ...) literal — 0 non-t()
lines changed.
- All 38 locale JSON files parse; 349 keys renamed, 38 colliding entries
deleted.
- Entry-count delta is -1 or 0 in every file — exactly the removed 'Verplicht'
entry, and 0 where the locale never had it. No unexplained loss.
- 0 duplicate keys, 0 orphaned Dutch source keys anywhere in l10n/.
- en_US.json flagged as lacking 'Required'; checked, and it never had either
key (675-entry partial locale). Nothing removed from it.
- nl.js EXECUTED: 2436 entries, sample key resolves to its Dutch text.
Nextcloud runs migrateSchemaOnly() on a first install: $previousVersion is '', so Installer::installAppLastSteps() skips BOTH pre-migration and post-migration, and <install> is the only unconditional hook. The upgrade path runs pre/post-migration and NOT install, so an app needs both blocks carrying the same baseline steps, each idempotent. Until now this app declared no <install> block at all, so the Procest register and twelve seed steps never arrived on a fresh instance. Only baseline-CREATING steps are added; migrations, backfills, renames and cross-app ingests stay upgrade-only so they never run against an empty database. <install> is placed after </post-migration> per the info.xsd sequence (pre-migration, post-migration, live-migration, install, uninstall), verified against the schema.
* chore: adopt nextcloud/coding-standard, .editorconfig and NC 34 Configuration only. The reformat is the next commit on purpose, so .git-blame-ignore-revs can name a revision containing nothing but whitespace. - .php-cs-fixer.dist.php + conduction/coding-standard, which extends nextcloud/coding-standard and can only ADD to it — enforced by that package's invariant test, not by review. - cs:check / cs:fix now run php-cs-fixer. They were aliases for phpcs/phpcbf, so the documented Nextcloud command reformatted code AWAY from Nextcloud's standard. - nextcloud/coding-standard dropped as a direct dependency. It arrives transitively at a version conduction/coding-standard has tested against; declared directly it was a dead dependency with no config and no invocation. - phpcs.xml is now a stub over the shared semantics-only ruleset, and the local phpcs-custom-sniffs/ copy is gone. The fleet was carrying six divergent versions of NamedParametersSniff.php — a custom RULE, not a setting. - .editorconfig, verbatim from nextcloud/server. No fleet app had one, so an editor configured by someone's previous Nextcloud work defaulted to tabs, which the old ruleset then rejected. - nextcloud/ocp -> ^34.0 and PHPUnit -> stable34. This app declared support for NC 34 while being analysed against 31, so a symbol REMOVED in 32/33/34 was invisible to the type checker. That is why the NC 34 removal of \OC::$server needed a hand-written PHPCS sniff. - the stylelint glob is quoted, so stylelint expands it rather than the shell. Unquoted, src/**/ matches exactly one directory level and nested components are silently unlinted. gate-65 (coding-standard-adoption) enforces all of the above from ConductionNL/.github@main. This app failed it; with this commit it passes. * style: reformat with nextcloud/coding-standard — whitespace only Applied by php-cs-fixer with conduction/coding-standard. Tabs, same-line braces, (int)$x, single-space concatenation, ordered imports — Nextcloud's dialect, which this app now passes unchanged. 915 file(s), no behaviour change. Isolated from the configuration change so .git-blame-ignore-revs can name a revision that touches nothing but formatting. Reviewing it line by line is not a useful activity; the previous commit is the review. * chore: ignore the reformat commit in git blame 6fd5463 touches 915 files and changes no behaviour. Without this, every line it reflowed attributes to it and the real author is one --skip away. GitHub honours the file automatically; locally it needs `git config blame.ignoreRevsFile .git-blame-ignore-revs` once. * fix: regenerate composer.lock for the new constraints The previous commit changed composer.json without touching the lock, so `composer install` refused with exit 4 and EVERY PHP job failed: Required (in require-dev) package "conduction/coding-standard" is not present in the lock file. Required (in require-dev) package "conduction/hydra-gates" is not present in the lock file. Required (in require-dev) package "nextcloud/ocp" is in the lock file as "v31.0.9" but that does not satisfy your constraint "^34.0". Nothing was wrong with the reformat or the ruleset — the jobs never got as far as running a tool. Measured on larpingapp#313 before this fix: phpcs, psalm, phpstan and both PHPUnit legs red, all of them at `composer install`. Hydra Gates passed in the same run, because it does not install composer dependencies. Now locked at conduction/coding-standard v1.0.0, conduction/hydra-gates v1.7.0, nextcloud/ocp v34.0.2 — the last of which is the point of the exercise: this app declares support for NC 34 and is now analysed against it. * fix(tests): the Doctrine SQLLogger stub is empty, and NC 34 makes that fatal Both PHPUnit legs died at bootstrap on run 31596763401, before a single test: PHP Fatal error: OCP\Diagnostics\IQueryLogger::startQuery() has #[\Override] attribute, but no matching parent method exists in server/lib/public/Diagnostics/IQueryLogger.php on line 29 The path in that message points at Nextcloud, which is misleading — the file that causes it is ours. `OCP\Diagnostics\IQueryLogger extends Doctrine\DBAL\Logging\SQLLogger`, and NC 34 added `#[\Override]` to its startQuery() and stopQuery(). PHP validates that attribute against whatever SQLLogger is loaded, and tests/bootstrap.php loads this stub FIRST on purpose, so ours is the one it checks. It declared neither method. The stub was already wrong on stable32; the attribute simply did not exist there yet to catch it. What this PR changed is the leg it is measured against. Both arms measured on php:8.3-cli and php:8.4-cli, empty stub vs this one: the empty interface reproduces the CI message verbatim, this one compiles. Not a waiver and not a scope change: nldesign's stable34 legs are green on the same shared workflow, so stable34 is sound and this defect is procest's. `composer cs:check`: Found 0 of 914 files that can be fixed. * fix(ci): test the whole declared Nextcloud range, not only the ceiling appinfo/info.xml declares <nextcloud min-version="32" max-version="34"/>, but nextcloud-test-refs was '["stable34"]' — so the declared floor and the middle major were advertised to the App Store with no job touching either. This is the coding-standard migration's own defect: its rollout REPLACED the ref list instead of extending it. The programme opened by reporting that nothing was tested on NC 34 and, in fixing that, made 32 and 33 the untested end. Same drift, other direction. stable34 stays first because newman, playwright and journeydoc-capture all read fromJSON(inputs.nextcloud-test-refs)[0] as their single server. Verified green on all three refs against nextcloud/ocp ^34 on portaliq (run 31599055849, six PHPUnit legs: 32/33/34 x PHP 8.3/8.4).
phpmd.xml becomes a 9-line stub referencing vendor/conduction/hydra-gates/quality-config/phpmd.xml, and the local phpmd-unusedparams.xml is deleted in favour of the central copy, which the unused-parameters leg of the composer phpmd script now points at. Both legs, their flags and the worst-exit-code behaviour are unchanged. Co-authored-by: Ruben van der Linde <release-bot@conduction.nl>
phpstan.neon now includes the shared base shipped in conduction/hydra-gates (quality-config/phpstan-base.neon) and keeps only what is genuinely local to this app. Requires hydra-gates v1.7.1 — v1.7.0's base declared bare relative paths, which PHPStan resolves against the file that declares them, so the run aborted before analysing anything. composer.lock is updated accordingly; no other package moved. Verified with phpstan dump-parameters before and after: level, paths, excludePaths, bootstrapFiles and scanDirectories resolve byte-identically, the same number of files is analysed on both sides, and the finding count is unchanged.
Moves the pin from 2.2.0-vue3.9 to the current vue3 dist-tag. vue3.16 ships the shared scripts/ directory (13 check scripts including check-integration-parity.js) in the published tarball and fixes that checker so it can no longer report a pass while correlating nothing. This unblocks hydra gate-24, which on 2.2.0-vue3.9 cannot find its own checker and therefore verifies nothing. Lockfile regenerated with npm 10.8.2 to match the Node 20 / npm 10 toolchain used in CI; npm ci verified from a clean node_modules. Verified locally: npm run build, npm run test:unit (34 files / 349 tests), eslint and stylelint all pass. No call-site migration was needed. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…830) Lock-only. composer.json is untouched: the ^1.0 constraint is correct and stays floating. v1.7.3 removes two conditional paths from the shared phpstan-base.neon (%cwd%/vendor-bin and %cwd%/lib/Resources/template). A conditional path in a shared base has no spelling that is safe on both PHPStan majors: plain is validated and ABORTS on PHPStan 2.x, the '(?)' marker is parsed as a NEON entity after a %...% expansion and crashes 2.x, and quoting it stops 1.x from stripping the marker so the exclusion silently matches nothing. This app is on PHPStan 1.12.x, so it is not broken today, but it carries the landmine until it moves to PHPStan 2. It has neither vendor-bin nor lib/Resources/template, so no phpstan.neon change is needed.
…ser-writable mailbox settings, unscoped dossier download, two per-case VTH reads (#828) Audit of procest's 79 @publicpage methods plus a gate-7 triage. Four guard-only fixes; gate-7 31 -> 27, gate-25 118 -> 116, no gate regressed and no finding introduced (compared by file+method, not line). - POST /api/stuf/{zaken,personen} had NO authentication and reached a responder dispatching zakLk01, zakLv01, npsLv01 (person query by BSN) and edcLk01. All four handlers are stubs today, so nothing leaked - which is why the guard lands now, before the body the comments promise. Same WSSE predicate inkomend() already used. - EmailTemplateController::{getSettings,saveSettings,testImap} wrote INSTANCE-WIDE IMAP config incl. the shared-mailbox password under @NoAdminRequired, and chained into an internal port prober. Now #[AuthorizedAdminSetting], the posture this controller already used for createTemplate(). - ZaakdossierDownloadController::downloadZip - the clearance filter compares a classification ordinal, not case membership. Sibling DossierExportController::plan() was already guarded. - The two per-case VTH sub-resource reads. Merged over a red Hydra Gates / Quality Report cell, which are red on development itself and are NOT caused by this branch: gates 3, 19, 26, 53, 57, 62 are byte-identical to development and gates 7 and 25 are strictly lower. phpmd went red on this branch and was fixed honestly (a genuinely imprecise return type), not suppressed. Evidence: run 31652875714 [pull_request] on d77fd9e - 28 cells green, 5 skipped, 2 red; COVERAGE 61 of 65 (61 of 61 applicable ran), gates log 147,206 bytes.
…oo (#833) CSS and SCSS were enforced by nothing: @nextcloud/stylelint-config carries no indentation rule. @nextcloud/prettier-config (useTabs: true, tabWidth: 4) covers CSS/SCSS as well as JS/Vue. eslint-config-prettier is spread LAST in eslint.config.js so the two formatters cannot demand opposite things. l10n/ and the two committed result-vue-quality-* report directories are in .prettierignore. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
This repo carries `@nextcloud/prettier-config` and a `format` script, but nothing ever ran that script in CI. The shared `quality.yml` has NO prettier job of its own — it mentions prettier ZERO times (eslint 9, stylelint 10) — and `frontend-checks` is the only opt-in that can invoke a repo's own npm scripts. So prettier was active in developers' editors and inert everywhere else: exactly the state the fleet's old `.prettierrc` was deleted for. Appending "format" to the existing `frontend-checks` array adds one `Frontend Check (format)` job. Every pre-existing entry and its order are unchanged. Measured on this tree before enabling, with the same prettier version and the same resolved config CI uses; the per-app scope and result are recorded in the comment above the input. Centralising the config never stopped drift; the gate does.
The glob was src/**/*.{vue,scss,css}, so this app's css/ directory had never
been linted. @nextcloud/stylelint-config 2.4.0 does ship indentation: 'tab' —
the rule worked, those files were never shown to it. A coverage gap, not a rule
gap.
Verified the widened glob actually reaches css/ by planting a space-indented
rule in a sibling app and seeing 'Expected indentation of 1 tab' reported. A
glob that matches nothing is indistinguishable from a clean directory.
handleFederatedTransfer, postRemoteActivity and listRemoteActivity are all #[PublicPage] with the share token in the URL as the only credential, and had no throttle of any kind. A guessed token accepts or rejects a case transfer between authorities. Unlike the other controllers in this sweep there is no single shared guard -- each endpoint resolves the token its own way (resolveFederatedTransferShare returning null; the collab service returning an error key). Registration is therefore at all three rejection points, but under ONE action, so guesses cannot be split across the three to stay under a per-endpoint ceiling. Both halves present: #[BruteForceProtection] enforces, registerAttempt() counts. Either alone is inert -- ADR-082. Limits 30/60 on the two acts, 60/60 on the read. The controller had no logger; one is injected alongside the throttler so the registerAttempt failure path can report rather than swallow. Tests: the federation test constructs this controller directly. Note there are TWO constructions in that file -- CaseSharingController and CaseFederationController -- and only the latter changed. Full suite: 1889 tests, 6415 assertions, Deprecations 7, Skipped 5, OK -- identical to the baseline captured before the change. NOT verified behaviourally; the 429 control was run against openregister federation.
…orce fix(security): brute-force throttle the case federation share tokens
Same migration as the pilot (ConductionNL/larpingapp#325) and the template (ConductionNL/nextcloud-app-template#146). eslint.config.mjs is the canonical copy; only the app-specific blocks at the end differ. Requires Node 22 — @nextcloud/eslint-config@9 declares engines.node ^22.14 || ^24 || >=26 and imports findPackageJSON from node:module, first available in 22.14. The shared workflows already default to 22 (ConductionNL/.github#450). Stale eslint-8-era direct deps and overrides are removed, and the two peers the config needs are declared at the right major (vue-eslint-parser ^10.3.0, @typescript-eslint/parser ^8.67.0). An overrides entry resolves nc-vue's OPTIONAL eslint peer against eslint 10 — optional means npm will not install it, not that a mismatched version is accepted. Coverage: all 231 .vue files parse, 0 fatal errors. 726 PRE-EXISTING VIOLATIONS ARE RECORDED, NOT FIXED ------------------------------------------------------ v9 enables rules this app has never run. --fix and prettier resolved the mechanical ones; 726 findings across 206 files remain and are recorded in eslint-suppressions.json using eslint's own bulk suppressions (--suppress-all), NOT by loosening any rule. Every rule keeps the severity @nextcloud/eslint-config gives it, the debt is counted per file, and --prune-suppressions shrinks it as it is paid down. A NEW violation still fails the run — verified on openconnector by appending a console.log after suppressing. 329 jsdoc/require-param-type 111 no-console 91 @nextcloud/l10n-enforce-ellipsis 86 @typescript-eslint/no-unused-vars 80 vue/custom-event-name-casing 11 @nextcloud/no-deprecated-library-props 8 no-unused-vars 3 eqeqeq 3 no-useless-assignment 1 no-undef 1 jsdoc/check-tag-names 1 vue/no-useless-v-bind TWO AUTOFIXES ARE DELIBERATELY WITHHELD (recorded as debt instead) ----------------------------------------------------------------- - @nextcloud/l10n-enforce-ellipsis rewrites '...' to the typographic '…' INSIDE translatable strings. That changes the translation KEY and orphans every l10n/*.json entry for it; on openconnector it turned the l10n parity check red while it was green on development. The migration must not silently drop translations, so the source strings are left alone. - vue/prefer-define-options rewrites 'export default {…}' in a plain <script> into 'defineOptions({…})' inside <script setup>, carrying props/data/computed across the block boundary. @vue/compiler-sfc then rejects the result outright (defineOptions() cannot declare props), which broke the build in softwarecatalog and docudesk. It is a semantic refactor, not a lint fix. TEST GLOBALS ARE DECLARED, NOT SUPPRESSED ----------------------------------------- Spec files that live under src/ have no framework globals, so no-undef reported every describe/it/expect as undefined — 1203 findings in openregister from just 7 identifiers. Declaring the environment removed ~1900 phantom findings fleet-wide. Suppressing them instead would have buried any REAL no-undef, which is the rule that catches a typo'd identifier. VERIFIED -------- npm run lint PASS npm run stylelint PASS npm run format PASS npm run build PASS
…ion (#832) * refactor(procest): translate Dutch vocabulary to English, with migration Applies the fleet Dutch->English vocabulary pass to procest: 326 schema property names plus the PHP identifiers that read and write them. zaak -> case zaaktype -> caseType besluit -> decision bezwaar -> objection behandelaar -> handler vergunning -> permit dwangsom -> penaltyPayment hersteltermijn -> remedyPeriod burgerservicenummer -> citizenServiceNumber ZGW TERMS ARE TRANSLATED, per Ruben's rule that Dutch terms for Dutch things get renamed. The exemption is narrow and unused here: an external product's proper name, and statutory WIRE field names at the adapter layer only. FLEET VOCABULARY IS SHARED, NOT PER-APP. Two tokens collided with choices shillinq already SHIPPED in #533/#534 — `toelichting` and `motivering`. Both were resolved in shillinq's favour (`notes`, `rationale`). A second app picking a different English word for the same Dutch one forks the fleet vocabulary, which is the opposite of the point. IDENTIFIERS RENAMED VIA AST, NOT REGEX. Same nikic/php-parser tool as shillinq. It earned its keep immediately here: the SCOPE-AWARE collision check caught 25 renames that would have been fatals, including private function resolveContestedDecisionAuthor( string $bezwaarSchema, // -> $objectionSchema string $objectionSchema, // ...which ALREADY EXISTS ) "Redefinition of parameter" — a fatal that a schema-level collision check cannot see, because the clash is between two PHP variables rather than two properties. MIGRATION. 326 column pairs, ported from shillinq's step with the register slug changed to `procest`. Verified free of duplicate SOURCE keys; 5 targets have more than one source and the step's guard REFUSES those rather than merging. Its @SPEC is a reason-bearing exclude: no canonical spec covers a vocabulary migration, and pointing at an unrelated one would report conformance to a requirement that says nothing about it. VERIFICATION IS INCOMPLETE, AND I AM NOT CLAIMING OTHERWISE procest requires PHP >= 8.3; this host runs 8.2.22, so composer's platform check fatals before phpunit, phpcs, phpmd, psalm or phpstan can start. What I COULD run locally, I ran: - php -l clean on every changed PHP file (this is what caught the fatal above); - every changed JSON parses; - info.xml parses and EVERY repair step it declares resolves to a real file; - the migration map has no duplicate source keys. The test suite and the static analysers are UNVERIFIED locally. CI is the only signal for those, and this must not merge until it is green. * fix(procest): two real defects the rename introduced, found via a container loop procest needs PHP >= 8.3 and this host runs 8.2, so composer's platform check fatals before any analyser starts. Rather than push blind and let CI find them, I ran the tools inside the nextcloud container (PHP 8.4) against a copy of the worktree. That turned a ~15-minute CI round trip into a local loop, and it found both defects below. 1. A PUBLIC PROMOTED PROPERTY WAS RENAMED, ORPHANING EVERY EXTERNAL READ `BrokerAssertionResult::$kvkNummer` is public readonly. The AST pass renamed the declaration to `$kvkNumber`, but it only rewrites property FETCHES through `$this->`, so `$assertion->kvkNummer` in a caller kept the old name and started reading null. Same class as the public-method exclusion, which I had already reasoned about and then failed to apply to promoted properties. The tool now collects PUBLIC and PROTECTED promoted properties as API and refuses to rename them — matched by the exact declaration position, not by name, so a local `$register` in an unrelated function is still renamed normally. Here the property keeps its English name and the CALLER moved instead. The three remaining `'kvkNummer'` string keys are deliberately untouched: incoming eHerkenning SAML JSON and the outgoing KvK API query parameter are WIRE fields, which is the one exemption this work honours. 2. TWO DIFFERENT CONFIG KEYS COLLAPSED ONTO ONE PARAMETER NAME `resolveContestedDecisionAuthor()` took both `$bezwaarSchema` and `$objectionSchema`. They are not duplicates — they come from DIFFERENT config keys, `bezwaar_schema` and `objection_schema`, and are used for different lookups. Renaming the first onto the second produced "Redefinition of parameter" and then "Argument already passed". Renamed to `$legacyObjectionSchema` so the two stay distinct. The config KEY stays `bezwaar_schema`: it is stored app config, so translating it is a data migration rather than a code change, and it is noted in the code as such. ALSO FIXED, both PRE-EXISTING and confirmed so on clean development: an over-long docblock line and a lower-case inline comment. phpcs error count goes 3 -> 2 against a baseline of 3. VERIFIED in the container, against a CONTROL run of clean development - phpstan [OK], psalm 0, phpmd 0 real violations. - phpunit 1889 tests: 4 errors / 2 failures — the EXACT counts the baseline produces, from a Symfony HeaderUtils class missing in my locally-built vendor. Not caused by this branch. * fix(procest): phpmd — five over-long renamed variables and one split method CI's phpmd found 8 violations my local run reported as zero. The command was right; my READING of it was not — I grepped its output for `^lib/` while phpmd prints ABSOLUTE paths, so a run with 8 findings looked clean. Same shape as the phpcs rig failure earlier in this programme: the check ran, I measured the wrong thing. 5x LongVariable — translations that came out longer than the limit: $parafeerActionService -> $signOffService $primairDecisionCaseId -> $primaryDecisionCase $authorisedRepresentative -> $representative $legacyObjectionSchema -> $legacyObjSchema $penaltyPaymentService -> $penaltyService A longer name is not more English, just longer. 1x CyclomaticComplexity — shardTables() in the migration I ported from shillinq. Split into registerIds() and openRegisterTableNames(); behaviour unchanged, and the split is documented as being for the limit rather than for its own sake. VERIFIED in the container against a control run of clean development: phpmd 0 · phpstan [OK] · psalm 0 · phpcs 2 errors against a BASELINE OF 3 (both remaining are pre-existing and identical on development). phpunit 1889 tests, 4 errors / 2 failures — the exact counts the baseline produces, from a Symfony HeaderUtils class absent in my locally-built vendor. * test(procest): cover the vocabulary migration — coverage guard, and a real gap The one remaining red cell was the coverage guard, not a failing test: Coverage current: 31.23% (14607/46778 statements) Coverage merge base: 31.30% (14607/46666 statements) FAIL: coverage dropped by 0.07% against the merge base. My migration added 112 statements and zero tests. The ratchet is right: a migration that moves customer data across 326 columns and is exercised by nothing is exactly the thing that should not merge. So this adds the tests rather than tuning the threshold. They assert the properties I had been checking BY HAND on every batch, which is the reason they belong in the suite — a hand-check does not run again when someone extends COLUMN_MAP: - every entry is snake_case. OpenRegister stores `requestedAmount` as the column `requested_amount`, so a camelCase entry never matches a real column: a migration that silently does nothing. - no target is also a source. A chain (a => b, b => c) moves data twice depending on iteration order. - an ambiguous rename (two sources, one destination, one table) is REFUSED. - and the negative control for it: a single source is NOT treated as a collision. Without that, a guard that always returned true would pass this suite while migrating nothing at all. POSITIVE CONTROL RUN, not assumed. I injected a rename chain and a camelCase entry into COLUMN_MAP: 2 failures. Restored: 5 tests, 1308 assertions, green. A test that has never been seen to fail is not evidence. * fix(procest): the schema rename never landed — the PR shipped a migration for renames that did not exist SERIOUS DEFECT IN MY OWN WORK, found only because Ruben asked what was left. While iterating on the AST tool I ran `git checkout -- .` to reset between attempts. One of those calls reverted the SCHEMA rename, which had been applied but not yet committed. Everything after it committed the identifier rename and the migration — and the commit message claimed "326 schema property names", which was false. Proof it never landed: all 326 map keys were still present as JSON keys in lib/Settings/. Now 0 of 326. WHY THIS WAS DANGEROUS, NOT MERELY INCOMPLETE The migration shipped a COLUMN_MAP for renames the registers never received. On upgrade it would have renamed `zaaktype` to `case_type` while the register still declared `zaaktype` — so MagicMapper would re-add an empty `zaaktype` column and every existing value would sit orphaned in `case_type`. Silent data loss on case records. The PR was at 30 GREEN CHECKS while carrying this. Nothing in CI asserts that a migration's source columns correspond to properties the registers actually renamed, so green meant only "the code compiles and the old tests still pass" — which is exactly what you would expect of a change that did nothing to the registers. The lesson I keep relearning, in a new costume: I verified the identifier rename (counts dropped 799 -> 422) and inferred the schema rename had landed too, because they were "the same change". They were two passes, and only one of them survived. This commit applies the schema rename for real: 3402 substitutions across 360 files. The suite and phpstan now surface 27 static errors and 27 test failures that were invisible while the registers were untouched. Those are the genuine cost of the translation and are being worked next; the PR is marked draft so it cannot merge in the meantime. * refactor(procest): redo the translation through a scripted pipeline Rebuilt from clean development. The by-hand sequence went wrong three times, so the checklist is now a script (/home/rubenlinde/translate-app.sh) with the failures encoded as ASSERTIONS rather than things to remember: - a `git checkout -- .` between tool iterations silently reverted the schema rename, and the migration then shipped for renames that never happened. Now: "map keys still present in lib/Settings: 0 of N" or the run ABORTS. - the property map was generated from a dictionary that was later corrected, so stale entries survived a re-run. Now the map is regenerated every run. - SCHEMA NAMES were renamed alongside properties: `bezwaar` collapsed onto an existing `objection` and DELETED a schema (85 -> 84). Now schema and register names are excluded from the map, and the count is asserted. WHAT THE TRANSLATION ACTUALLY TOUCHES: 313 property names, 2896 identifiers, 469 comment references, 17 docblock shapes. 3012 substitutions across 306 files. A FALSE FRIEND, AND WHY THE DICTIONARY LOST AN ENTRY Dutch `last` (as in `last onder dwangsom`) is also the English word "last". The token mapping turned `lastError` into `orderError` in three files, and eight register properties (`lastLoginAt`, `lastPaymentDate`, ...) with it. A bare token that is ALSO ordinary English cannot be disambiguated by a renamer, so `last` is out of the dictionary; the Dutch sense is expressed as whole compounds (`lastonderdwangsom`, `dwangsom`) instead. STRINGS THAT ARE CONTRACTS WITH SOMETHING ELSE — six more, all restored: - CSV column headers (`rolNaam`): the header row of a file authored outside this codebase. Renaming it fails every existing import. - ZGW RESOURCE_KEYS (`zaaktype`, `resultaat`, `rol`): statutory wire names that ALSO form stored config keys (`zgw_mapping_zaaktype`), so a rename strands every saved mapping. The list is restored verbatim from development. - a Nextcloud GROUP name (`behandelaar`): a renamed group matches nobody. - a mock's method name: `->method('akkoord')` names a PUBLIC method the AST pass deliberately does not rename. - email template placeholders `{{naam}}`: authored content, not code. - two config keys behind `bezwaar_schema`, kept distinct from `objection_schema` — they are different values and collapsing them is a bug. TOOL FIXES EARNED HERE: named arguments now respect the public-API guard (a public promoted property is not renamed, so its call sites must not move either), and the parenthesised duplicate-coalesce form `$x['a'] ?? ($x['a'] ?? null)` is collapsed — psalm flagged 7 of those. VERIFIED in the container (PHP 8.4; this host is 8.2 and cannot run procest's tools at all), each result compared against a CONTROL run of clean development: phpstan [OK] · psalm 0 (baseline 0) · phpmd 0 · phpcs 1 error against a BASELINE OF 3 · phpunit 1894 tests with 4 errors / 2 failures, the EXACT counts the baseline produces from a Symfony HeaderUtils class missing in the locally-built vendor. * test(procest): cover the vocabulary migration — the coverage ratchet was right The one remaining red cell was the coverage guard, not a failing test: the migration added statements and no tests, so coverage fell against the merge base. That is the correct complaint — a step that moves customer data across 313 columns and is exercised by nothing should not merge. Nine tests, asserting the properties I had been checking BY HAND on every batch. That is the point: a hand-check does not run again when someone extends COLUMN_MAP. - every entry is snake_case. OpenRegister stores `requestedAmount` as `requested_amount`, so a camelCase entry never matches a real column — a migration that silently does nothing. - no target is also a source, so no rename chains (`a => b`, `b => c` moves data twice depending on iteration order). - NO TARGET IS HALF-TRANSLATED. `ingangsdatum_gewenst -> effective_date_gewenst` actually shipped in this branch before this assertion existed. - an ambiguous rename (two sources, one destination, one table) is REFUSED. - the negative control for it: a single source is NOT a collision. Without that, a guard hard-wired to true passes the previous test while migrating nothing at all. - run() with no resolvable registers touches nothing and reports — the fail-soft path for an install without the registers. - a failing statement is swallowed, because a repair step that throws aborts the whole upgrade; and its counterpart, that a successful one reports true. POSITIVE CONTROL RUN, not assumed: injecting a rename chain and a half-Dutch target produced 2 failures; restored, 9 tests and 1573 assertions green. A test that has never been seen to fail is not evidence. One case was dropped rather than forced: `quoteSingleIdentifier` cannot be configured on the DBAL platform mock in this version. * test(procest): cover run()'s rename path — closes the coverage ratchet Adds the case that exercises the path which actually MOVES customer data: registers resolve, the shard table matches the marker, the old column is present and the new one is not, therefore ALTER ... RENAME COLUMN. Mocked through OCP\DB\IResult and IPreparedStatement rather than the Doctrine classes, which are not resolvable here. Coverage against the merge base was -0.06%, then -0.01% after the map tests. This closes the remainder. MY FIRST POSITIVE CONTROL FOR THIS TEST WAS INVALID, WHICH IS WORTH RECORDING I injected `RENAME KOLOM` with a first-occurrence replace and the test still passed — I read that as "the test is weak". It was not: the replace had hit the phrase inside a DOCBLOCK on line 15, not the SQL on line 468. The code under test never changed, so of course nothing failed. Re-run against the actual statement: 1 failure with the defect, clean when restored. A control that does not perturb the thing under test proves nothing, and it looks exactly like a control that does. * refactor(procest): use politicalGroup for fractie, matching what decidesk shipped The fleet dictionary must not fork: one Dutch word, one English word, across every app. decidesk already SHIPPED `fractie -> political_group` on development; procest was about to land `faction` for the same word. Resolved in favour of the shipped term. Caught while running the pipeline over decidesk, whose existing RenameDutchVocabularyColumns still carried the mapping — which is the only reason the conflict surfaced at all. The pipeline now reads an app's existing vocabulary migration before appending to it. VERIFIED in the container: phpunit and phpstan unchanged from the previous run. * fix(procest): snake_case the migration targets my regex left camel The realignment to `politicalGroup` was applied with a word-boundary regex that also rewrote COLUMN_MAP, producing `political_group_results` as `politicalGroup_results`. COLUMN_MAP is snake_case by contract — OpenRegister stores `politicalGroupResults` as the column `political_group_results`, so a camelCase entry matches no column and migrates nothing. Caught by the testEveryEntryIsSnakeCase case added two commits ago. That is the assertion earning its place: the same defect shipped silently before the test existed, because a migration that matches nothing looks exactly like one with nothing to do. phpunit back to the baseline 4 errors / 2 failures. * fix: the frontend still used property names the registers had renamed THE PIPELINE HAD A HOLE AND THIS IS WHAT FELL THROUGH IT. Its property pass rewrites QUOTED occurrences, which is right for PHP and JSON. Javascript does not quote object keys, and property access has no quotes at all: { waardering: 3 } an object key POSTed to the API obj.naam a read "naam" the quoted form the old pass already handled So Vue components kept posting and reading names the registers no longer declare. This is a PRODUCTION defect, not a test artifact: on softwarecatalog the review dialog never closed because the POST 400d silently, and the only thing that noticed was an e2e assertion that a dialog should be hidden. Fleet-wide it was 100 source files — procest 57, softwarecatalog 37, pipelinq 6. decidesk had none, which is why it went green first time and hid the class. Fixed by a new pass (rename-frontend.js) covering unquoted keys, property access and quoted forms. Its pairs come from THIS repo diff of lib/Settings crossed with the app own migration map — never from the shared .compose.json, which is one path reused by every app run and was stale enough to make an earlier check report a false clean. Frontend files only; no PHP touched, so the PHP gates are unaffected. `node --check` clean on every changed .js/.ts. * chore: re-trigger CI The frontend-only commit did not produce a Code Quality run — only CodeQL fired. An empty commit re-triggers the full matrix so the frontend rename is actually verified rather than inheriting a green from an earlier SHA. * fix(procest): BAG/WOZ query parameters are wire names, not app vocabulary The frontend pass renamed `huisnummer` to `house_number` in the bagApi and wozApi specs. That is a QUERY PARAMETER of the BAG — the Dutch national address register — so it is exactly the statutory-wire exemption. The service still sends `huisnummer`; only the expectation moved, which is how the tests caught it. The half-rename is the tell: `huisletter` and `huisnummertoevoeging` sit in the same params object and were left alone because they are not in the dictionary. A wire vocabulary moves as a set or not at all, and this one does not move. Both specs restored from development. Full frontend suite: 34 files, 349 tests, all passing locally with the app's own runner. * fix(procest): re-apply the property rename on top of the fleet reformat The PR had gone CONFLICTING, and `--admin` does not override a conflict — that is why it refused to merge while every other signal said it was fine. The conflict was the fleet-wide reformat landing on development: it rewrote every line of these files, so git offered whole-file conflicts in 381 files and there was nothing to merge hunk-by-hunk. Resolving by hand across that many files is how a rename gets silently dropped in one of them. So: take development's reformatted copy wholesale, then re-run the renamer over it. The rename is mechanical and re-runnable; the reformat is not something to re-derive. This also FIXED a regression hand-merging would have missed — the merge brought in new development code still using the old names, in files that were never in conflict at all. VERIFIED, not assumed: a residue check reports 0 surviving uses of the 932 property names the registers no longer declare. Because a checker that reports zero has to be shown it can report non-zero, injecting one old name makes it report 1. eslint 0 errors, webpack build OK. * fix(procest): finish the translation — 92 names were half English, half Dutch The rename this PR ships emitted a new name whenever SOME token was translatable and let the rest through untouched, so it produced names like `accountantsverklaringRequired` and `afstandToArbeidsmarkt` — English grammar around a Dutch word. 92 of the names it introduced still carried one. Merging that is worse than not renaming: the schema ends up in a third language nobody can search for. The number is measured with real wordlists — 274,937 English words and 164,174 Dutch, a token counting as Dutch when the Dutch list has it and the English list does not. Two earlier instruments were wrong in opposite directions: matching against the translation dictionary found almost nothing, because the names that broke are built from words the dictionary never knew; matching against a hand-written English vocabulary flagged ordinary words like `transaction` and `income`. Control on the real one: `opbrengst`/`dienst`/`termijn` flag, `transaction`/`settlement`/`allocation` pass. CASE was wrong too. A dictionary value containing an underscore turned a camelCase name into snake_case mid-schema. Style now comes from the schema name that was REMOVED, never from the migration map's left-hand side — that side is a COLUMN name and is always snake_case, so asking it whether the original was snake_case answers yes for every multi-word name. My first attempt did exactly that and rewrote `adviesAuthority` as `advice_authority`. NO SECOND MIGRATION: the branch is unmerged, so these names have never existed in a database. The correction rewrites them to the final name everywhere, including the migration map's RIGHT-hand side, so the map points the original Dutch column straight at the correct English one. Every rename is registered in its snake spelling as well — the map is keyed on column names, and without that a camelCase correction never reaches it and the repair step would migrate data into a column the schema no longer declares. VERIFIED: PHPUnit identical to a control run of the branch without these corrections, phpstan [OK], psalm 0, eslint 0 errors, build OK, 0 surviving uses of any renamed property in src/, every register file parses. NOT IN SCOPE, measured rather than assumed: 273 property names that were ALREADY Dutch on development and which the first pass never touched. A separate tranche, not a defect in this PR. * style(procest): re-run prettier after the name corrections The renames changed identifier lengths, so prettier's wrapping no longer matched in 107 files and `Frontend Check (format)` went red. `development` is fully prettier-clean, so this is drift the correction introduced, not pre-existing. Ran the project's own `format:fix`. `git diff -w` is attribute wrapping only and no import line moved — worth checking, because a formatter that reorders side-effect CSS imports changes behaviour while looking cosmetic. eslint 0 errors, build OK. * fix(procest): the BAG and WOZ shims send Dutch query parameters — restore them Five vitest tests went red and development is fully green, so this was mine. `bagApi.js` and `wozApi.js` are thin shims over the BAG and WOZ lookups; their query parameters — `huisnummer`, `huisletter`, `huisnummertoevoeging`, `nummeraanduidingId`, and the `waarde` field in the response — are those registries' names, not ours. `huisnummer -> house_number` IS a legitimate schema rename, and both things are true at once: the property in our register is `houseNumber`, the query parameter on the wire stays `huisnummer`. WHAT MADE IT ASYMMETRIC, and it is worth writing down because it will happen again. The frontend renamer rewrites an object KEY, matching `name:`. The shim builds its parameters with SHORTHAND: const params = { postcode, huisnummer } // source — no colon, no match params: { postcode: '1234AB', huisnummer: '10' } // test — colon, rewritten So the source kept sending the correct wire name and only the test moved. The shims are byte-identical to development, so their specs are restored to development too. Also re-ran prettier: the renames changed identifier lengths, so wrapping no longer matched in 8 files and `Frontend Check (format)` went red. `development` is prettier-clean, so that drift came from this branch. `git diff -w` is attribute wrapping only and no import line moved. VERIFIED: vitest 349/349, eslint 0 errors, build OK, format clean, PHPUnit identical to a control run without these corrections. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
`@nextcloud/no-deprecated-library-props` autofixed NcButton's deprecated
`type="primary"` to `variant="primary"`. The rule is right — @nextcloud/vue v9
did repurpose `type` as the native button type and moved the visual style to
`variant` — but the rename changes the RENDERED DOM, and tests assert on it:
openbuild 10 tests failed on `button[data-nc-button-type="primary"]`
returning undefined; 1375/1375 pass on `development`.
So this is a runtime change, not a formatting one, and it belongs in its own
PR together with the test updates it requires. It joins the two autofixes
already withheld for the same reason:
@nextcloud/l10n-enforce-ellipsis edits the CONTENT of translatable strings,
changing the translation key
vue/prefer-define-options moves options across SFC block boundaries
and breaks the build
All three keep their @nextcloud/eslint-config severity and are recorded in
eslint-suppressions.json, so the debt is counted and any NEW occurrence still
fails the run.
Verified after the change: openbuild is back to 1375/1375, and lint / format /
stylelint / build pass on every app touched here.
The pull_request run never materialised for this branch head — the PR carried only the CodeQL check runs, and 'gh pr checks' reporting 4 green entries is exactly what a workflow that never started looks like. An empty commit re-fires the event so the branch is judged on a real run rather than on the absence of one.
The ten #[PublicPage] endpoints with no ceiling: DSO intake, the PWA service worker and web manifest, the dwangsom payment callback, the raadsinformatie feed (3) and the StUF-ZKN receivers (3). No brute-force counters. None of these takes a guessable credential -- the receivers authenticate by the caller's own provider credential and the feeds are published open data. The limits are NOT uniform, and the reasoning matters more than the numbers: 300/60 on the machine receivers (DSO intake, payment callback, StUF). These callers retry on their OWN schedule and a burst is normal. Setting these tight would DROP statutory submissions and payment notifications -- failing closed on somebody else's delivery guarantee is a worse outcome than absorbing a burst, and the failure would land on their side where we do not see it. 120/60 on the raadsinformatie feed. Published open government data; public access is the statutory point, so this is a runaway ceiling, not a gate. 240/60 on the PWA assets, fetched on install and every update check. These are guesses, not measurements. Nobody has traffic data for a real municipal StUF middleware here, and the honest thing is to say so rather than present a chosen number as a tuned one. Erring loose on the receivers is the safer side of that uncertainty. Full suite: 1899 tests, identical to baseline (Warnings 1, Deprecations 7, Skipped 5 before and after).
# Conflicts: # src/dialogs/AddAssignmentDialog.vue # src/dialogs/BeschikkingComposerDialog.vue # src/dialogs/ConsultationCreateDialog.vue # src/dialogs/DsoCaseDetail.vue # src/dialogs/RolEditorDialog.vue # src/dialogs/VoorstelCreateDialog.vue # src/modals/AddCaseRelationModal.vue # src/modals/MandaatEditor.vue # src/modals/TermijnDefinitieEditor.vue # src/views/cases/components/BeschikkingDetailView.vue # src/views/cases/components/WooIntakeForm.vue # src/views/cases/components/beroep/BeroepEscalationPanel.vue # src/views/cases/components/bezwaar/BezwaarIntakeForm.vue # src/views/complaints/components/ComplaintCreateDialog.vue # src/views/settings/tabs/GeneralTab.vue
15 files conflicted; resolution took development's bytes for every one of them and then re-ran this repo's own formatter and the eslint autofix chain, so the merge cannot silently prefer this branch's reformatting over incoming logic.
build(lint): migrate to eslint 10 + @nextcloud/eslint-config 9
* ci: fast structural checks on every branch * ci: close the branch-trigger gap * ci: scope the JSON check — JSONC configs are not a defect * ci: scope the marker check to code — prose that documents a conflict is not one * ci: the JSON check reached a template's editor settings
…82) (#841) 65 publicly reachable methods across the ten ZGW API controllers carried no rate limit. All 65 declare themselves public with the LEGACY @publicpage ANNOTATION rather than the #[PublicPage] attribute, which is why the fleet sweep that closed ADR-082 did not see them: it line-anchored the attribute form and excluded docblock matches. This app already had 13 attribute-form endpoints throttled, which is why it looked finished. The annotation is a live declaration, proven on two other apps in this fleet - openregister GraphQLController::execute and opencatalogi CatalogiController:: index both answer 200 unauthenticated with only the annotation on them. Limits are chosen per endpoint rather than uniformly: OpenAPI index/spec 240/60 fetched by tooling and client generators; a tight ceiling breaks them rather than protecting anything reads, audit trails 120/60 zoek, download 60/60 most expensive query; moves bytes writes, publish*, uploadChunk, 30/60 uploadChunk is the cheapest way for notificatieCreate an anonymous caller to consume storage; a notification fans out to every subscribed channel publicResponsePost, cancel, 20/60 consultation stuffing; a destructive export cancel behind only a link token; a cheap request that buys a lot of server work AnonRateLimit only, not BruteForceProtection: these endpoints check no credential, and the latter without a paired registerAttempt() is the inert half of a two-half mechanism. AnonRateLimit applies only to anonymous callers, so authenticated ZGW client traffic is untouched - which matters here, because these are statutory wire APIs other systems integrate against. Verification: php -l clean on all 10 files; diff is 89 added lines and 0 removed; gate-82 reports 78 public methods, 78 throttled, 0 unthrottled.
…y watched (#844) * refactor(stuf)!: rename inkomend to inbound, and test the guard nobody watched TWO THINGS, ONE CAUSE. The endpoint was Dutch, and it was untested. Both are fixed here because the second is what made the first worth doing carefully: renaming a webhook route without a test suite means finding out from a municipality. THE GUARD WAS REAL BUT UNWATCHED inbound() verifies a WSSE UsernameToken and answers 422 on mismatch, and NOTHING tested it - no test in the repository referenced the endpoint at all. Delete the verifyWsse() call and every test stayed green. Its two sibling public routes already had that cover in StufSoapRequestDispatcherAuthTest, whose docblock states the standard: these are #[PublicPage] routes, nothing in Nextcloud's middleware will refuse a caller, so the refusal has to come from the app and has to be tested. Negative control run before pushing: with the guard deleted the suite gives 2 failures, and the informative one is that logInbound was called with a TAMPERED envelope - the message was accepted and recorded. Restored, 5/5 pass. The five arms pin: 422 with logInbound never reached (an endpoint that records first and refuses second has already accepted the message); 400 for an unresolvable sender with verifyWsse never called; 400 for an empty body; a POSITIVE control that a verified sender is acknowledged (without it, `return 422` on line one would pass every refusal test); and that the token is checked against THAT sender's stored credentials, so one configured system cannot impersonate another. WHY IT WAS UNTESTABLE `file_get_contents('php://input')` has no seam and OCP\IRequest exposes no raw-body accessor. Added a protected readRawBody() the test overrides. The production path is unchanged - same call, one indirection. THE URL IS A WIRE CONTRACT /api/stuf/inbound is canonical. /api/stuf/inkomend stays as an explicitly deprecated alias to the same method, because that URL lives in the UPSTREAM zaaksysteem's configuration, not ours: renaming it alone turns a working webhook into a silent 404 on somebody else's schedule. It is commented as a migration step with an end. DUTCH DELIBERATELY LEFT OUT OF THIS PR 'direction' => 'inkomend' and the ["uitgaand","inkomend"] enums are STORED DATA VALUES. Existing rows carry them, so renaming the code without migrating the data breaks every read - that belongs in the Dutch-to-English programme's data tranche. The sibling routes /api/stuf/zaken and /api/stuf/personen are Dutch and are wire contracts too. Verification: php -l clean; full unit suite 1904 tests, 8007 assertions, 0 failures on PHP 8.4 in the container (this box runs 8.2, the app needs ^8.3); no stale reference to stuf#inkomend, ->inkomend( or function inkomend in lib/, tests/ or appinfo/. Closes #843. * fix(routes): the legacy alias needs its own method, not a postfix CI caught this, and it was mine. The alias registered a SECOND routes.php entry under the same name, `stuf#inbound`, distinguished only by 'postfix' => 'legacy-dutch-alias'. Nextcloud's own RouteParser does support `postfix` - it appends to the generated route name. openregister's AppHost Routes::standard() does NOT: its assertNoDuplicateNames() keys on `name` alone and never reads `postfix`, so the two-entries-one-name form throws InvalidArgumentException: Duplicate route name "stuf#inbound" in AppHost Routes::standard($extra) at boot. That does not fail one endpoint, it takes the whole app's routing down - which is why it surfaced as procest's E2E SEED failing rather than as a routing test. The alias now routes to its own method, inboundLegacyPath(), which delegates to inbound(). The name is unique, the identifier is English, and the Dutch spelling survives only in the URL string - which is the wire contract held in the upstream zaaksysteem's configuration and the whole reason the alias exists. The constraint is written at both the route and the method so the next person does not rediscover it by breaking boot. Verification, redone against the RIGHT tree. The earlier full-suite run was made against the shared bind-mounted checkout, which is sitting on another workstream's branch (refactor/adr-083-inject-openregister, 8 commits behind development) where SamenwerkverzoekService gained a constructor argument its test was never updated for - 8 errors that belong to that branch, not to this one. Re-run against this worktree with vendor copied in: 1904 tests, 8007 assertions, 0 failures. StufControllerInboundTest still 5/5. Also verified directly: parsing appinfo/routes.php gives 388 entries and ZERO duplicate names, and the same check flags a planted duplicate - so the check is not vacuous.
… move rows (#845) * refactor(direction)!: English direction values, with the migration to move rows inkomend -> inbound, uitgaand -> outbound, intern -> internal, in the code and in the stored data. THE PART THAT NEEDED CARE: `intern` MEANS TWO DIFFERENT THINGS It is also a value of the statutory ZGW vertrouwelijkheidaanduiding enum (openbaar, beperkt_openbaar, intern, zaakvertrouwelijk, vertrouwelijk, confidentieel, geheim, zeer_geheim - ZgwRulesBase::VERTROUWELIJKHEID_LEVELS). Those are wire values of the standard this app consumes and emits, so they are exempt from the vocabulary rule. A WORD-BASED REWRITE WOULD HAVE CORRUPTED EVERY CONFIDENTIALITY FIELD IN THE INSTALL. What keeps them apart is that the migration is scoped to the `direction` COLUMN, not to the word. Two tests pin it and the negative control confirms they bite: setting COLUMN to vertrouwelijkheidaanduiding turns the suite red. THE MIGRATION RenameDutchDirectionValues, modelled on RenameDutchDeadlineColumns and reusing its information_schema discovery and the reasoning behind it. It differs in one way that matters: the sibling moves data between COLUMNS because MagicMapper never renames one; this step changes no schema at all - the column is already `direction` and only the strings inside it move. Scope, measured rather than assumed: - BOTH procest (17) and procest-default (2424) carry the three schemas with a direction property, so registers resolve by slug PREFIX; one exact slug would migrate half the rows and report success. - 105 shard tables on this install have a `direction` column, across pipelinq, decidesk, shillinq, scholiq and openconnector. Out of scope, and a test pins it - a procest step rewriting those edits another app's data. - portaalBericht stores `citizen_to_handler` in its own direction column, a different vocabulary sharing a column name. Only the three known strings are rewritten. Non-destructive, idempotent (no English target is also a source - pinned by a test), soft-deleted rows included, one unreadable table logs rather than aborting. Post-migration only, excluded from install like its siblings. THE READ THAT WOULD HAVE FAILED SILENTLY findOutboundByReferentienummer() filtered on 'uitgaand'. Querying only 'outbound' makes a pre-migration row return NULL rather than an error, and the caller reads null as "no outbound message to confirm" - a Bv01 confirmation dropped silently. It now falls back to the legacy value; findOne() takes scalar filters only, so this cannot be an IN. Marked for removal. Verification: php -l / JSON / XML valid on all 9 files; new suite 8 tests and 26 assertions with a passing negative control; full unit suite 1912 tests, 8033 assertions, 0 failures. Measured first: zero Dutch direction rows exist on this instance, so the step is a no-op here - which is why it reports its counters even when they are zero. * test(repair): cover the migration's real code paths, not just its constants The coverage guard caught this and it was a fair catch: the step added 92 statements and the tests covered 5, so coverage fell 0.05% against the merge base (31.43% vs 31.48%). Every assertion inspected a constant through reflection; nothing drove run(). Five tests added, and the one that matters drives run() end to end against mocked SQL: a procest shard table with a `direction` column gets exactly three UPDATEs, one per mapped value, and asserts on the statements issued - - the target is oc_openregister_table_17_928, - the column quoted in every statement is `direction`, - no statement mentions vertrouwelijkheidaanduiding, - another app's table (register 16) appears in information_schema and contributes NOTHING, so the register filter is exercised rather than assumed. The other four cover the paths a migration is judged on when it goes wrong: an install with no procest register reports "nothing to do" instead of passing silently and issues no statement at all; a failed register lookup logs and skips rather than throwing, so one broken install cannot abort the repair run; a table whose columns cannot be inspected is skipped rather than updated blind; and the step names itself for occ maintenance:repair. 13 tests, 48 assertions. The coverage delta itself is CI-measured - there is no xdebug or pcov in the dev container, so I could not confirm the percentage locally, only that the statements are now executed.
) * refactor(procest): translate 139 pre-existing Dutch property names Tranche 2, and the largest so far: 179 Dutch names measured, 139 renamed, 22 held back with a machine-recorded reason, 18 already covered. The map was DRAFTED from the tranche-1 token dictionaries rather than written by hand, then reviewed. The generator splits rather than guesses — resolved when every token of the RESULT is provably English, partial when any token is not, refused when the name is a schema slug, a lifecycle value, or would collide inside its own schema. It found 7 partials that needed a token (`afwijkend`, `deurwaarders`, `geadviseerde`, `inkomens`, `juridische`, `overschreden`) and 21 refusals, and I only had to read those 28. An early version called `aangezochtBevoegdGezag -> aangezochtCompetentAuthority` "resolved": it applied translations only to tokens the DUTCH WORDLIST knew, and `aangezocht` is a participle the wordlist does not carry, so it passed through as if it were English. It now applies any known translation and then verifies every RESULT token is English — which is the property that actually matters. FIVE THINGS THIS APP RENAMED THAT ARE NOT PROPERTY NAMES, each caught by a test and each now excluded by path: StUF/ZKN adapters `<zkn:omschrijving/>` became `<zkn:description/>` and getElementsByTagName('referentienummer') became 'referenceNumber'. Those are element names in a PUBLISHED STANDARD — Dutch by specification. ZGW mapping config `zgw_mapping_zaak` is a STORED app-config key. Email templates `{{omschrijving}}` is a placeholder, not an identifier. MandaatCsvParser REQUIRED_COLUMNS are the headers of the CSV a user uploads. One of four was translated — the same shape as the DSO defect in tranche 1. triggerTerugvordering Both a property and a service METHOD. The applier renames method CALLS but not the DECLARATION, so the test moved and the service did not. And two more `X ?? X` tautologies, from dual-spelling fallbacks written with parentheses — `$zioData['zaak'] ?? ($zioData['case'] ?? '')`. The guard added for softwarecatalog only matched the unparenthesised form. psalm names these precisely (ParadoxicalCondition), which is the only reason they were not shipped: collapsing the pair silently stops accepting the old spelling. VERIFIED against a control run of the same tree: PHPUnit 1899 tests with IDENTICAL errors and failures on both sides (4 errors, 2 failures, all pre-existing); phpstan [OK] both; psalm "No errors found!" both; vitest 349/349; eslint 0; prettier clean; build OK; 383 routes resolved, 0 broken; 0 slugs changed; 0 surviving uses in src/. 139 migration entries appended; the map has 452 entries with no duplicate and no identity pairs. * fix(procest): shorten a variable the rename pushed past phpmd's limit phpmd caps a variable name at 20 characters and `$escalationRecommended` is 21 — the rename lengthened it. The ARRAY KEY it feeds is the property and is unaffected; only the local moves, to `$escalationAdvised`. phpmd is green on development, so this one was mine. Also merges development's 2 commits (the ZGW volume-ceiling fix and a CI change). The merge is clean and brought in no code using the old Dutch names — re-running the applier over the merged tree reports 0 substitutions, which is the check that matters after a merge: development can always add fresh code using names this branch has already moved. The remaining PHPUnit cell failure is the coverage ratchet at 0.01% — 46789 statements on both sides, 4 fewer COVERED. Nothing was added; four statements stopped being reached. Re-measuring against the refreshed merge base. * fix(procest): pin the 13 properties an EXCLUDED adapter still reads The coverage ratchet was right and I had read it as noise. 46789 statements on both sides, **4 fewer COVERED** — nothing added, four statements stopped being reached. Chasing those four found a real defect. Excluding the StUF/ZGW adapters protected their published wire element names — `<zkn:omschrijving/>`, `getElementsByTagName('referentienummer')` — but those same files also read OUR properties off our own objects: `$case['zaakIdentificatie']`, `$data['externIdentificatie']`, `$row['synchronisatieStatus']`. The rename moved the objects to English and left the adapters reading keys that no longer exist. A missing subscript is `null`, so nothing threw; the only trace anywhere was four statements dropping out of coverage. THE RULE, now enforced by the applier: **a file excluded from the rename pins every property it reads.** Half-excluding a file is worse than either choice made whole. 13 names pinned — zaakIdentificatie, externEntiteit, externIdentificatie, laatsteSynchronisatie, synchronisatieStatus, fout, zaakIdentificatieStrategie, bronId, authenticatie, gebruikersnaam, wachtwoordKluisRef, ontvangenOp, omschrijving — leaving 126 renames. The tree is RE-DERIVED from `origin/development` with the corrected map rather than patched. Reverting `description` -> `omschrijving` and `error` -> `fout` in place would have hit every pre-existing English `description` and `error` in the app; re-deriving produces exactly the tree the corrected map would have made in the first place, and needs no history rewrite. Also carries the phpmd fix (`$escalationRecommended` is 21 chars against a 20-char cap; the array key it feeds is unaffected). VERIFIED against a control run of the same tree: PHPUnit 1899 with IDENTICAL errors and failures (4/2, all pre-existing); psalm "No errors found!"; phpstan [OK]; vitest 349/349; prettier clean; 383 routes resolved, 0 broken; 126 migration entries, no duplicate or identity pairs. The two remaining src/ hits are `fout` and `zaakIdentificatieStrategie` — both pinned, both still declared by the schema, so those reads are correct. * fix(procest): restore five StUF element names the vocabulary pass translated A PRODUCTION DEFECT already on `development`, found while chasing four missing covered statements. #832 renamed five StUF element names, and an element name is not ours to translate: StufZknMessageResponder::extractFields() passes each entry of `fieldNames` straight to getElementsByTagName(). `toelichting` had become `notes` and `einddatum` had become `endDate`, so those two tags never matched and the fields were silently dropped from every inbound zakLk01. StufFieldMappingService's mapping tables key on the ELEMENT name on the LEFT and the internal property on the RIGHT. Three ZKN keys (`toelichting`, `einddatum`, `registratiedatum`) and two BG keys (`geslachtsnaam`, `voornamen`) had been translated, so those fields never mapped either. Restored all five, verified by diffing every mapping key against the last commit before the vocabulary programme: **0 keys now differ from the specification.** I NEARLY BROKE A SIXTH THE OTHER WAY. `DEFAULT_ZENDER` looks like the same shape — `'organisation' => 'Procest'` next to `'applicatie' => 'Procest'`, one translated and one not. It is NOT an element list: StufResponseBuilder hardcodes `<stuf:organisatie>` and reads the array by key, so the English key is correct and "fixing" it would have emitted an empty element. The difference is only visible in the CONSUMER, which is the thing to read before deciding. VERIFIED: PHPUnit 1899 with identical errors and failures to a control run of the same tree (4/2, all pre-existing), phpstan [OK], psalm "No errors found!". * fix(procest): the excluded StUF tests kept feeding the OLD config keys The coverage cell was failing for -7 statements and nothing else was red. The clover diff put all of it in one file: StufMessageBuilder, 179 covered on development and 171 here. Cause: I excluded the whole StUF directory from the rename to protect the wire element names, and that exclusion covered the TESTS as well. The source moved on — it reads `$endpoint['freeMessagesTemplates']`, `['senderOrganisation']`, `['recipientApplication']` — while the fixtures went on supplying `vrijeBerichtenTemplates`, `zenderOrganisatie`, `ontvangerApplicatie`. Six keys. Every read returned null, the free-message template branch was never entered, and THE TESTS STILL PASSED, because they assert on the envelope they get rather than on the branch being taken. The only signal was eight statements quietly leaving coverage. That is the ratchet earning its place: no test failed, no analyser complained, and the code under test was not being reached. An endpoint CONFIG key is ours. Only the XML tag names belong to the specification, and those are untouched — verified separately: every mapping key in StufFieldMappingService matches the pre-programme spec names, 0 differ. VERIFIED: PHPUnit 1899 with 4 errors and 2 failures, identical to a control run of the same tree, and the eight statements are exercised again. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…alive (#846) Finishes what #844 started on the StUF surface. Both old URLs keep answering: POST /api/stuf/cases canonical /api/stuf/zaken deprecated alias POST /api/stuf/persons canonical /api/stuf/personen deprecated alias Same reasoning as /api/stuf/inkomend: the URL lives in the SENDING zaaksysteem's configuration, not ours, so renaming it alone turns a working SOAP receiver into a silent 404 on somebody else's schedule. Each alias needs its OWN method because AppHost Routes::standard() rejects duplicates by `name` and ignores `postfix` - that form throws at boot and takes the whole app's routing down. Verified: 390 route entries, 0 duplicate names. `zaken` MEANS TWO DIFFERENT THINGS AND ONLY ONE OF THEM MOVED The StUF $service token is INTERNAL - the sending endpoint is resolved from the envelope's zender, and the token's only other use is log context. It never reaches the wire, so it is now SERVICE_CASES / SERVICE_PERSONS. Deliberately untouched because they are statutory: ZrcController / ZgwService / ZgwRulesDispatcher $resource === 'zaken' is the ZGW REST resource AcController scopesContain(...,'zaken') is a ZGW autorisaties scope That distinction is the whole risk in this change and a test asserts it. Fault messages moved to English - 'Authenticatie mislukt', 'Ongeldig XML bericht', 'Onbekend berichttype'. The StUF fault CODE (StUF001) is statutory and unchanged; only the free-text faultstring moved. Verification: a new 4-test suite asserts each alias dispatches with the CORRECT service constant, not merely that it dispatches - passing the wrong service would still return 200 and route StUF-ZKN traffic through the person handler. Negative control: wiring casesLegacyPath() to persons(), the plausible copy-paste slip, makes it fail; restored, green. php -l clean; full unit suite 1921 tests, 8690 assertions, 0 failures.
Contributor
Author
|
Closing. All six findings this PR carried are already fixed on
Why it is closed rather than mergedIt is +1,189,085 / −64,894 lines and CONFLICTING/DIRTY, opened 2026-05-28. A diff that size is not a security review anyone can perform — the six real fixes are buried in what is almost certainly build or vendored output. Merging it to obtain changes that are already present would import that bulk for no gain, and rebasing it would cost more than the fixes are worth now that they exist. The branch is left in place. Only the PR is closed; nothing is deleted, so the history remains reachable if any part of it is wanted later. One caveat worth recording: I verified the six findings by reading |
Contributor
Author
|
Correction to my note above, and it matters for anyone reading this later. This was Two consequences:
|
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
4 participants
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.
Summary
Wave-3 critical security fixes for procest. All 4 REAL findings + 1 PARTIAL (SSRF half) + BONUS fail-open fixed.
deliverToSubscriptionnow validates callback URL with https-only allowlist + RFC1918/loopback/link-local CIDR block before any outbound POST.SettingsService.getPublicSettings()redactsai_api_keyandappointment_backend_api_keyto'***'for non-admin callers;SettingsController.index()now uses the redacted variant unless caller is admin.scopeGrantCovers()now verifies full scope name (zaken.aanmaken) not just suffix (.aanmaken) — addsCOMPONENT_SCOPE_PREFIXmap; prevents intra-component scope confusion.ParaferingService+ParaferingController(operated entirely in-memory, no persistence, client-supplied audit trail forgeable). Live engine isParafeerActieService. Routes removed.callAiModelvalidates admin-configuredai_model_urlagainst scheme allowlist + DNS pin + CIDR block before curl. TLS/redirect claims from report were FALSE — not fixed.ZrcController.checkZaakReadAccess: changedcatch(\Throwable)-return-null(fail-open) toreturn permissionDeniedResponse()— any unexpected exception now denies access (fail-closed).Test plan
Ref: /tmp/triage-procest.md