feat(admin): .env が反映されない環境で設定変更を警告・抑止する (#6130) #6959
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
|
|
||
| use Eccube\Controller\AbstractController; | ||
| use Eccube\Form\Type\Admin\SecurityType; | ||
| use Eccube\Service\EnvFileService; | ||
| use Eccube\Util\CacheUtil; | ||
| use Eccube\Util\StringUtil; | ||
| use Symfony\Bridge\Twig\Attribute\Template; | ||
|
|
@@ -25,11 +26,27 @@ | |
|
|
||
| class SecurityController extends AbstractController | ||
| { | ||
| /** | ||
| * この画面が .env へ書き込む環境変数キー（OS 環境変数によるオーバーライド判定に使用）. | ||
| */ | ||
| private const ENV_KEYS = [ | ||
| 'ECCUBE_FRONT_ALLOW_HOSTS', | ||
| 'ECCUBE_FRONT_DENY_HOSTS', | ||
| 'ECCUBE_ADMIN_ALLOW_HOSTS', | ||
| 'ECCUBE_ADMIN_DENY_HOSTS', | ||
| 'ECCUBE_FORCE_SSL', | ||
| 'TRUSTED_HOSTS', | ||
| 'ECCUBE_ADMIN_ROUTE', | ||
| ]; | ||
|
|
||
| /** | ||
| * SecurityController constructor. | ||
| */ | ||
| public function __construct( | ||
| protected TokenStorageInterface $tokenStorage, | ||
| private readonly CacheUtil $cacheUtil, | ||
| private readonly EnvFileService $envFileService, | ||
| ) { | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -44,13 +61,20 @@ public function index(Request $request): RedirectResponse|array | |
| $form->handleRequest($request); | ||
|
|
||
| if ($form->isSubmitted() && $form->isValid()) { | ||
| // .env ファイル自体が読み込まれない状況（全キー反映不可）では保存を拒否する | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(); | ||
| if ([] !== $ineffectiveReasons) { | ||
| $this->addError('admin.common.save_error', 'admin'); | ||
|
Contributor
There was a problem hiding this comment. [低] POST 拒否時のメッセージが汎用の GET 時は $reasons = $this->envFileService->getIneffectiveReasons(self::ENV_KEYS);
if ([] !== $reasons) {
$this->addError('admin.common.save_error', 'admin');
foreach ($reasons as $reason) {
$this->addError('admin.system.env.ineffective.'.$reason, 'admin');
}
return $this->redirectToRoute('admin_setting_system_security');
}
Contributor
Author
There was a problem hiding this comment. @nanasess |
||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addError('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
|
|
||
| return $this->redirectToRoute('admin_setting_system_security'); | ||
| } | ||
|
|
||
| // OS 環境変数やカスケードファイルで上書きされているキーは書き込んでも反映されないため除外する | ||
| $overriddenKeys = $this->envFileService->getOverriddenKeys(self::ENV_KEYS); | ||
|
|
||
| $data = $form->getData(); | ||
| $envFile = $this->getParameter('kernel.project_dir').'/.env'; | ||
| $env = file_get_contents($envFile); | ||
|
|
@@ -69,20 +93,29 @@ public function index(Request $request): RedirectResponse|array | |
| array_filter(\explode("\n", StringUtil::convertLineFeed($data['admin_deny_hosts'])), fn ($str) => StringUtil::isNotBlank($str)) | ||
| ); | ||
|
|
||
| // 上書きされているキーは書き込み対象から除外する | ||
| $replace = array_diff_key([ | ||
| 'ECCUBE_FRONT_ALLOW_HOSTS' => "'{$frontAllowHosts}'", | ||
| 'ECCUBE_FRONT_DENY_HOSTS' => "'{$frontDenyHosts}'", | ||
| 'ECCUBE_ADMIN_ALLOW_HOSTS' => "'{$adminAllowHosts}'", | ||
| 'ECCUBE_ADMIN_DENY_HOSTS' => "'{$adminDenyHosts}'", | ||
| 'ECCUBE_FORCE_SSL' => $data['force_ssl'] ? '1' : '0', | ||
| 'TRUSTED_HOSTS' => $data['trusted_hosts'], | ||
| ], array_flip($overriddenKeys)); | ||
|
|
||
| if ([] !== $replace) { | ||
| $env = StringUtil::replaceOrAddEnv($env, $replace); | ||
| file_put_contents($envFile, $env); | ||
| } | ||
|
|
||
| // 上書きされ反映されなかったキーは名指しで警告する | ||
| if ([] !== $overriddenKeys) { | ||
| $this->addWarning(trans('admin.system.env.ineffective.overridden', ['%keys%' => implode(', ', $overriddenKeys)]), 'admin'); | ||
| } | ||
|
|
||
| // 管理画面URLの更新. 上書きされておらず変更されている場合はログアウトし再ログインさせる. | ||
| $adminRoute = $this->eccubeConfig['eccube_admin_route']; | ||
| if ($adminRoute !== $data['admin_route_dir'] && !in_array('ECCUBE_ADMIN_ROUTE', $overriddenKeys, true)) { | ||
| $env = StringUtil::replaceOrAddEnv($env, [ | ||
| 'ECCUBE_ADMIN_ROUTE' => $data['admin_route_dir'], | ||
| ]); | ||
|
|
@@ -115,13 +148,21 @@ public function index(Request $request): RedirectResponse|array | |
| $this->addWarning('admin.setting.system.security.admin_url_warning', 'admin'); | ||
| } | ||
|
|
||
| // .env ファイル自体が読み込まれない状況では警告を出し、登録ボタンを無効化する。 | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(); | ||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addWarning('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
|
|
||
| // 個々のキーが OS 環境変数等で上書きされている場合は該当キーを名指しで警告する。 | ||
| $overriddenKeys = $this->envFileService->getOverriddenKeys(self::ENV_KEYS); | ||
| if ([] !== $overriddenKeys) { | ||
| $this->addWarning(trans('admin.system.env.ineffective.overridden', ['%keys%' => implode(', ', $overriddenKeys)]), 'admin'); | ||
| } | ||
|
|
||
| return [ | ||
| 'form' => $form->createView(), | ||
| 'envWritable' => [] === $ineffectiveReasons, | ||
| ]; | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| use Eccube\Form\Type\Admin\TemplateType; | ||
| use Eccube\Repository\Master\DeviceTypeRepository; | ||
| use Eccube\Repository\TemplateRepository; | ||
| use Eccube\Service\EnvFileService; | ||
| use Eccube\Util\CacheUtil; | ||
| use Eccube\Util\StringUtil; | ||
| use Symfony\Bridge\Twig\Attribute\Template; | ||
|
|
@@ -33,10 +34,15 @@ | |
|
|
||
| class TemplateController extends AbstractController | ||
| { | ||
| /** | ||
| * この画面が .env へ書き込む環境変数キー（OS 環境変数によるオーバーライド判定に使用）. | ||
| */ | ||
| private const ENV_KEYS = ['ECCUBE_TEMPLATE_CODE']; | ||
|
|
||
| /** | ||
| * TemplateController constructor. | ||
| */ | ||
| public function __construct(protected TemplateRepository $templateRepository, protected DeviceTypeRepository $deviceTypeRepository, private readonly CacheUtil $cacheUtil, private readonly EnvFileService $envFileService) | ||
| { | ||
| } | ||
|
|
||
|
|
@@ -59,10 +65,28 @@ public function index(Request $request): array|RedirectResponse | |
| $form->handleRequest($request); | ||
|
|
||
| if ($form->isSubmitted() && $form->isValid()) { | ||
| // .env への書き込みが反映されない状況では保存を拒否する | ||
| // - .env が存在しない / 書き込み不可 | ||
| // - .env.local.php（dump-env の最適化済みスナップショット）が .env より優先される | ||
| // - ECCUBE_TEMPLATE_CODE が OS 環境変数やカスケードファイルで .env を上書きしている | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(); | ||
| $overriddenKeys = $this->envFileService->getOverriddenKeys(self::ENV_KEYS); | ||
| if ([] !== $ineffectiveReasons || [] !== $overriddenKeys) { | ||
| $this->addError('admin.common.save_error', 'admin'); | ||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addError('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
| if ([] !== $overriddenKeys) { | ||
| $this->addError(trans('admin.system.env.ineffective.overridden', ['%keys%' => implode(', ', $overriddenKeys)]), 'admin'); | ||
| } | ||
|
|
||
| return $this->redirectToRoute('admin_store_template'); | ||
| } | ||
|
|
||
| $Template = $this->templateRepository->find($form['selected']->getData()); | ||
|
|
||
| $envFile = $this->getParameter('kernel.project_dir').'/.env'; | ||
| $env = file_get_contents($envFile); | ||
|
|
||
| $env = StringUtil::replaceOrAddEnv($env, [ | ||
| 'ECCUBE_TEMPLATE_CODE' => $Template->getCode(), | ||
|
|
@@ -72,33 +96,36 @@ public function index(Request $request): array|RedirectResponse | |
|
|
||
| $this->addSuccess('admin.common.save_complete', 'admin'); | ||
|
|
||
| $this->cacheUtil->clearCache(); | ||
|
|
||
| return $this->redirectToRoute('admin_store_template'); | ||
| } | ||
|
|
||
| // .env への書き込みが反映されない状況では警告を出し、登録ボタンを無効化する。 | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(); | ||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addWarning('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
|
|
||
| // 対象キー（ECCUBE_TEMPLATE_CODE）が上書きされている場合は名指しで警告する。 | ||
| $overriddenKeys = $this->envFileService->getOverriddenKeys(self::ENV_KEYS); | ||
| if ([] !== $overriddenKeys) { | ||
| $this->addWarning(trans('admin.system.env.ineffective.overridden', ['%keys%' => implode(', ', $overriddenKeys)]), 'admin'); | ||
| } | ||
|
|
||
| return [ | ||
| 'form' => $form->createView(), | ||
| 'Templates' => $Templates, | ||
| // この画面が扱うキーは 1 つのみのため, 上書きされていれば実質全滅として無効化する。 | ||
| 'envWritable' => [] === $ineffectiveReasons && [] === $overriddenKeys, | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * テンプレート一覧からのダウンロード | ||
| */ | ||
| #[Route(path: '/%eccube_admin_route%/store/template/{id}/download', name: 'admin_store_template_download', requirements: ['id' => '\d+'], methods: ['GET'])] | ||
| public function download(\Eccube\Entity\Template $Template): BinaryFileResponse | ||
|
Contributor
There was a problem hiding this comment. [中]
PR 本文の互換性チェックリストは Service クラスについてしか言及していませんが、コントローラの public メソッドも同じく拡張点なので、
のどちらかが良いと思いますが、いかがでしょうか。4.4 はメジャー更新なので削除自体が不可能とは思いませんが、レビュー単位としては分けたほうが追いやすいと感じました。
Contributor
Author
There was a problem hiding this comment. @nanasess この引数削除は、実は CI の Rector ジョブが要求しています。 対応として 2 案あります。どちらが望ましいでしょうか。
個人的には、拡張点である public メソッドの互換を優先するなら (A) が筋だと思いますが、プロジェクトの Rector 運用方針に合わせたく、ご意見をいただけますか。合意後に反映します。 |
||
| { | ||
| // 該当テンプレートのディレクトリ | ||
| $templateCode = $Template->getCode(); | ||
|
|
@@ -155,7 +182,7 @@ public function download(Request $request, \Eccube\Entity\Template $Template): B | |
| } | ||
|
|
||
| #[Route(path: '/%eccube_admin_route%/store/template/{id}/delete', name: 'admin_store_template_delete', requirements: ['id' => '\d+'], methods: ['DELETE'])] | ||
| public function delete(\Eccube\Entity\Template $Template): RedirectResponse | ||
| { | ||
| $this->isTokenValid(); | ||
|
|
||
|
|
||
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[高] 7 キーの OR 判定なので、1 キーが OS 環境変数として設定されているだけで、反映されるはずの残り 6 キーまで保存できなくなります。
EnvFileService::getIneffectiveReasons()は対象キーのどれか 1 つでも該当すればREASON_OVERRIDDENを返してbreakします（EnvFileService.php:69-74）。その結果、isEffective(self::ENV_KEYS)（SecurityController.php:65）はセキュリティ設定画面全体をブロックします。ECCUBE_ADMIN_ROUTEだけを OS 環境変数で設定する構成は現実的で、docker-compose.yml:50-59も各キーを個別にコメントアウトして例示しています（ECCUBE_ADMIN_ROUTEは 54 行目）。この構成では:ECCUBE_ADMIN_ROUTEの変更 → 確かに反映されない（ブロックは妥当）TRUSTED_HOSTS/ECCUBE_FORCE_SSL/ IP 制限 4 キー →.envに書けば反映されるにもかかわらず、後者も含めて登録ボタンが無効化され、POST も拒否されます。
index.php:41-46のコメント（「OS 環境変数を保護しつつ、.env 側の非 OS 変数は反映される」）が示すとおり、Docker 環境でも.env側のキーは生きているので、これは今まで管理画面から変更できていた設定が変更できなくなる退行になります。PR 本文の「既存機能の仕様変更はありません」とも齟齬があると思います。getIneffectiveReasons()が「どのキーが上書きされているか」まで返すようにして、not_found/not_writable/local_php（＝キーによらず全滅するケース）に限定するという切り分けはいかがでしょうか。少なくとも「1 キーの上書きで画面全体を止める」のは過剰だと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nanasess
過剰ブロックの退行、ご指摘のとおりでした。切り分けを入れました。
EnvFileService::getOverriddenKeys(array $keys)を新設し、上書きされているキーの部分集合を返すようにしました。getIneffectiveReasons()はファイル単位で全滅する理由（not_found/not_writable/local_php）だけに限定。ECCUBE_TEMPLATE_CODEの 1 つだけなので、それが上書きされていれば実質全滅として従来どおり保存拒否＋無効化にしています。「1 キーの上書きで画面全体を止める」挙動は解消しました。