Release: merge development into beta - #1983
Conversation
The 0.4.4 release bumped the version on main. Without this, development stays behind main and the next development -> main promotion conflicts on the version file. Version files resolve to development's side, which is the higher line, so this never moves a version backwards.
…260912204215 chore(release): 0.4.7-unstable.20260912204215
….4.4 chore(release): sync main back into development
…0260913184338 chore(sync): carry beta back into development
…lists (Q6.20, 12.3) (#1991) * spec(openspec): an outbound webhook is signed by default, gap register row Q6.20 * spec(openspec): the Objecten and Objecttypen API as an integriq facade, gap register row 12.3
* spec(connections): integriq's half of the connection registry * feat(connections): the connection schema, declaration contract, events and D4 resolver * feat(connections): sync, reports, health job and link-a-source endpoint * test(connections): sync, probes, listeners, health job and link endpoint * fix(connections): coding standard on the new classes, and SourcesController drops its unread logger * feat(connections): App connections page, status formatters and the link a source dialog * test(connections): Playwright coverage for the App connections page and dialog * feat(connections): adopt the dossiq amendments, unconfiguredMessage, the rule 5 message and the /connections route * chore(connections): demo rows for the connection schema, and move the version so the new repair step runs on upgrade * test(connections): named arguments and wrapped lines, so the new tests pass the coding standard too * fix(connections): rename the schema slug to app_connection, because stackiq owns connection
…ed source (#1997) Round 4 discovery cluster 26 and depth-study cluster CT-5, decision D2. One provider contract with suggest, resolve and describe, bindings over the PDOK, Haal Centraal BRP and KvK sources integriq already holds, and provenance on every resolved value so read live is a checkable claim. Asks openregister for x-openregister-property-source by name, and says plainly that it is not x-openregister-object-source. The umbrella gains a discovery wave 1 section. It records that D12 was answered for Nextcloud Mail rather than integriq, so integriq opens no mail-account change and cluster 28 moves to dossiq.
…rces and the expression allowlist (#2001) * docs(openspec): the statutory gateways and the frameworks we claim Round 4 discovery cluster 56, twelve candidates, six passers of which five documented, proving system xxllnc-zaken. Decisions D21 and D6. Size L. Nine requirements: the claim on the catalogue entry, the broker as configuration, CORV and GGK, Wmebv obligations per route, publication by reference, the ZGW registry as a binding, the on-premise bridge, jurisdiction per gateway and the WKPB registration. * docs(openspec): users and groups come from the directory and stay in step Round 4 discovery cluster 33, candidates C-access-and-privacy-82 (matrix hole), C-integrations-34 and C-integrations-21. Number 7 of the twenty-five loudest, four driven passers, proving system glpi. Size M. Nextcloud keeps the accounts; integriq keeps the connection, the mapping and the run. C-integrations-21 is recorded and not built. * docs(openspec): record every outbound message per recipient and per step Round 4 discovery cluster 23, eight candidates, six must, three matrix holes, rows 6.11, 6.20 and 6.23, proving system glpi. Size M. The record, the body behind its own permission, retry over the existing replay act, forwarding as its own record, three honest delivery states, a last-contact query and the external address as a recipient. No account, no transport: D12 gives the mail account to Nextcloud Mail. * docs(openspec): every outbound call is readable, replayable and governed by a policy Round 4 discovery cluster 27, eight candidates, four must, three matrix holes, row 6.11, proving system gitlab. Number 8 of the twenty-five loudest, five driven passers. Size M. The call record, replay over dead-letter-replay, firing by hand, the retry schedule as configuration, mapping versions on a replay, external verdicts and a blocking pre-check. dossiq retires its hardcoded StufRetryJob schedule. * docs(openspec): a channel is a declared adapter with a routing rule Round 4 discovery cluster 45, six candidates, proving system xxllnc-zaken. Size M. One adapter contract, routing as configuration with a review inbox, the signed submission route generalised from open-formulieren-intake, location and media on the inbound shape, and a reply over the arriving channel. Three candidates recorded and not built: the smart picker under D9, and the native mobile app on documented passers only under D21. * docs(openspec): integriq holds the sources a migration reads from Round 4 discovery cluster 8, integriq's half by the cluster's own mechanism line. Candidates C-configuration-88 and C-configuration-16, both matrix holes, plus the read half of C-configuration-95. Numbers 2 and 13 of the twenty-five loudest, eight driven passers on the migration path. Size M; openregister keeps the engine, the preview and the conflict policy. * docs(openspec): an expression reaches outside the instance only through an allowlist Depth study D-casetype-20 and consolidated candidate C-access-and-privacy-40, passer Valtimo. Size S. One prefixed source contract, env: resolving only an administered key with no wildcard, redaction before buffering and a declared write capability. Openregister keeps the expression language under D3 and the rest of cluster 4. * docs(openspec): index wave 3 in the integriq parity umbrella Six clusters open, cluster 60 stays closed under D12. Adds the wave 3 table, the loudest-25 numbers it answers, what other apps owe, and the eight candidates recorded and not built with their reasons. * docs(openspec): correct the wave 3 counts in the umbrella Five of the seven waiting clusters open, not six, and seven changes open, not six. Cluster 28 and cluster 60 both stay closed, each for its own reason.
…-only rows and knows limited (#2010) * feat(connections): simulated values, JSON paths, reported-only rows and the limited status Adapter values can now be a provider name or a field inside a JSON settings blob (adapter.simulatedValues, adapter.jsonPath), with defaults that keep every existing declaration's meaning. reportedOnly skips rules 3 and 5, and a simulated report stands against a newer probe (rule 4a). limited joins the status enum. The health job resolves every row after its probes. Contract: hydra#673, design D12. * test(connections): each D12 amendment, the unchanged defaults and the job's resolve without a call * test(connections): no inline ifs in the new tests, for the coding standard * refactor(connections): config reading moves to ConnectionConfigReader, so the resolver stays under the complexity limit
#2012) * docs(openspec): open the outbound sender identity cluster and index it Discovery cluster 61, the last integriq cluster recorded and not opened. The re-read D12 asked for is done: an identity is a face on a Nextcloud Mail account. Extends outbound-communication-log. * docs(openspec): correct the cluster number to 61 in the integriq umbrella
…4 and 6.27 already carried (#2013) * spec(parity): records owned by an external source, with a policy for when they disappear Closes gap row 5.19 (rated no for dossiq). Integriq declares the ownership mode and the disappearance policy on the synchronisation, projects ownership and last-seen onto the record, and refuses a local delete of a source-owned record without a written reason. Specs only, no implementation. * spec(parity): a one-off recipient on a single message, or a standing one suppressed with a reason Closes gap row 6.23 (rated no for dossiq). The recipient list of one message becomes a recorded decision: standing, added and suppressed, with a reason on every suppression and a refusal for a recipient the caller marked required. Specs only, no implementation. * docs(openspec): index the pending-proposal half of the parity programme Two rows open a change here (5.19 and 6.23) and two are already carried by outbound-communication-log in substance (6.24 and 6.27). The reservation on REQ-OCL-006 is recorded rather than resolved quietly.
…n step (#2015) * feat(directory): a directory connection that keeps Nextcloud groups in step Tasks 1 to 4 of directory-and-group-sync: the directory source and its mock fixture, the declared mapping with its create-or-refuse setting, the open-work reporter that asks rather than reads, the scheduled and on-demand runs, and the SCIM 2.0 endpoint gated by the existing consumer-backed credential. * feat(directory): the directory sync screen, its manifest page and the strings Tasks 1, 2, 5 and 6 of directory-and-group-sync on the frontend: one screen for the connections, their mapping, a preview that changes nothing, a run that the removal guard can stop, and what every run changed. Dutch and English strings for all of it. * test(directory): the run, the mapping, the reporter and the SCIM gate Tasks 1 to 6: a new member joins, a leaver is removed, a preview changes nothing, a truncated directory is guarded before writing, an unknown target group refuses naming the group, a silent consumer reads unknown and not zero, and an unauthenticated SCIM call reads nothing. Also moves the screen onto the Sources index and a typed logs page, so no new custom page is added, and adds the anonymous rate ceiling every public SCIM route needs. * feat(directory): the e2e scenarios, the SCIM contract tests and the admin docs Six Playwright scenarios drive the real screens over a mock-mode fixture, so the reader, the mapping and the membership writer under test are the production ones. The Newman folder asserts what a collection can assert without a seeded credential: every SCIM route rejects an unauthenticated call before any user is read. Ticks the change's tasks and records where the screen landed and what the hand-offs to dossiq are. * fix(directory): finish the lint pass, and keep the guard messages their translations match The reword of the ratio-guard and unknown-group messages had orphaned the four l10n files this branch itself added, so Dutch would have fallen back to the English source. The original wording is restored: it also names where the limit and the create-missing setting live. Also: one test per SCIM route for an unauthenticated call, the unused created prop dropped from the run summary, and the import order and optional catch binding the linter asked for. * fix(directory): the two guard messages fit the line limit, and every label has Dutch The ratio-guard and unknown-group messages ran over the 150 character limit, so they are trimmed and the four l10n files are trimmed with them. Shortening the source string without moving the catalogue key is what had left Dutch falling back to English. The Added and Removed columns on Directory runs had no nl.json key, which nothing else reports: check:l10n-js compares nl.json to nl.js, and a string missing from both reads as in sync. The open watcher on the run modal now carries the @SPEC gate 16 asks for. * fix(directory): drop the else in the membership write, and baseline what phpmd cannot be talked out of The full suite caught what the diff check cannot: it does not run phpmd. Nine findings, all in the new directory files. The else in the membership write is gone, because that one was worth fixing rather than suppressing. The other six are a named constructor (DirectoryEntry::fromArray), value objects carrying a boolean because it is data and not a flag, dryRun and confirmRemovals which are always called by name, and the $argument that TimedJob::run forces on every job (RegisterBootstrapJob is already baselined for exactly that). The entries are appended, not regenerated. Regenerating would have written 302 where 318 stood and dropped 16 stale entries in silence.
…ec tasks (#2018) #1558 dropped the dead Codeberg issue links; two archived tasks.md files still linked to codeberg.org/Conduction/openconnector/issues/*, which no longer resolve. They now name the pre-migration issue as plain text, and the visual-flow note records that GitHub is the only tracker. Ported from the unmerged tail of fix/repoint-codeberg-links-to-github (7d878fd); its third file already reads that way on development.
* feat(connections): a settings refresh retires older reports and probes ConnectionRefreshRequestedEvent now stamps refreshedAt on the affected rows, every row of the app when the key is null, before resolving them. D4 rules 4a and 4b count only a report or probe that is not older than refreshedAt, so a fixed setting clears an old error on save. The row keeps the observation for reading. A sync, a report, a probe and the plain resolve leave refreshedAt alone. Schema app_connection moves to 1.2.0. Contract: hydra connection-registry D3, D4, D6 and D12 item 5. * test(connections): a refresh retires older observations, and only a refresh writes refreshedAt Covers both spec scenarios at the resolver and at the service, equal times (also across offsets), a probe newer than the refresh, a retired probe and a retired simulated report, a null key stamping every row of the app, and a sync, report and plain resolve keeping refreshedAt. Inverting the older-than comparison turns both scenario tests red on their status assertions. * docs(openspec): a refresh retires older connection observations Adds the two REQ-CONN-005 scenarios from hydra#674, the refreshedAt rule to REQ-CONN-003 and REQ-CONN-004, the choices integriq makes (two refresh paths, comparison by instant, equal counts) and task group 6. * refactor(connections): both refresh paths share one resolve loop, so the classes stay under the complexity limit phpmd ExcessiveClassComplexity flagged ConnectionRegistryService at 53 and ConnectionStatusResolver at 51 (limit 50). refresh() and refreshRequested() now share resolveRows(), the version check in needsSync() is one array_diff, and the refreshedAt comparison lives in readObservation(). Behaviour is unchanged; the mutation check was repeated on the new comparison. * docs(openspec): refreshRequested saves a row whose data changed
…nside JSON (#2022) * feat(connections): requiredConfig reads false as empty and can look inside JSON A requiredConfig entry is now an app-config key, always the whole key even with dots, or {configKey, jsonPath}. A value is empty when it reads as "", false or 0 after trimming, or is JSON false, 0 or null, or the path is missing. A switch stored under the bool type is read with getValueBool, so false no longer counts as a filled setting. The path walk and the array-type read are the ones adapter.jsonPath already uses. Contract: hydra connection-registry D2, D4 and D12 items 6 and 7. * docs(openspec): a switch stored as false is not a filled setting Adds the three REQ-CONN-003 scenarios from the hydra requiredConfig amendment, the entry and emptiness rules to the requirement, the choices integriq makes (one list for every value form, typed keys read by type, objects and lists count as filled) and task group 7.
…mpty JSON list counts as empty (#2024) * feat(connections): a switched-off connection reads disabled, and an empty JSON list counts as empty A declaration can carry switch {configKey, jsonPath?, offValues?} and disabledMessage. D4 rule 2b sits below rule 2 and above rules 3 and 4: a switch that reads as off gives disabled, on reportedOnly rows too. Without offValues off means empty; with offValues it means one of them, so an unset key is off only when "" is listed. The value is read through the same path walk and emptiness rule as requiredConfig, in ConnectionConfigReader. disabled joins the status enum: app_connection moves to 1.3.0, reports may send it, and the formatter shows Switched off (Uitgeschakeld). A JSON empty array or object, decoded or stored as text such as [] or { }, now counts as empty. Contract: hydra connection-registry D2, D3, D4 and D12 items 8 and 9 (hydra#677). * test(connections): the five switch and empty-list scenarios, their controls and the validator refusals Covers the five spec scenarios, an off switch above a newer probe, a simulated report and a mock adapter, a switch with jsonPath (text and array type), unset keys with and without offValues, a reported disabled at the resolver and the service, [], {}, [ ] and [0] (filled), and declarations without a switch resolving exactly as before. Validator and schema agree on switch fixtures and refuse one without configKey or with an unknown key. * docs(openspec): a switched-off connection reads disabled, and an empty JSON list counts as empty Adds the four REQ-CONN-003 scenarios and the REQ-CONN-004 scenario from hydra#677, the switch rule and the wider emptiness rule to REQ-CONN-003, the seventh status to REQ-CONN-004, the choices integriq makes (switch read as a requiredConfig entry, rule number 2, kept time, which text is decoded) and task group 8. * test(connections): wrap a long fixture line for the coding standard * fix(l10n): translate the requiredConfig schema strings, so the schema l10n count is back at its baseline * refactor(connections): judging a config value moves to ConnectionConfigValue, so reader and resolver stay under the complexity limit phpmd ExcessiveClassComplexity rated ConnectionConfigReader at 55 and ConnectionStatusResolver at 51 (limit 50) with the switch added. Filled or empty, one of a list, and scalar as text now live in ConnectionConfigValue. The reader takes an empty adapter configKey and a missing switch itself, so the resolver's rule methods lose a branch each. Reader 45, resolver 49, value class 13. A stored switch without a string configKey never reads as off.
… every CI install
`composer install` fails on any runner whose PHP ships ext-redis below 6.1,
which is every GitHub-hosted runner — PHP 8.3 there carries 5.3.7:
Problem 1
- ext-redis is present at version 5.3.7 and cannot be modified by Composer
- symfony/cache is locked to version v7.4.14 and an update of this
package was not requested.
- symfony/cache v7.4.14 conflicts with ext-redis <6.1.
Your lock file does not contain a compatible set of packages.
symfony/cache is not a direct dependency: symfony/expression-language ^6.4
pulls it in, and the lock resolved v7.4.14, which declares
"conflict": { "ext-redis": "<6.1", ... }
Upstream has since dropped that conflict, so the fix is forward rather than a
pin. `composer update symfony/cache --with-all-dependencies` moves three
packages, all within their current major:
symfony/cache v7.4.14 -> v7.4.19
symfony/service-contracts v3.7.1 -> v3.7.3
symfony/var-exporter v7.4.0 -> v7.4.18
v7.4.19 declares no ext-redis conflict at all, and no other package in the
lock declares one either — checked across every entry in `packages` and
`packages-dev`.
composer.json is untouched; only the lock moves.
WHERE THIS WAS FOUND, AND WHY IT MATTERS BEYOND THIS REPO.
Not here. It surfaced in ConductionNL/portaliq, whose PHPUnit matrix installs
integriq as a sibling app (portaliq#564 added it to `additional-apps`). All six
of portaliq's matrix cells die before a single test runs, because the shared
workflow refuses to continue on a half-installed sibling — correctly, since it
would otherwise report integriq's missing classes as portaliq's failures. That
red then reaches every open pull request on portaliq through its merge commit.
This repository's own Code Quality on `development` is red as well.
HOW FAR THIS WAS VERIFIED, stated plainly because one step does not hold.
What is established: the CI log names the conflict and the installed ext-redis
version; the old lock carries symfony/cache v7.4.14 whose own metadata
declares `ext-redis: <6.1`; the new lock carries v7.4.19, whose metadata
declares no such conflict; and no other locked package declares one.
What is NOT established locally: this machine has no ext-redis at all, so a
plain `composer install` here passes on the OLD lock too and proves nothing. I
tried to simulate the runner with `config.platform.ext-redis`, and that test
is not discriminating either — the old lock passes under it as well, so it was
discarded rather than reported as a pass. CI is the verification.
Refs ConductionNL/portaliq#567
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed (#2041) Prettier 3.9.6 is pinned, so this is unformatted code rather than tooling drift. Every file is exactly what prettier produces from the previous version.
…2043) Directory runs send OCS-APIRequest (the route answered 412 without a requesttoken); REQ-DS-005 pages to a uniquely named row; manifest-pages lists AppConnections and DirectoryRuns; connection-registry asserts on the App select alone.
fix: two unbound interfaces answering 500 on nine routes, and the record corrections from the #1983 review
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Security re-review of the delta since my approval — 2 blockers, withdrawing that approval
I approved this at f370f878. Twelve commits and four feature PRs have landed since (#2128 Teams intake, #2130 event broker transport, #2132 the government identity broker, #2134). Reviewed f370f878..9cd5755a with a security focus. Two findings would be unwise to promote, so this supersedes the approval.
🔴 1 — The Teams reply sends the bot's bearer token to a URL taken from the inbound payload
lib/Intake/Adapter/TeamsChannelAdapter.php:260
$raw = $message->getRawPayload();
$serviceUrl = trim((string)($raw['serviceUrl'] ?? $configuration['serviceUrl'] ?? ''));The payload-supplied value takes precedence over the source-configured one, and $raw is the inbound activity as received and stored. At :282 the outbound POST carries Authorization: Bearer <configuration['accessToken']> to whatever host that names. There is no allow-list, no scheme check, and no internal-range refusal.
The app already has SSRF containment — CallService.php:395-460, and AuthenticationService blocks loopback / 169.254 / RFC-1918 — but this transport calls IClientService directly and reaches none of it. Microsoft's own Bot Framework guidance is that serviceUrl must be validated before use, for exactly this reason: an activity is attacker-influenceable, and this turns the reply leg into a credential-exfiltration primitive with an SSRF attached.
Fix: prefer $configuration['serviceUrl'] over the payload's, and refuse a host that is not on a configured trusted list (default: the Bot Framework connector hosts).
🔴 2 — The signature verifies the raw body; the adapter acts on a different set
lib/Controller/IntakeChannelsController.php:126 vs :157
$rawBody = $this->getRawContent(); // :126 — what is VERIFIED
...
$message = $adapter->receive($this->request->getParams()); // :157 — what is ACTED ONNextcloud's parameter set is a merge, not the body. From this workspace's own lib/private/AppFramework/Http/Request.php:123:
$this->items['parameters'] = array_merge(
$this->items['get'], $this->items['post'], $this->items['urlParams'], $this->items['params']
);with the JSON body merged last (:412). So body keys win on collision, but any key absent from the signed body can be injected through the query string and survives — and TeamsChannelAdapter::receive() stores that merged set verbatim as rawPayload, which finding 1 then reads as an egress destination.
The controller's own docblock at :325-333 asserts the opposite: "Verification MUST run over the exact bytes the sender signed, not the framework's normalised params, which would desync." The raw body is used for the check and the normalised params for the action.
Fix: json_decode($rawBody, true) and pass that to receive(), so the bytes verified are the bytes acted on.
The controller is pre-existing and untouched in this delta, but #2128 is what first routes a channel through it whose stored payload later becomes an outbound destination carrying a credential.
🟡 Broker credentials can be re-pointed without ever being read
SaveObject.php:4427 (OpenRegister) carries omitted write-only values forward from the stored object — deliberately, per openregister#463, so a GET/edit/PUT round-trip does not destroy a secret the client was never shown.
So an actor changes protocolSettings.broker.baseUrl, omits the password, and the server sends the preserved RabbitMQ/Kafka credentials to their host. writeOnly stops them reading the secret; they do not need to.
EventsController::updateSubscription() is #[NoAdminRequired] gated by event.update-subscription, which is seeded ["admin"] — so today this is admin-only, which is why it is amber rather than red. There is no ownership check on $subscriptionId, so it is "an admin can exfiltrate any subscription's broker credentials", and it becomes materially worse the moment that action is widened to a group, which is precisely what the action matrix exists to allow.
🟡 Other
- No replay protection on the
teamsscheme and no dedupe. Microsoft's scheme carries no timestamp, soverifyUntimestamped()is correct — butexternalIdis used only for rule matching, never as an idempotency key. A captured request reopens cases indefinitely, bounded only by#[AnonRateLimit(300,60)]. - Personal data on the broker leg is unbounded and unredacted.
EventService.php:1802publishes the payload verbatim; the webhook leg at least redacts its trace viaSensitiveFieldRegistry. For a Dutch government product a CloudEventdatablock can carry a BSN. Worth an explicit bound, or a documented "the broker is a trusted processor" position. SubscriptionSigningPolicyhas zero callers. Pre-existing, not from this delta — but it is a security-policy class the running code never consults.
The government identity broker (#2132) — checked hard, found nothing
This was the addition I expected to be worst and it is the best-built thing in the delta. POST /api/idp/envelope/exchange is #[PublicPage], so it is the only thing between an anonymous request and a verified citizen identity. It holds up:
hash_equals on the consumer secret, and an unknown consumer is compared against a random string of the same shape so the timing of "unknown consumer" and "wrong secret" match · 256-bit random_bytes codes · one-time redemption via atomic IMemcache::cad(), with the code burnt even on a consumer mismatch so it cannot be probed · createDistributed() with isAvailable(), failing closed without a shared cache · a separate jti replay guard on atomic add() · AlgorithmManager([new HS256()]), so alg confusion and none are impossible · exp checked in both directions, rejecting a lifetime stretched beyond iat + MAX_TTL · BSN pseudonymised with HMAC-SHA256 scoped by organisation and a "�" separator, polymorphic pseudonyms preferred so the BSN never appears, and a 32-byte salt minimum enforced fail-closed · secrets in IAppConfig, not a register object · one undifferentiated 401 · rate-limited.
One gap, 🟢. The signing key has MINIMUM_KEY_BYTES enforced fail-closed and the organisation salt has MINIMUM_SALT_BYTES = 32 enforced fail-closed. The consumer secret has no minimum at all — and the controller's own comment calls it "the only thing between an anonymous request and a verified citizen identity". Two of three secrets are length-guarded; the third, arguably the most exposed, is not.
Also clean
The Teams HMAC itself is correct: hash_equals throughout, HMAC-SHA256 over the raw body, the shared secret base64-decoded in strict mode and failing closed on a bad secret, verification before any side effect, and no oracle — the same 401 for "no configured channel source" and "bad signature". No new schemas in the delta, so no new default-open row store. No new unbound interfaces. No TLS weakening. protocolSettings genuinely is unreadable through the API — the writeOnly strip is not gated on _rbac, so neither subscriptions() nor subscriptionMessages() returns the broker password.
Requesting changes on the two 🔴. Both are small — a host allow-list plus a precedence swap, and a json_decode. I am writing them now and will link the PR here.
…ams reply host Two findings from the security re-review of #1983's delta since f370f87. **The signature verified the raw body; the adapter acted on something else.** `IntakeChannelsController::inbound()` verified `getRawContent()` and then called `$adapter->receive($this->request->getParams())`. Nextcloud builds that set as `array_merge($get, $post, $urlParams, $params)` with the JSON body merged LAST (`Request.php:123`, `:412`), so body keys win a collision but **any key absent from the signed body could be injected through the query string** and reached the adapter — which stores what it is given verbatim as `rawPayload`. The method's own docblock already said this must not happen: "Verification MUST run over the exact bytes the sender signed, not the framework's normalised params, which would desync." The check honoured it; the caller did not. `decodeVerifiedBody()` now derives the payload from `$rawBody` alone. Form-encoded senders still work, because the fix must not turn a signature desync into a broken integration — `parse_str` runs over the same signed bytes. **The Teams reply sent the connector bearer token wherever the payload said.** `$raw['serviceUrl'] ?? $configuration['serviceUrl']` took the PAYLOAD first, and the reply POSTs `Authorization: Bearer <accessToken>` to it. No allow-list, no scheme check, no internal-range refusal — and this transport uses IClientService directly, so it reaches none of the SSRF containment in CallService. An activity is attacker-influenceable; Microsoft's own guidance is that serviceUrl must be validated before use. Configured now wins over payload, and either way the host must be, or be a subdomain of, a trusted suffix over https. The match is on the parsed host, never a substring of the URL, so `https://evil.example/smba.trafficmanager.net` and `https://smba.trafficmanager.net.evil.example/` both fail. An operator replaces the list per source for a sovereign connector. Every fix is mutation-verified. Restoring `getParams()` fails the new controller test with the injected key visible in the diff; restoring payload-first precedence fails one Teams test; removing the allow-list fails four, including both lookalike hosts and plain http. Refs #1983 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The coverage ratchet refused #2137: the two fixes added 30 statements to the files they touch and only some of them were reached, so coverage of the code this branch keeps or adds fell 79.20% -> 78.93%. The uncovered statements were the ones worth pinning anyway, because each is a fallback — the path taken when something is absent or malformed, which is exactly the path nobody exercises by hand: - the form-encoded branch of decodeVerifiedBody(). Reading the body instead of the framework's merged parameters must not break a sender that posts application/x-www-form-urlencoded, so the signed bytes are parsed with parse_str when they are not JSON. - a configured trustedServiceHosts REPLACING the shipped list rather than extending it. An operator narrowing the list to their own tenant expects the public hosts to stop being trusted; an additive implementation would make that narrowing silently ineffective. - an unusable list ('' , [], a bare string, null) falling back to the shipped default — a half-finished edit must not take Teams replies down, nor delete the control. - three refusal branches of isTrustedServiceUrl(): parse_url() returning false outright, a URL with a scheme and no host, and a blank entry in a configured list, which must be skipped rather than read as "trust all". Every one was mutation-verified: flipping each guard to its fail-open form kills exactly one test, so no case is redundant with another. 3834 tests green. phpcs and phpmd clean on both changed lib files. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…c-and-teams-ssrf fix(intake): act on the bytes that were signed, and allow-list the Teams reply host
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Re-review of the delta — both blockers closed, lifting my objection
The delta since my CHANGES_REQUESTED at 9cd5755a is exactly #2137 and nothing else. origin/development equals the PR head 85e5de846d, so nothing is in flight, and beta has not moved either — merge-base is still 73a5d540b0. So this PR is what I reviewed, plus the fix.
The two 🔴, re-verified in the merged tree
Not from the diff — read back out of 85e5de846d, and probed for the ways a fix like this usually fails:
| Check | Result |
|---|---|
Any getParams() still read on the signed path? |
No. The one remaining call (:220) is in saveRule(), authenticated and action-gated — not the webhook path |
| Can the conversation id alter the POST host? | No. rawurlencode escapes / and @ (19%3Athread%40thread.tacv2) |
Userinfo trick — https://smba.trafficmanager.net@evil.example/ |
Refused. parse_url yields host=evil.example, which fails the suffix match |
| Precedence actually swapped in the merged file? | Yes, :291 reads configuration first |
One residual — 🟢, not worth blocking beta
The allow-list is a suffix match over three broad domains, so it narrows the destination from "anywhere on the internet" to "Microsoft's Bot Framework estate" — not to a single host. Reaching it still requires controlling a *.skype.com / *.botframework.com / *.smba.trafficmanager.net host, which is a subdomain-takeover premise this code cannot close.
Separately, Nextcloud's Client::buildRequestOptions() installs an on_redirect hook that blocks redirects to local addresses, but a cross-host redirect to a public host is still followed. Guzzle strips Authorization on a cross-host redirect — I could not verify that in this checkout; Guzzle is not vendored here and not present in the running container, so I am going on recollection and will not make it load-bearing. 'allow_redirects' => false on that one POST would close it without depending on the answer.
An operator who pins configuration.serviceUrl gets exact-host safety today, since configuration now takes precedence. Worth saying so in the connector docs.
Still open from my last review — now filed rather than left in a comment
None of these is a beta blocker, but none should ship recorded only in a review comment:
- Broker credentials re-pointable without ever being read.
updateSubscription()passes$subscriptionIdstraight tosaveObject(uuid: …)with no ownership check; the only gate isevent.update-subscription, seeded["admin"]. Amber because it is admin-gated today — it turns red the moment that action is widened to a group, which is exactly what the action matrix exists to allow. - No replay protection or dedupe on the
teamsscheme.externalIdis documented "unique per channel" but used only for rule matching, never as an idempotency key. - The broker leg publishes the payload verbatim. I checked whether it also writes an unredacted trace: it does not, because it writes no trace step at all — the only
addStepis the webhook leg, which already redacts. So this is delivery-side data minimisation, plus a minor auditability gap. SubscriptionSigningPolicyhas zero callers, and the IdP broker's consumer secret has no length minimum while the signing key and organisation salt both enforce 32 bytes fail-closed.
Verdict
Approving. CI is green — 52 pass, 8 skipping, 0 failures.
One caveat on the record rather than buried: I wrote the fix I am signing off. @bbrands02's approval on #2137 is the independent review here; mine is a self-check, and I would rather say so than have this read as two sets of eyes.
Both 🔴 are fixed and merged as #2137; superseded by the approval in review 5291271191.
|
Follow-up to my approval above, on two points. The four findings I left open are now filed, so they are not carried into beta recorded only in a review comment:
My approval covers the delta I reviewed, not the whole release — this PR is still correctly blocked. @WilcoLouwerse's review 5278999788 remains open, and I have re-checked its items at
So the BSN item he raised is live exactly as written. It is latent only because the seeded I dismissed my own two earlier |
…isters `suggest()` and `resolve()` were the only methods in this controller with no authorization decision — the sole `requireAction` was on `resync()`. Both are `#[NoAdminRequired]`, and `suggest()` takes a free-text query and returns each hit's BSN, so any signed-in account could ask the BRP for a person by name (integriq#1983 review 5278999788). Both carry a `@no-admin-idor-exempt` reason, and both reasons are TRUE — a registry key has no per-object owner to compare against. That is exactly why this was missed: they answer the IDOR question correctly and the endpoint's actual question, *may this account query the BRP*, not at all. The exposure is latent today because the seeded `brp-haalcentraal` source carries `configuration.mock: true`. It opens on the go-live step — paste the RvIG credentials, remove `mock` — which is a configuration change by an operator with no code review in the path. This release introduces the path; `beta` has no PropertySourceController at all. The gate is keyed as an EXEMPTION list, not a sensitive list. Keyed the other way round, a personal-data provider added later ships OPEN whenever someone forgets to update it, which is the failure that produced this bug. Keyed this way a new provider is gated until someone consciously declares it public, so what a maintainer must remember fails in the safe direction. `bag` and `kvk` are open registries and stay ungated — the applicant address lookup that `suggest()` exists for keeps working, and closing BRP does not put an address lookup behind an administrator. The action is unseeded on purpose: `getAllowedGroups()` answers `['admin']` for an unknown action and `requireAction()` reads `['admin']` as "no non-admin passes", so a provider is admin-only the moment it is added, with no seed entry to forget. An operator delegates it to `brp-beheer` in Admin Settings without a deploy — the convention `sensitive-sources.md` prescribes, which until now nothing enforced. Deliberately NOT an OpenRegister RBAC check, and the docblock says why so it is not "fixed" into one: the source row IS the credential store (`configuration.headers.Authorization`, `.cert`, `.ssl_key` are outside the exact-path nested write-only list), so `read` cannot express "may query but may not see the secrets"; and `findSourceBySlug()` reads with `_rbac: false` by ocon#147's design, so `PermissionHandler` never runs on this path and an `authorization` block on the source would be inert. The action matrix is the one surface `_rbac: false` cannot switch off. Per-object granularity needs the rebuild: integriq#2112, integriq#2125, openregister#2432, openregister#4034. One pre-existing test changed meaning: an unknown provider now meets the gate before provider lookup, so an anonymous caller gets 401 rather than a 404 naming the id. That ordering is deliberate — answering 404 first lets an anonymous caller enumerate which registries an instance is wired to. 3839 tests green. Both guards mutation-verified: removing the gate kills three tests, and re-keying the list as a sensitive list kills the one that pins the safe direction. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y-source-queries fix(property-source): gate registry queries, exempting the public registers
Closing round — delta reviewed, CI green, one judgement call left for @WilcoLouwerseThe delta since my approval is exactly #2143 ( CI: 52 pass, 8 skipping, 0 failures, including the full PHPUnit matrix (PHP 8.3/8.4 × NC stable32–35), which the feature PR itself never exercised. @WilcoLouwerse's review 5278999788, re-checked in the merged treeRead out of
On item 3, with one correction in your favourYou flagged But the row holds less than the finding states. It is described as carrying "the template, the payload and the resulting document reference"; the schema deliberately stores none of the first two:
So what a non-admin can read is render metadata — My read is 🟡 and not a beta blocker. It is your finding, so the call is yours. It is carried by #2126 (which explains why the obvious one-fragment lockdown silently breaks One thing deliberately not in #2143Your item 2 had two halves — "hands it to any signed-in account unmasked and with no record that it was asked for." #2143 closes the first. There is no audit record, by choice, to keep the fix narrow while beta was blocked. Also filed from my own rounds, so nothing rides on a review comment#2139 broker credentials re-pointable without being read · #2140 no replay protection on the Where that leaves the PRFrom my side the release is fit to promote, and my approval stands. What remains is your re-review, and concretely two questions: whether render metadata clears your 🟡 bar, and whether the missing audit record belongs before beta or after. |
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict re-review of the delta f370f87..bd5e54f. Five inline comments follow; the verdict comes separately.
There was a problem hiding this comment.
REQUEST_CHANGES — Strict re-review of the delta f370f87..bd5e54f
All four 🔴 from my first review are closed. What blocks now is one red required check and three 🟡 in the new feature code. Every fix belongs on development, because this PR is recreated on each sync.
Closed since last round
| Item | State at bd5e54f |
|---|---|
DnsResolverInterface / CallDispatcherInterface unbound, nine routes 500 |
Closed by #2127: Application.php:541-542, and AppOwnedInterfaceBindingTest pins the class. #2124 closed. |
suggest() / resolve() ungated, suggest() returns BSN |
Closed by #2143: both call requireQueryPermission() (:135, :168) before the registry is touched, and BRP is admin-only by default. |
False @no-admin-idor-exempt on preview() · max-version="35" |
Corrected, and recorded as a channel-pairing decision. |
The post-merge reviews of #2127, #2137 and #2143 raised only 🟡/🟢, now tracked in #2146 and #2147 or on #2137 itself.
What blocks
🔴 quality / Quality Report is red on bd5e54f. The cause is the nightly E2E run (03:18 today): 26 of 294 failed, mostly seeding an outbound message must succeed and Validation failed: required property (name) is missing. These are not regressions. The failing specs were added by the feature PRs they test (#2052, #2055, #2057, #2062, #2063, #2080), all part of this release. The nightly was green on 778e2208 (09-18) and has been red every night since. E2E is skipped on pull_request events, so none of those PRs ever ran its own specs, and the green PR checks on this PR never included them. Either the specs are wrong or the features are, and at the moment nobody has looked.
🟡 The Teams bot token is plaintext configuration.accessToken (#2128). The doc prescribes a credentialRef that nothing resolves, and the path the token actually lives on is not write-only, so admins read it in cleartext.
🟡 The IdP broker's signing key, consumer secrets and BSN salts are set without --sensitive (#2132). Whoever reads occ config:list can mint an envelope for any subject with trust: high.
🟡 The assertion validity window has no upper bound (#2132), so the 900 s replay memory can be outlived. This is latent until the vendor half calls the guard.
Also raised, not blocking: the guard/trust/mint ordering is not enforced · a tenant alias can re-rank a built-in trust level.
Filed as tracking issues
This PR is recreated from development on every sync, so these threads do not survive the next cut. All three are assigned to @rubenvdlinde, author of the feature PRs concerned:
- #2148: the 26 E2E specs that have never passed, and E2E not running on
pull_request. - #2149: the plaintext Teams connector token.
- #2150: the IdP broker's non-sensitive secrets and unbounded assertion window.
@rjzondervan's two questions
documentGenerationJob: render metadata clears my bar for beta. Downgraded, stays in #2126.- The BRP access record: after beta, but before go-live or any delegation. With BRP admin-only and not yet delegable (#2147), the gap is limited to administrators. It stays in #2125.
Provenance and residual
Seven PRs merged in this range. #2127, #2137 and #2143 were each approved by @bbrands02 and reviewed post-merge by me today, so they were spot-checked here. #2128, #2130, #2132 and #2134 (~8k lines) had no review of their own, only @rjzondervan's rounds on this PR, which produced #2137 and #2139–#2142. Those four got the full-depth security read. Clean on that read: hash_equals throughout, single-use codes via cad(), HS256-only, fail-closed without a distributed cache, no BSN in log context, broker transports on IClientService (local-address and redirect guards apply), and one new route (idpBroker#exchange, public, rate-limited). Not read: test bodies beyond the binding test, openspec and admin-doc prose beyond the lines cited, BrokerResult / BrokerPublication / LogBrokerTransport, the l10n files, and IntakeRoutingService's case-join logic.
Checks: Hydra Gates, phpcs, phpstan, psalm, phpmd and the full PHPUnit matrix are green on bd5e54f, consumed rather than re-run. branch-protection / check-branch is green.
Connected issues
| Issue | State | Bearing |
|---|---|---|
| #2124 | closed | Closed by #2127, as above. |
| #2125 | open | Task 1 (the gate) finished by #2143; the access record, bounded suggest() and findSourceBySlug bypass remain. Not a beta blocker, per the answer above. |
| #2126 | open | Downgraded, not blocking beta. |
| #2139, #2140, #2141, #2142 | open | @rjzondervan's delta findings. Not re-raised here; unaffected. |
| #2145, #2146, #2147 | open | Unaffected. |
| #2148, #2149, #2150 | open | Filed today from this review, as above. |
No issue state changed from this review.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
APPROVE — the three 🟡 carried to follow-up issues, by agreement with @rjzondervan
Approving bd5e54f, and replacing my REQUEST_CHANGES from this morning. Nothing in the code changed in between. What changed is the decision: the concerns go to development as follow-ups instead of blocking beta.
- All four 🔴 from my first review are closed, as that verdict records (#2127, #2143).
- The three 🟡 are accepted as follow-up, not fixed. The Teams connector token is #2149; the IdP broker secrets without
--sensitiveand the unbounded assertion window are #2150. The IdP one is latent until the vendor half lands, and #2150 should land before that PR does. - @rjzondervan's two questions stand as answered:
documentGenerationJobafter beta (#2126), and the BRP access record after beta but before go-live or delegation (#2125).
This approval does not make the PR mergeable. The required quality / Quality Report check is red on bd5e54f from the nightly E2E run: 26 specs that the release's own feature PRs added have never passed. That is #2148, for @rubenvdlinde / @rjzondervan to decide on. Either fix the specs, or rerun the PR-event run so the required check reflects it, and record why E2E is not part of the gate.
4 participants
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.