Skip to content

fix(archimate): unset AMEF ids were handed on as empty strings past a === null guard (gate-50 14 → PASS) - #472

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/amef-config-empty-ids
Aug 9, 2026
Merged

fix(archimate): unset AMEF ids were handed on as empty strings past a === null guard (gate-50 14 → PASS)#472
rubenvdlinde merged 1 commit into
developmentfrom
fix/amef-config-empty-ids

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

gate-50 security-config-fail-mode: 14 → PASS. hydra-gates 651e5c5bb3ba8764903e5d6fc5bac5a208bd67fc, measured at the CI scope (--scope-to-diff --base origin/beta).

I nearly dismissed these as false positives

My first read was that gate-50's 10-line window was too narrow and the consumers validate anyway — so I was going to leave all 14 red with that as the reason. Checking every consumer rather than the two convenient ones showed the opposite.

The legacy fallback in both getAmefConfig() implementations read every register/schema id with '' as its default and returned them. The consumers guard with === null:

ViewService:254, :320   if ( === null ||  === null) { throw … }
ViewService:711          = ['register_id'] ?? null;   // then used, unguarded

'' === null is false. An empty id passes the guard and is pinned into an OpenRegister query as the register/schema — and an unpinned query returns rows, which reads exactly like a correct result rather than like a failure.

Why nothing has broken yet

The fallback writes plural key names (views_schema, elements_schema) while every consumer reads singular ones (view_schema, element_schema). Measured producer/consumer overlap: the empty set. So the lookups miss and fall back to null, and the throw fires.

That is an accident of naming, not a defence. Adding the singular keys — the obvious cleanup, and something I nearly did myself earlier in this session — converts it into a live fail-open.

The fix

  • resolveConfiguredId() reads each id and returns null, with a warning naming the key, when it is empty or whitespace. The guard is now at the read, and there is one read instead of eight per file.
  • The fallback array_filters the nulls out, so ?? null downstream yields null — exactly what every consumer already checks for.
  • ViewService::getModulesData() gains the missing register guard and fails closed rather than issuing an unpinned query; its schema loop now uses empty() rather than === null for the same reason.

Narrower than it first looks — and the tests said so

The fallback is only reached when amef_config is malformed, because its default '{}' is valid JSON and decodes to []. My first draft of the tests failed for exactly that reason and taught me the branch condition; serviceWithConfig() now sets amef_config to 'not-json' deliberately, and the unconfigured case asserts [] explicitly so it cannot pass vacuously by iterating an empty array.

Can-fail proof

Reverting the three services to origin/development:

1) testPartialConfigurationKeepsSetIdsAndDropsUnsetOnes
   Failed asserting that an array does not have the key 'views_schema'.
2) testUnconfiguredInstanceOmitsEveryIdRatherThanEmittingEmptyStrings
   Failed asserting that two arrays are identical.
3) testWhitespaceOnlyIdIsTreatedAsUnset
   Failed asserting that an array does not have the key 'register_id'.
Tests: 4, Assertions: 6, Failures: 3.

[gate-50] security-config-fail-mode: FAIL — 14 unsafe security-config read(s)

The fourth test (a configured id comes through unchanged) passes in both directions by design — it is the regression net, not a mirror of the fix.

Other measurements

  • phpcs lib/0 errors / 87 warnings, identical to origin/development.
  • phpmd (repo baseline), psalm, phpstan — clean.
  • Unit suite — 523 tests, 1838 assertions, 0 failures.
  • No other gate count moved.

… `=== null` guard

gate-50 security-config-fail-mode 14 -> PASS, measured with hydra-gates 651e5c5
at the CI scope (--scope-to-diff --base origin/beta).

I nearly dismissed these 14 as false positives of the gate's 10-line window,
on the grounds that the consumers validate. Checking every consumer rather
than the two convenient ones showed the opposite.

The legacy fallback in both getAmefConfig() implementations read every
register/schema id with '' as its default and returned them. The consumers
guard with `=== null`:

    ViewService:254, :320   if ($registerId === null || $viewSchemaId === null) throw
    ViewService:711         $registerId used with NO guard at all

and `'' === null` is false. An empty id therefore passes the guard and is
pinned into an OpenRegister query as the register/schema — and an unpinned
query returns rows, which reads exactly like a correct result.

Nothing reaches a query TODAY only because the fallback writes PLURAL key
names (`views_schema`, `elements_schema`) while every consumer reads SINGULAR
ones (`view_schema`, `element_schema`), so the lookups miss and fall back to
null. Measured producer/consumer key overlap: the empty set. That is an
accident of naming, not a defence — adding the singular keys, the obvious
"cleanup", turns it into a live fail-open.

- resolveConfiguredId() reads each id and returns null, with a warning naming
  the key, when it is empty or whitespace. The guard is now AT the read, and
  there is one read instead of eight.
- The fallback array_filters the nulls out, so `?? null` downstream yields
  null — which is what every consumer already checks for.
- ViewService::getModulesData() gains the missing register guard and fails
  closed rather than issuing an unpinned query; its schema loop now uses
  empty() rather than `=== null` for the same reason.

Narrower than it first looks, and the tests say so: the fallback is only
reached when `amef_config` is MALFORMED, because its default '{}' is valid
JSON and decodes to []. My first draft of the tests failed for exactly that
reason and taught me the branch condition.

Can-fail: reverting the three services turns 3 of the 4 new tests red and puts
gate-50 back to 14.

phpcs lib/ 0 errors, phpmd/psalm/phpstan clean, unit suite 523 tests green.
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ c6a8072

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
composer ✅ 128/128
npm ✅ 718/718
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-09 20:55 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit f8c32f7 into development Aug 9, 2026
29 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/amef-config-empty-ids branch August 9, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant