fix(security): fail closed on unconfigured register/schema bindings (gate-50, gate-54, gate-1) - #413
Closed
rubenvdlinde wants to merge 4 commits into
Closed
fix(security): fail closed on unconfigured register/schema bindings (gate-50, gate-54, gate-1)#413rubenvdlinde wants to merge 4 commits into
rubenvdlinde wants to merge 4 commits into
Conversation
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.
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.
…ured 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.
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.
Contributor
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| test | ✅ | ||||
| test-unit | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 114/114 | |||
| npm | ✅ | ✅ 646/646 | |||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-09 15:45 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Author
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.
Gates closed (package
7f5f9e78, =.githubmain + PR #286)Can-fail proof for gate-50: removing one guard produced exactly
FAIL — 2, naminglib/Service/FinancialExtractionService.php:259and:260— the two reads whose guard was removed, and nothing else.'docudesk', and the five accessors keep it that way — the reads stayed visible rather than being hidden behind a computed key.The defect
Every gate-50 finding was one shape: a register/schema binding read with an empty-string default, passed straight into
saveObject()/find(). An administrator sets these in the admin UI and nothing auto-provisions them, so an unbound instance wrote to register''and schema''and carried on — for signing, the audit trail behind an eIDAS-level signature.SettingsServicenow owns the reads with one guarded accessor per binding, returningnullrather than throwing (so it needs no new import — see the coupling note). Each consumer decides what unconfigured means:SigningServicethrowsRegisterNotConfiguredException(already handled bySigningController),GlAccountSuggestionServicelets it propagate to controllers that catchException,PortalSigningReceiverControllerdenies.Two sites gate-50 did NOT flag are fixed too —
getRequest()and the decline path sat inside the gate's 10-line window thanks to 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.SigningVerificationService::getSigningSecret()now returns?string. Its single caller already failed closed 85 lines away, so there was no live defect — but''is a valid HMAC key, and a caller that forgot the check would verify signatures against a publicly derivable key and call them genuine.PHPMD: four metrics went over, no threshold changed
composer phpmdexit 0.SigningServicecoupling was resolved by droppingIAppConfigentirely — its three remaining reads duplicated the keys and the defaults ('30','SES','native') thatSettingsService::loadFeatureToggles()already owns, two sources of truth free to drift.lint/phpcs/psalm/phpstan/phpmdall exit 0.What is left (why this is a draft)
vendor/bin/phpunit: 13 errors + 5 failures, all mine. Three suites buildSettingsServicemocks without stubbing the newresolve*Binding()accessors, so they return null and the fail-closed guard fires — which is the guard working, not a code defect.SigningServiceTestis already fixed this way (33/33 green). Remaining:GlAccountSuggestionServiceTestbuilds three mocks (lines 78, 414, 469) and only two are stubbed;PortalSigningReceiverControllerTestandFinancialExtractionServiceTesteach have an unstubbed instance. Mechanical, but unfinished and therefore not claimed as done.