Issue #1306: パスワードハッシュの自動マイグレーション機能の追加（Argon2id対応）#1374
Open
nanasess wants to merge 6 commits intoEC-CUBE:masterfrom
Open
Issue #1306: パスワードハッシュの自動マイグレーション機能の追加（Argon2id対応）#1374nanasess wants to merge 6 commits intoEC-CUBE:masterfrom
nanasess wants to merge 6 commits intoEC-CUBE:masterfrom
Conversation
ログイン成功時に旧アルゴリズム(SHA1/HMAC-SHA256)のパスワードハッシュを 現在の設定アルゴリズムへ自動マイグレーションする機能を追加。 PASSWORD_HASH_ALGOS に PASSWORD_DEFAULT や PASSWORD_ARGON2ID を設定することで PHP標準の password_hash()/password_verify()/password_needs_rehash() を使用した 最新アルゴリズムへの透過的な移行が可能になる。 - sfIsPasswordHashAlgos(): PASSWORD_HASH_ALGOS がpassword_hash()対応か判定 - sfNeedsReHash(): 再ハッシュ判定（password_needs_rehash()活用） - sfReHashPassword(): 再ハッシュ実行 - sfGetHashString()/sfIsMatchHashPassword(): password_hash形式のサポート追加 - 会員/管理者ログイン時の自動マイグレーション処理追加 - AUTH_TYPE=PLAIN用テストのCI個別実行対応 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughログイン時に既存ハッシュを検証し、必要ならば新しいハッシュへ自動で再生成・DB更新する自動マイグレーション機能を追加。関連ユーティリティ、初期設定、コンテナ環境、テスト、CIワークフローも併せて更新。 Changes
Sequence DiagramsequenceDiagram
participant User as ユーザー
participant Server as サーバー<br/>(認証処理)
participant DB as データベース<br/>(dtb_customer / dtb_member)
User->>Server: ログイン要求 (ID + パスワード)
Server->>DB: 顧客/会員情報取得 (password, salt)
DB-->>Server: 取得済みハッシュ返却
Server->>Server: sfIsMatchHashPassword(pass, hash, salt)
alt 検証失敗
Server-->>User: 認証失敗
else 検証成功
Server->>Server: sfNeedsReHash(hash, salt)
alt 再ハッシュ不要
Server-->>User: 認証成功
else 再ハッシュ必要
Server->>Server: sfReHashPassword(pass) -> newHash, newSalt
Server->>DB: UPDATE password, salt WHERE id=...
DB-->>Server: 更新完了
Server-->>User: 認証成功 (自動マイグレーション済)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1374 +/- ##
==========================================
- Coverage 54.42% 54.35% -0.08%
==========================================
Files 84 84
Lines 10817 10859 +42
==========================================
+ Hits 5887 5902 +15
- Misses 4930 4957 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
新規インストール時およびデフォルト設定で、password_hash() による
bcrypt ハッシュが使用されるようになる。
PHP のバージョンアップに伴い PASSWORD_DEFAULT が変更された場合、
自動的に最新の推奨アルゴリズムが適用される。
- SC_Initial.php, tests/config.php, eccube_install.sh: PASSWORD_DEFAULT に変更
- html/install/index.php: インストーラのアルゴリズム選択を PASSWORD_DEFAULT に簡素化
- docker-compose*.yml: 環境変数を '2y' (PASSWORD_DEFAULT の値) に変更
- sfIsMatchHashPassword(): レガシーハッシュ検証時は常に hash_hmac('sha256') を使用するよう修正
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
php_unit_method_casing ルールに準拠するよう、テストメソッド名から アンダースコアを除去してキャメルケースに変更。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yml (1)
86-88:auth_type_plainをディレクトリまで固定すると CI の取りこぼしが出ます。この step は main run の
--exclude-group auth_type_plainと対になっているので、ここが唯一の実行経路です。tests/class/util/SC_Utils/まで絞ると、別ディレクトリに@group auth_type_plainを追加した瞬間に CI から漏れます。グループで切り分ける方針ならパス指定は外すか、少なくともtests/class/まで広げたいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yml around lines 86 - 88, The CI step currently hardcodes the test path in the docker command (the phpunit invocation that uses --bootstrap tests/require_auth_type_plain.php and targets tests/class/util/SC_Utils/), which will miss any other tests marked with `@group` auth_type_plain; update the phpunit command to remove the specific directory filter (or at least broaden it to tests/class/) so it relies on --group auth_type_plain to select tests (i.e., keep the --bootstrap tests/require_auth_type_plain.php and --group auth_type_plain flags but change the target path passed to phpunit to either nothing or tests/class/ instead of tests/class/util/SC_Utils/).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/class/util/SC_Utils.php`:
- Around line 1774-1789: sfGetHashString's optional salt parameter currently
diverges from sfIsMatchHashPassword's legacy-empty-salt handling, breaking
verification for empty-salt hashes; preserve the public signature and ensure
generation follows the same legacy SHA1 path when $salt == '' by routing
sfGetHashString($str, '') to the same legacy algorithm used by
sfIsMatchHashPassword (use AUTH_MAGIC/legacy SHA1 logic) instead of the HMAC
branch that uses PASSWORD_HASH_ALGOS, and ensure constants AUTH_MAGIC and
PASSWORD_HASH_ALGOS are used consistently so existing plugins keep working.
- Around line 1779-1781: The code currently calls hash_hmac(PASSWORD_HASH_ALGOS,
...) even when sfIsPasswordHashAlgos() is false, which passes unsupported
password_hash-only algos (e.g. argon2id) into hash_hmac; change the logic in
SC_Utils.php around SC_Utils_Ex::sfIsPasswordHashAlgos() so that: (1) when
sfIsPasswordHashAlgos() is true continue to use password_hash with
PASSWORD_HASH_ALGOS; (2) when false, distinguish legacy HMAC-allowed algos from
unsupported config and map to a safe HMAC algorithm (e.g. 'sha256') before
calling hash_hmac; update all hash_hmac(PASSWORD_HASH_ALGOS, ...) sites
(including the other occurrences noted and any callers in SC_Customer.php and
LC_Page_Admin_Index.php) to use this whitelist-or-fallback approach rather than
passing PASSWORD_HASH_ALGOS directly.
In `@html/install/index.php`:
- Line 1017: The generated config currently writes PASSWORD_HASH_ALGOS as a
string literal (from $algos = '...') causing sfIsPasswordHashAlgos() to fail
because password_algos() returns algorithm names; change the generator to emit
the constant reference instead of a quoted string: set the code that assigns
$algos (and the later generation that writes define('PASSWORD_HASH_ALGOS', ...))
to use the PASSWORD_DEFAULT constant (unquoted) so the produced line is
define('PASSWORD_HASH_ALGOS', PASSWORD_DEFAULT); ensuring
sfIsPasswordHashAlgos() and password_hash() use the correct algorithm values.
In `@tests/require_auth_type_plain.php`:
- Around line 1-6: tests/require_auth_type_plain.php defines the AUTH_TYPE
constant as 'PLAIN' before including other files, but
data/mtb_constants_init.php unconditionally defines AUTH_TYPE = "HMAC", causing
E_NOTICE on redefinition; update data/mtb_constants_init.php to only define
AUTH_TYPE if it is not already defined (wrap the define('AUTH_TYPE', 'HMAC') in
an if (!defined('AUTH_TYPE')) check) so existing definitions from
tests/require_auth_type_plain.php are respected and notices are avoided.
---
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 86-88: The CI step currently hardcodes the test path in the docker
command (the phpunit invocation that uses --bootstrap
tests/require_auth_type_plain.php and targets tests/class/util/SC_Utils/), which
will miss any other tests marked with `@group` auth_type_plain; update the phpunit
command to remove the specific directory filter (or at least broaden it to
tests/class/) so it relies on --group auth_type_plain to select tests (i.e.,
keep the --bootstrap tests/require_auth_type_plain.php and --group
auth_type_plain flags but change the target path passed to phpunit to either
nothing or tests/class/ instead of tests/class/util/SC_Utils/).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6dbe405-b453-4931-8088-2788054cfea9
📒 Files selected for processing (16)
.github/workflows/unit-tests.ymldata/class/SC_Customer.phpdata/class/SC_Initial.phpdata/class/pages/admin/LC_Page_Admin_Index.phpdata/class/util/SC_Utils.phpdocker-compose.mysql.ymldocker-compose.pgsql.ymldocker-compose.sqlite3.ymldocker-compose.ymleccube_install.shhtml/install/index.phptests/class/util/SC_Utils/SC_Utils_sfIsMatchHashPassword_authTypePlainTest.phptests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypeHmacTest.phptests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypePlainTest.phptests/config.phptests/require_auth_type_plain.php
dtb_member.salt の NOT NULL 制約により、password_hash() 形式で 再ハッシュ時に空文字列を設定すると PostgreSQL/SQLite3 で NOT NULL 制約違反が発生する問題を修正。 SC_Utils::PASSWORD_HASH_SALT_DUMMY 定数を導入し、 password_hash() 使用時はダミー値を設定するようにした。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sfIsPasswordHashAlgos() が false の場合のフォールバックで PASSWORD_HASH_ALGOS をそのまま hash_hmac() に渡していたが、 PASSWORD_HASH_ALGOS が password_hash() 専用アルゴリズム(argon2id等) の場合に hash_hmac() が失敗する可能性があった。 sfIsMatchHashPassword() と同様に 'sha256' をハードコードして レガシーHMACパスの安全性を確保する。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
data/class/util/SC_Utils.php (1)
1808-1825:hash_equals()による定時間比較の統一
password_verify()の分岐は内部で定時間比較を行いますが、PLAIN および レガシーハッシュ の分岐は===を使用しているため、タイミング攻撃に対して脆弱です。認証処理では両方の分岐をhash_equals()に統一し、すべての比較を定時間で実行することが安全です。修正案
if (AUTH_TYPE == 'PLAIN') { - return $pass === $hashpass; + return hash_equals($hashpass, $pass); } // password_hash() 形式のハッシュを検出 ($2y$..., $argon2id$... 等) $info = password_get_info($hashpass); if ($info['algo'] !== null && $info['algo'] !== 0) { return password_verify($pass, $hashpass); } // 既存ロジック: HMAC-SHA256 / SHA1 if (empty($salt)) { // 旧バージョン(2.11未満)からの移行を考慮 $hash = sha1($pass.':'.AUTH_MAGIC); } else { // レガシーハッシュの検証は常にHMAC-SHA256を使用 $hash = hash_hmac('sha256', $pass.':'.AUTH_MAGIC, $salt); } - return $hash === $hashpass; + return hash_equals($hashpass, $hash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/class/util/SC_Utils.php` around lines 1808 - 1825, The PLAIN and legacy-hash branches use === which is vulnerable to timing attacks; change those equality checks to constant-time comparisons using hash_equals. Specifically, in the AUTH_TYPE == 'PLAIN' branch replace "return $pass === $hashpass;" with a hash_equals call (e.g. return hash_equals((string)$hashpass, (string)$pass);), and after computing the legacy $hash (both the sha1 and hash_hmac branches) replace "return $hash === $hashpass;" with return hash_equals((string)$hashpass, (string)$hash); while keeping the password_verify($pass, $hashpass) branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/class/util/SC_Utils.php`:
- Around line 1846-1870: The code currently treats any non-password_hash()
configuration as legacy; update sfNeedsReHash (and the similar block around
sfReHashPassword) to only silently treat the explicit legacy algorithm 'sha256'
as legacy, and for any other unsupported password_hash algorithm (i.e. when
SC_Utils_Ex::sfIsPasswordHashAlgos() returns false but the configured algorithm
is not 'sha256') raise a clear configuration error via SC_Helper_HandleError (do
not fall back to HMAC-SHA256); locate the checks in sfNeedsReHash and the
corresponding logic in sfReHashPassword, add an explicit branch that checks the
configured algorithm string, allow legacy only for 'sha256', and call
SC_Helper_HandleError to report unsupported password_hash() algorithm
configuration.
- Around line 1833-1836: The sfIsPasswordHashAlgos function currently calls
in_array(PASSWORD_HASH_ALGOS, password_algos()) using loose comparison; change
the in_array call in sfIsPasswordHashAlgos to pass the strict=true third
argument so it uses strict type comparison (in_array(PASSWORD_HASH_ALGOS,
password_algos(), true)) to avoid false positives when PASSWORD_HASH_ALGOS is
non-string or falsy.
---
Nitpick comments:
In `@data/class/util/SC_Utils.php`:
- Around line 1808-1825: The PLAIN and legacy-hash branches use === which is
vulnerable to timing attacks; change those equality checks to constant-time
comparisons using hash_equals. Specifically, in the AUTH_TYPE == 'PLAIN' branch
replace "return $pass === $hashpass;" with a hash_equals call (e.g. return
hash_equals((string)$hashpass, (string)$pass);), and after computing the legacy
$hash (both the sha1 and hash_hmac branches) replace "return $hash ===
$hashpass;" with return hash_equals((string)$hashpass, (string)$hash); while
keeping the password_verify($pass, $hashpass) branch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49e75c8a-9fff-43e2-a6de-c5024af7f19d
📒 Files selected for processing (2)
data/class/util/SC_Utils.phptests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypeHmacTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypeHmacTest.php
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
data/class/util/SC_Utils.php (1)
1817-1826: タイミング攻撃対策としてhash_equals()の使用を推奨します。Line 1825 で
===を使用してハッシュ値を比較していますが、これはタイミングセーフではありません。理論的には、攻撃者が応答時間の差異を利用してハッシュ値を推測する可能性があります。
password_verify()は内部でタイミングセーフな比較を行いますが、レガシーパス（SHA1/HMAC-SHA256）ではhash_equals()を使用することを推奨します。♻️ タイミングセーフな比較への修正案
// レガシーハッシュの検証は常にHMAC-SHA256を使用 $hash = hash_hmac('sha256', $pass.':'.AUTH_MAGIC, $salt); } - return $hash === $hashpass; + return hash_equals($hashpass, $hash); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/class/util/SC_Utils.php` around lines 1817 - 1826, Replace the non-timing-safe strict comparison (return $hash === $hashpass;) with a timing-safe comparison using PHP's hash_equals; specifically, in the SC_Utils function where $hash and $hashpass are computed, return hash_equals((string)$hashpass, (string)$hash); so legacy SHA1/HMAC-SHA256 checks use a constant-time comparison and avoid type warnings by casting to string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@data/class/util/SC_Utils.php`:
- Around line 1817-1826: Replace the non-timing-safe strict comparison (return
$hash === $hashpass;) with a timing-safe comparison using PHP's hash_equals;
specifically, in the SC_Utils function where $hash and $hashpass are computed,
return hash_equals((string)$hashpass, (string)$hash); so legacy SHA1/HMAC-SHA256
checks use a constant-time comparison and avoid type warnings by casting to
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3571022a-1f0e-4d2e-aea0-a98e9228cb5f
📒 Files selected for processing (1)
data/class/util/SC_Utils.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1306
ログイン成功時に旧アルゴリズム(SHA1/HMAC-SHA256)のパスワードハッシュを現在の設定アルゴリズムへ自動マイグレーションする機能を追加。
PASSWORD_HASH_ALGOSのデフォルト値をPASSWORD_DEFAULT（bcrypt）に変更し、PHP標準のpassword_hash()/password_verify()/password_needs_rehash()を使用した最新アルゴリズムへの透過的な移行が可能になります。マイグレーションパス
変更内容
data/class/util/SC_Utils.phpsfIsPasswordHashAlgos(),sfNeedsReHash(),sfReHashPassword()追加。sfGetHashString(),sfIsMatchHashPassword()をpassword_hash()形式に対応data/class/SC_Customer.phpdata/class/pages/admin/LC_Page_Admin_Index.phpdata/class/SC_Initial.phpPASSWORD_HASH_ALGOSのデフォルト値をPASSWORD_DEFAULTに変更html/install/index.phpPASSWORD_DEFAULTに簡素化docker-compose*.ymlPASSWORD_HASH_ALGOSを2y（PASSWORD_DEFAULTの値）に変更tests/config.phpPASSWORD_DEFAULTに変更eccube_install.shPASSWORD_DEFAULTに変更tests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypeHmacTest.phptests/class/util/SC_Utils/SC_Utils_sfNeedsReHash_authTypePlainTest.phptests/class/util/SC_Utils/SC_Utils_sfIsMatchHashPassword_authTypePlainTest.php@group auth_type_plain追加tests/require_auth_type_plain.php.github/workflows/unit-tests.ymlauth_type_plainグループの除外と個別実行ステップ追加設定方法
後方互換性
PASSWORD_HASH_ALGOS = 'sha256'を明示指定すれば従来動作を維持可能Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
改善
テスト
Chore