>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3619748. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !31 >>> <p>Three audits have run on this module and none of them looked for duplication. A sweep of the whole tree, tests included, found the same logic written out more than once in twenty places.</p> <p>Two of those copies sign chain rows, and their own docblocks say so: <code>ChainArchiver::getPayloadFromRow()</code> carried "Mirrors <code>AuditTrailVerifier::getPayloadFromRow()</code> exactly [...] Both implementations must stay in lockstep: diverging here breaks archive replay." That is the failure mode. A column added to the canonical payload on one side and not the other makes the archiver write NDJSON the verifier reads as tampered, and nothing fails until an operator verifies an archive.</p> <p><strong>How the duplication was found</strong></p> <p>Windowed-hash clone detection over every PHP file in the module, with identifiers and literals normalized so a renamed variable cannot hide a clone, each hit extended greedily past the window to its real length. Then expression-level greps for idioms shorter than the window. Every hit was read by hand: boilerplate a framework requires (a <code>create()</code>, a <code>getEditableConfigNames()</code>, an entity attribute) is not duplication and is dismissed.</p> <p><strong>Production code</strong></p> <ul> <li>The canonical column set a row's hash is computed over existed three times, in the verifier, in the archiver and in the entry detail page. It is now <code>Chain\ChainPayload</code>, read by all three, with a unit test pinning the exact column set and the object-row / array-row equivalence.</li> <li><code>verifyRow()</code> and <code>verifyRowPublic()</code> each carried their own copy of the chain-link and public-hash checks. Both now call <code>verifyRowStructure()</code>, and the eleven break messages go through <code>buildBrokenRowMessage()</code> instead of repeating the same prefix and its two arguments.</li> <li>Both verifier walks kept the same three accumulator locals and closed a broken range in four places between them. That bookkeeping is <code>Chain\BrokenRangeCollector</code>: a range left open at end-of-chain silently under-reports a tamper, so it is worth having one implementation of it.</li> <li>Five of the twelve lifecycle signatures passed eight stamps positionally. <code>computeLifecycleHmacFromRow()</code> takes only the stamps the transition writes and reads the other six off the row by name; the remaining direct calls now use named arguments.</li> <li>Ten production sites computed <code>(int) (microtime(TRUE) * 1_000_000)</code> themselves, one of them behind a private helper whose docblock claimed to be the canonical one. <code>Time\Microtime::getCurrent()</code> is now that one place.</li> <li>The cron worker had five identical try / catch / warn blocks around a per-segment lifecycle op. They are <code>applyLifecycleOp()</code>, next to the <code>runStage()</code> that already existed for whole stages.</li> <li>The walk-scoped lazy secret cache, including the decision to remember an unavailable secret as NULL, existed twice in <code>ChainArchiver</code>.</li> <li>The filter walk and the contributor walk logged a plugin failure in the same shape, including the <code>'chain' =&gt; FALSE</code> that keeps the diagnostic from recursing back through the walk. That guard now lives in <code>reportPluginFailure()</code>.</li> <li>The chain form built contributor rows and filter rows with two copies of the same three cells. One <code>buildPluginRow()</code> now serves both, with the per-family CSS classes, callbacks and element keys spelled out in one table rather than assembled from the family name, so they stay greppable.</li> <li>The secret form and the TSA provider form built the same three action buttons. That is <code>Form\LifecycleEntityActionsTrait</code>, and since both entity interfaces declared the same <code>isActive()</code> / <code>isPending()</code> contract, they now share <code>LifecycleStatusInterface</code>, which is what lets the trait type its argument instead of describing it.</li> <li>The file bridge and the user-auth bridge each carried a full copy of the per-event opt-in form and its submit handler. A bridge now extends <code>Form\EventOptInSettingsFormBase</code> and declares only its own action verbs.</li> </ul> <p><strong>Tests</strong></p> <ul> <li>Twenty-one kernel classes installed the same four chain tables; <code>installAuditTrailTables()</code> says once why installing a subset only moves the failure.</li> <li>Eighteen sites wrote the same UPDATE to break a row's integrity. <code>tamperRow()</code> names what that fixture is for.</li> <li>The same segment counter existed as <code>countArchives()</code> in one class and <code>countSegments()</code> in another, across thirty call sites.</li> <li><code>ChainTimestamperTest</code> carried byte-identical copies of two helpers already in the shared trait.</li> <li><code>ChainArchiverTest::readSegmentRow()</code> and <code>AuditTrailCronArchiveHooksTest::writeAgedRow()</code> now reuse the neighboring helper they were duplicating.</li> </ul> <p><strong>Examined and left alone</strong></p> <ul> <li>The five segment confirm forms already share a base class. What repeats between them is per-operation copy and the placeholders each message actually uses, which differ.</li> <li>Constructor injection, <code>create()</code>, <code>getEditableConfigNames()</code> and entity attributes: shapes the framework requires, not logic.</li> <li>The two-line refuse-and-bounce idiom in the confirm forms. Six sites, but naming it would cost a cross-module trait to save one line each.</li> <li><code>CanonicalizeBytesTest</code>'s payload fixture. It is a hand-written mirror of the canonical shape next to the exact bytes it must produce, and that is the point of the test.</li> </ul> <p><strong>Two deliberate behavior changes</strong></p> <p>The rest of this is a refactor, but giving the canonical payload one owner exposed two ways a caller could end up with a payload signed over something that was not the row, and both are closed. <code>createFromRow()</code> takes <code>\stdClass|array</code> rather than <code>object</code>: casting any other class mangles the name of every non-public property into <code>"\0*\0chain"</code>, dropping the column that belongs in the payload, and reading the public properties instead would drop it just as quietly.</p> <p>The second is a row arriving short, from a SELECT with a partial field list. The absent column reads as an empty string and hashes to a different value, which nothing downstream could tell apart from a tamper, and the archiver would write that payload into an NDJSON. That is a mistake in this module's own queries rather than anything a request can cause, so an assertion refuses it: live in test and development where such a query gets written, compiled out of production where this runs once per row of every chain walked.</p> <p><strong>Testing</strong></p> <p>Three suites cover the new shared code: a unit test pinning the canonical column set and the rows it refuses, a unit test pinning the broken-range bookkeeping, whose every failure mode under-reports a tamper without failing anything, and a kernel test covering the shared opt-in settings form, which had no coverage at all before. The full unit and kernel suites pass. phpcs (Drupal and DrupalPractice, whole module, all extensions) reports nothing outside the pre-existing markdown line-length warnings, and phpstan at the module's level 3 is clean with no ignores added.</p> <p>Measured against 1.0.0-alpha6 with a kernel probe rather than argued: the write path, the one that runs inside the audited site's own request, is unchanged at 2.00 queries and 0.090 ms per row. The verify walk costs 6.35 us per row against 6.20, and it runs on cron, on drush and on two admin buttons, never in a request the audited site makes. An earlier version of the shared payload extractor was 8.6x slower per row than the code it replaced; that was found by auditing the hot path and fixed before this was proposed.</p> <p>AI-Generated: Yes (audit, refactor, tests, issue summary; reviewed before submission)</p>