fix(#2171,#2172): canonical column names, enforced scanner_version, correct blocker validation - #2174
Merged
Merged
Conversation
…venance
Doctrine keys listTableColumns() by the column's QUOTED name when the
identifier needs quoting, while Column::getName() stays canonical. Three
production callsites read array_keys()/isset() on that map:
- DatabaseFieldAccessInventoryScanner emitted a live key <type>|*|"key"
that no definition can classify, so unclassified_entries was never
empty and ready was never true for any consumer with a reserved-word
column, and the schema fingerprint carried the same literal;
- LiveEntitySchemaFingerprint computed the boot-side half of that same
fingerprint;
- DatabaseAuthorizationCodeRepository::ensureColumn() reported such a
column absent and re-ran ADD COLUMN on every boot.
Both fingerprint halves were consistently wrong, so they agreed by
accident. Correcting only the scanner would make every production boot
fail as "stale for the current framework or schema" on a reserved-word
column, which is worse than the defect, so they change together and a
regression test pins their agreement with a mutation control proving a
genuine schema change still moves the fingerprint.
Same root cause as #2163 at callsites that fix did not reach. A fourth
instance of one mistake is why TableColumnNames is now the only way to
ask the question.
Also #2171: scanner_version was parsed and never compared. The generation
decides which tables the fingerprint covers (v2 narrowed it to entity
storage, #2143), so an artifact from another generation answers a
different question while staying internally consistent with a valid
checksum. CURRENT_SCANNER_VERSION is now enforced in both directions.
Retracts the original report's broader claim: stale artifacts did NOT
fail open. assertReady() already refused a missing or malformed artifact,
framework-version drift, schema-fingerprint drift, checksum mismatch, a
forged ready flag, and an honest ready:false. Those paths are unchanged
and now pinned, with a positive control so the suite cannot pass by
rejecting everything.
Also #2172: array_all() invokes its callback as ($value, $key), so the
bare array_all($value, 'is_string') raised ArgumentCountError for every
NON-EMPTY blocker list — meaning any artifact that actually carried a
blocker was reported as "malformed" instead of naming it. Failed closed,
so never a hole; an operator was handed the wrong diagnosis mid-deploy,
and it is why the honest not-ready path had no coverage.
spec-reviewed: docs/specs/infrastructure.md
spec-drift flagged cli-kernel.md and entity-system.md as stale. Both are
genuine contract changes, so they are documented rather than silenced
with a trailer:
- cli-kernel.md: preflight inventory keys are canonical column names.
The key FORMAT is unchanged (entityType|bundle|field); what changed
is that `field` is now the canonical name rather than whatever
Doctrine used as a map key, which for a reserved word was the quoted
identifier.
- entity-system.md: CURRENT_SCANNER_VERSION is now a contract rather
than a recorded value, enforced in both directions, with a note on
when to bump it — when the MEANING of a scan changes, not its
implementation. Carries the correction that the pre-existing
fail-closed checks were never broken.
infrastructure.md was already acknowledged by the trailer on 09b6bd5.
Note: the detector also warns that packages/oidc/ maps to no spec, so
the DatabaseAuthorizationCodeRepository change was not coupling-checked.
Left as-is: adding an oidc entry to PATTERN_TO_SPEC is a tooling change
outside this fix's scope.
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
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.
fix(#2171, #2172): reserved-word columns and unverifiable preflight provenance
Closes #2171
Closes #2172
Found while moving
jonesrussell/rhtcircleonto the publishedv0.1.0-alpha.284artifacts, where the field-access preflight could never reachready: true.1. Reserved-word columns broke the preflight — three callsites
Doctrine keys
listTableColumns()by the column's quoted name whenever the identifier needs quoting: a column namedkeyarrives under the array key'"key"', whileColumn::getName()remains the canonical'key'.array_keys()on that map yields identifier literals, not column names.packages/cli/src/Security/DatabaseFieldAccessInventoryScanner.php:65array_keys(listTableColumns(...))<type>|*|"key"that no field definition can classify, sounclassified_entrieswas never empty andreadywas nevertrue— for any consumer with a reserved-word column. Also poisoned the schema fingerprint with the same literal.packages/foundation/src/Kernel/Preflight/LiveEntitySchemaFingerprint.php:34array_keys(listTableColumns(...))packages/oidc/src/Repository/DatabaseAuthorizationCodeRepository.php:164isset($columns[$column])ALTER TABLE … ADD COLUMNon every bootWhy both fingerprint halves had to change together
They were consistently wrong, so they agreed by accident. Correcting only the scanner would make every production boot fail as
"stale for the current framework or schema"on any entity with a reserved-word column — strictly worse than the original defect.ReservedWordColumnPreflightTest::the_boot_side_and_artifact_side_fingerprints_agreepins the agreement, and a companion mutation control proves a genuine schema change still moves the fingerprint (otherwise agreement could hold vacuously).Canonical fix
Same root cause as #2163 (
DBALSchema::fieldExists()) at callsites that fix did not reach. A fourth independent instance of one mistake is why this adds a single accessor rather than patching three call sites:packages/database-legacy/src/Schema/TableColumnNames.php—for()andsortedFor(), returning canonical names viagetName(). All three callsites now route through it.packages/field/migrations/2026_05_25_000005_index_classification_label.php:49usesarray_key_exists()on the same map.classification_labelis not reserved, so it is currently benign; left unchanged and noted.2. Unverifiable preflight provenance was accepted (#2171, second half)
Correction to the original report
The issue originally claimed stale artifacts "fail open" because
framework_versionandschema_fingerprintdrift went unchecked. That was wrong and is retracted.FieldAccessActivationPreflight::assertReady()already failed closed on: a missing artifact, a malformed one,framework_versiondrift,schema_fingerprintdrift, checksum mismatch, a forgedreadyflag, and an honestready: false. Those paths are unchanged here and are now pinned by regression tests.The genuine gap
scanner_versionwas parsed (FieldAccessActivationPreflight.php:29) and then never compared to anything.The generation determines what a preflight actually swept: v2 narrowed the schema fingerprint from every physical table to entity storage only (#2143). An artifact from a different generation therefore answers a different question than the running framework asks — while remaining internally consistent, carrying a valid checksum, and potentially matching on fingerprint where the two table sets coincide. Nothing downstream catches it.
The checksum does not help:
scanner_versionis insidecanonicalData(), so a hand-edited value is caught, but an artifact legitimately produced by another generation is self-consistent.Fixed with
FieldAccessPreflightData::CURRENT_SCANNER_VERSION, enforced in both directions — a newer artifact is as opaque to this framework as an older one.3. Every real blocker was misreported as a malformed artifact (#2172)
array_all()invokes its callback as($value, $key), so the barearray_all($value, 'is_string')raisedArgumentCountErroron any non-empty list. SincestringList()parsesconflicts,unclassified_entries,v1_drivers,serialized_entitiesandlegacy_payloads, every artifact carrying a blocker — exactly the case the guard exists to report — was diagnosed as a corrupt file instead of by name.Boot still refused, so this was never a hole. It was an operator handed the wrong diagnosis mid-deploy, and it is why the honest not-ready path had no coverage. An empty list short-circuits before the callback runs, which is why clean artifacts worked and it went unnoticed. Swept
packages/for the same misuse: sole instance.Tests
packages/cli/tests/Unit/Security/ReservedWordColumnPreflightTest.php— 14 tests acrosskey,order,group,index:ready: truefor a fully classified schemapackages/foundation/tests/Unit/Kernel/Preflight/FieldAccessActivationFailsClosedTest.php— 10 tests:packages/foundation/tests/Unit/Kernel/FieldAccessActivationPreflightTest.php— fixture pinned toCURRENT_SCANNER_VERSIONinstead of a stale1. This is not an accommodation: two tests in that file expect exceptions, and with a stale literal they would have passed for the scanner-version reason rather than the tampering and blocker reasons they name.Not included
No release. This branch is validated locally and linked into RHT Circle for a full linked-stack acceptance run before any tag is cut.