Problem/Motivation
Here are some findings from Claude Code's Opus 4.7's pre-beta audit of the 4.x branch. Each item below should land as its own commit referencing this issue so the upgrade story stays bisectable and reviewable. Severity tags reflect impact, not effort.
Security
- [SEC] XSS in
NameFormatter::viewElements()— the user-supplied
nameis rendered as#markupand not escaped. Switch to#plain_text
(or escape explicitly). - No
hook_entity_field_access()and no access handler. Either wire them up or remove the file
to avoid misleading site builders. - [SEC] Fingerprint stored as MD5 (
SshKeyItem::onChange(); existing
@todoin code). MD5 collisions are practical. Switch to the already-implemented
Utils::getFingerprintSha256()and provide an update hook for existing rows. - [SEC] No upper bound on the SSH-key value length. Storage column is
text/big. Reject anything past a sane ceiling (e.g. 16 KB — covers RSA up to 16384-bit with a
long comment) inSshKeyConstraintValidatoror via a Length constraint inpropertyDefinitions().
Cleanup
- Remove the scaffolding leftover
fooin
field.storage_settings.sshkey_default(config/schema/sshkey.schema.yml). - Remove or wire up
sshkey_theme()+theme_sshkey_fingerprint()— currently
defined but no renderer calls them. - Fix the misleading "entity property" comment in
SshKeyItem::setValue()— left over from
EntityReferenceItem. - Resolve or file follow-ups for the three lingering
@todocomments
(SshKeyItemlines 18, 96;Utilsline 37). - Either consume
Utils::getFingerprintSha256()from production code or remove it (relates
to the MD5 item above). - Grammar fix in
sshkey.info.ymldescription: "to collect a ssh public keys" →
"for SSH public keys". - Reconcile the
fingerprintproperty'ssetRequired(TRUE)with the storage
schema's'not null' => FALSE. - Document (or constrain) that the same key is allowed across multiple entities — no unique index on
fingerprint today. - Replace
generateSampleValue()with a real wire-format SSH key so devel-generate-style
fixtures actually exercise the validator.
Validator coverage
- Structural validation for ed25519 / DSS is currently just a prefix match.
phpseclib3\Crypt\PublicKeyLoader::load()can verify parseability for any algorithm — extend
the validator past the prefix check.
Comments
Comment #2
colanWorking in !8...
Comment #18
colanMR !8 covers all thirteen audit items, in 13 logical commits plus three small CI fixups (cspell wordlist tweaks, a phpcs line-length, and one test fixture that needed sharper teeth — phpseclib parsed the original "garbage" payload as e=0, n=0). CI is green; the SHA-256 migration hook was also verified locally against real data.
Security-tagged items addressed: the unescaped
#markupXSS inNameFormatter::viewElements(), the absent length cap on the raw key (16 KB ceiling now lives as a Length constraint on thevalueproperty), the unenforcedsshkey.permissions.yml(removed — access already flows through the host entity's field-access logic), and the MD5 fingerprint (now OpenSSH-canonicalSHA256:<base64>, withsshkey_update_10001that recomputes existing rows in batched chunks and tightens the column to NOT NULL when everything parses). Structural validation now routes throughphpseclib3\Crypt\PublicKeyLoaderfor every algorithm, so curve-invalid ed25519 and malformed RSA payloads fail even when the prefix check would have passed.Cleanup-tagged items: info.yml grammar, the misleading
EntityReferenceItem-leftover comment inSshKeyItem::setValue(), thefooplaceholder infield.storage_settings.sshkey_default, the deadsshkey_theme()+theme_sshkey_fingerprint()(the .module file is gone entirely — the formatter renders directly), the three trailing@todo/@DCGmarkers,generateSampleValue()now returns a wire-format key that passes the validator (so devel-generate fixtures actually exercise downstream paths), and the README has been rewritten to match what 4.x actually ships. New unit/Kernel test coverage was added alongside each behavior change. Ready for review/merge.Comment #35
colanMerged!