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
    name is rendered as #markup and 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
    @todo in 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) in SshKeyConstraintValidator or via a Length constraint in propertyDefinitions().

Cleanup

  • Remove the scaffolding leftover foo in
    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 @todo comments
    (SshKeyItem lines 18, 96; Utils line 37).
  • Either consume Utils::getFingerprintSha256() from production code or remove it (relates
    to the MD5 item above).
  • Grammar fix in sshkey.info.yml description: "to collect a ssh public keys"
    "for SSH public keys".
  • Reconcile the fingerprint property's setRequired(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

colan created an issue. See original summary.

colan’s picture

Working in !8...

  • colan committed 279125a7 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Make generateSampleValue() produce a parseable...

  • colan committed f678d319 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Cap the SSH-key value at 16 KB.
    
    SshKeyItem...

  • colan committed 72641175 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Escape admin-supplied name in NameFormatter (...

  • colan committed 0b0d9107 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Drop the dead sshkey.module file.
    
    sshkey_theme...

  • colan committed c88e0119 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Remove the 'foo' placeholder from storage-...

  • colan committed bc4deef1 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Fix misleading comment in SshKeyItem::setValue...

  • colan committed 2ab473bb on 3590156-pre-stable-audit
    Issue #3590156 by colan: Fix grammar in module description.
    
    "Provides a...

  • colan committed a94bbd0a on 3590156-pre-stable-audit
    Issue #3590156 by colan: Fix CI failures from prior commits.
    
    Three...

  • colan committed b4620230 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Migrate fingerprints from MD5 to OpenSSH SHA-...

  • colan committed 44109ceb on 3590156-pre-stable-audit
    Issue #3590156 by colan: Fix the SHA-256 migration commit's CI and...

  • colan committed e3ff8071 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Validate full key structure via phpseclib.
    
    The...

  • colan committed c85b4666 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Remove unenforced sshkey.permissions.yml.
    
    The...

  • colan committed 2fb54ca7 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Fix CI failures from the structural-check...

  • colan committed 075b7182 on 3590156-pre-stable-audit
    Issue #3590156 by colan: Resolve trailing @todo and @DCG scaffold...

  • colan committed 621707ba on 3590156-pre-stable-audit
    Issue #3590156 by colan: Rewrite README to match the module as it...
colan’s picture

MR !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 #markup XSS in NameFormatter::viewElements(), the absent length cap on the raw key (16 KB ceiling now lives as a Length constraint on the value property), the unenforced sshkey.permissions.yml (removed — access already flows through the host entity's field-access logic), and the MD5 fingerprint (now OpenSSH-canonical SHA256:<base64>, with sshkey_update_10001 that recomputes existing rows in batched chunks and tightens the column to NOT NULL when everything parses). Structural validation now routes through phpseclib3\Crypt\PublicKeyLoader for 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 in SshKeyItem::setValue(), the foo placeholder in field.storage_settings.sshkey_default, the dead sshkey_theme() + theme_sshkey_fingerprint() (the .module file is gone entirely — the formatter renders directly), the three trailing @todo/@DCG markers, 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.

  • colan committed affcf7bd on 4.x
    Merge branch '3590156-pre-stable-audit' into '4.x'
    
    Resolve #3590156 "...

  • colan committed 621707ba on 4.x
    Issue #3590156 by colan: Rewrite README to match the module as it...

  • colan committed 2fb54ca7 on 4.x
    Issue #3590156 by colan: Fix CI failures from the structural-check...

  • colan committed 075b7182 on 4.x
    Issue #3590156 by colan: Resolve trailing @todo and @DCG scaffold...

  • colan committed e3ff8071 on 4.x
    Issue #3590156 by colan: Validate full key structure via phpseclib.
    
    The...

  • colan committed c85b4666 on 4.x
    Issue #3590156 by colan: Remove unenforced sshkey.permissions.yml.
    
    The...

  • colan committed 44109ceb on 4.x
    Issue #3590156 by colan: Fix the SHA-256 migration commit's CI and...

  • colan committed b4620230 on 4.x
    Issue #3590156 by colan: Migrate fingerprints from MD5 to OpenSSH SHA-...

  • colan committed a94bbd0a on 4.x
    Issue #3590156 by colan: Fix CI failures from prior commits.
    
    Three...

  • colan committed 279125a7 on 4.x
    Issue #3590156 by colan: Make generateSampleValue() produce a parseable...

  • colan committed f678d319 on 4.x
    Issue #3590156 by colan: Cap the SSH-key value at 16 KB.
    
    SshKeyItem...

  • colan committed 72641175 on 4.x
    Issue #3590156 by colan: Escape admin-supplied name in NameFormatter (...

  • colan committed 0b0d9107 on 4.x
    Issue #3590156 by colan: Drop the dead sshkey.module file.
    
    sshkey_theme...

  • colan committed c88e0119 on 4.x
    Issue #3590156 by colan: Remove the 'foo' placeholder from storage-...

  • colan committed bc4deef1 on 4.x
    Issue #3590156 by colan: Fix misleading comment in SshKeyItem::setValue...

  • colan committed 2ab473bb on 4.x
    Issue #3590156 by colan: Fix grammar in module description.
    
    "Provides a...
colan’s picture

Status: Active » Fixed

Merged!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.