docs: AIエージェント向けレイヤ規約Skillを追加(security/twig-template/event-subscriber/plugin)+検証レビュー拡充 - #6853
Conversation
…/plugin) src/Eccube の実コードを裏取りし、既存Skillと同じ構成(対象/基本ルール/ 実装パターン/よくある間違い/実行・確認方法)で4本を追加。AGENTS.mdの Skill一覧表にも追記。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isTokenValid()の実装はAccessDeniedHttpExceptionを投げる(AbstractController)。 新規securitySkillとの整合をとる。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- twig-template: AbstractExtensionがTwig標準(\Twig\Extension\AbstractExtension)である旨を明記 - plugin: 無効化はコンソールコマンドが無く管理画面から行う点を補足 - security: 独自Voterはautoconfigureでsecurityvoterタグがつくことを明示 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
責務分離だけだった自己レビューに、security(認可/CSRF/IDOR)・ twig(XSS/raw)・event(業務ロジック偏り)・plugin(proxy再生成)の観点と、 層境界の横断観点を追加。詳細は各レイヤSkillの「よくある間違い」を参照する DRY構成とし、本Skillは全層を束ねる点検リストに徹する。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEC-CUBE 4.4 向けのスキル文書を新規追加・更新し、 ChangesEC-CUBE 4.4 スキルドキュメント追加・更新
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/skills/security/SKILL.md (1)
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winトークン名定数の明示化を検討。
Line 53 で
Constant::TOKEN_NAME = '_token'と記述していますが、このファイルが新規 Skill ドキュメントだけに、Constantクラスの所在・用途が初見の読者にはわかりにくい可能性があります。例を示す際に、実際のインポートパス(src/Eccube/Common/Constant.phpなど)を括弧書きで添えるか、または'_token'という文字列値そのものを明示することで、理解性を高められます。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/security/SKILL.md around lines 52 - 60, The documentation references `Constant::TOKEN_NAME = '_token'` without clarifying where the Constant class comes from or its exact location. To improve readability for new readers, modify the explanation on line 53 to either add the file path of the Constant class in parentheses (such as `src/Eccube/Common/Constant.php`) when first mentioning `Constant::TOKEN_NAME`, or explicitly state that the token name value is the string `'_token'` to make it immediately clear what value is being used. This will help readers understand the token name without needing to search for the Constant class definition.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.claude/skills/security/SKILL.md:
- Around line 52-60: The documentation references `Constant::TOKEN_NAME =
'_token'` without clarifying where the Constant class comes from or its exact
location. To improve readability for new readers, modify the explanation on line
53 to either add the file path of the Constant class in parentheses (such as
`src/Eccube/Common/Constant.php`) when first mentioning `Constant::TOKEN_NAME`,
or explicitly state that the token name value is the string `'_token'` to make
it immediately clear what value is being used. This will help readers understand
the token name without needing to search for the Constant class definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43281a9a-c8a9-408d-ab7e-6073312f30e0
📒 Files selected for processing (7)
.claude/skills/controller/SKILL.md.claude/skills/event-subscriber/SKILL.md.claude/skills/plugin/SKILL.md.claude/skills/review-responsibility/SKILL.md.claude/skills/security/SKILL.md.claude/skills/twig-template/SKILL.mdAGENTS.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6853 +/- ##
==========================================
- Coverage 74.78% 74.76% -0.03%
==========================================
Files 463 463
Lines 24029 24029
==========================================
- Hits 17970 17965 -5
- Misses 6059 6064 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PRレビューでフォールスポジティブを生んだ根因を「よくある間違い」へ還流: - bare な $this->isTokenValid(); を「CSRF未検証」と誤読しない(無効時は例外を投げ 戻り値は常にtrue。if (!isTokenValid()) の false 分岐はデッドコード) - #[Template] は配列を返したときのみ engage(Response/Redirect 返却パスでは未描画) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Doctrine ORM 3.x で Order/OrderItem の DECIMAL 金額プロパティが ?string に統一され、計算が bcmath 化されている実装 (src/Eccube/Entity/Order.php, OrderItem.php) を裏取りし、 entity Skill の「状態ロジック」節と「よくある間違い」に追記する。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…照を除去 - plugin Skill: 新規プラグインは手書きでなく eccube:plugin:generate で雛形生成する 定石を追記(src/Eccube/Command/PluginGenerateCommand.php で裏取り)。 - plugin / event-subscriber / security / twig-template / review-responsibility: ローカル限定 Skill「docker-qa」への参照を除去し、OSS 正典の AGENTS.md「開発コマンド」参照に置換。security の「vendor/bin/* を直接叩かない」も削除。 → マージ後に利用者の手元に無い Skill への dead link や Docker 必須の押し付けを防ぐ。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ProductReview プラグインの全層レビューで発見。コアの SaveEventSubscriber (src/Eccube/Doctrine/EventSubscriber/SaveEventSubscriber.php) が prePersist/preUpdate で method_exists により setCreateDate/setUpdateDate/setCreator を自動セットするため、 エンティティ側で自前の #[ORM\PrePersist] を書くのは二重実装になる。setter を生やすだけでよい。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ProductReview プラグインの独立レビューで、review-responsibility(自己レビュー)が 拾えなかった2点を各スキルへ還流する。 - entity: 他エンティティ(特にコアの Product/Customer)への関連で親削除時の挙動を 未決定だと退会・商品削除が FK で失敗/孤児化する点を追記。コアは onDelete を 限定使用(95 JoinColumn 中 2 件)で多くを Service 側で整理している事実を裏取り済み。 - repository: 画面表示の一覧・関連取得を無制限に全件取得しない(ページング/上限)。 - review-responsibility: 点検軸に Entity/Repository を追加し、各スキルの 「よくある間違い」へ委譲。これまで entity/repository 層が点検次元に無かった穴を塞ぐ。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AIエージェント向けレイヤ規約に、これまで欠けていた5領域を追加する。 既存スキルと同じ本文構成(対象/基本ルール/実装パターン/よくある間違い/実行・確認方法)で、 全記述を src/Eccube/ の実コードで裏取り済み(推測なし)。 - purchase-flow: PurchaseFlow の Processor/Validator。パイプライン段階・Item/ItemHolder・ ItemValidator は常に warning・冪等な Preprocessor・bcmath・登録は YAML タグ(priority)と #[CartFlow]系属性の2系統。 - mail: MailService 集約・BaseInfo の差出人・safe_textmail・MailHistory は受注/出荷のみ・ flush は呼び出し側・MAIL_* イベント。 - customize: app/Customize の trait+EntityExtension・FormTypeExtension・services.yaml の decorates(@.inner)・テンプレート上書き。plugin との使い分けを明示。 - csv: CsvImport/ExportService・dtb_csv 定義・StreamedResponse・SJIS-win・CSV_EXPORT イベント。 - command: #[AsCommand]+autoconfigure・parent::__construct 必須・execute():int・ 業務は Service へ・cron は OS 側(コアに Scheduler 機構なし)。 AGENTS.md のスキル一覧表にも5本を追記。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| ## 対象 / 前提 | ||
|
|
||
| `app/Customize/` は **プロジェクト固有の改変** を置く場所。コア(`src/Eccube/`)を直接書き換えず、 | ||
| ここで拡張・上書きすることで **コアのアップグレード(composer update)の影響を受けない** のが目的。 |
There was a problem hiding this comment.
アップグレードの影響を受けないのはパッチバージョンのアップグレードのみです。
また、Entity のカスタマイズはできません。(trait での拡張は可能)
app/Customize でオーバーライドした処理は脆弱性パッチなども個別適用する必要があり、慎重に使用した方がよいことを注釈しておきましょう
There was a problem hiding this comment.
ご指摘ありがとうございます。「アップグレード安全」を過信しない注意(影響回避はパッチ版まで/コアEntityは置換不可・trait拡張のみ/override は脆弱性パッチを個別再適用が必要で慎重に)を追記しました。 f0da0c4
| > 目的: 自己完結したパッケージとして機能を追加し、コアやプロジェクト固有カスタマイズ(`app/Customize/`)と混同しないこと。 | ||
| > プロジェクト固有の 1 回限りの改変は `app/Customize/`、再配布・着脱可能な機能は `app/Plugin/`。 | ||
|
|
||
| ## 雛形の生成(まず CLI で骨組みを作る) |
There was a problem hiding this comment.
雛形を作成したあと、 bin/console eccube:composer:require の --from オプションを使って別ディレクトリからシンボリックリンクした方が開発効率が良いです
app/Plugin 配下で開発していると、 プラグイン削除のテストをした瞬間すべて消えて無くなります
| 変更差分について、対応する規約を読み込んで確認する: | ||
| - コントローラ: Skill [`controller`](../controller/SKILL.md) | ||
| - サービス: Skill [`service`](../service/SKILL.md) | ||
| ## 層境界(横断観点・per-layer では拾えないもの) |
There was a problem hiding this comment.
蓄積型(stored)XSS の観点(保存値も未信頼として出力時にエスケープ)を追記しました。ありがとうございます。 f0da0c4
| ## よくある間違い(認可・CSRF・IDOR — ツールでは検出できない観点) | ||
|
|
||
| - ❌ 管理アクションを `%eccube_admin_route%` 配下**以外**に置く → ✅ 配下に置き admin firewall の保護下にする | ||
| - ❌ フォームを介さない POST/DELETE/Ajax で CSRF 未検証 → ✅ `$this->isTokenValid()` を呼ぶ(GET 以外) |
There was a problem hiding this comment.
Ajax は $request->isXmlHttpRequest() でのチェックを必ず併用する旨を追記しましょう
There was a problem hiding this comment.
Ajax 専用アクションは CSRF に加え $request->isXmlHttpRequest() を併用する旨を追記しました。ありがとうございます。 f0da0c4
初見の読者向けに Constant クラスの所在を補足。 `Constant::TOKEN_NAME`(src/Eccube/Common/Constant.php、値は '_token')と明示する。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@nanasess のレビュー指摘を反映: - customize: 「アップグレード安全」は過信しない注意(パッチ版のみ/コアEntityは置換不可・trait追加のみ/override は脆弱性パッチを個別再適用)。 - plugin: app/Plugin 直下開発は uninstall テストで消えるため、別dir開発+eccube:composer:require --from でシンボリックリンク配置を推奨。 - security: Ajax 専用アクションは CSRF に加え $request->isXmlHttpRequest() を併用。 - review-responsibility: 蓄積型(stored)XSS の観点を追加。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
過去の GHSA セキュリティ修正がほぼ Twig テンプレート XSS(特に管理画面)と Twig サンドボックスに集中していた点を踏まえ、各スキルへ反映: - twig-template: 管理画面テンプレートも XSS シンク(admin だから安全ではない)。 ユーザー編集テンプレートは Twig サンドボックス(template_from_string+sandboxed、 SecurityPolicyDecorator)を通す(外すとテンプレートインジェクション)。 - csv: fputcsv の escape 引数省略は PHP 8.4 で deprecation(コアは第5引数まで明示)。 - security: eccube_restrict_file_upload + RestrictFileUploadListener による アップロード系管理ルートの遮断機構を明記。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…il 検証) EC-CUBE 公式の脆弱性履歴(4系)にディレクトリトラバーサルがあり、コアに明確な 防御イディオムが存在するため反映。ユーザー指定パスは `..` を拒否し realpath() で 解決、許可ベースディレクトリ内かを str_starts_with(realpath(target), realpath(base)) で検証する(src/Eccube/Controller/Admin/Content/FileController.php::checkDir が手本)。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
概要
AI コーディングエージェント向けのレイヤ規約 Skill(
.claude/skills/)を拡充する PR です。実装でカバーできていなかった領域に 新規 Skill を 9 本追加し、あわせて既存 Skill を拡充・修正、実装直後の自己レビュー Skill を全層対応に拡張します。各 Skill は既存と同じ本文構成(対象 / 基本ルール / 実装パターン / よくある間違い / 実行・確認方法)・日本語トーンを踏襲し、内容はすべて
src/Eccube/の実コードで裏取りしています(推測を載せていません)。ドキュメント追加のみで、src/の挙動には影響しません。追加した Skill(9 本)
横断・基盤レイヤ
securitytwig-template|raw/escape('js')/is_safeの扱い・テンプレート上書きパスと名前空間event-subscribergetSubscribedEvents・EccubeEvents定数・優先度の向きplugincomposer.jsonのextra.code・#[EntityExtension]・proxy 再生成・eccube:plugin:generateでの雛形生成ドメインレイヤ
purchase-flowbcmath・登録は YAML タグ(priority)と#[CartFlow]系属性の 2 系統mailMailService集約・BaseInfoの差出人・safe_textmailエスケープ・MailHistoryは受注/出荷のみ・flushは呼び出し側・MAIL_*イベントcustomizeapp/Customizeの trait+#[EntityExtension]・AbstractTypeExtension・services.yamlのdecorates(@.inner)・テンプレート上書き。pluginとの使い分けcsvCsvImport/ExportService・dtb_csv定義・StreamedResponse・SJIS-win 変換・CSV_EXPORTイベントでの列追加command#[AsCommand]+autoconfigure・parent::__construct必須・execute(): int・業務は Service へ・cron は OS 側(コアに Scheduler 機構なし)既存 Skill の拡充・修正
review-responsibilityを「責務分離のみ」から 全層の自己レビュー チェックリストに拡張。セキュリティ(認可/CSRF/IDOR/XSS)・テンプレート・イベント・プラグイン、さらに Entity / Repository を点検軸に追加し、各レイヤ Skill の「よくある間違い」を参照する DRY 構成。controller: CSRF 失敗時の例外型の誤記を修正(BadRequestHttpException→AccessDeniedHttpException)。AbstractController::isTokenValid()は失敗時にAccessDeniedHttpExceptionを投げ、戻り値を捨てた bare 呼び出しでも検証は成立する点(if (!isTokenValid())の false 分岐はデッドコード)を明記。entity: 金額(DECIMAL)は?string/bcmathで扱う、create_date/update_dateはSaveEventSubscriberが自動セットするため自前#[ORM\PrePersist]は二重実装、他エンティティへの関連は親削除時の挙動(onDeleteか後始末)を決める、を追補。repository: 画面表示の一覧・関連取得を無制限に全件取得しない(ページング/上限)を追補。その他
AGENTS.mdの Skill 一覧表に追加 9 本を追記。AGENTS.md「開発コマンド」を参照する形に統一(コマンドを各 Skill に重複させない DRY 構成)。.claude/skills/。.codex/skills.agents/skills(symlink)経由でも参照可能なことを確認済み。動作確認
各 Skill が「規約に準拠するだけでなく実際に動くコードを生む」かを、白紙のエージェントで検証しました。スキル名や実装手段(「イベントで」「CSRF を」等)は一切伝えず、ゴールだけを段階的に指示して商品レビュー機能のプラグインをゼロから実装させ、各ステップで対応 Skill が発火・追従するかを確認しています。
plugineccube:plugin:generateで雛形生成(手書きしない)entity/repository#[ORM\...]属性・Types::・プロキシ対応ラッパ・QueryBuilder+setParametercontroller/formtype/securitytwig-template{{ }}+nl2br(|raw不使用)で XSS 回避securityisTokenValid()+所有権チェック(IDOR)event-subscriberFRONT_SHOPPING_COMPLETE_INITIALIZEを自力で購読(static・EccubeEvents定数・Service へ委譲)security%eccube_admin_route%配下に配置・削除は CSRF 検証customize#[EntityExtension]トレイトで会員にカラム追加 →schema:updateで実カラム化・投稿で加算command/mailMailService経由で実送信・再実行で再送しない(冪等)csvStreamedResponseで SJIS-win の CSV を配信purchase-flowDiscountProcessorでチェックアウトに 50 円引きを実適用review-responsibility結果: 生成されたプラグインを稼働環境(Docker)で install/enable し、ブラウザで通しの動作確認まで実施 — レビュー投稿(CSRF)→ 商品詳細での表示(
<script>ペイロードが実行されずエスケープ表示)→ 自分のレビュー削除(CSRF+IDOR)→ 管理画面の一覧、会員へのレビュー依頼メール送信(メールキャッチャに着信・冪等)、CSV ダウンロード、注文時の 50 円引き適用まで、いずれもエンド・ツー・エンドで正常動作しました。スキルに従うだけで、認可・CSRF・IDOR・XSS・文字コード・受注処理への組み込みまで満たした実働コードが得られることを確認しています。補足(裏取りで判明した知見の例)
検証中に「
create_date/update_dateはコアのSaveEventSubscriber(グローバル Doctrine prePersist/preUpdate)がmethod_existsで自動セットするため、エンティティ側で自前の#[ORM\PrePersist]を書くのは二重実装になる」という非自明な点が見つかり、entitySkill の「よくある間違い」に反映しています。🤖 Generated with Claude Code
Summary by CodeRabbit