Skip to content

fix(auth): trim CHAR-padded password_algo and password_salt#970

Open
AH-dark wants to merge 1 commit into
cedar2025:masterfrom
AH-dark:fix/password-algo-char-padding
Open

fix(auth): trim CHAR-padded password_algo and password_salt#970
AH-dark wants to merge 1 commit into
cedar2025:masterfrom
AH-dark:fix/password-algo-char-padding

Conversation

@AH-dark

@AH-dark AH-dark commented Jun 1, 2026

Copy link
Copy Markdown

Problem

v2_user.password_algo and v2_user.password_salt are declared as CHAR(10).
MySQL right-pads CHAR values with spaces on insert. The switch in
Helper::multiPasswordVerify uses strict equality:

case 'sha256': return hash('sha256', $password) === $hash;

So a legacy v2board user whose password_algo is stored as 'sha256 '
(6 chars + 4 trailing spaces) NEVER matches 'sha256', falls through to the
default: branch (password_verify), and can never log in. The salted
variants are doubly broken because the padded salt corrupts the hash
recomputation.

Affected algorithms: md5 (3 chars), sha256 (6 chars), md5salt (7 chars).
sha256salt (10 chars) accidentally works because it exactly fills the column.

Fix

  1. New migration converts both columns to VARCHAR(10) and runs an
    idempotent raw-SQL TRIM() backfill for any existing space-padded rows.
  2. Helper::multiPasswordVerify now defensively trims and null-coalesces
    $algo and $salt before the switch, and uses hash_equals() for
    timing-safe hex comparison.
  3. New unit test HelperPasswordVerifyTest covers all 5 verification
    branches plus padded / null / whitespace edge cases.
  4. LoginServiceTest gains two integration cases that seed
    legacy-padded rows and prove login succeeds end-to-end.

Helper signature is unchanged — fully backward compatible.

Migration down() caveat

Rolling back to CHAR(10) reintroduces the bug functionally. The down
method is provided for completeness but is documented as a "do not run in
production without a follow-up data migration" path.

Verification

  • php artisan test — full suite green (SQLite default).
  • MySQL Docker: column type changes to varchar(10), padded rows trimmed.
  • Migration idempotency: re-running migrate is a no-op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant