MCP tools: security hardening and cleanup after review #18

Follow-up to #17. A security and implementation review of the MCP tools found the points below. The urgent ones are fixed in #17 (all MCP tools admin-only, mcp.deploy switch, destructiveHint on irreversible tools, api-platform/core ^4.3.18, nested {} in tool arguments). The rest is hardening and cleanup; none of it is exploitable with the default configuration (all switches off).

Security hardening

• Configuration lookup (FlowableClientLocator::resolveExplicit()): ignores active, so deactivated engines stay usable with their credentials; the prefix goes unescaped into LIKE (%, _); the error messages distinguish "not found", "type X" and "ambiguous" and name other configuration types, which makes a prefix oracle over all ApiConfigurations. Accept only [0-9a-f-]{8,36}, filter on type=flowable and active=true before matching, return one generic error. Add the pattern to the {uuid} branch of the input schemas too.
• Input schemas of read tools are advertised, not enforced: the handler runs with validate=false and the providers do not validate, so additionalProperties: false and the apiConfiguration pattern are documentation only for the read tools and flowable_dmn_deploy (uploadedResource()). Validate MCP arguments in the providers, or document the schema as advisory; the docblock of McpToolInputSchemaFactory claims the opposite.
• IDs from tool arguments are not validated (TimerJobExecuteProcessor, TaskCompleteProcessor, ExecutionTriggerProcessor, CompositeRowsTrait): . and .. survive rawurlencode() and HttpClient removes dot segments, so a caller can pick the upstream endpoint; an array id becomes "Array" with an E_WARNING. Add a shared helper: is_string plus ^[A-Za-z0-9][A-Za-z0-9:._-]*$.
• File name of single-file MCP uploads (AbstractFlowableProcessor::uploadedResource(), FlowableClient::multipartBody()): only the extension is checked; ", CR/LF, / and .. reach Content-Disposition unescaped. Reuse the name rule of DeploymentBundleProcessor::files(), extended by quotes and control characters, and escape in multipartBody().
• Upstream errors go to the agent unfiltered (FlowableApiException): engine exception bodies and transport errors with internal URLs end up in the JSON-RPC error. Return a generic message with a correlation id and log the details.
• Switches and the metadata cache: the switches are baked into the cached metadata. With %env(bool:...)% a switch turned off without cache:clear leaves the tools active. Reject env placeholders or document it.
• Size limits for inline content: mcpBundle.input.json has no maxItems/maxLength, and the payload is held in memory several times in DeploymentBundleProcessor. Add schema limits and a cap on the decoded bytes.
• .bar/.zip via flowable_dmn_deploy go to the engine unchecked. Allow only .dmn over MCP.
• Optional BPMN lint before deploy: reject or flag scriptTask, flowable:type="shell|http", flowable:class and expressions in deployments that come in over MCP.
• Audit: the audit context has no transport (via=mcp) and no tool name, and only successes are logged, so agent and human actions cannot be told apart. Add both; log attempts, with a sha256 of deployed content.
• Free-form deployment metadata: tenantId, category and deploymentSource in DeploymentBundleProcessor are free strings; deploymentSource can be spoofed. Prefix server-side or check against an allowlist (relevant for multi-tenant engines).
• Object-level access for reads: the HTTP read operations give every ROLE_USER all tasks and up to 200 variables of every instance, with no check on assignee, starter or tenant. MCP is admin-only now; the HTTP side still needs a voter or a dedicated reader role, and a variable denylist.
• McpToolInputSchemaFactory applies app-wide without the namespace guard of the switch factory; a broken path throws and takes down the whole tool registry; .. is not rejected.
• McpResource entries are not switched; future resources have no fail-closed default. Treat McpResource as read.
• Rate limiting: document a Symfony RateLimiter in front of /mcp in the README.
• Test: no test sends a real tools/call as ROLE_USER to an admin tool; McpToolMetadataTest compares the security expression as a string only. Add a kernel or handler test with access denied.

Implementation

• OperationInputTrait: read tools drop apiConfiguration in the {uuid} form silently and fall back to implicit resolution, which gives a misleading error with several engines. Move configurationId() into the trait.
• DeploymentBundleProcessor: only the last extension is checked, so rates.dmn.xml and case.cmmn.xml pass despite the documented .dmn rejection. Match full suffixes.
• FlowProcessStatusProvider / FlowProcessHistoryProvider: executions have no *Total (README says otherwise); history sorts ascending, so the 200-row cap drops the newest activities; decisionsTotal is computed from the first page.
• flowable_dmn_evaluate: the description promises rows as "a map of output name to value", but normalizeResult() passes the engine's resultVariables through (lists of {name, type, value}); idempotentHint: true holds only functionally, every call adds a history row. Fix the description or the normalisation; add a state test for the result shape.
• executeTimerJob(): verify against a live engine; "run now" needs an active async executor.
• CompositeRowsTrait::row() serialises by reflection and bypasses #[Groups]; normalise with the serializer and the read group.

Tests

• Switch decorator together with CachedResourceMetadataCollectionFactory and OperationMetadataFactory.
• A test through ApiPlatform\Mcp\Server\Handler instead of the hand-built context in StateTestCase.
• Multipart HTTP tests for DMN and event uploads, a test for TaskInputSchemaController, a MockHttpClient test for executeTimerJob().
• ext-zip in require-dev.

DRY

• Inline file decoding exists twice (DeploymentBundleProcessor::files() and uploadedResource()) with different checks.
• schemaPath() and McpToolInputSchemaFactory::path() build the same path.
• The apiConfiguration schema block is copied about 20 times and has already drifted (string vs oneOf); the pagination block 3 times.
• Advertised schema (extra properties) and enforced schema (schemaPath(...)) are separate references; a test should check they match.

Documentation

• "failures are tool errors" in several tool descriptions is wrong: the handler returns JSON-RPC errors, not isError.
• README: oneOf is dropped from the input schemas as well, not only allOf/anyOf.
• structuredContent is JSON-LD while the outputSchema is built for json; set outputFormats or document it.
• The priority comment in config/services.yaml for the switch decorator is ambiguous.
• The MCP branches in DeploymentUploadProcessor and EventDeploymentUploadProcessor are used by no tool yet; the processDefinition filter on FlowExternalWorkerJob is unrelated to MCP.