>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3620118. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !33 >>> <h3>Problem/Motivation</h3> <p>An audit of the 1.x head at 02ca175, the commit 1.0.0-alpha7 currently points at, found seventeen defects ahead of the release. phpcs and PHPStan at level 5 are silent on all seventeen: each one is a gap between what the module documents and what it actually enforces, which is the class of defect that matters most in a module whose product is a record nobody can forge.</p> <h3>Security</h3> <ul> <li><strong>The forensic stamp loses to caller context.</strong> Both write paths merge uid, ip, request_uri and message_template into the transient bucket with the array union operator, which keeps whatever key is already present, and that bucket is seeded from the caller's own context array. A PSR-3 call passing a uid key therefore replaces the recorded actor, and the row is then HMAC-signed and permanently attested with the wrong one. No compromise is required, only a key-name collision. No shipped bridge passes any of the four keys, so nothing currently depends on that precedence.</li> <li><strong>ChainArchiver::isWebAccessiblePath() misses most web-accessible paths.</strong> It is a two-prefix string test for the public stream wrapper, and it is the only gate keeping NDJSON archives, which carry uids, IP addresses, request URIs and before/after snapshots, out of the served tree. An absolute path inside the docroot passes it, so does a relative path into the public files directory, and the settings form invites exactly that by describing the field as accepting an absolute host path. The docblock claims no false negatives are wanted.</li> <li><strong>The settings form is the one route gated on a permission that is not the module's own.</strong> 32 of the 34 routes require view audit trail reports, run audit trail verification or administer audit trail, and one requires the entity bridge's own permission. The settings form requires administer site configuration, and it sets the archive directory, the four retention windows, the auto-verify switch and the checkpoint policy. A role holding only the generic site-configuration permission can therefore relocate archives, stop verification and shorten retention.</li> </ul> <h3>Correctness</h3> <ul> <li><strong>The two ingresses resolve a channel to different chains.</strong> AuditTrail::resolveChain() tries a chain-id match before the channels claim; AuditTrailLogger::resolveChain() tries the claim first. Given a chain "foo" and a chain "bar" whose channels list contains "foo", an event on channel "foo" lands in a different chain depending on which API the caller reached for. The docblock on the first asserts it applies the same order as the second. Separately, an explicit chain id in the context targets that chain through the logger and is ignored by event(), which reads the key only as a truthy switch.</li> <li><strong>Four entries filters emit MySQL-only SQL.</strong> The uid, search, client_ip and request_uri filters use raw JSON_UNQUOTE and JSON_EXTRACT fragments. SQLite has json_extract but no JSON_UNQUOTE, and PostgreSQL has neither name, so those filters raise a database exception instead of returning rows. Reproduced by running the controller's own kernel test class against SQLite: 3 errors, "no such function: JSON_UNQUOTE". The predicates carry a docblock claiming Drupal's database layer normalizes them, and no such normalization exists. audit_trail.install meanwhile advertises the schema as portable across MySQL, PostgreSQL and SQLite, and nothing in the project declares a driver requirement. The Kernel job runs on MySQL only, which is why this has stayed green.</li> </ul> <h3>Performance</h3> <ul> <li><strong>The segments list runs one COUNT and three secret loads per row.</strong> AuditTrailSegmentsController::list() calls ChainArchiver::buildSegmentRow() once per row at a page size of 50. Each call issues a COUNT against the audit_trail table for the segment range, and resolves three secret slots, each of which loads and sorts every audit_trail_secret entity. That is the same N+1 shape #3619114 removed from the chains list, and ChainRepository now has a plural sibling for every singular reader except getRowCountInRange().</li> <li><strong>Three of the six cron lifecycle stages have no per-tick cap.</strong> runCoverage() selects every uncovered row on the chain with no range() and applies its cap to the resulting buckets afterwards, so the whole result set materializes in PHP first. runLivePurge() and runFilePurge() iterate every eligible segment, each iteration taking the chain write lock and deleting a range or unlinking a file. The other three stages each carry a MAX_ ... _PER_CRON_RUN cap, so the intent is established; these three miss it.</li> <li><strong>Every chained write reloads the whole secret inventory twice.</strong> KeyBackedSecretRepository routes all six of its public methods through one private loadEntities(), which calls loadMultiple() with no ids and then sorts the result. A single row write calls it twice, once for the current id and once for the bytes. Config entities are not statically cached: ConfigEntityType sets static_cache to FALSE, and EntityStorageBase::loadMultiple() gates its static cache on the ids argument being set, which a null-argument call fails. So each call re-runs listAll() and re-hydrates every entity object.</li> <li><strong>The canonical write API reloads all chain entities per event, while the PSR-3 one memoizes them.</strong> AuditTrailLogger builds a chain registry once per request and invalidates it from a hook. AuditTrail::resolveChain(), reached by every event() call and therefore by every create, update and delete on a bridged entity type, loads all chain entities, filters for active, and sorts them again on every call. The same data, two policies, and the un-memoized one is on the hotter path.</li> </ul> <h3>Documentation</h3> <ul> <li><strong>The archive filename pattern is documented wrong in seven places</strong>, two of them operator-facing: the archive-directory description on the settings form, and the directory option help on the drush archive command. Both still describe the retired flat layout rather than the per-chain, per-year path the archiver writes. An operator restoring from cold storage follows that help, looks in the wrong directory, and concludes the archive is missing.</li> <li><strong>The CHANGELOG section is still open as Unreleased</strong>, and #3619136 is cited nowhere in it.</li> <li><strong>The four-stage lifecycle is now five.</strong> Compaction shipped as a real stage with its own config key, cron branch and drush command, but the README, docs/index.md, docs/commands.md, docs/roadmap.md and two docblocks all still say four. docs/roadmap.md compounds it by proposing a fifth "forget" stage under Brainstorming for the problem compaction already addresses, opening with the claim that the shipped lifecycle ends at file-purge.</li> <li><strong>Five docblocks name classes and keys that do not exist:</strong> a PSR-3 adapter class that was never in this repository, a default identity contributor that ships nowhere, a retired add-archive form, a dropped pre-release form narrated in the present tense, and a context key spelled actor_uid where the stored key is uid.</li> </ul> <h3>Drupal standards</h3> <ul> <li><strong>Config schema uses label as a description field.</strong> Labels run to several sentences, the longest around 500 characters, where core's longest in system.schema.yml is 74. A label is the short name a config UI puts next to the key, and DataDefinition reads a sibling description key, which is where the prose belongs.</li> <li><strong>There is no audit_trail.api.php</strong>, though the module ships a custom alter hook, implemented by the TSA submodule, and two attribute-discovered plugin types intended for third parties. None of the three is documented where a Drupal developer looks for it.</li> <li><strong>One library hardcodes a version</strong> where the other five correctly omit the key and inherit the extension version, so that one asset URL stops busting cache across releases.</li> <li><strong>The README has no Requirements, Installation or Configuration section.</strong> It never states that drupal/key is a hard runtime dependency, that PHP 8.2 is the floor, or that a private file system has to be configured before an archive or an import can work.</li> </ul> <h3>Proposed resolution</h3> <p>One merge request, one commit per finding. Every behavior change carries a test that was run against the unfixed code and seen to fail there; the merge request description lists which ones failed and how many.</p> <ul> <li>The forensic keys are stamped last and win. A colliding caller value is kept under <code>_caller_supplied</code> rather than dropped, and only when it actually differs, since core hands the module its own normalised values on every request-bound call. The actor-forgery row in docs/security.md now says what is still caller-controlled.</li> <li>A stream wrapper is judged by name and a schemeless host path by containment in the document root. Core's visibility flags cannot separate the two: <code>PrivateStream::getType()</code> reports <code>LOCAL_NORMAL</code>, exactly like <code>PublicStream</code>, and a site's private directory is protected wherever it sits.</li> <li>The settings form moved to administer audit trail, noted in the CHANGELOG upgrade block, which already tells operators to re-grant permissions for this release.</li> <li>Both ingresses share one chain registry with one resolution order, the logger's, since an explicit channel claim should beat an incidental id collision. Along the way this fixed a row-dropper: <code>CHAIN_ONLY</code> is the string <code>only</code>, and the resolver read any string as a chain id, so it looked for a chain by that name, found none, and discarded the entry outright.</li> <li>The four JSON predicates are spelled per driver by <code>Chain\ContextJsonText</code>, unit-tested across the dialect matrix because the Kernel job runs a single driver. The normalisation claim is gone.</li> <li><code>ChainRepository::getLiveRowCountsForSegments()</code> answers the whole page in one grouped query. The three secret loads per row went away with the request-scoped inventory memo rather than a threaded map.</li> <li>The three uncapped stages take a per-tick cap. The coverage scan drops its trailing partial bucket the way the still-open-bucket branch already did, but never the only bucket, or a bucket wider than the cap would be dropped forever.</li> <li>The filename pattern is corrected in all seven places. Closing the CHANGELOG section with the release date remains part of cutting the tag.</li> <li>Memoize the sorted secret inventory on the repository for the request, cleared by the secret entity's own save and delete hooks, the way AuditTrailLogger::resetRegistry() and the chain registry hook already do it for chains.</li> <li>Keep label short in every config schema key and move the prose to description; add audit_trail.api.php for the alter hook and the two plugin types; drop the hardcoded library version; give the README its Requirements, Installation and Configuration sections.</li> <li>Correct the stage count in all six places, rewrite the roadmap section to describe what compaction does and what it deliberately does not, and fix the five dangling docblock references.</li> </ul> <h3>Remaining tasks</h3> <p>ChainArchiver is 4265 lines and 60 methods in one class, about 15 percent of the module's production code, and the segments-list N+1 exists because a listing helper sits next to the archiving machinery instead of in a reader. That belongs in its own issue, not this one.</p> <h3>User interface changes</h3> <p>None, beyond corrected help text on the settings form and the four entries filters working on SQLite and PostgreSQL.</p> <h3>API changes</h3> <p>None on the consumer surface: AuditTrailInterface and AuditTrailSubject are untouched, so the bridges keep working. One new method on ChainRepository, one new optional argument on ChainArchiver::buildSegmentRow(), one permission requirement changed on an admin route, and a new audit_trail.api.php documenting the alter hook and the two plugin types.</p> <h3>Data model changes</h3> <p>None. No schema change, no update hook, and the canonical signed payload is untouched, so existing chains keep verifying across the upgrade.</p> <p>AI-Generated: Yes (Claude Code was used to audit the module and to help draft this issue summary, and to write the code and tests on the merge request. I reviewed and ran the work myself before posting it: each new test was run against the unfixed code and seen to fail there.)</p>