Skip to content

fix: XMLHttpRequestフォームのCSRFトークンチェック条件を修正#6602

Merged
dotani1111 merged 2 commits intoEC-CUBE:4.3from
dotani1111:dev/fix-token-check-condition
Feb 5, 2026
Merged

fix: XMLHttpRequestフォームのCSRFトークンチェック条件を修正#6602
dotani1111 merged 2 commits intoEC-CUBE:4.3from
dotani1111:dev/fix-token-check-condition

Conversation

@dotani1111
Copy link
Contributor

@dotani1111 dotani1111 commented Jan 23, 2026

概要(Overview・Refs Issue)

XMLHttpRequestフォームのCSRFトークンチェック条件式に論理的な誤りがあったため修正。

方針(Policy)

既存コードで使用されている推奨パターンに統一しました。

// 修正前
if (!$request->isXmlHttpRequest() && $this->isTokenValid()) {
    throw new BadRequestHttpException();
}
// 修正後
if (!$request->isXmlHttpRequest() || !$this->isTokenValid()) {
    throw new BadRequestHttpException();
}

参考: Admin/Order/OrderController.php, Admin/AdminController.php 等で同様のパターンを使用

実装に関する補足(Appendix)

ClassNameController::moveSortNo() については、元のコードで 2重チェックされておりましたので、併せて修正しました。

テスト(Test)

管理画面ログイン状態にて、該当のURLに対して、CSRFトークンなしで画像アップロードできるかテストするスクリプトを実行
修正前 200
修正後 403 AccessDeniedHttpException

相談(Discussion)

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

Summary by CodeRabbit

バグ修正

  • 管理画面のAJAX機能のセキュリティを強化しました。商品分類の並び替え、画像処理、メール設定機能でのリクエスト検証をより厳密にし、不正なアクセスを防止します。

テスト

  • セキュリティ検証の強化に対応したテストを更新しました。

条件式 `!isXmlHttpRequest() && isTokenValid()` を
`!isXmlHttpRequest() || !isTokenValid()` に修正。

対象ファイル:
- ClassNameController.php
- ProductController.php
- PaymentController.php
- MailController.php
- ProductControllerTest.php
- ClassNameControllerTest.php
@dotani1111 dotani1111 force-pushed the dev/fix-token-check-condition branch from 39dbab7 to 54392b7 Compare January 23, 2026 09:07
@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.84%. Comparing base (54392b7) to head (37c5250).
⚠️ Report is 11 commits behind head on 4.3.

Files with missing lines Patch % Lines
...ube/Controller/Admin/Product/ProductController.php 33.33% 2 Missing ⚠️
...ontroller/Admin/Setting/Shop/PaymentController.php 0.00% 2 Missing ⚠️
...e/Controller/Admin/Setting/Shop/MailController.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##                4.3    #6602   +/-   ##
=========================================
  Coverage     78.83%   78.84%           
  Complexity     6631     6631           
=========================================
  Files           475      475           
  Lines         26539    26539           
=========================================
+ Hits          20923    20924    +1     
+ Misses         5616     5615    -1     
Flag Coverage Δ
Unit 78.84% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dotani1111 dotani1111 enabled auto-merge February 5, 2026 01:59
@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

ウォークスルー

複数のコントローラーにおけるAJAXリクエストとCSRFトークンのアクセス制御ロジックが変更されました。既存の条件判定を反転させて、XmlHttpRequest確認とトークン検証の両条件をより厳格に強制するようになります。テストも対応するCSRFトークンヘッダーが追加されました。

変更内容

コホート / ファイル(s) 要約
コントローラーのアクセス制御ロジック
src/Eccube/Controller/Admin/Product/ClassNameController.php, src/Eccube/Controller/Admin/Product/ProductController.php, src/Eccube/Controller/Admin/Setting/Shop/MailController.php, src/Eccube/Controller/Admin/Setting/Shop/PaymentController.php
複数のエンドポイント(moveSortNo、loadProductClasses、imageProcess、imageRevert、preview)の条件判定をAND論理からOR論理に変更。非XmlHttpRequestリクエストまたは無効なトークンの場合にBadRequestHttpExceptionをスロー。
テストの更新
tests/Eccube/Tests/Web/Admin/Product/ClassNameControllerTest.php, tests/Eccube/Tests/Web/Admin/Product/ProductControllerTest.php
AJAXリクエストにECCUBE CSRFトークンヘッダー(HTTP_ECCUBE_CSRF_TOKEN)を追加。

見積もりコード評価労力

🎯 2 (Simple) | ⏱️ ~12 分

🐰 トークンチェック、より強くなり
OR条件で守りを厳しく
エラーは素早く、はじき飛ばし
セキュリティーの扉、ぴったり閉じて
安全な旅、続きます ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed プルリクエストのタイトルは、XMLHttpRequestフォームのCSRFトークンチェック条件式の論理誤りを修正するという、変更セットの主要な目的を明確に説明しています。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@dotani1111 dotani1111 merged commit 5b1fe2d into EC-CUBE:4.3 Feb 5, 2026
100 checks passed
@dotani1111 dotani1111 deleted the dev/fix-token-check-condition branch February 5, 2026 02:54
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.

3 participants