Conversation
- テンプレートのHTMLコンテンツに|purifyフィルターを追加 - JSON出力にJSON_HEX_TAG等のセキュリティオプションを追加 - 安全なJSONエンコード用のjson_encode_safeフィルターを追加 修正対象: - Product.description_detail, description_list - News.description - tradelaw.description, activeTradeLaw.description - class_categories_as_json() - FileController, EditController, ShippingController, OwnerStoreController Refs EC-CUBE#6634 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough複数のコントローラーとテンプレートファイルにおいて、JSON出力時のXSS脆弱性を修正しました。json_encode()呼び出しにJSON_HEX_*フラグを追加し、新規Twigフィルター「json_encode_safe」を導入することで、スクリプトコンテキストでのエスケープ処理を強化しました。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Eccube/Controller/Admin/Store/OwnerStoreController.php (1)
246-256:⚠️ Potential issue | 🟠 Major
requiresをJSON文字列に変更するとplugin_confirm.twigのTwigiテレーション部分が壊れますコントローラーが
requiresをJSON文字列として渡すようになったことで、plugin_confirm.twigには以下の問題が生じています：1.
requires|lengthが配列長ではなく文字列長を返す（Line 213）
Twigのlengthフィルターは文字列に対してmb_strlen()を呼ぶため、"[]"の長さは2です。2 > 0が常に真となり、依存プラグインがない場合でも「必要プラグイン」カードが表示されてしまいます。2.
{% for plugin in requires %}が空ループになる（Line 219）
Twigのtwig_ensure_traversable()は文字列を空配列[]として扱うため、ループが一切実行されず依存プラグインの一覧が表示されません。推奨修正：コントローラー側で PHP 配列のまま渡し、テンプレート側で本PRで追加された
json_encode_safeフィルターを使用してください。🔧 doConfirm の修正案
return [ 'item' => $item, - 'requires' => json_encode($requires, JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT), + 'requires' => $requires, 'is_update' => false, ];🔧 doUpdateConfirm の修正案
return [ 'item' => $item, - 'requires' => json_encode([], JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT), + 'requires' => [], 'is_update' => true, 'Plugin' => $Plugin, ];そして
plugin_confirm.twigのLine 119 を以下のように変更してください：- var requires = {{ requires|raw }}; + var requires = {{ requires|json_encode_safe }};Also applies to: 531-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Eccube/Controller/Admin/Store/OwnerStoreController.php` around lines 246 - 256, The controller changed 'requires' to a JSON string which breaks Twig iteration/length checks; revert the change in the doConfirm and doUpdateConfirm handlers so they pass 'requires' as a PHP array (not json_encode'd) and ensure templates use the new json_encode_safe filter when a JSON string is needed (e.g., plugin_confirm.twig uses requires|json_encode_safe for inline JS) — update the return payloads in the doConfirm and doUpdateConfirm methods to return the raw $requires array and remove json_encode there so Twig's length and for-loop behavior work correctly.
🧹 Nitpick comments (1)
src/Eccube/Resource/template/default/Shopping/index.twig (1)
397-397:|rawフィルターは冗長です。|purifyのみで十分ですexercise/htmlpurifier-bundle 5.0 の
purifyフィルターはis_safe => ['html']で登録されており、HTMLサニタイズ後の出力をTwigに対して安全なHTMLとして宣言します。Twigのrawフィルターは最後に適用される場合にのみ自動エスケープを無効化するため、後続のpurifyフィルターが存在する場合は機能しません。
{{ activeTradeLaw.description|purify }}に変更し、|rawを削除することで、より明確で冗長性のないコードになります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Eccube/Resource/template/default/Shopping/index.twig` at line 397, Remove the redundant |raw filter and leave only the purify filter on the activeTradeLaw.description output: replace the expression that applies both |purify and |raw so it uses only {{ activeTradeLaw.description|purify }}, ensuring sanitization is applied and Twig sees the result as safe without the unnecessary |raw call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Eccube/Controller/Admin/Store/OwnerStoreController.php`:
- Line 248: OwnerStoreController currently writes 'requires' =>
json_encode($requires, ... ) without handling json_encode() returning false;
update the code around the json_encode call in OwnerStoreController so you
either (a) use json_encode($requires,
JSON_HEX_TAG|JSON_HEX_AMP|JSON_HEX_APOS|JSON_HEX_QUOT|JSON_THROW_ON_ERROR)
inside a try/catch and log/fallback on exception, or (b) capture the result into
a variable (e.g. $requiresJson = json_encode(...);) and if $requiresJson ===
false set $requiresJson = '[]' (or safe JSON string) before assigning to the
template, ensuring the template always receives valid JSON for the requires
variable.
In `@src/Eccube/Resource/template/admin/Store/plugin_confirm.twig`:
- Around line 119-122: The Twig template treats requires as a PHP string
(pre-encoded JSON) which breaks the length check and for-loop; update
plugin_confirm.twig to decode the JSON before Twig-side operations by applying
Twig's JSON decode (e.g., use requires|json_decode or equivalent) wherever you
do requires|length and {% for plugin in requires %} so Twig sees an array, or
alternatively revert OwnerStoreController.php to pass a PHP array instead of a
JSON string and keep the current Twig iteration; ensure all references to
requires in the template (the length check and the for-loop that renders
dependency cards) reference the decoded/array form.
---
Outside diff comments:
In `@src/Eccube/Controller/Admin/Store/OwnerStoreController.php`:
- Around line 246-256: The controller changed 'requires' to a JSON string which
breaks Twig iteration/length checks; revert the change in the doConfirm and
doUpdateConfirm handlers so they pass 'requires' as a PHP array (not
json_encode'd) and ensure templates use the new json_encode_safe filter when a
JSON string is needed (e.g., plugin_confirm.twig uses requires|json_encode_safe
for inline JS) — update the return payloads in the doConfirm and doUpdateConfirm
methods to return the raw $requires array and remove json_encode there so Twig's
length and for-loop behavior work correctly.
---
Nitpick comments:
In `@src/Eccube/Resource/template/default/Shopping/index.twig`:
- Line 397: Remove the redundant |raw filter and leave only the purify filter on
the activeTradeLaw.description output: replace the expression that applies both
|purify and |raw so it uses only {{ activeTradeLaw.description|purify }},
ensuring sanitization is applied and Twig sees the result as safe without the
unnecessary |raw call.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Eccube/Controller/Admin/Content/FileController.phpsrc/Eccube/Controller/Admin/Order/EditController.phpsrc/Eccube/Controller/Admin/Order/ShippingController.phpsrc/Eccube/Controller/Admin/Store/OwnerStoreController.phpsrc/Eccube/Resource/template/admin/Store/plugin_confirm.twigsrc/Eccube/Resource/template/default/Block/news.twigsrc/Eccube/Resource/template/default/Help/tradelaw.twigsrc/Eccube/Resource/template/default/Product/detail.twigsrc/Eccube/Resource/template/default/Product/list.twigsrc/Eccube/Resource/template/default/Shopping/confirm.twigsrc/Eccube/Resource/template/default/Shopping/index.twigsrc/Eccube/Twig/Extension/EccubeExtension.php
| return [ | ||
| 'item' => $item, | ||
| 'requires' => $requires, | ||
| 'requires' => json_encode($requires, JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT), |
There was a problem hiding this comment.
json_encode の失敗を考慮していません（マイナー）
json_encode() は不正な UTF-8 などで false を返す可能性があります。その場合、テンプレートで var requires = false; と出力され、JavaScriptの .map() 呼び出しで TypeError が発生します。上記のコントローラー修正を採用する場合は不要ですが、現行コードを保持する場合は false チェックを追加することを検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Eccube/Controller/Admin/Store/OwnerStoreController.php` at line 248,
OwnerStoreController currently writes 'requires' => json_encode($requires, ... )
without handling json_encode() returning false; update the code around the
json_encode call in OwnerStoreController so you either (a) use
json_encode($requires,
JSON_HEX_TAG|JSON_HEX_AMP|JSON_HEX_APOS|JSON_HEX_QUOT|JSON_THROW_ON_ERROR)
inside a try/catch and log/fallback on exception, or (b) capture the result into
a variable (e.g. $requiresJson = json_encode(...);) and if $requiresJson ===
false set $requiresJson = '[]' (or safe JSON string) before assigning to the
template, ensuring the template always receives valid JSON for the requires
variable.
- OwnerStoreController: requiresをPHP配列のまま渡すように修正
- plugin_confirm.twig: |raw を |json_encode_safe に変更
これによりTwig側の requires|length や {% for plugin in requires %}
が正しく配列として動作し、JavaScript出力も安全にエンコードされる
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.3 #6635 +/- ##
=========================================
Coverage 79.33% 79.33%
- Complexity 6638 6639 +1
=========================================
Files 475 475
Lines 26568 26571 +3
=========================================
+ Hits 21078 21081 +3
Misses 5490 5490
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:
|
@dotani1111 のレビュー指摘に対応。 purify関連の修正は別PR EC-CUBE#6627 で対応されているため、 本PRからpurify関連の変更を削除しJSON関連の修正のみに限定。 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@dotani1111 ご指摘ありがとうございます。purify関連の変更を削除し、JSON関連の修正のみに限定しました。 |
|
@kurozumi |
| return [ | ||
| 'item' => $item, | ||
| 'requires' => [], | ||
| 'requires' => json_encode([], JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT), |
There was a problem hiding this comment.
こちらはテンプレート側でjson_encodeしているため、不要かと思われます。
src/Eccube/Resource/template/admin/Store/plugin_confirm.twig
119行目
var requires = {{ requires|json_encode_safe }};
2 participants
Summary
|purifyフィルターを追加してXSSを防止JSON_HEX_TAG等のセキュリティオプションを追加json_encode_safeフィルターを追加修正内容
1. テンプレートの修正（
|raw→|raw|purify）default/Product/detail.twigProduct.description_detaildefault/Product/list.twigProduct.description_listdefault/Block/news.twigNews.descriptiondefault/Help/tradelaw.twigtradelaw.descriptiondefault/Shopping/index.twigactiveTradeLaw.descriptiondefault/Shopping/confirm.twigactiveTradeLaw.description2. JSON出力の修正
EccubeExtension.phpgetClassCategoriesAsJson()にJSON_HEX_*オプション追加、json_encode_safeフィルター追加FileController.phpjson_encode()にJSON_HEX_*オプション追加EditController.phpshippingDeliveryTimesをjson_encode()に変更ShippingController.phpshippingDeliveryTimesをjson_encode()に変更OwnerStoreController.phprequiresをコントローラー側でJSON化Test plan
EccubeExtensionTest: 5テスト成功FileControllerTest: 42テスト成功EditControllerTest: 18テスト成功Related Issue
Fixes #6634
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート