feat: MCP サーバ用 OAuth2 scope と firewall を追加 - #190
Merged
Conversation
- scopes.available に mcp:product:read / mcp:order:read / mcp:customer:read / mcp:plugin:read を追加 (本体同梱の MCP サーバ機能の領域別 read scope。 GraphQL の read/write とは名前空間 mcp: で分離) - ApiExtension::prepend で ^/<admin>/mcp 用の OAuth2 stateless firewall を admin の前に注入 (本体側 Tool の IsGranted と AND 評価で認可) - ApiExtensionTest: firewall 順序 / shape / 既存 admin/customer の csrf_token_generator・anonymous 削除を検証 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMCP（Multi-Channel Protocol）向けにOAuth 2.0ディスカバリ、動的クライアント登録（DCR）、管理画面でのMCPトークン発行/失効、死蔵DCRクライアント清掃コマンドを追加する。新規エンティティ・リポジトリ・サービス・コントローラー・テンプレート・翻訳・ファイアウォール設定・テストを含む。 ChangesMCP OAuth2/DCR機能追加
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientRegistrationController
participant ClientManager
participant DcrClient
participant McpTokenController
participant McpTokenService
Client->>ClientRegistrationController: POST /register
ClientRegistrationController->>ClientManager: save(新規公開クライアント)
ClientRegistrationController->>DcrClient: persist/flush（登録時刻記録）
ClientRegistrationController-->>Client: 201 登録情報
Client->>McpTokenController: create() トークン発行フォーム送信
McpTokenController->>McpTokenService: issue(member, label, scopes, expireDays)
McpTokenService-->>McpTokenController: JWT
McpTokenController-->>Client: 発行済みトークン表示（一度のみ）
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
削除リンク生成に使う admin_api_oauth_delete の identifier 制約が \w+ で ハイフンを許さないため、 ハイフン入り client_id があると一覧の URL 生成で例外になる。 [\w-]+ に緩める。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MCP サーバは OAuth 自動探索メタデータを持たず、 店舗オーナーが自力でトークンを得る 手段が無かった。 league の AccessToken を再利用し、 管理画面で手動発行 (1 度だけ表示) する PAT 型の UX を追加する。 認証・失効は既存 firewall の経路にそのまま乗る。 - OAuth 管理画面を「API 新規追加 / MCP 新規追加」の 2 ボタンに分離 - MCP 発行: ラベル + MCP scope (mcp:*:read) + 有効期限 (30/90/180/365日) → JWT を 1 度表示 - 一覧に MCP トークンを表示し失効ボタンを追加 (revoke → 即 401) - 専用クライアント mcp_pat を PluginManager::enable で冪等生成し API 一覧からは除外 - 発行は scope/期限/発行者を防御的に検証し、 AccessToken model とメタを 1 トランザクションで 永続化してから署名する (途中失敗で失効不能な生トークンを残さない) - McpToken エンティティ (plg_api_mcp_token) で表示用メタ (ラベル/scope/期限/発行者) を保持 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- 発行したトークンが /admin/mcp の firewall を通る (200) - 失効すると同じトークンで 401 - mcp:*:read 以外の scope / プリセット外の有効期限は発行できない (フォームバイパス防御) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
テストが redirectUris を持たない client を拾い league の検証で 400 になっていた。 setUp で redirectUris/scope を持つ confidential client を生成して使う。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
手動トークンに代わり、 クライアントが 401→メタデータ→動的登録→管理ログイン+同意→ トークン取得 を自動で行えるようにする。 手動トークン方式は併存。 - 401 の WWW-Authenticate に RFC 9728 の resource_metadata ポインタ (mcp firewall の entry_point) - GET /.well-known/oauth-protected-resource (RFC 9728) / oauth-authorization-server (RFC 8414) を公開 (Host 詐称によるキャッシュ汚染を避けるため no-store) - POST /register (RFC 7591 DCR): public client を作成。 redirect_uri は https/loopback のみ許可し、 パーサ差異 (parse_url vs WHATWG) による host 詐称 (\ / @ / fragment) を fail-closed で拒否。 grant=authorization_code+refresh_token・scope=mcp:*:read に固定、 IP+グローバルの 2 段レート制限、 save 失敗と拒否を監査ログに記録 - 公開ルートは実在 2 well-known と /register に完全一致で限定 (security:false) - 同意画面に MCP scope の説明文言を追加 (read/write と同様、 未翻訳キーの露出を解消) - メタデータ/WWW-Authenticate を OAuthMetadataBuilder で一元生成 (canonical resource URI を一致) - 本番は TRUSTED_HOSTS / TRUSTED_PROXIES の設定が前提 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- .well-known 2 本が公開で正しい JSON を返す - /admin/mcp 401 が resource_metadata を広告する - DCR は loopback redirect を受理し scope/grant を MCP read に固定 - 外部 http redirect・パーサ差異 (バックスラッシュ) redirect は拒否 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eccube:plugin:enable の時点では league の doctrine persistence が未配線で、 Model\Client のメタデータが解決できず enable が MappingException で失敗する。 クライアント生成を実行時 (初回トークン発行時) の冪等生成に移し、 enable は EC-CUBE エンティティのみ触るようにする。 grant=authorization_code・ scope=mcp:*:read 固定は維持。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- /admin/mcp (本体同梱の route _mcp_endpoint) を叩く統合テストは、 プラグイン単体 CI に MCP サーバが無く実行不能のため削除。 トークン受理/失効は本体 mcp ジョブの McpTokenRevocationContractTest、 401 の resource_metadata 広告は McpFirewallContractTest が担う - McpTokenControllerTest はプラグインが所有する発行ロジック (scope 限定・有効日数縛り) に絞る - mcp_oauth_public firewall 追加で変わった順序・公開条件 (security:false/完全一致) と mcp の entry_point を ApiExtensionTest の期待値に反映 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dotani1111
force-pushed
the
feat/mcp-server-scorp
branch
from
June 19, 2026 07:30
8633afd to
9649345
Compare
DCR (RFC 7591) で量産される放置クライアントを定期削除する。 - eccube:api:mcp:cleanup-dcr-clients [--days=30] [--dry-run]: 登録から grace 日数を 過ぎ、 有効な access/refresh トークンを持たない DCR client を削除 - league の oauth2_client に作成時刻が無いため、 grace 判定用の作成時刻を側テーブル plg_api_dcr_client (DcrClient) で追跡し、 DCR 登録成功時に記録する - 削除は 1 トランザクションで client と追跡レコードをまとめて消す (部分失敗での不整合を防ぐ)。 client の access token は FK ON DELETE CASCADE、 孤立する refresh は明示削除する - 掃除対象は追跡レコードのある client のみ (導入前/記録漏れは作成時刻が無く対象外) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mix-up 攻撃対策として、 認可応答 (oauth2_authorize の redirect) に発行元 issuer を示す iss パラメータを付ける。 league は認可コード応答にフックが無いため kernel.response で Location に後付けし、 issuer は AS メタデータと同一値 (OAuthMetadataBuilder::baseUrl) を使う。 - AuthorizationResponseIssListener: code/error を持つ認可 redirect にのみ iss を付与 (ログイン redirect 等は対象外) - メタデータの authorization_response_iss_parameter_supported を true 化 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
league はエラー応答を、 登録 redirect_uri に '#' を含む場合 fragment に出すため、 query だけを見ると iss を取りこぼし、 メタデータの宣言 (iss 対応=true) と齟齬が出る。 query と fragment の両方で code/error を判定し、 乗っている側に iss を付ける。 エラー応答にも iss が付くことをテストで担保する。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- OAuthMetadataBuilder: 「Phase2 の aud」を「token の aud クレーム」に (ロードマップ用語を除去) - AuthorizationResponseIssListener: issuer 空ガードの注記を、 baseUrl() が request 不在時に '' を返す仕様への対処として説明 (「実質到達しない」の誇張を修正) - DcrClientCleaner: access の CASCADE コメントを、 refresh を先に消す順序保証と結びつける Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
Repository/DcrClientRepository.php (1)
37-45: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
findRegisteredBeforeに上限がありません。DCRクライアントが大量に蓄積した環境では一度に全件をメモリへロードすることになり、cronでの実行時にメモリ・DB負荷が問題になり得ます。バッチ処理（setMaxResultsでのページング等）の導入を検討してください。🤖 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 `@Repository/DcrClientRepository.php` around lines 37 - 45, `DcrClientRepository::findRegisteredBefore` は全件取得になっており、大量データ時にメモリとDB負荷が高くなります。`createQueryBuilder('d')` で組み立てている検索に `setMaxResults` を追加し、必要ならページング前提で分割取得できるようにしてください。呼び出し側と合わせて、閾値条件・並び順・取得件数の制御をこのメソッド内で明確に扱うように修正してください。Service/DcrClientCleaner.php (2)
49-79: 🩺 Stability & Availability | 🔵 Trivial同時実行（cron重複）に対する保護がありません。
このメソッドはコマンドから呼ばれ、cronでの定期実行を想定しています（コメントにも「定期実行 (cron) を想定」とあります）。同一クライアントに対して2つのプロセスが同時に
cleanupを実行した場合、片方が削除した直後にもう片方が同じDcrClientレコードをremoveしようとして、DoctrineのEntityNotFoundExceptionやUnit of Work不整合を引き起こす可能性があります。SymfonyのLockコンポーネント等でコマンド実行の排他制御を行うことを推奨します。🤖 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 `@Service/DcrClientCleaner.php` around lines 49 - 79, The DcrClientCleaner::cleanup workflow is vulnerable to overlapping cron runs that can race on the same DcrClient records. Add an execution lock around the command path that invokes cleanup (for example, using Symfony Lock in the command that calls DcrClientCleaner) so only one process can enter this cleanup at a time. Keep the cleanup logic in DcrClientCleaner unchanged except for relying on the caller’s lock, and make sure the lock is acquired before calling cleanup and always released afterward.
56-79: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value候補ごとにN+1クエリが発生します。
hasLiveTokenは候補1件につき最大2回のSELECTを発行し（Line 60）、削除フェーズでも1件ごとにDQL DELETEを発行しています（Line 86-89、deleteDeadClient内）。候補数が多い環境では、cron実行時にDB往復回数が線形に増加します。IN (:clientIdentifiers)を用いた一括クエリでライブトークンを持つクライアント集合を1回のSELECTで取得する形にまとめることを検討してください。cronのオフラインバッチ処理という性質上、現状でも致命的ではありませんが、対象件数が多い運用では改善余地があります。🤖 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 `@Service/DcrClientCleaner.php` around lines 56 - 79, The cleanup flow in DcrClientCleaner is doing per-candidate database work, causing N+1 queries in both the live-token check and the delete loop. Refactor the candidate classification around hasLiveToken/deleteDeadClient so the set of client identifiers is processed in bulk, ideally by fetching all identifiers with live tokens using a single IN (:clientIdentifiers) query and then deriving $dead/$keptActive from that result. Also update the deletion phase to remove dead records in a batched way instead of issuing one DQL DELETE per record, while keeping DcrCleanupResult behavior unchanged.Tests/Service/DcrClientCleanerTest.php (1)
73-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winrevoke済み・未期限切れトークンのケースが未カバー
hasLiveToken()はrevoked = false AND expiry > nowで判定していますが、「revoke済みだが有効期限内」のアクセストークンを持つクライアントが正しく削除対象になることを検証するテストがありません。既存のissueAccessToken()に$revoked = trueを渡すだけで追加できるため、既存パターンをそのまま複製できます。♻️ 追加テスト例
+ public function testDeletesClientWithOnlyRevokedToken(): void + { + $id = $this->registerDcrClient('dcr-old-revoked', 40); + $this->issueAccessToken($id, new \DateTimeImmutable('+1 hour'), true); + + $result = $this->runCleanup(30, false); + + $this->assertContains($id, $result->deletedClientIdentifiers); + $this->assertNull($this->clientManager->find($id), 'revoke済みトークンのみの client は削除される'); + }🤖 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 `@Tests/Service/DcrClientCleanerTest.php` around lines 73 - 83, `DcrClientCleanerTest::testKeepsClientWithLiveAccessToken` only covers a non-revoked live token, so add a sibling test that issues an access token with `$revoked = true` but an unexpired `DateTimeImmutable` and verifies the client is deleted by `runCleanup()`. Reuse `issueAccessToken()`, `runCleanup()`, and `clientManager->find()` to assert that `hasLiveToken()` does not treat revoked-but-unexpired tokens as live and that the client is removed as expected.Tests/Web/Admin/OAuth2Bundle/AuthorizationControllerTest.php (1)
160-165: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win拒否フローの
iss検証が許可フローより緩い許可フロー(115-116行目)は
issの存在に加えホスト名の一致まで検証していますが、拒否フローはissetのみです。AuthorizationResponseIssListenerはcode/error両方に同一issuerを付与する実装なので、拒否フロー側も値まで検証すると回帰検知力が上がります。♻️ 提案
self::assertEquals('access_denied', $callbackParams['error']); // RFC 9207: エラー応答にも iss が付与される (AuthorizationResponseIssListener) self::assertTrue(isset($callbackParams['iss']), 'エラー応答にも iss が付与される'); + self::assertStringContainsString($this->client->getRequest()->getHttpHost(), $callbackParams['iss']);🤖 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 `@Tests/Web/Admin/OAuth2Bundle/AuthorizationControllerTest.php` around lines 160 - 165, 拒否フローの iss 検証が許可フローより弱いので、AuthorizationControllerTest の拒否ケースでも AuthorizationResponseIssListener が付与する iss の存在確認だけでなく、許可フローと同じ issuer 値まで一致しているかを検証するようにしてください。parseCallbackParams() で取得した callbackParams を使い、access_denied の後に iss の中身をホスト名付きの期待値と比較する形に揃えると、両フローで同一 issuer が付与される実装の回帰を拾いやすくなります。Tests/Web/Admin/McpTokenControllerTest.php (1)
14-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winクラス名とテスト対象のギャップ
McpTokenControllerTestという名前・Tests/Web/Adminという配置ですが、実際には HTTP リクエストを発行せずMcpTokenService::issue()を直接呼び出すサービス層テストです。31行目のコメントで意図的な設計と分かりますが、この命名だと「管理画面コントローラーのHTTPレベル(権限・CSRF・テンプレート)テストが本リポジトリに存在する」と誤解されるおそれがあります。Tests/Service/McpTokenServiceTest.php等への改名・移動を検討してください。🤖 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 `@Tests/Web/Admin/McpTokenControllerTest.php` around lines 14 - 46, `McpTokenControllerTest` は実際には `McpTokenService::issue()` を直接検証するサービス層テストなので、クラス名と配置の意図がずれています。HTTP/管理画面のコントローラーテストに見えないよう、`McpTokenControllerTest` と `Tests/Web/Admin` 配下の扱いを見直し、`McpTokenService` と `McpTokenRepository` を対象にした名前と配置（例: サービス層テスト）へ改名・移動してください。Resource/config/services.yaml (1)
7-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDCR レート制限に
fixed_windowではなくsliding_windowを推奨。
fixed_windowは境界跨ぎで実質最大2倍のバーストを許容し得るため、濫用抑止という目的に対してはsliding_windowの方が適しています。設定変更のみで対応可能です。♻️ 提案
mcp_dcr_register: - policy: 'fixed_window' + policy: 'sliding_window' limit: 20 interval: '1 hour' cache_pool: cache.app mcp_dcr_register_global: - policy: 'fixed_window' + policy: 'sliding_window' limit: 200 interval: '1 hour' cache_pool: cache.app🤖 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 `@Resource/config/services.yaml` around lines 7 - 19, The DCR rate limiter entries in the rate_limiter configuration are using fixed_window, which allows boundary burst behavior; update both mcp_dcr_register and mcp_dcr_register_global to use sliding_window instead. Keep the existing limits, intervals, and cache_pool values unchanged, and make the change in the services configuration where the rate_limiter policies are defined.Entity/DcrClient.php (1)
45-49: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
create_dateにインデックス追加を検討。
DcrClientRepository::findRegisteredBefore()がこのカラムでフィルタ・ソートするため、インデックスがないと清掃対象クライアントが増えた際にフルスキャンになります。レート制限で増加ペースは抑えられていますが、将来的な運用コスト削減のため検討の価値があります。♻️ 提案
- #[ORM\Column(name: 'create_date', type: 'datetimetz')] + #[ORM\Column(name: 'create_date', type: 'datetimetz')] + // #[ORM\Index(name: 'idx_dcr_client_create_date', columns: ['create_date'])] をテーブルレベルで検討 private \DateTime $createDate;🤖 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 `@Entity/DcrClient.php` around lines 45 - 49, `create_date` の検索性能改善が必要です。`DcrClientRepository::findRegisteredBefore()` が `DcrClient::$createDate` でフィルタ・ソートしているため、`Entity/DcrClient.php` の `createDate` にインデックスを付ける対応を検討してください。`DcrClient` の ORM マッピングにインデックス定義を追加し、既存の取得処理はそのまま活かしつつ、将来の件数増加時にフルスキャンにならない形にしてください。Controller/Admin/McpTokenController.php (1)
38-74: 🔒 Security & Privacy | 🔵 Trivial発行画面のレスポンスにキャッシュ制御を検討。
JWTを一度だけ表示する設計は妥当ですが、レスポンスに
Cache-Control: no-store等のヘッダーが付与されていないと、ブラウザ/プロキシキャッシュにJWTが残存するリスクがあります(テンプレート側の実装は未確認)。🤖 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 `@Controller/Admin/McpTokenController.php` around lines 38 - 74, The one-time JWT issuance view in McpTokenController::create should prevent browser/proxy caching. Add cache-control handling to the response returned by the token issuance render path (the mcp_token_issued.twig response) so it sends no-store or equivalent headers, and ensure the normal form render path remains unchanged.Form/Type/Admin/McpTokenType.php (1)
50-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win有効期限の選択肢ラベルが未翻訳。
'30日'などのラベルが直接日本語でハードコードされており、trans()を経由していません。英語管理画面でもラベルが日本語のまま表示されます。♻️ 翻訳キー経由に変更する提案
->add('expire', ChoiceType::class, [ 'mapped' => false, 'choices' => [ - '30日' => 30, - '90日' => 90, - '180日' => 180, - '365日' => 365, + trans('api.admin.oauth.mcp_token.expire_30') => 30, + trans('api.admin.oauth.mcp_token.expire_90') => 90, + trans('api.admin.oauth.mcp_token.expire_180') => 180, + trans('api.admin.oauth.mcp_token.expire_365') => 365, ],🤖 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 `@Form/Type/Admin/McpTokenType.php` around lines 50 - 57, The expire ChoiceType labels are hardcoded Japanese strings in McpTokenType, so they bypass translation. Update the choices in the form field definition to use translation keys through trans() or a translated label source instead of literal values like '30日', keeping the existing numeric expiration values unchanged. Use the McpTokenType choice configuration as the place to wire the translated labels so the admin UI can display the correct language.
🤖 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.
Inline comments:
In `@EventListener/AuthorizationResponseIssListener.php`:
- Around line 65-78: `AuthorizationResponseIssListener` currently skips
responses when `iss` is already present, which leaves a potentially untrusted
issuer in place. Update the listener logic so the final redirect `Location`
always rewrites or injects `iss` with the trusted value from
`metadata->baseUrl()` for the component carrying `code` or `error` (query or
fragment), instead of returning early on existing `iss`. Keep the empty-issuer
guard, but ensure the trusted issuer is applied unconditionally in the
`AuthorizationResponseIssListener` flow.
In `@Resource/template/admin/OAuth/mcp_token_issued.twig`:
- Around line 27-32: The MCP token issued page renders a bearer token in HTML,
so the completion response from McpTokenController should explicitly prevent
caching. Update the success response handling in
Controller/Admin/McpTokenController.php to set the Cache-Control header to
no-store, private when returning the mcp_token_issued.twig view, using the
controller/action that builds that response so the token cannot be re-shown from
browser or shared-device cache.
In `@Service/DcrCleanupResult.php`:
- Line 19: The DcrCleanupResult class currently uses the PHP 8.2-only final
readonly class syntax, so make it compatible with the documented PHP 7.2+
support. Update DcrCleanupResult to remove the class-level readonly modifier and
apply readonly only to individual properties if needed, or otherwise adjust the
compatibility contract consistently across the codebase; keep the fix localized
to the DcrCleanupResult definition and related property declarations.
In `@Service/McpTokenService.php`:
- Around line 108-129: The `AccessTokenModel` being saved inside
`McpTokenService` currently hardcodes an empty scope array, so the persisted
access token loses the scopes already collected in the loop. Update the
`wrapInTransaction` block in `McpTokenService` to pass the same `$scopes` value
into `accessTokenManager->save(...)` when creating the `AccessTokenModel`,
keeping the persisted OAuth token scopes in sync with the `McpToken` metadata.
In `@Service/OAuthMetadataBuilder.php`:
- Around line 39-62: The discovery URLs are being built from only the scheme and
host, so they break when the app is hosted under a subdirectory. Update
Service/OAuthMetadataBuilder’s baseUrl/resourceUri/protectedResourceMetadataUrl
logic to include the request base path or switch to router-generated absolute
URLs, and ensure the methods that assemble resource, authorization_endpoint,
token_endpoint, and registration_endpoint all use the same base-path-aware
source.
---
Nitpick comments:
In `@Controller/Admin/McpTokenController.php`:
- Around line 38-74: The one-time JWT issuance view in
McpTokenController::create should prevent browser/proxy caching. Add
cache-control handling to the response returned by the token issuance render
path (the mcp_token_issued.twig response) so it sends no-store or equivalent
headers, and ensure the normal form render path remains unchanged.
In `@Entity/DcrClient.php`:
- Around line 45-49: `create_date`
の検索性能改善が必要です。`DcrClientRepository::findRegisteredBefore()` が
`DcrClient::$createDate` でフィルタ・ソートしているため、`Entity/DcrClient.php` の `createDate`
にインデックスを付ける対応を検討してください。`DcrClient` の ORM
マッピングにインデックス定義を追加し、既存の取得処理はそのまま活かしつつ、将来の件数増加時にフルスキャンにならない形にしてください。
In `@Form/Type/Admin/McpTokenType.php`:
- Around line 50-57: The expire ChoiceType labels are hardcoded Japanese strings
in McpTokenType, so they bypass translation. Update the choices in the form
field definition to use translation keys through trans() or a translated label
source instead of literal values like '30日', keeping the existing numeric
expiration values unchanged. Use the McpTokenType choice configuration as the
place to wire the translated labels so the admin UI can display the correct
language.
In `@Repository/DcrClientRepository.php`:
- Around line 37-45: `DcrClientRepository::findRegisteredBefore`
は全件取得になっており、大量データ時にメモリとDB負荷が高くなります。`createQueryBuilder('d')` で組み立てている検索に
`setMaxResults`
を追加し、必要ならページング前提で分割取得できるようにしてください。呼び出し側と合わせて、閾値条件・並び順・取得件数の制御をこのメソッド内で明確に扱うように修正してください。
In `@Resource/config/services.yaml`:
- Around line 7-19: The DCR rate limiter entries in the rate_limiter
configuration are using fixed_window, which allows boundary burst behavior;
update both mcp_dcr_register and mcp_dcr_register_global to use sliding_window
instead. Keep the existing limits, intervals, and cache_pool values unchanged,
and make the change in the services configuration where the rate_limiter
policies are defined.
In `@Service/DcrClientCleaner.php`:
- Around line 49-79: The DcrClientCleaner::cleanup workflow is vulnerable to
overlapping cron runs that can race on the same DcrClient records. Add an
execution lock around the command path that invokes cleanup (for example, using
Symfony Lock in the command that calls DcrClientCleaner) so only one process can
enter this cleanup at a time. Keep the cleanup logic in DcrClientCleaner
unchanged except for relying on the caller’s lock, and make sure the lock is
acquired before calling cleanup and always released afterward.
- Around line 56-79: The cleanup flow in DcrClientCleaner is doing per-candidate
database work, causing N+1 queries in both the live-token check and the delete
loop. Refactor the candidate classification around hasLiveToken/deleteDeadClient
so the set of client identifiers is processed in bulk, ideally by fetching all
identifiers with live tokens using a single IN (:clientIdentifiers) query and
then deriving $dead/$keptActive from that result. Also update the deletion phase
to remove dead records in a batched way instead of issuing one DQL DELETE per
record, while keeping DcrCleanupResult behavior unchanged.
In `@Tests/Service/DcrClientCleanerTest.php`:
- Around line 73-83: `DcrClientCleanerTest::testKeepsClientWithLiveAccessToken`
only covers a non-revoked live token, so add a sibling test that issues an
access token with `$revoked = true` but an unexpired `DateTimeImmutable` and
verifies the client is deleted by `runCleanup()`. Reuse `issueAccessToken()`,
`runCleanup()`, and `clientManager->find()` to assert that `hasLiveToken()` does
not treat revoked-but-unexpired tokens as live and that the client is removed as
expected.
In `@Tests/Web/Admin/McpTokenControllerTest.php`:
- Around line 14-46: `McpTokenControllerTest` は実際には `McpTokenService::issue()`
を直接検証するサービス層テストなので、クラス名と配置の意図がずれています。HTTP/管理画面のコントローラーテストに見えないよう、`McpTokenControllerTest`
と `Tests/Web/Admin` 配下の扱いを見直し、`McpTokenService` と `McpTokenRepository`
を対象にした名前と配置（例: サービス層テスト）へ改名・移動してください。
In `@Tests/Web/Admin/OAuth2Bundle/AuthorizationControllerTest.php`:
- Around line 160-165: 拒否フローの iss 検証が許可フローより弱いので、AuthorizationControllerTest
の拒否ケースでも AuthorizationResponseIssListener が付与する iss の存在確認だけでなく、許可フローと同じ issuer
値まで一致しているかを検証するようにしてください。parseCallbackParams() で取得した callbackParams
を使い、access_denied の後に iss の中身をホスト名付きの期待値と比較する形に揃えると、両フローで同一 issuer
が付与される実装の回帰を拾いやすくなります。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e122f3c-8181-47d2-8e28-9ab8b31aab39
📒 Files selected for processing (28)
Command/CleanupDcrClientsCommand.phpController/Admin/McpTokenController.phpController/Admin/OAuthController.phpController/ClientRegistrationController.phpController/WellKnownController.phpDependencyInjection/ApiExtension.phpEntity/DcrClient.phpEntity/McpToken.phpEventListener/AuthorizationResponseIssListener.phpForm/Type/Admin/McpTokenType.phpRepository/DcrClientRepository.phpRepository/McpTokenRepository.phpResource/config/services.yamlResource/locale/messages.en.yamlResource/locale/messages.ja.yamlResource/template/admin/OAuth/index.twigResource/template/admin/OAuth/mcp_token.twigResource/template/admin/OAuth/mcp_token_issued.twigSecurity/McpAuthenticationEntryPoint.phpService/DcrCleanupResult.phpService/DcrClientCleaner.phpService/McpTokenService.phpService/OAuthMetadataBuilder.phpTests/DependencyInjection/ApiExtensionTest.phpTests/Service/DcrClientCleanerTest.phpTests/Web/Admin/McpTokenControllerTest.phpTests/Web/Admin/OAuth2Bundle/AuthorizationControllerTest.phpTests/Web/OAuthDiscoveryTest.php
MCP ログイン/権限（api44）のレビュー（実機検証ベース）ec-cube#6832 と併用して実機で確認しました。権限をすり抜けられる致命的な穴は見つからず、戻り先URL検証・権限の閲覧scope固定・登録レート制限などは妥当に機能していました。修正すると良くなる点を以下にまとめます。
なお「トークン失効に発行者チェックが無い」点は、一覧が全管理者に全トークンを表示する設計（ |
- mcp firewall は Member(ROLE_ADMIN) を認証するだけで、 領域/read は各ツールの ScopeChecker 依存だった。 mcp scope を 1 つも持たない token の /admin/mcp 到達を access_control 段で塞ぐ - core が prependExtensionConfig(先頭 unshift) で入れる ^/<admin> → ROLE_ADMIN に shadow されないよう、 compiler pass で security.access_map の add() 列の先頭に mcp matcher を差し込む (AccessMap は先頭一致) - 要求 role は本体 McpToolScopeMap を唯一のソースにして二重管理を避ける Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nanasess
reviewed
Jul 22, 2026
nanasess
reviewed
Jul 22, 2026
nanasess
reviewed
Jul 22, 2026
- compiler pass が McpToolScopeMap を無条件参照し、 MCP 本体を持たない ec-cube (Api44 単体 CI 等) でコンテナコンパイルが class-not-found で fatal になっていた - McpToolScopeMap が無い環境では保護対象の /admin/mcp ツールも無いため、 class_exists ガードで no-op にする Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- McpToolScopeMap::MAP 由来だと本体 (MCP) を持たない環境の phpstan で array_values の template 型が解決できずエラーになる - read scope role を直書きにする (領域追加時は本体 McpToolScopeMap と同期する) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AccessTokenModel に JWT と同じ scope を積む。 空だと oauth2_access_token.scopes が JWT と恒久的に食い違い、 introspection・監査・scope ベースの掃除が壊れる (McpTokenControllerTest で model 側の scope を assert) - discovery URL の baseUrl に getBaseUrl() を足し、 サブディレクトリ配下 (例 /shop) 公開時の base path 落ちを防ぐ - OAuthMetadataBuilder の docblock を 2 者一致に訂正。 JWT の aud は league が client 識別子を入れるため resource URI にはならない - 有効日数を McpTokenService::AVAILABLE_EXPIRE_DAYS の単一ソース参照にし、 choices と Assert\Choice の二重管理を解消 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
レビューありがとうございます。 #2（必須）は AccessTokenModel に JWT と同じ scope を積むようにしました。 #1 は #3 は docblock を 2 者一致（PRM の resource / WWW-Authenticate）に訂正しました。 #4 は有効日数を |
- AuthorizationResponseIssListener: 応答に既にある iss は redirect_uri 由来 (client 制御) なので、 除去してから AS の issuer で上書きする。 league は iss を出さないため既存 iss は信用できず、 RFC 9207 の mix-up 対策として AS が iss を強制する必要がある - McpTokenController: 発行完了画面 (bearer token を HTML に埋め込む) に Cache-Control: no-store, private を付け、 ブラウザ・共有端末のキャッシュ残りを防ぐ - AuthorizationResponseIssListenerTest を追加 (iss 付与・client 制御 iss の上書き・非認可応答の除外) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- 本プラグインは EC-CUBE 4.4 (PHP 8.2+) 用で、 コードも readonly class 等 8.2 の機能を使う。 README の 7.2 は旧記載のため実要件に揃える Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nanasess
requested changes
Jul 31, 2026
Collaborator
There was a problem hiding this comment.
MCP の認証・認可を api44 に集約する方針、および firewall / access_control / scope / DCR / PAT の分離は妥当だと考えます。既出指摘 (CodeRabbit 5 件・私の 3 件) はいずれも対応済みであることを確認しました。本体側 #6832 とは McpScope の role 名 (ROLE_OAUTH2_MCP:*:READ)、mcp.http.path = /%eccube_admin_route%/mcp と firewall pattern、docs/mcp/admin-mcp-scope-boundary.md が求める「最低 1 つの mcp scope を要求する access_control」の実装が整合していることも確認しています。
そのうえで、マージ前に対応いただきたい点が 2 件あるため request changes とさせてください。いずれも軽微な修正で済む範囲だと思います。
- DCR の 2 段レート制限の消費順序 — グローバル枠を先に消費するため、単一 IP から全体の登録枠を枯渇させられます。順序を入れ替えるだけで解消します。
- 認可応答 URL の再構築による redirect_uri クエリの破壊 —
2764f14による退行です。issの配置とクライアント制御issの上書きを直した点は妥当なので revert ではなく、組み立て直しの方法だけ見直していただければと思います。
なお本レビューは api44 (#190) の全差分を対象とし、本体 #6832 は #190 との結合点 (scope map / firewall / access_control / ルーティング / rate limiter / 監査ログ設定) に絞って確認しています。#6832 本体 92 ファイルの全面レビューは別途必要です。
権限管理 (AuthorityRole) との関係について (#6832 との横断・要否のご判断だけ伺えれば)
EC-CUBE の権限管理は AuthorityVoter が deny_url とパスを突き合わせる方式ですが、MCP は 11 ツールすべてが /admin/mcp の 1 URL に集約されるため、この仕組みでは領域を絞れません。既定では api44 の PluginManager が店舗オーナー (Authority::OWNER = 1) に /api の deny を入れており、発行画面 /admin/api/oauth/mcp/new へ到達できるのはシステム管理者だけなので、既定構成では問題になりません。ただし運用で /api の deny を外した場合、「/customer を deny されている管理者が mcp:customer:read を発行して顧客 PII を読む」経路が成立します。docs/mcp/admin-mcp-scope-boundary.md にこの前提 (権限管理は MCP に効かない／発行画面は /api の deny に依存している) を 1 行足しておくと、運用側の事故を防げると思います。
- 拒否時はトークンを消費しないため、グローバルを先に消費させると単一 IP が IP 拒否後もグローバル枠を減らし続け、1 IP でグローバル枠を食い潰して全体を締め出せる - IP 単位を先に判定し、単一 IP がグローバル枠に積めるのを IP 上限までに抑える Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- parse_str/http_build_query での全分解は、名前の '.'・空白を '_' に変換し重複キーを畳み arr[] を arr[0] に書き換えるため、クエリ付き redirect_uri が壊れる - 判定にのみ parse_str を使い、iss の除去・追記は raw query/fragment の生文字列で行う - クエリ付き redirect_uri が原文のまま保たれることのテストを追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nanasess
reviewed
Aug 3, 2026
- 制限管理者 (authority != ADMIN) は URL 認可で管理画面を塞がれても stateless な mcp firewall では再評価されず、全 scope トークンを自分名義で発行して制限を回避できた - create() の認可チェックを ADMIN 要求に変更し、対話型同意フロー AuthorizationRequestResolveListener と対称にする - 制限管理者が発行できないことのテストを追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nanasess
approved these changes
Aug 4, 2026
nanasess
left a comment
nanasess
left a comment
Collaborator
There was a problem hiding this comment.
本 PR（MCP サーバ用 OAuth2 scope / firewall）を、本体 PR #6832 と組み合わせて Claude Code を実 MCP クライアントとしてローカルで動作確認・レビューしました。指摘した major は修正・テストとも確認済み、blocker なしのため Approve します。
指摘対応の確認（a6553b31）
[major] MCP PAT 発行の Authority::ADMIN ゲート → 解消を確認:
McpTokenController::create()の認可を!$member instanceof Member || Authority::ADMIN !== $member->getAuthority()?->getId()に変更。同意フローAuthorizationRequestResolveListenerと対称、かつ?->で authority null も fail-closed。McpTokenControllerAuthorityTest::testCreateForbiddenForNonAdminAuthorityで制限管理者（OWNER）が有効フォーム送信でも 403 になることを検証。- revoke は scope 付与を伴わないため据え置き（妥当な判断）。
OAuth / firewall 動作確認（実測）
.well-known/oauth-protected-resource/oauth-authorization-serverが 4 scope（mcp:{product,order,customer,plugin}:read）を公開。- 動的クライアント登録
POST /register（201）、PKCE(S256)、認可応答の RFC 9207iss（redirect_uri のクエリ原文保持3e68530含む）。 - 未認証
/admin/mcp→ 401 +WWW-Authenticate: Bearer resource_metadata=...（RFC 9728、fail-closed）。 - OAuth 自動ディスカバリ（Claude Code の
/mcp、localhost 動的ポート redirect_uri が DCR を通過）と Bearer 直の両方式で接続成功。 - スコープ二層 fail-closed: product のみのトークンで
tools/listは product 系のみ表示、非表示ツールを直接tools/callしてもInsufficient scopeで拒否。access_controlが^/admin/mcpに最低 1 scope を要求し、ScopeFilteringRegistryの list 層 fail-open は HTTP 経路から到達不能（多層防御が閉じている）。
残る非 blocker（フォローアップ余地）
- トークン一覧/失効に所有者スコープ無し（共有 admin モデルなら by-design。revoke 据え置き方針と整合）。
- 永続
AccessTokenの scope が[]（JWT クレームで認可するため無害だが、oauth2_access_tokenを監査する将来ツールが誤読の恐れ。実 scope を渡すと整合）。 RateLimitListenerの IP バケットはtrusted_proxies設定に依存（リバースプロキシ配下の運用ドキュメントで前提を明記）。
（PHPStan / ユニットテストはローカル未実行＝CI に委ねます）
対応ありがとうございました。LGTM 👍
- OAuth ディスカバリの URL は Host 由来で組むため、 TRUSTED_HOSTS 未設定だと Host 偽装で resource_metadata に偽ドメインを載せられる - README で TRUSTED_HOSTS / TRUSTED_PROXIES を必須化し、 本番で未設定のまま metadata を出す経路で警告ログを出す - 本番×未設定→警告 / dev・設定済み→無警告 のガードテストを追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- JWT の aud は resource URI に縛られず、 トークンが有効な間はその scope で PII を読めるため露出期間を抑える - AVAILABLE_EXPIRE_DAYS から 365 を外す (フォーム選択肢・発行検証は同定数由来で自動追従) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@ttokoro20240902 ① Host 由来 metadata — README で TRUSTED_HOSTS / TRUSTED_PROXIES を必須化し、 本番で未設定のまま配信すると警告ログを出すガードを追加いたしました（150f7ec）。 ② PKCE — 本体側でなくプラグイン側 (ApiExtension の prepend) で require_code_challenge_for_public_clients を明示 pin し、 本 PR に入れました（27c1ca4）。 |
- league の require_code_challenge_for_public_clients は既定 true だが、 将来の既定変更で静かに無効化されないよう prepend で明示的に固定する - prepend が league の config 断片を先頭に積むため、 ApiCompilerPass の鍵パス取得を位置非依存の走査に直す (鍵無し断片が先頭でも core の鍵を拾う) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- 365 日が発行不可・上限が 180 日であることを縛り、 上限短縮の意図が静かに戻らないようにする Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- public クライアントが code_challenge 無しで認可要求すると invalid_request で弾かれることを実挙動で確認する - config 値の assert は「宣言」の確認に留まり、 強制が効いているかを区別できないため挙動テストで補う Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- README のドキュメント URL 更新 (doc4.ec-cube.net) と MCP セキュリティ設定節の追記を統合 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- トークン発行 tooltip の最長日数を 180 に修正 (有効日数上限 [30,90,180] と一致) - iss listener の class docstring を query/fragment 両対応の実装に合わせる Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
3 participants
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 by CodeRabbit