Repository navigation
fix(review): #2643 review findings, round 1 - #2686
Merged
Merged
Conversation
…ty password is persisted on an open schema The envelope kept on a `stuf_message` row for a retry no longer carries the WS-Security header: StufZdsCaseClient builds the message without a token and StufZdsOutbound::transmit() adds the UsernameToken from StufZdsCredentialResolver at send time, for the first attempt and for every retry. A password that cannot be decrypted at send time becomes a permanent CREDENTIALS fout instead of an exception after the row was opened. - new register fragment 99-stuf-message-lockdown.json: every verb on `stuf_message` is admin-only. Integriq's own reads and writes of the rows use `_rbac: false` (StufZdsMessageLog already did; StufZknSyncService now does for its four stuf_message calls). The legacy inbound webhook profile no longer requires create/update rights for its account on `stuf_message`, since the schema is admin-only and the audit rows are engine writes; the zaak/document upserts still run as the account. - a row that keeps an envelope gets `expires` (24 h), cleared when it concludes; LogCleanUpTask::run() sweeps `stuf_message`. New `expires` property on the schema. - SchemaAuthorizationRatchetTest: stuf_message moves from KNOWN_OPEN to CLOSED. Tests: StufZdsCaseClientTest (persisted envelope holds neither password nor UsernameToken, first send and retry both carry the token), StufZdsMessageBuilderTest (withUsernameToken), StufMessageLockdownTest (admin-only on all four verbs). Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml on tests/Unit/Service/StufZkn, tests/Unit/Settings, StufZknSyncServiceTest, XmlWebhooksConsumerTest: OK (407 tests); phpcs on the touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fter the admin-only lockdown LogCleanUpTask::cleanupSchema() now reads and deletes with `_rbac: false, _multitenancy: false`. Under cron there is no session user, so with RBAC on the admin-only `call_log` rule hid every expired row and the sweep deleted nothing. The same method sweeps the other three log schemas and the new `stuf_message` retention sweep (iq-f1), so all of them read and delete as the engine. Both swallowed exceptions (a refused delete in cleanupSchema, a failed strip save in stripExpiredBodies) are now logged as a warning through an injected LoggerInterface; the loops still continue with the next record. Test: LogCleanUpTaskTest records the `_rbac` argument of every findAll and deleteObject call and asserts false (call_log expired row is deleted); a second case asserts a refused deletion is logged and the sweep continues. The first fails without the fix. Class sweep: other call_log / stuf_message readers checked (ConnectionAlertService, CallService, EndpointMessageGate already use `_rbac: false`; SourcesController::logs is the admin-facing read). No further change. Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/BackgroundJob tests/Unit/Service/StufZkn: OK; phpcs LogCleanUpTask.php: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…akes any handle
DocumentSearchRequestedListener and DocumentFetchRequestedListener now resolve
the asking app's connection from (sourceApp, connectionKey), as design D3 says,
through the new DocumentConnectionGuard: the app needs an app_connection row
under that key (an empty key only when the app declares exactly one) whose
`source` points at a source that still exists. An unlinked app gets no Graph
call: a search answers the notice `not-linked`, a fetch stays unhandled; both
are logged. A failure to read the rows refuses (fail closed).
The listeners' audit line now carries the connection key and the search terms
or the fetched handle, next to the app and the user.
GraphDriveSearch::itemPath() refuses a `.` or `..` segment in a handle: rawurlencode
leaves dots alone, so a hand-built `driveItem:{drive}:..` would climb out of /items/.
Not done (optional in the finding): binding a fetch to handles a recent search
issued. The guard cannot tell which source a linked row points at: the Microsoft 365
adapter is instance-scoped (one credential in app config), so the link proves the
app is connected to some integriq source, not specifically to Microsoft 365.
Tests: DocumentSearchAndFetchListenerTest (unlinked app refused for search and
fetch with the adapter never called; guard resolution by app and key, empty key,
deleted source, unreadable rows), Microsoft365DocumentSearchTest (dot segments).
Thread: #2643 (comment)
Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/EventListener
tests/Unit/Service/Adapter: OK (202 tests); phpcs on the touched lib files: no new findings.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ends a body-less request CallReplayService::replay() now refuses a record that no longer holds the request it would replay: no `replayRequest`, a method other than GET/HEAD, and bodies that were stripped (`bodyExpiredAt`), withheld or never captured (`bodyCaptured: false`), or, for a record that predates `bodyCaptured`, a stored request without a body. The item reports `sent: false` with "The request is no longer kept, so this call cannot be replayed."; nothing is dispatched and no attempt is appended. replayAll() reports the item the same way and goes on with the next one. A dry run of such a record is refused the same way. CallServiceDispatcher::dispatch() also accepts the stored record's `url`: it is taken as the endpoint when it lies under the source's `location` (at a segment boundary) and has no redacted part; otherwise the dispatch is refused instead of calling a placeholder. testAReplayNamesTheMappingVersionItRanUnder replayed one record twice with a succeeding dispatcher; the first success removes `replayRequest`, so the second replay silently sent a bare POST. It now fails the replays (503) so the record keeps its request. Tests: CallReplayServiceTest (stripped record and withheld record never reach the dispatcher, bulk reports the item sent:false and still sends the next, a GET still goes out), new CallServiceDispatcherTest (stored url becomes the endpoint, untrusted urls refused). Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/Outbound: OK (137 tests); phpcs on the touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tside an investigation window CallService::finalizeCall() now builds the trace step from what the body policy allows: unless the source's investigation window is open or the caller passed `logBody`, the step's request and response lose their bodies (BodyCapturePolicy::strip), exactly as the call log's do. The withholdBodies case keeps its extra `withhold()`. BodyCapturePolicy gains a public `captures()` that exposes the decision apply() already makes, so the trace does not repeat the window logic. Tests: CallServiceBodyCaptureTest asserts a traced call outside a window leaves no request body/json and no response body in the step (fails without the fix), and that the step keeps both bodies inside a window and with logBody. Not changed, left for the author: `logBody: true` stores bodies with no expiry and a failed call's `replayRequest` keeps the request body for the error retention outside a window. Both are deliberate in the #2645 design (replay needs the request); the spec sentence "no request body is stored outside a window" and that behaviour disagree. Class sweep: the only trace step of type `call` is the one in finalizeCall. Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit --filter "CallService|Trace": OK (213 tests); phpcs on CallService.php and BodyCapturePolicy.php: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1 that does not exist tasks.md task 6 named `buildBv01`, which was never written: StUFXMLBuilder gained `buildFo02`, and the receiver answers an accepted write with the existing `buildBv03` and a refused kennisgeving with `buildFo03`. Task 6 now says that. Same stale answer codes in the sibling files of the change, aligned with what StufProviderReceiver sends and with the `stuf-provider-writes` delta (Bv03/Fo03): proposal.md (answers and refusal), spec.md (the "answers go back as StUF responses" requirement, which the writes delta MODIFIES, and the "unanswered query" requirement that names the kennisgeving fault, which it does not). Thread: #2643 (comment) Check after the last edit: git grep -n "buildBv01" returns nothing; git grep -n "Bv01\|Fo01" -- openspec/changes/stuf-provider-receiver returns nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… put to the owning app StufProviderReceiver now refuses, before dispatching any event, a zakLv01 with no non-empty criterion and an npsLv01 whose burgerservicenummer is not nine digits: a Fo02 (StUF058) with HTTP 400, and the owning app is never asked. The zakLa01 renders at most 100 cases (the rest is dropped with a warning), so an owning app that answers "everything" cannot make the receiver return an unbounded body. StufCaseQueryEvent documents that the criteria names and values are the sender's and must be treated as caller-controlled by an app answering the event. Tests: StufProviderReceiverTest sends a zakLv01 with an empty and a blank `gelijk` and an npsLv01 without `inp.bsn`, with an empty, a short, a long and a non-numeric one, and asserts a 400 Fo02 and that no event is dispatched; a second case asserts the 100-case cap. Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit --filter Stuf: OK; phpcs on the touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eiling before parsing
A new StufZknRequestBody holds the one 2 MiB ceiling (MAX_BYTES) and a bounded read:
`read()` takes at most MAX_BYTES + 1 bytes from `php://input`, so an oversized body never
reaches memory in full. Both public StUF controllers (StufZdsConfirmationController and
StufProviderController) read through it, and StufProviderReceiver now uses the shared
constant instead of its own private copy.
StufZdsConfirmationReceiver::receive() answers 413 with a Fo03 (StUF001, new
`body_too_large` entry in the acknowledgement builder's fault catalogue) as its first
step, before any parse, source or row lookup.
Tests: StufZdsConfirmationReceiverTest passes str_repeat('a', 2097153) and asserts a 413
Fo03 with no lookup, save or event (and that a body exactly at the ceiling is not refused
for its size); new StufZknRequestBodyTest asserts the read stops one byte past the ceiling.
`php://input` cannot be driven from a unit test, so the controller seam is covered through
the helper it delegates to.
Class sweep: the legacy StUF-ZKN bridge controller (StufZknController::inbound) also reads
the whole body, but it must keep the exact bytes for its HMAC check and is a separate
endpoint; not changed here.
Thread: #2643 (comment)
Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/Service/StufZkn
tests/Unit/Controller: OK (755 tests); phpcs on the touched lib files: no new findings.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…success audit (inbound secret split left to the author) New StufWsseInboundVerifier, used by StufProviderReceiver and StufZdsConfirmationReceiver in place of their two copies of the password compare. Besides user name and password it now checks the replay window of a UsernameToken that carries `Nonce` and `Created`: `Created` must lie within five minutes of now, and the nonce must be new for this source and user (shared cache, 10 minute TTL); a repeat is refused exactly like a wrong password (401 on the provider route, 422 for a confirmation), before the owning app or the message log is touched. A token with only one of the two is refused. A token with neither is still accepted, so existing senders keep working, unless the source sets `authentication.wsse.requireNonce: true` (documented in the stuf-zkn config schema). With no distributed cache a presented nonce cannot be checked: it is logged, and refused only when the source requires one. StufProviderReceiver::process() writes one info record per authenticated request: source, berichttype, referentienummer, route or forwarded origin, answering app and HTTP status. No criterion, burgerservicenummer or answer is logged. Not done, left to the author (see the PR hand-over): separate inbound and outbound secrets per source (needs a source configuration field and a migration of existing sources), and a per-source throttle next to the per-IP AnonRateLimit. Tests: StufProviderReceiverTest (replayed nonce refused and the owning app never asked, new nonce accepted, stale/future/unreadable Created and half a token refused, plain token still accepted, requireNonce refuses a plain token, one audit record without criterion or BSN), StufZdsConfirmationReceiverTest (replay, stale Created, requireNonce). Thread: #2643 (comment) Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit --filter "Stuf|Config": OK; phpcs on the touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…:readRawBody The diff-scoped spec-coverage gate flagged the changed readRawBody() as missing a @SPEC tag. Check after the last edit: run-hydra-gates.sh --scope-to-diff, see the gate run below. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… envelopes are never persisted The class docblock said the raw envelope is never persisted on `stuf_message`. The StUF-ZDS case client keeps the (password-free) envelope of a retrying kennisgeving on its row for up to 24 hours. The docblock now says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ck still describe account rights on stuf_message New change openspec/changes/stuf-message-admin-only (proposal, tasks, and a delta MODIFYING REQ-020 of stuf-zkn-bridge, the convention for edits to a main spec): the connection's account needs no right on `stuf_message`, the audit rows are engine writes, a missing connection or account answers 503, and a zaak or document write OpenRegister refuses is recorded as a `failed` row and answered with a Fo03 (HTTP 200 from receiveInbound), not 503. WebhookProfiles::stufZkn() docblock describes the same behaviour instead of "answers 503". Check after the last edit: php -l on the touched file; docs only otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… must be documented as transitional The default stays `requireNonce: false` so existing case systems keep working. The stuf-provider-receiver spec (authentication requirement, plus a replayed-nonce and a requireNonce scenario), docs/features/stuf-adapter.md and the verifier docblock now state that a token without Nonce/Created is accepted on its password alone while `requireNonce` is false, that an attacker can strip both elements from a captured request, that sources should set `requireNonce` as soon as their case system sends them, and that PasswordDigest is the follow-up. Check after the last edit: docs and docblock only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n a plain ICache Documented in the docblock of StufWsseInboundVerifier::firstUse(): the check is atomic only with an IMemcache (`add()`); on a plain ICache two simultaneous identical requests can both pass. Check after the last edit: docblock only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rd is an info line the default log level drops StufProviderReceiver now logs it at warning level with the fixed prefix `[StUF audit] provider request accepted`, so Nextcloud's default log level (2) keeps it and an operator can filter on the prefix. Still no criterion, burgerservicenummer or answer in it. No persisted-row pattern was used: integriq's `stuf_message` inbound rows would need a retention rule of their own, and the provider route is public and takes queries, so a row per request is a write per request. That choice is left to the author. Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/Service/StufZkn/StufProviderReceiverTest.php: OK (24 tests); phpcs StufProviderReceiver.php: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…source is a Microsoft 365 source DocumentConnectionGuard now requires the linked source's `type` to equal the adapter id (new Microsoft365Adapter::ADAPTER_ID, which getId() returns), and fails closed otherwise, so an app linked to a BRP or any other source cannot search or fetch through the organisation-wide Microsoft 365 credential. Source `type` is a free-form string, and no code in the repo creates a Microsoft 365 source: the adapter id as `type` is the convention assumed here and is not verified against a real source row. Tests: guard refuses a BRP-linked connection (api source, slug brp-haalcentraal), and search and fetch for a BRP-linked app never reach the adapter; the existing positive cases now use a `microsoft-365` source. Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/EventListener tests/Unit/Service/Adapter: OK (204 tests); phpcs on the two touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ent in a stored URL CallServiceDispatcher::endpointOf() now refuses a path with a `.` or `..` segment, plain, percent-encoded (`%2e%2e`, `%2E%2E%2F`) or double-encoded, before the endpoint reaches the engine. Dots inside a name (`a.b`) and in the query string are still fine. Test: CallServiceDispatcherTest refuses six dot-segment URLs and accepts a name with a dot and a `../` in the query. Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/Outbound: OK (137 tests); phpcs CallServiceDispatcher.php: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ete a concluded audit row LogCleanUpTask::cleanupSchema() takes a guard (`only`); the stuf_message sweep passes `status: retrying`. The query asks for retrying rows only, and each returned row is checked again before deleting, since an OpenRegister filter is a hint. A concluded row (`sent`, `failed`) is an audit record and stays, whatever `expires` says. Test: LogCleanUpTaskTest returns a retrying, a sent and a failed expired row and asserts only the retrying one is deleted and the query carried the status filter. Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/BackgroundJob: OK (104 tests); phpcs LogCleanUpTask.php: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…365 adapter has no source row The `type === microsoft-365` comparison added in the previous pre-push commit refused every real caller: nothing creates a source of that type. The Microsoft 365 adapter takes its credential from the app-config value `microsoft-365_credential_id` and the catalog lists it with an empty sourceTemplateSlug, so there is no source row to compare. The comparison, Microsoft365Adapter::ADAPTER_ID (getId() returns the same literal again) and the two BRP-refusal tests are removed. DocumentConnectionGuard keeps the (sourceApp, connectionKey) resolution, the linked-source-still-exists check and the fail-closed behaviour. Its docblock now states the limit: the link proves the app is connected to an integriq source, not to a Microsoft 365 one; binding it needs a Microsoft 365 source template or a connection-row declaration to match on. Check after the last edit: vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/EventListener tests/Unit/Service/Adapter: OK (202 tests); phpcs on the two touched lib files: no new findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse
requested review from
Rem-Dam,
bbrands02,
rjzondervan and
rubenvdlinde
as code owners
October 15, 2026 16:37
Contributor
Quality Report — ConductionNL/integriq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ❌ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 143/143 | |||
| npm | ✅ | ✅ 647/647 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ 5518/5518 · 1 skipped (no test failed, the job failed for another reason — see its log) | ||||
| Newman | ✅ 130/130 | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ | ||||
| Hydra gates (axe) | ✅ |
Quality workflow — 2026-10-15 17:07 UTC
Download the full PDF report from the workflow artifacts.
…ew fixes phpstan: StufWsseInboundVerifier::firstUse() tested `$cache instanceof ICache === false`, which parses as `(instanceof) === false` and is always false because createDistributed() returns an ICache. The test is dropped; the isAvailable() check stays. phpmd, without changing behaviour: - StufZknRequestBody::read() is an instance method and the controllers take it by constructor injection, instead of static access; the `@` on fopen() is an explicit error handler. - CallServiceDispatcher::endpointOf() hands the address check and the dot-segment check to restUnderSource() and assertNoDotSegment(). - StufWsseInboundVerifier::verify() hands the credential comparison and the requireNonce read to matches() and requiresNonce(); `$at` is `$timestamp`. - StufZdsConfirmationReceiver::receive() hands the body check and envelope read to intake() and the token check to authenticated(); the body ceiling is a class constant alias, which also takes the class back under the coupling threshold. - StufZdsOutbound is an orchestrator that gained the builder and the credential resolver when the UsernameToken moved to send time; it carries the same CouplingBetweenObjects suppression as StUFZKNService and OsoService, with the reason. phpcs: the saveObject() call in StufZknSyncService::persistMessage() is wrapped (157 > 150 chars). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…used body strip The coverage ratchet failed by 0.04% on the code this branch adds: the plain-ICache and no-cache paths of StufWsseInboundVerifier::firstUse() and the refused-save path of LogCleanUpTask::stripExpiredBodies() had no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a descriptions The envelope description changed (it carries no WS-Security credentials) and the expires description is new; both lacked a catalogue key, which check:schema-l10n refuses. The stale key of the old envelope description is replaced, en.json is the identity and nl.json the Dutch text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Quality Report — ConductionNL/integriq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 143/143 | |||
| npm | ✅ | ✅ 646/646 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ 5524/5524 · 1 skipped | ||||
| Newman | ✅ 130/130 | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ | ||||
| Hydra gates (axe) | ✅ |
Quality workflow — 2026-10-16 09:53 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Author
|
The second round of fixes for the reviews of #2643 is in #2692. It is cut from |
WilcoLouwerse
added a commit
that referenced
this pull request
Oct 16, 2026
One branch fewer keeps StufProviderReceiver under the phpmd class complexity limit once #2686's query checks land beside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for the review of #2643 (#2643 (review)). Merge into
development; the next sync PR carries them to beta. Nothing here is merged by the reviewer.Findings → commits
eef8fb3bf,c51999b8f,2db6d092f,7c32d6200transmit()adds the UsernameToken at send time on every attempt. New99-stuf-message-lockdown.json(admin-only on every verb),_rbac:falseon every internalstuf_messageread/write, retrying rows expire after 24 h and onlyretryingrows are swept. REQ-020 updated in the new changestuf-message-admin-only, since the bridge no longer needs a right onstuf_message.ac08b6584cleanupSchemareads and deletes with_rbac:false, _multitenancy:false; the swallowed exceptions are logged.8719ab35e,84a68e916,2773001cfDocumentConnectionGuardresolves (sourceApp, connectionKey) through app_connection and requires a linked, existing source, failing closed;itemPathrefuses./..segments. A source-type check was tried and dropped again (the Microsoft 365 adapter has no source row).088b2d226,66dd18284replay()refuses a record whose request is gone; the stored url is accepted as the endpoint only under the sourcelocation, with noREDACTEDpart and no dot segments.d400653c5logBody(BodyCapturePolicy::captures()).0f987f354buildBv01removed from tasks.md; the proposal and spec now say Bv03/Fo03 to match the writes delta.136c3775e2ad4ed38a,62af91713StufZknRequestBodyreads at most 2 MiB + 1 byte; both receivers answer 413 before any parse or lookup.1c011d98d,422139e89,03f44c7a9,366268554StufWsseInboundVerifier: a token withNonce+Createdmust be fresh (±5 min) and unseen (per source and user, atomic with an IMemcache);requireNonceopts a source into refusing tokens without them. Documented as transitional (PasswordDigest is the follow-up). Each accepted provider request writes a[StUF audit]warning line with no BSN or criteria.Left to the author (threads stay open)
BrpRelationReader::read()changes the by-name contract dossiq calls, so it has to land together with a dossiq change. Recommendation: arelationAppsallowlist on the source,sourceAppin$config,reason: not-permittedotherwise, andfiredByon the withheld record. Whether doelbinding/verwerking headers are mandatory is a question for the municipality's RvIG authorisation.authentication.inboundWsseplus a repair step that copies the current pair), a per-source throttle, a persisted audit row instead of a log line (needs a retention decision), and makingrequireNoncethe default once case systems send a Nonce.logBody: truekeeps bodies with no expiry, andreplayRequestkeeps the request body for the error retention outside a window, while the spec says no request body is stored there; which of the two gives way is a design call.WebhookProfiles::stufZkn()now hasrequiredActions: [](the preflight only checked rights onstuf_message, which is admin-only now); zaak and document upserts still run as the connection account. The legacy HMAC bridgeStufZknController::inboundstill reads its body unbounded (it needs the exact bytes for the signature).Tracked elsewhere
Checks run on this branch
bash hydra/scripts/run-hydra-gates.sh --scope-to-diff --base e391fac5f→ exit 0, COVERAGE 55 of 55 applicable gates ran (42 not applicable)vendor/bin/phpunit -c phpunit-unit.xmlontests/Unit/Service/StufZkn, Settings, BackgroundJob, Outbound, EventListener, Service/Adapter, Controller andStufZknSyncServiceTest→ OK (1483 tests);--filter "CallService|Trace|Webhook|Stuf"→ OK (488 tests). The new tests for iq-f2 and iq-f6 were confirmed to fail without their fix.vendor/bin/phpcs --standard=phpcs.xmlon every changedlib/file → no new findings (the remaining ones are identical on the base commit)pre-pushcommits above, round 3: no findingstests/Unit/AppInfo/Application*Test(Cannot assign MockObject_IAppContainer to property OCP\AppFramework\App::$container); they touch none of these files and were not checked on the base commit.CI follow-up
f5b6fbb3phpstan, phpmd and phpcs findings on the review fixes: an always-trueinstanceof ICachecheck removed;StufZknRequestBodyinjected instead of called statically and its@fopenreplaced by an explicit error handler;endpointOf(),verify()andreceive()split into helpers without changing the order of checks; one long line wrapped.a3ff3a0etests for the nonce-cache fall-backs and the refused body strip (coverage ratchet).0bda1be7catalogue keys (en/nl) for the two changedstuf_messageschema descriptions.🤖 Generated with Claude Code