Open
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 infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough複数のコントローラーとテンプレートでXSS対策を実施。JSON出力にHTML特殊文字エスケープ（JSON_HEX_*）を追加し、テンプレートの生HTML出力にpurifyフィルターを挿入、さらにTwigに安全なjson_encodeフィルタを追加しました。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 78.84% 78.84%
- Complexity 6631 6632 +1
=========================================
Files 475 475
Lines 26539 26542 +3
=========================================
+ Hits 20924 20928 +4
+ Misses 5615 5614 -1
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:
|
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
|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