fix(gate-54): a genuine cross-app reference could not be authored at all - #286
Merged
Merged
Conversation
`x-external-register` appeared ZERO times in check_relation_dialect.py, so the
checker had no concept of a reference to a schema owned by another app — and
for such a property BOTH arms of the gate failed:
WITH "$ref": "case" -> (f) "$ref 'case' does not resolve to a schema key
in the register set (case-exact)"
WITHOUT "$ref" -> (b) "relation-shaped property (format:uuid +
relation description) lacks canonical $ref
(ADR-062 rule 7)"
Measured 2026-08-09 on docudesk at package e7bde0a: `correspondence.
caseReference` and `generatedDocument.zaakId` both point at a Zaak that lives
in Procest's register, and both are marked `x-external-register: procest`.
There was no authoring that satisfied the gate. The only route to green was to
reword the description until `_RELATION_DESC_RE` stopped matching — degrading
documentation to dodge a regex, which is exactly what a gate must never reward.
That is an unclosable gate (gate-59's own subject matter).
OpenRegister resolves `$ref` inside ONE register set, so a foreign schema is
not expressible as a relation. A property carrying `x-external-register` is
therefore a plain identifier: it must not be asked for a `$ref`, and it must
not carry one.
The exemption is deliberately narrow, and the true positive survives:
* rule (b) skips a property that declares `x-external-register`;
* rule (f) still reports one that declares BOTH `x-external-register` and a
`$ref`, because that `$ref` is one OpenRegister can never resolve — with a
message naming the actual fix ("drop the $ref") instead of the generic
"does not resolve", which pointed the author the wrong way;
* a dangling `$ref` WITHOUT the marker keeps its original message;
* filter-token validation is untouched.
Four tests added, each of which fails before this change (two on the message,
two as controls that pin the widening). Full helper suite: 28 tests, all green.
Contributor
Author
|
Independent confirmation from a second repo and a different cross-app shape — this fixes openconnector too. I hit the same wall on openconnector and filed #289 before seeing this. Same working tree, two package refs, nothing changed but the checker:
The property is Why this is a useful second data pointdocudesk's cases point at a Zaak in Procest. Mine points at a credential in OpenRegister's credential broker:
The narrowness holdsI confirmed the exemption suppresses only the two One note for whoever merges thisUntil this lands, the annotation is inert on main and the gate reports the original finding byte-for-byte — so a repo that adopts #289 can be closed as a duplicate of this once it merges. |
rubenvdlinde
added a commit
to ConductionNL/docudesk
that referenced
this pull request
Aug 9, 2026
…ured (gates 1, 50, 54) (#414) * fix(spdx): PreferencesController carried no @copyright gate-1 (spdx-headers) reported one missing @copyright fleet-wide for this repo, on lib/Controller/PreferencesController.php. Every other controller in lib/Controller/ carries the full Conduction tag block plus the two SPDX identifier lines; this file was written with @author/@license only. Adds @copyright and the SPDX-FileCopyrightText / SPDX-License-Identifier lines, matching PolicyController.php verbatim in shape. * fix(register): drop two $refs OpenRegister can never resolve gate-54 (relation-dialect) reported: correspondence.caseReference — $ref 'case' does not resolve to a schema key generatedDocument.zaakId — $ref 'case' does not resolve to a schema key Both are correct. There is no `case` schema in DocuDesk's register set, and there cannot be: both properties reference a Zaak that lives in PROCEST's register, which is why both already carry `x-external-register: procest`. OpenRegister resolves `$ref` within one register set, so these two `$ref`s named a schema the engine could never reach — a relation that only looked like one. Nothing consumed them as relations either: PdfService reads `caseReference` as `(string) ($options['caseReference'] ?? '')` and GeneratedDocumentLogger passes `zaakId` straight through. Dropping the `$ref` leaves the correct dialect — a bare `format: uuid` identifier plus `x-external-register` recording where the target lives. The descriptions were Dutch prose ("UUID verwijzing naar de bron zaak/case in Procest"); re-authored in English per the fleet rule, and expanded to say why there is no `$ref`. `Zaak` is a standardised ZGW term and is kept as such — only the surrounding prose changed, and the property name `zaakId` is untouched. NOTE: this alone does not close gate-54. The gate had no concept of a cross-app reference, so removing the `$ref` merely flipped the finding to the opposite arm ("relation-shaped property ... lacks canonical $ref"). Both arms failed — an unclosable gate. Fixed in ConductionNL/.github#286; with that package this file measures gate-54 PASS. * fix(security): fail closed when a register/schema binding is unconfigured gate-50 (security-config-fail-mode): 29 findings -> 0. Every finding was the same shape: a register/schema binding read with an empty-string default and passed straight into saveObject()/find(). An administrator sets these in the admin settings UI and nothing auto-provisions them, so on an unbound instance DocuDesk wrote to register '' and schema '' and carried on. For signing that means the audit trail behind an eIDAS-level signature; for financial extraction the supplier, IBAN, KvK, VAT id and amounts read off an invoice. Nothing surfaced. SettingsService now owns the reads, one guarded accessor per binding (resolveSigningRequestBinding, resolveSignerRecordBinding, resolveFinancialExtractionBinding, resolveGlAccountBookingBinding, resolveGlAccountMappingRuleBinding). They return null rather than throwing so that SettingsService needs no new import — see the coupling note below — and each consumer decides what "unconfigured" means for it: * SigningService throws RegisterNotConfiguredException, which SigningController already catches to render a calm "not configured" empty state. Two further sites gate-50 did NOT flag are converted too: getRequest() and the decline path sat inside the gate's 10-line window because of an adjacent null-check, but carry the same defect — find() against register '' returns null, and both report "not found", so an unconfigured instance answered 404 for every signing request that does exist. * GlAccountSuggestionService lets it propagate; both its controllers catch Exception, so it becomes an honest error instead of a wrong answer. "No booking history" and "not configured" produced the same empty array, and the ranker treats the first as a legitimate cold start — an unbound instance silently ranked every supplier as brand new. * PortalSigningReceiverController denies. This is the anti-IDOR boundary (REQ-DDPSA-004) and register/schema are two of the four filters scoping the lookup; passing '' made the boundary's correctness depend on OpenRegister choosing to match nothing for an empty filter, an assumption this code never stated and does not control. The existing catch collapses it to the same null the wrong-email and wrong-request cases return, so no new signal is exposed. SigningVerificationService::getSigningSecret() now returns ?string. Its single caller already failed closed correctly, 85 lines away and outside the gate's window, so there was no live defect — but '' is a VALID HMAC key, and a future caller that forgot the check would verify signatures against a publicly derivable key and report them genuine. null cannot reach hash_hmac() by accident. PHPMD: the guards pushed four metrics over threshold, and no threshold was changed. SigningService's coupling was resolved by dropping IAppConfig entirely — its three remaining reads (signing_request_expiry_days, signing_default_level, signing_provider) duplicated both the keys AND the defaults that SettingsService::loadFeatureToggles() already owns, two sources of truth free to drift. SettingsService stays at its previous coupling because the accessors return null instead of importing the exception type. GlAccountSuggestionService's class complexity is back under threshold because the guards moved out of it. SigningServiceTest stubs the two binding resolvers. That is not incidental: an unstubbed mock returns null, which is exactly what the fail-closed guard is there to catch, and 24 of its tests failed until the configured path was stated explicitly. * test: stub the new binding resolvers in three suites Partial. FinancialExtractionServiceTest, GlAccountSuggestionServiceTest and PortalSigningReceiverControllerTest construct SettingsService mocks that do not stub the new resolve*Binding() accessors, so they return null and the fail-closed guard fires. GlAccountSuggestionServiceTest builds three separate mocks (lines 78, 414, 469) and only two are stubbed; the Portal suite has a second mock instance still unstubbed. 18 tests remain red for this reason. * fix(security): route binding resolution through OpenRegisterResolver Completes the gate-50 fail-closed work. SettingsService owns the READ and returns null when a binding is unset; OpenRegisterResolver turns that null into RegisterNotConfiguredException, alongside the template accessors that already did exactly this. The split keeps the exception type out of SettingsService, whose object coupling sits at its PHPMD ceiling, and puts the 'an unset binding is an error' decision in the class named for it. GlAccountSuggestionService, FinancialExtractionService and PortalSigningReceiverController now inject the resolver and no longer inject IAppConfig at all — every config read they had was a binding read, so the dependency became unused once those moved. Two further sites gate-50 did NOT flag are converted for the same reason as the signing ones: FinancialExtractionService::addCorrection() sat inside the gate's 10-line window because of an adjacent null-check, but find() against register '' returns null and it reports 404 'not found' — so an unconfigured instance denied corrections for extractions that exist. Tests construct a REAL OpenRegisterResolver over a stubbed SettingsService rather than mocking the resolver: the resolver is the piece that turns an unset binding into the exception, so mocking it would remove the behaviour under test. 1136 tests pass; composer check:strict passes. * docs: these accessors return null; the caller is what fails closed * test: cover the fail-closed arm of every register/schema binding The coverage ratchet caught this: the change added 48 statements and dropped coverage 61.53% -> 61.39% against the merge base. The uncovered statements were the ones that matter most — the arm that fires when a binding is UNSET, which is precisely the path that had never been exercised before this work. 23 tests, data-driven over all five bindings and all four resolver methods: * each SettingsService::resolve*Binding() returns the configured pair; * each returns NULL when the register half is unset; * each returns NULL when the schema half is unset (both halves are required — a pair containing '' is what silently wrote to register ''); * each OpenRegisterResolver::get*() returns the binding when configured; * each THROWS RegisterNotConfiguredException when it is not. Verified to fail: replacing one throw with a `['register' => '', 'schema' => '']` fallback produced exactly one failure — testResolverThrowsWhenTheBindingIsUnset with data set "glAccountBooking" — and nothing else. A test that only proved the happy path would have passed against the old code too, which is the failure mode this whole change exists to remove.
This was referenced Aug 9, 2026
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.
The gate was unclosable
x-external-registerappears zero times incheck_relation_dialect.py. The checker has no concept of a reference to a schema owned by another app — and for such a property both arms fail:"$ref": "case"$ref 'case' does not resolve to a schema key in the register set (case-exact)$refrelation-shaped property (format:uuid + relation description) lacks canonical $ref (ADR-062 rule 7)Reproduced directly against
main(e7bde0a), one finding on each arm.Measured on docudesk:
correspondence.caseReferenceandgeneratedDocument.zaakIdboth reference a Zaak that lives in Procest's register, and both already declarex-external-register: procest. There is no authoring that satisfies the gate. The only route to green was to reword the description until_RELATION_DESC_REstopped matching — degrading documentation to dodge a regex, which is exactly what a gate must never reward. This is gate-59's own subject matter.The rule
OpenRegister resolves
$refinside one register set, so a foreign schema is not expressible as a relation. A property carryingx-external-registeris therefore a plain identifier: it must not be asked for a$ref, and it must not carry one.The exemption is narrow, and the true positive survives
x-external-register;x-external-registerand a$ref— that$refis one OpenRegister can never resolve. The message now names the actual fix ("drop the$ref") instead of the generic "does not resolve", which pointed the author in the wrong direction and is what produced the flip-flop in the first place;$refwithout the marker keeps its original message;Tests
Four added to
test_check_relation_dialect.py, each failing before this change:test_external_reference_without_ref_is_accepted— the closable armtest_external_reference_with_a_dangling_ref_is_still_reported— true positive survives, message is actionabletest_a_local_dangling_ref_is_unaffected— control: widening a checker until it catches nothing is not a fixtest_external_marker_does_not_excuse_a_bad_filter_token— control: the exemption is scoped to the$refrules onlyFull helper suite: 28 tests, all green (24 pre-existing + 4 new). Verified the 2 pre-fix failures with
python3 -m unittestbefore implementing.End-to-end
docudesk
gate-54: FAIL — 2 findings→PASS, with its register also corrected to drop the two dangling$refs (ConductionNL/docudesk, separate PR). Both halves were required: the register fix alone flipped the finding to the other arm.