-
Notifications
You must be signed in to change notification settings - Fork 719
feat(admin): .env が反映されない環境で設定変更を警告・抑止する (#6130) #6959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.4
Are you sure you want to change the base?
Changes from 5 commits
82bbf22
462396d
d1f67ab
04bf71f
000627c
9bb1d80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) | ||
| { | ||
| public function __construct( | ||
| protected TokenStorageInterface $tokenStorage, | ||
| private readonly CacheUtil $cacheUtil, | ||
| private readonly EnvFileService $envFileService, | ||
| ) { | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -44,8 +61,8 @@ public function index(Request $request): RedirectResponse|array | |
| $form->handleRequest($request); | ||
|
|
||
| if ($form->isSubmitted() && $form->isValid()) { | ||
| // .envファイルが存在しないときに設定は失敗する | ||
| if (file_exists($this->getParameter('kernel.project_dir').'/.env') === false) { | ||
| // .env への書き込みが反映されない状況では保存を拒否する | ||
| if (!$this->envFileService->isEffective(self::ENV_KEYS)) { | ||
| $this->addError('admin.common.save_error', 'admin'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [低] 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess |
||
|
|
||
| return $this->redirectToRoute('admin_setting_system_security'); | ||
|
|
@@ -115,13 +132,15 @@ public function index(Request $request): RedirectResponse|array | |
| $this->addWarning('admin.setting.system.security.admin_url_warning', 'admin'); | ||
| } | ||
|
|
||
| // .envファイルが存在しない場合警告を出す。 | ||
| if (file_exists($this->getParameter('kernel.project_dir').'/.env') === false) { | ||
| $this->addWarning('admin.setting.system.security.not_found_env_file', 'admin'); | ||
| // .env への書き込みが反映されない状況では警告を出し、登録ボタンを無効化する。 | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(self::ENV_KEYS); | ||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addWarning('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
|
|
||
| return [ | ||
| 'form' => $form->createView(), | ||
| 'envWritable' => [] === $ineffectiveReasons, | ||
| ]; | ||
| } | ||
| } | ||
| 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) | ||
| public function __construct(protected TemplateRepository $templateRepository, protected DeviceTypeRepository $deviceTypeRepository, private readonly CacheUtil $cacheUtil, private readonly EnvFileService $envFileService) | ||
| { | ||
| } | ||
|
|
||
|
|
@@ -59,10 +65,20 @@ 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 を上書きしている | ||
| if (!$this->envFileService->isEffective(self::ENV_KEYS)) { | ||
| $this->addError('admin.common.save_error', 'admin'); | ||
|
|
||
| return $this->redirectToRoute('admin_store_template'); | ||
| } | ||
|
|
||
| $Template = $this->templateRepository->find($form['selected']->getData()); | ||
|
|
||
| $envFile = $this->getParameter('kernel.project_dir').'/.env'; | ||
| $env = file_exists($envFile) ? file_get_contents($envFile) : ''; | ||
| $env = file_get_contents($envFile); | ||
|
|
||
| $env = StringUtil::replaceOrAddEnv($env, [ | ||
| 'ECCUBE_TEMPLATE_CODE' => $Template->getCode(), | ||
|
|
@@ -72,33 +88,29 @@ public function index(Request $request): array|RedirectResponse | |
|
|
||
| $this->addSuccess('admin.common.save_complete', 'admin'); | ||
|
|
||
| // 次のいずれかの場合、.env への書き込みが起動時のロードで反映されない: | ||
| // 1. ECCUBE_TEMPLATE_CODE がプロセス環境変数として設定されている(Docker などで明示的に設定した場合)。 | ||
| // bootEnv は OS 環境変数を上書きしないため .env の値が使われない。 | ||
| // 2. .env.local.php(dump-env の最適化済みスナップショット)が存在する。 | ||
| // bootEnv は .env より .env.local.php を優先するため、再度 `composer symfony:dump-env` を | ||
| // 実行するまで .env の変更が反映されない。 | ||
| $envLocalPhp = $this->getParameter('kernel.project_dir').'/.env.local.php'; | ||
| if (false !== getenv('ECCUBE_TEMPLATE_CODE') || file_exists($envLocalPhp)) { | ||
| $this->addWarning('admin.store.template.env_override_warning', 'admin'); | ||
| } | ||
|
|
||
| $this->cacheUtil->clearCache(); | ||
|
|
||
| return $this->redirectToRoute('admin_store_template'); | ||
| } | ||
|
|
||
| // .env への書き込みが反映されない状況では警告を出し、登録ボタンを無効化する。 | ||
| $ineffectiveReasons = $this->envFileService->getIneffectiveReasons(self::ENV_KEYS); | ||
| foreach ($ineffectiveReasons as $reason) { | ||
| $this->addWarning('admin.system.env.ineffective.'.$reason, 'admin'); | ||
| } | ||
|
|
||
| return [ | ||
| 'form' => $form->createView(), | ||
| 'Templates' => $Templates, | ||
| 'envWritable' => [] === $ineffectiveReasons, | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * テンプレート一覧からのダウンロード | ||
| */ | ||
| #[Route(path: '/%eccube_admin_route%/store/template/{id}/download', name: 'admin_store_template_download', requirements: ['id' => '\d+'], methods: ['GET'])] | ||
| public function download(Request $request, \Eccube\Entity\Template $Template): BinaryFileResponse | ||
| public function download(\Eccube\Entity\Template $Template): BinaryFileResponse | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [中]
PR 本文の互換性チェックリストは Service クラスについてしか言及していませんが、コントローラの public メソッドも同じく拡張点なので、
のどちらかが良いと思いますが、いかがでしょうか。4.4 はメジャー更新なので削除自体が不可能とは思いませんが、レビュー単位としては分けたほうが追いやすいと感じました。
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess この引数削除は、実は CI の Rector ジョブが要求しています。 対応として 2 案あります。どちらが望ましいでしょうか。
個人的には、拡張点である public メソッドの互換を優先するなら (A) が筋だと思いますが、プロジェクトの Rector 運用方針に合わせたく、ご意見をいただけますか。合意後に反映します。 |
||
| { | ||
| // 該当テンプレートのディレクトリ | ||
| $templateCode = $Template->getCode(); | ||
|
|
@@ -155,7 +167,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(Request $request, \Eccube\Entity\Template $Template): RedirectResponse | ||
| public function delete(\Eccube\Entity\Template $Template): RedirectResponse | ||
| { | ||
| $this->isTokenValid(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1429,6 +1429,10 @@ admin.setting.system.security.ip_limit_invalid_ip_and_submask: "%ip%はIPv4/ビ | |
| admin.setting.system.security.ip_limit_invalid_https: "httpの場合には設定できません。" | ||
| admin.setting.system.security.admin_url_warning: 管理画面URLは、セキュリティのため推測されにくいものを設定してください。 | ||
| admin.setting.system.security.not_found_env_file: .envファイルが見つかりません。.envを利用していない場合はセキュリティ設定を管理画面から変更できません。 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [低] 参照されなくなった翻訳キーが残っています。 本 PR で
削除するか、あるいは
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess
後者が持っていた「原因キーを名指しする」案内は、 |
||
| admin.system.env.ineffective.not_found: '.env ファイルが存在しないため、この設定は管理画面から変更できません。サーバーの環境変数を直接設定してください。' | ||
| admin.system.env.ineffective.not_writable: '.env ファイルに書き込み権限がないため、この設定は管理画面から変更できません。.env ファイルの書き込み権限を確認してください。' | ||
| admin.system.env.ineffective.local_php: '.env.local.php が存在するため、.env への変更は反映されず、管理画面からは変更できません。サーバー側で .env を更新してから composer dump-env を再実行するか、.env.local.php を削除して管理画面からの変更を再開してください。' | ||
| admin.system.env.ineffective.overridden: 'この設定の環境変数が OS のプロセス環境変数として設定されているため、.env への変更は反映されません。サーバーの環境変数を直接変更してください。' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [中] どの環境変数が原因なのかがメッセージから分かりません。 セキュリティ設定は 7 キーを対象にしているため(
admin.system.env.ineffective.overridden: 'この設定の環境変数(%keys%)が OS のプロセス環境変数として設定されているため、.env への変更は反映されません。サーバーの環境変数を直接変更してください。'上書きされているキーを名指しできれば、セキュリティ設定画面の過剰ブロック(
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess |
||
| admin.setting.system.security.trusted_hosts: 信頼できるホスト名 | ||
| admin.setting.system.security.trusted_hosts_sample: ^example\.com$,^example\.org$ | ||
| admin.setting.system.security.trusted_hosts_description: | | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,88 @@ | ||||||||||||||
| <?php | ||||||||||||||
|
|
||||||||||||||
| /* | ||||||||||||||
| * This file is part of EC-CUBE | ||||||||||||||
| * | ||||||||||||||
| * Copyright(c) EC-CUBE CO.,LTD. All Rights Reserved. | ||||||||||||||
| * | ||||||||||||||
| * http://www.ec-cube.co.jp/ | ||||||||||||||
| * | ||||||||||||||
| * For the full copyright and license information, please view the LICENSE | ||||||||||||||
| * file that was distributed with this source code. | ||||||||||||||
| */ | ||||||||||||||
|
|
||||||||||||||
| namespace Eccube\Service; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * .env ファイルへの書き込みが実行時に反映されるかを判定するサービス. | ||||||||||||||
| * | ||||||||||||||
| * セキュリティ設定・テンプレート設定などは .env を書き換えて機能を実現するが, | ||||||||||||||
| * docker-compose の環境変数や本番環境では .env が使われず, 書き換えても反映されない. | ||||||||||||||
| * ユーザーが変更できないことに気付けるよう, 反映されない理由を検出する. | ||||||||||||||
| * | ||||||||||||||
| * @see https://github.com/EC-CUBE/ec-cube/issues/6130 | ||||||||||||||
| */ | ||||||||||||||
| class EnvFileService | ||||||||||||||
| { | ||||||||||||||
| /** .env ファイルが存在しない(.env を利用していない) */ | ||||||||||||||
| public const REASON_NOT_FOUND = 'not_found'; | ||||||||||||||
|
|
||||||||||||||
| /** .env ファイルに書き込み権限がない */ | ||||||||||||||
| public const REASON_NOT_WRITABLE = 'not_writable'; | ||||||||||||||
|
|
||||||||||||||
| /** .env.local.php(dump-env の最適化済みスナップショット)が存在し, .env より優先される */ | ||||||||||||||
| public const REASON_LOCAL_PHP = 'local_php'; | ||||||||||||||
|
|
||||||||||||||
| /** 対象の環境変数が OS のプロセス環境変数として設定されており, .env の値を上書きしている */ | ||||||||||||||
| public const REASON_OVERRIDDEN = 'overridden'; | ||||||||||||||
|
|
||||||||||||||
| public function __construct(private readonly string $projectDir) | ||||||||||||||
| { | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * .env への書き込みが実行時のロードに反映されない理由を返す. | ||||||||||||||
| * | ||||||||||||||
| * 空配列であれば .env への書き込みが有効に反映される. | ||||||||||||||
| * | ||||||||||||||
| * @param string[] $keys 対象の環境変数キー(OS 環境変数によるオーバーライド判定に使用) | ||||||||||||||
| * | ||||||||||||||
| * @return string[] REASON_* 定数の配列 | ||||||||||||||
| */ | ||||||||||||||
| public function getIneffectiveReasons(array $keys = []): array | ||||||||||||||
| { | ||||||||||||||
| $reasons = []; | ||||||||||||||
|
|
||||||||||||||
| $envFile = $this->projectDir.'/.env'; | ||||||||||||||
| if (!file_exists($envFile)) { | ||||||||||||||
| $reasons[] = self::REASON_NOT_FOUND; | ||||||||||||||
| } elseif (!is_writable($envFile)) { | ||||||||||||||
| $reasons[] = self::REASON_NOT_WRITABLE; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // .env.local.php があると bootEnv は .env より優先するため, .env の変更は反映されない | ||||||||||||||
| if (file_exists($this->projectDir.'/.env.local.php')) { | ||||||||||||||
| $reasons[] = self::REASON_LOCAL_PHP; | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+72
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [中]
また 同じ検証スクリプトでの実測結果です(
ただし $appEnv = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? null;
$cascades = array_filter([
'.env.local',
$appEnv ? ".env.{$appEnv}" : null,
$appEnv ? ".env.{$appEnv}.local" : null,
]);
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess |
||||||||||||||
|
|
||||||||||||||
| // bootEnv は OS のプロセス環境変数を上書きしないため, getenv が値を返すキーは .env の変更が反映されない | ||||||||||||||
| foreach ($keys as $key) { | ||||||||||||||
| if (false !== getenv($key)) { | ||||||||||||||
| $reasons[] = self::REASON_OVERRIDDEN; | ||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [高]
一方 Symfony Dotenv は
A では なお、ブート後は // Dotenv が実際に .env 系から populate したキーは SYMFONY_DOTENV_VARS に載る
$dotenvVars = array_flip(array_filter(explode(',', $_SERVER['SYMFONY_DOTENV_VARS'] ?? $_ENV['SYMFONY_DOTENV_VARS'] ?? '')));
foreach ($keys as $key) {
if (isset($dotenvVars[$key])) {
continue; // .env 系の値が実際に適用されている = 書き込みは反映される
}
if (isset($_ENV[$key]) || isset($_SERVER[$key]) || false !== getenv($key)) {
$reasons[] = self::REASON_OVERRIDDEN;
break;
}
}この形にすると、A/B の判別に加えて、nginx の
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nanasess
コアの |
||||||||||||||
|
|
||||||||||||||
| return $reasons; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * 対象の環境変数について, .env への書き込みが有効に反映されるか. | ||||||||||||||
| * | ||||||||||||||
| * @param string[] $keys 対象の環境変数キー | ||||||||||||||
| */ | ||||||||||||||
| public function isEffective(array $keys = []): bool | ||||||||||||||
| { | ||||||||||||||
| return [] === $this->getIneffectiveReasons($keys); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
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 キーの上書きで画面全体を止める」挙動は解消しました。