feat: route AI requests through Drupal AI module

[jjroelofs]

Summary

• Add AiChatController that proxies CKEditor AI Agent requests through the ai module's AiProviderPluginManager, eliminating API key exposure in the browser
• Update getDynamicPluginConfig() and getCkEditorConfig() to set endpointUrl with engine dxai instead of passing apiKey when the ai module is available
• Add ai and ai_provider_dxpr as module and composer dependencies

Architecture: Frontend → Drupal Controller → ai module → ai_provider_dxpr → Kavya API

Follows the same pattern as dxpr/dxpr_builder#4061.

Closes #42

Files changed

File	Change
src/Controller/AiChatController.php	New — proxy endpoint based on dxpr_builder's controller
ckeditor_ai_agent.routing.yml	Add /api/ckeditor-ai-agent/ai/chat route
ckeditor_ai_agent.info.yml	Add ai and ai_provider_dxpr dependencies
composer.json	Add drupal/ai and drupal/ai_provider_dxpr
src/Plugin/CKEditor5Plugin/AiAgent.php	Pass endpointUrl + engine dxai instead of apiKey
src/AiAgentConfigurationManager.php	Same change for getCkEditorConfig()
ckeditor_ai_agent.services.yml	Add module_handler and url_generator to config manager

Test plan

• Install ai and ai_provider_dxpr modules
• Configure DXPR provider key
• Open any CKEditor 5 editor with AI Agent enabled
• Verify AI generation works through the proxy endpoint
• Verify API key is NOT exposed in browser (check drupalSettings / editor config)
• Verify streaming responses work correctly
• Verify fallback still works when ai module is not installed (direct API key mode)

[jjroelofs]

PR Review: feat: route AI requests through Drupal AI module

Overall this is a solid architectural improvement — moving API keys server-side via a proxy controller is the right pattern. However, there are several concerns ranging from security issues to architectural decisions.

---

Critical Issues

1. Security: Route permission is too permissive
ckeditor_ai_agent.routing.yml — The route uses _permission: 'access content', which is granted to anonymous users on many Drupal sites. This means any unauthenticated visitor could hit the AI proxy endpoint and consume API credits. This should at minimum require a dedicated permission like 'use ckeditor ai agent', or even better, add a CSRF token check.

2. Security: No CSRF protection
The POST endpoint at /api/ckeditor-ai-agent/ai/chat has no CSRF token validation. Drupal's REST/JSON:API routes typically use _csrf_request_header_token or similar. Without it, a malicious page could make cross-origin POST requests to this endpoint if the user has a session. Add:

requirements:
  _permission: 'use ckeditor ai agent'
  _csrf_request_header_token: 'TRUE'

3. Security: No input validation/sanitization
In AiChatController::chat(), request body fields like $data->prediction, $data->providers, $data->allowed_html_tags, etc. are passed through to the provider with minimal validation. An attacker could inject arbitrary configuration. At minimum, validate/whitelist expected values.

---

Architectural Concerns

4. Hard dependency on ai and ai_provider_dxpr is wrong
The info.yml and composer.json make these hard dependencies, but getCkEditorConfig() and getDynamicPluginConfig() both have if ($useAiModule) fallback logic for when the module is not installed. This is contradictory — if they're hard dependencies, the fallback code is dead. If you want the fallback, these should be soft dependencies (listed under suggests in composer and removed from info.yml dependencies). The PR description itself says "Verify fallback still works when ai module is not installed."

5. Controller hardcodes 'dxpr' provider

$provider = $this->aiProviderManager->createInstance('dxpr');

This tightly couples the controller to a single AI provider. The ai module's architecture is designed for provider abstraction. Consider using the configured default provider or accepting the provider ID as configuration, making this more reusable.

6. Duplicate logic in two places
Both AiAgentConfigurationManager::getCkEditorConfig() and AiAgent::getDynamicPluginConfig() contain near-identical AI module detection and URL generation logic. This violates DRY. The plugin should ideally delegate to the configuration manager, or the shared logic should live in one place.

---

Code Quality Issues

7. No rate limiting
The proxy endpoint has no rate limiting. A user with access content permission could flood the endpoint with requests, burning through API credits.

8. ob_flush() may fail with output buffering disabled
In AiChatController.php:153-154, ob_flush() will emit a warning if output buffering isn't active. Wrap it:

if (ob_get_level() > 0) {
    ob_flush();
}
flush();

9. Model default 'kavya-m1' is hardcoded

$model = $data->model ?? 'kavya-m1';

This default should come from configuration, not be hardcoded in the controller.

---

Minor/Nit

• $useAiModule naming: Drupal PHP coding standards use snake_case for variables ($use_ai_module).
• The $config['jsonrpc'] = FALSE; line lacks a comment explaining why JSON-RPC is disabled — the inline comment says "OpenAI client library compatibility" but this deserves more context.

---

Summary

Category	Issue	Severity
Security	access content permission too broad	Critical
Security	No CSRF protection on POST endpoint	Critical
Security	No input validation on proxy passthrough	High
Architecture	Hard deps contradict fallback logic	High
Architecture	Provider hardcoded to 'dxpr'	Medium
Architecture	Duplicate config logic in two classes	Medium
Reliability	ob_flush() warning when no buffering	Low
Code quality	Hardcoded model default	Low
Code quality	Variable naming conventions	Low

Recommendation: Do not merge until the security issues (permission, CSRF, input validation) and the hard-vs-soft dependency contradiction are resolved. The core approach is sound and well-structured, but it needs hardening before it's production-ready.

[jjroelofs]

Thanks for the thorough review! Pushed fixes in b139a39. Here's what I addressed and where I'm pushing back:

Addressed

1. Permission too permissive — Fixed. Added a dedicated use ckeditor ai agent permission and switched the route to use it.

2. CSRF protection — Fixed. Added _csrf_request_header_token: 'TRUE' to the route. CKEditor's JS fetch requests already send the X-Drupal-Ajax-Token header.

3. Input validation — Fixed. The controller now:

• Validates messages is an array
• Validates each message has role (string, whitelisted to system/user/assistant) and content (string)
• Validates model against [a-zA-Z0-9._-]+ pattern
• Type-checks all passthrough fields (providers must be array of strings, allowed_html_tags/allowed_html_classes must be strings, prediction/response_format must be objects)

4. Hard deps → soft deps — Fixed. Removed ai:ai and ai_provider_dxpr:ai_provider_dxpr from info.yml dependencies. Moved to suggest in composer.json. The fallback code path (direct API key mode) is now properly reachable.

8. ob_flush() guard — Fixed. Wrapped with if (ob_get_level() > 0).

10. Variable naming — Fixed. $useAiModule → $use_ai_module throughout.

Pushing back

5. Hardcoded 'dxpr' provider — This is intentional. The entire architecture of this PR (and dxpr_builder#4061) is specifically to route through the DXPR provider → Kavya API. The ai module's provider abstraction is useful when you want the site admin to pick a provider, but here we're building a specific product integration. The dxpr_builder controller does the exact same thing. Making this configurable would add complexity with no use case.

6. Duplicate logic in two classes — The shared logic is 3 lines (module check + URL generation + engine assignment). getDynamicPluginConfig() operates on per-editor config with fallbacks; getCkEditorConfig() operates on global config. They're different code paths that happen to share the same AI-module-detection snippet. Extracting a shared method would couple these two classes more tightly for negligible DRY benefit. Happy to revisit if the proxy setup logic grows more complex.

7. Rate limiting — This is a cross-cutting infrastructure concern, not specific to this endpoint. Drupal has contrib modules for rate limiting (flood_control, etc.), and it can be handled at the reverse proxy level. Adding endpoint-specific rate limiting here would be out of scope and inconsistent with how other Drupal AJAX endpoints work (including dxpr_builder's own AI endpoint, which also has no rate limiting).

9. Hardcoded model default 'kavya-m1' — This is just a safety-net fallback. In practice, the model always comes from the JS plugin's config (which gets it from getDynamicPluginConfig()). The default only triggers if the JS somehow sends a request without a model field. kavya-m1 is the correct default for the DXPR provider. Making this configurable adds config surface for a code path that shouldn't be hit.

11. jsonrpc comment — The inline comment already says "Disable JSON-RPC agent/status messages — they are not compatible with the openai-php/client library used by the JS plugin." I've expanded the comment slightly in the new commit to clarify it's specifically about agent/status messages.

[jjroelofs]

Update (e5679ad): Corrected my pushback on issue #5 — the controller now uses the default configured provider instead of hardcoding 'dxpr':

$default = $this->aiProviderManager->getDefaultProviderForOperationType('chat');
$provider = $this->aiProviderManager->createInstance($default['provider_id']);

This means any provider configured in /admin/config/ai/settings (DXPR, OpenAI, Anthropic, Ollama, etc.) will work. The model default also comes from the ai module config rather than being hardcoded. Returns a clear 503 error with instructions if no default provider is configured.

The DXPR-specific passthrough fields (providers, allowed_html_tags, etc.) are kept since they're safely ignored by non-DXPR providers.

[jjroelofs]

Reviewed current head eed0a07 with the DXPR Builder AI-module integration as a reference point.

Important baseline: because AI is a core feature in ckeditor_ai_agent, I think the hard dependency on ai:ai / drupal/ai is correct. I would not make the AI module optional here. Provider modules such as ai_provider_dxpr should remain optional/suggested and be configured through the AI module.

Findings I would treat as urgent before merge:

1. [P1] Settings UX still exposes and requires the legacy direct API setup.
   src/Form/AiAgentFormTrait.php still renders API Key, AI Engine/Model, and API Endpoint URL fields, with the global API key select required. That conflicts with the new architecture where site builders configure provider credentials and the default Chat provider at /admin/config/ai/settings. In practice this can mislead admins, block global settings saves unless a legacy key is selected, and makes the module feel half migrated.
   Suggested fix: replace the “Connection & Model Settings” section with an AI provider status panel plus a link to AI settings. Keep only CKEditor-agent-specific settings here. Legacy key_provider / apiKey / endpointUrl can remain for update hooks/backward compatibility if needed, but should not be active setup UI for the AI-module path.

2. [P1] Browser/stale CKEditor model config still overrides the AI module default model.
   The controller correctly starts with getDefaultProviderForOperationType('chat'), but then accepts $data->model and uses it over the AI module default. PHP config generation still injects a model, falling back to openai:gpt-4o, and the JS dxai fallback defaults to kavya-m1. This means a site configured for another AI provider/default model can accidentally send an incompatible model string through the proxy.
   Suggested fix: omit model from the CKEditor config/request by default and let the AI module default provider/model win. If per-editor model override is still desired, make it explicit, provider-aware, and documented.

3. [P1] Route access is protected, but the editor UI is not gated by the new permission.
   The proxy route now requires use ckeditor ai agent, which is good Drupal integration. However, plugin config always injects a tokenized endpoint, and the JS enables the AI plugin whenever endpointUrl exists. Users without the new permission can still see AI controls and only discover the issue after a 403.
   Suggested fix: include current-user permission/provider readiness in dynamic plugin config and omit or disable the endpoint/UI when the user cannot use the AI Agent. This matches the availability-gating pattern used in DXPR Builder.

4. [P2] The proxy drops some output security context sent by the client.
   The JS sends allowed_html_tags, allowed_html_classes, allowed_html_styles, allowed_html_attributes, and allows_all_* flags for dxai, but the controller currently forwards only tags/classes into provider configuration. That makes the server-side behavior less complete than the client expects.
   Suggested fix: forward the full allowlist contract where supported by the provider, or remove the unused client fields so the integration contract is explicit.

5. [P2] PR docs/test plan still mention an optional-AI fallback.
   The PR body/test plan still references verifying behavior when the ai module is not installed / direct API-key mode. That is now stale. Since AI is required for this module, the relevant fallback is “AI module installed but no chat-capable provider/default provider configured,” which the new requirements check handles well.

The strong parts of this PR: server-side routing, CSRF protection, no browser API key exposure, default-provider lookup, SSE-compatible streaming, and provider diagnostics are all directionally consistent with the DXPR Builder AI integration. The remaining work is mostly making the admin/editor UX and config contract fully match that architecture.

[jjroelofs]

Review of latest commits

The new commits have addressed the major issues from the earlier review: the hard dependency contradiction is resolved (ai module is now fully required, fallback code removed), the adapter indirection is gone (controller uses AiProviderPluginManager directly), the AiAgentKeyService is cleaned up, and there's a solid functional test covering access control.

A few items that still apply:

1. Duplicate getTokenizedProxyEndpointUrl()

The identical method exists in both src/Plugin/CKEditor5Plugin/AiAgent.php and src/AiAgentConfigurationManager.php. Consider extracting it to a shared trait or having one class delegate to the other.

2. Stream default logic readability

In AiChatController::chat():

$is_streamed = !empty($data->stream) || !isset($data->stream);

This is functionally correct (streaming on by default, disabled when stream is explicitly falsy), but the intent reads more clearly if the "default on" case comes first:

$is_streamed = !isset($data->stream) || !empty($data->stream);

Minor, but the current ordering puts the override case before the default case, which requires a double-take to parse.

3. loadInclude call in form trait

In AiAgentFormTrait, the provider status panel calls:

\Drupal::moduleHandler()->loadInclude('ckeditor_ai_agent', 'install');
$check = _ckeditor_ai_agent_check_ai_provider();

Calling a procedural .install function from inside a form trait works, but couples the form rendering to the install file. If _ckeditor_ai_agent_check_ai_provider() were a method on a service (or on the configuration manager, which is already injected in both calling contexts), it would be testable in isolation and avoid the loadInclude pattern.

4. Unrelated changes bundled in

These are fine individually, but add noise to the diff:

• Indentation-only fixes in AiAgentFormTrait.php (array alignment)
• @phpstan-consistent-constructor annotation + self to static in AiAgentSettingsForm.php
• CI linter script overhaul (prepare-drupal-lint.sh, run-drupal-check.sh, review.yaml)

The CI changes are arguably related (needed for the drupal/ai dependency in static analysis), but the formatting fixes could be a separate commit to keep the feature diff focused.

None of these are blockers.