>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3620282. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !85 >>> <p>An audit over security, performance, Drupal standards, documentation, translations and duplicated logic, run against 1.x at 0ade549. Security came back clean and the evidence is recorded below so the next audit starts from it: 61 declared routes all gated with none carrying _access: TRUE, no secret compared or logged, no interpolated SQL, the return-URL guard fail-closed on both browser landings, and the webform download path resolving through the viewer's own handle so a submission reviewer cannot reach someone else's bytes. phpcs over 353 files and PHPStan at level 5 over the whole module are both clean, and the Drupal 12 lane on this commit's own pipeline ran all 78 test classes green with zero skips.</p> <h3>The changelog is dropped</h3> <p>CHANGELOG.md is removed. It claimed to follow Keep a Changelog and did not: three releases in a row were tagged with the release still sitting under Unreleased, and the file is the only thing in the repository that has to be closed by hand before a tag. Nothing links to it, it is not in the mkdocs navigation, and the release notes on each release node already carry the same information for the audience that reads it. The git history and the issue queue are the record.</p> <p>Dropping it also retires three claims in it that no longer matched the code. It credited a WeightedEntityListBuilderTrait that exists nowhere in the module, when the shipped mechanism is core's own SORT_KEY constant, one line per list builder and better than the trait it describes. It said the three batch forms take getCancelUrl() from BatchActionFormTrait when four forms use that trait and one of them overrides the method. And it announced that the four pdv.offer.* log keys had become pdv.consent.*, while two of them had not.</p> <h3>The cross-site API spec is missing the unlock relay</h3> <p>docs/openapi.yaml documents no unlock endpoint at all. Both halves of the passphrase handoff are absent: the browser landing at /pdv-api/unlock/start and the server-to-server code exchange at /pdv-api/unlock/exchange. A consumer implemented from the spec therefore has no way to unlock a passphrase-protected vault, which is the one flow it cannot work around.</p> <p>Two more gaps in the same file. The vault_locked 403 body that any read or write can return is undocumented, while its sibling consent_required is documented in full, so the same consumer cannot recognize the signal that tells it to send the owner to unlock. And no operation documents 429, though every data endpoint and the consent landing pass through the flood guard, which throws TooManyRequestsHttpException. The listing endpoint documents no error response at all.</p> <p>The spec also states that all data responses are sent Cache-Control: no-store. One endpoint does not: kindLabels() is the only handler that never calls the response finalizer, so it sends neither that header nor the X-Correlation-ID echo that the threat model says every request carries. The finalizer is called noStore(), which names one of the two things it does, and that name is why skipping it at a non-PII endpoint reads as harmless. It is renamed for both jobs and kindLabels() calls it.</p> <h3>Three dispatched audit events are in no catalog</h3> <p>pdv dispatches pdv.grant.resolved, pdv.trust.declined and the whole pdv.user channel, whose single action is purged: an entire vault crypto-erased. None of the three appears in pdv_audit_trail's shipped config, its config schema, or the checkbox list its settings form builds, so an operator cannot see them and cannot turn them off. The bridge defaults an unknown key to enabled, so the rows do reach the chain today; what is missing is the catalog, and the schema would reject the key if anyone tried to set it. All three are added in the four places, with their French.</p> <h3>A trait import that is one deleted override away from a fatal</h3> <p>ItemDeleteForm uses BatchActionFormTrait, whose getCancelUrl() calls manageReturnUrl(). That method lives in OwnerOrAdminManageTrait, which ItemDeleteForm does not use. The class survives only because it redeclares getCancelUrl() itself, so the trait's copy is never reached; a later pass that removes the redeclaration as a duplicate turns the batch-delete confirmation into a call to an undefined method. Static analysis cannot see it, because the class method wins and the trait is never analyzed against this class, and no test exercises the trait path.</p> <h3>Performance: the third expiring table has no index</h3> <p>pdv.install indexes expires_at on pdv_grant and on pdv_consumer_authorization, with a comment explaining that without it a bounded WHERE expires_at &lt;= ? LIMIT N still full-scans the table on every cron run. pdv_grant_request carries the same column and the same query shape and was left out. Three places hit it: the store-request purge on cron, which additionally sorts on expires_at and therefore filesorts the whole matching set before applying its limit, the stale-request reaper, and the backlog count that runs on every Status Report render. The index is added.</p> <h3>Naming: what the last two sweeps left behind</h3> <p>Two log keys still carry the retired pdv.offer namespace, both dispatched on the pdv.consent channel, and one of them also keeps the space-separated shape the log-key sweep removed everywhere else. Three more keep that shape under a live namespace: pdv.item.store begin, pdv.item.store failed and pdv.gc reaped. All five now read as the event's identity, which is the convention the sweep established.</p> <p>accessRequestsTable() is the one member of its family that does not lead with a verb; its seven siblings are all build*Table(). Eight method names referenced in docblocks no longer exist, every one of them renamed by the verb-first sweep: clientRef, keyBytes, newSalt twice, readableItems, resolveBytes, storeRequestsTable, trustedKinds and trustedKindsByScope, plus a test method still named after the old newSalt. The ECA plugin's help text, its French and the submodules page all offer stored as an example action; no event has used that word since pdv.item.store done became pdv.item.created, so an operator following the example builds a model that never fires.</p> <p>Three smaller items: pdv.libraries.yml declares three libraries with no version key while pdv_webform sets version: VERSION as every core library does; the audit-trail config schema still labels its consent group Upload-offer consent events, a translatable operator-facing string carrying a retired word; and PdvGrantRequest reads an empty expires_at as zero where its two sibling entities read it as no expiry, and exposes isExpired() where they expose isActive().</p> <h3>Translations: one string filed under the wrong project, and a CI job so the next one is caught</h3> <p>Both directions were run per project rather than against the union of the twelve .po files, which is the only way the question can be asked correctly: Drupal imports a module's translations/fr.po only when that module is installed, so a string's French has to live in the catalog of the project whose sources contain it.</p> <p>The owner downloads a full copy of their vault (data portability) is translated in the root pdv catalog and its source is in pdv_audit_trail, so it is simultaneously the root's only orphan and pdv_audit_trail's only missing string, and a site that installs the bridge reads that description in English. It sits one line below Data exports (pdv.export), which the previous audit did move: the label moved and its description did not. One other string has no French at all, the warning PdvTenantForm shows when a tenant's Master KEK changes. Everything else is clean, and every remaining potx-extracted string with no entry is a logger message, correctly absent.</p> <p>A misfiled string is not something an audit should be finding, so pdv gets the translations job orchestra and yoyaku already run: scripts/check-translations.php with translations/untranslated.txt beside it, drupal/potx as a development dependency, and an advisory job in the validate stage at the same severity as its peers. It reports two directions per project, missing and misfiled, and extraction is potx rather than a search for t(), because knowing what counts is the hard half: label: in config/schema counts and the same key in config/install does not. The one string this audit found is precisely what the job exists to catch, and pdv is the third project of the three to have hidden that class of bug behind a whole-tree check that cannot see it. pdv_consumer_console, which ships no catalog on purpose because it is hidden and development-only, is recorded in untranslated.txt with that reason rather than left to be re-flagged by every future run.</p> <h3>Duplicated logic: six identical bodies</h3> <p>getExpiresAt() and isActive() are byte-identical on PdvGrant and PdvConsumerAuthorization, the pair left standing when ten other accessors were folded into seven entity traits, and getFile() is byte-identical on PdvItem and PdvGrantRequest, the same two entities the sealed-payload trait already serves. Both are folded into traits alongside their siblings. Three test fixtures were written twice and are folded as well.</p> <p>Two hardening items are included rather than filed separately. The cross-site unlock landing has no flood guard while its consent sibling has one, and each call writes a private tempstore row. And the vault overview declares a user cache context and four list tags while its content turns on the unlock cookie, which no context varies on; nothing is cached today because two embedded forms force max-age zero, which is what the render array should say for itself.</p> <p>AI-Generated: Yes (Claude Code was used to run this audit, draft this issue summary, and write the fixes, tests and translations on the merge request. I reviewed and ran the work myself before posting it.)</p>