Access, tenancy and credential boundaries the audit found open

Problem/Motivation

Seven places where a boundary the module documents is not the boundary the code keeps. The tenant one is the most serious: nothing orders the tenant resolvers, so which tenant an authenticated API call acts in depends on module registration order.

Found by a complete file-by-file audit of the whole module at commit bc205f4f: 1130 tracked files, 1126 read in full, 4 binary assets recorded as skipped. Every linter was already clean at that commit, so none of this is something a tool reports: phpcs Drupal over 1008 files and DrupalPractice over 814 both at zero, PHPStan level 5 with no errors, cspell against the CI configuration over 1061 files with no issues, eslint with no errors, stylelint with no problems, composer validate, and the per-project translation check reporting every string translated in the catalog of the project that ships it.

Findings

High

1. Neither tenant resolver declares a tag priority, so a domain binding can override an OAuth consumer's tenant (modules/orchestra_server_api/orchestra_server_api.services.yml line 12, security). Both shipped tenant resolvers are tagged orchestra.tenant_resolver with no priority, while TenantContext's constructor docblock promises 'in priority order'; the iteration order is therefore incidental (module registration order), and DomainTenantResolver answers before ConsumerTenantResolver. An OAuth consumer bound to tenant A that calls /orchestra-api/* on a hostname bound to tenant B is served in tenant B - listing, reading, starting, signalling and writing variables on another tenant's instances - and ConsumerTenantResolver's documented fail-closed guarantee for an unbound or dangling consumer never runs. Fix: Give the consumer resolver an explicit high priority and the domain resolver an explicit low one, state the ordering contract on TenantResolverInterface and in docs/multi-tenancy.md, and pin it with a test.

Medium

1. The subprocess child-process select is not tenant-scoped (src/Plugin/TaskType/SubprocessTask.php line 205, security). buildConfigurationForm() offers every workflow on the site with no tenant filter, although the engine refuses at runtime to start a child not available in the parent's tenant, and neither the form nor its validate checks it. An administrator modelling a workflow scoped to tenant A picks tenant B's workflow as the child process; it saves cleanly and the first run to reach that node dead-letters with a LogicException instead of being refused while the author is looking at the select. Fix: Filter the options with appliesToTenant() against the edited workflow's tenant, as Status.php:86 already does for its own picker; failing that, refuse an out-of-tenant child in validateConfigurationForm().
2. The Workflow views filter offers every tenant's workflow labels (modules/orchestra_views/src/Plugin/views/filter/WorkflowFilter.php line 67, security). getValueOptions() calls loadMultiple() on orchestra_workflow with no tenant scoping, although a workflow carries a tenant and the sibling InstanceStatus filter in the same directory scopes its option list to the acting tenant by default. A tenant-scoped operator opening a dashboard that exposes the Workflow filter reads the human labels of every other tenant's workflows in the dropdown; the query stays scoped so selecting one returns nothing, but the names have been disclosed. Fix: Give WorkflowFilter the same limit_to_tenant option defaulting to TRUE, and the orchestra_tenant cache context, as InstanceStatus.
3. The comment page carries the bearer capability handle in its URL with no Referrer-Policy (modules/orchestra_interaction/src/Plugin/Interaction/CommentInteraction.php line 81, security). CommentInteraction::respond() redirects to /interaction/comment?handle=(signed capability token), but orchestra_interaction.comment is the one public credential-bearing route that never gets the Referrer-Policy: no-referrer header the dispatcher applies to its two siblings. An external party on the comment page whose theme pulls a third-party resource, or who clicks any off-site link, sends that third party a Referer containing a live 30-day branch-scoped capability token, which alone is enough to complete the workflow step from anywhere. Fix: Attach Referrer-Policy: no-referrer to the CommentForm build and the redirect, or lift InteractionController::withNoReferrer() into a shared helper the comment surface also calls.
4. The money operations route is the only per-token operation route with neither the instance permission nor the tenant access check (modules/orchestra_payment/orchestra_payment.routing.yml line 6, security). orchestra_payment.token_payment_outcome requires only administer orchestra, while every sibling per-token operation requires 'access orchestra instances+administer orchestra' plus InstanceTenantAccessCheck::token, and the operations hook adds the links unconditionally. A tenant-scoped operator holding access orchestra instances is shown Mark payment paid and Mark payment failed on their own tenant's parked payment step and gets Access Denied on click. Fix: Gate the two links on $url->access(), or broaden the route with a tenant access check owned by this module.
5. Cover arranged by an administrator reaches both parties unattributed and unexplained (modules/orchestra_delegation/src/Controller/DelegationsController.php line 88, security). The Delegation entity stores no author, and neither personal listing renders the note column, so a delegation created by a holder of 'administer orchestra delegations' carries nothing on either party's page saying who arranged it or why. Both the delegator and the delegate see cover neither of them declared, with no name and no reason, which is exactly the case the administrative permission advertises. Fix: Add the Note column to both personal listings, and store the acting uid on the row at save time.

Low

1. An unknown resolved tenant id silently becomes the default tenant (src/TenantContext.php line 43, security). getCurrentTenant() treats a resolver that answers a tenant id whose config entity is missing exactly like one that returned NULL, falling through to the default tenant, and neither interface documents that, so the fail-closed decision is silently left to every connector. A site whose tenant binding goes dangling serves the default tenant's workflows and instances on that domain, with nothing in the logs or the UI saying the intended tenant is gone; the two shipped resolvers already disagree about how to guard it. Fix: Decide the policy at the seam: throw a TenantException on an answered-but-unloadable id, or state in TenantResolverInterface that an unknown id is a deferral so a connector knows it must fail closed itself.

Proposed resolution

One merge request, a commit per finding, each naming the finding it closes. Every defect carries a test that fails against the unfixed code. All seven are fixed.

What the merge request changes beyond the findings

Auditing the fixes turned up more of the same ground, and closing it properly needed a little more than the findings asked for.

• The subprocess scoping reaches both editors, not one. The Modeler API hands the model owner the id of the workflow being edited; the owner dropped it and built every task settings form on a bare form state, so the BPMN.io canvas still offered every tenant. Only the option list on that surface: the Modeler API validates a plugin form on a form state of its own making, which carries no model, so the refusal cannot fire there. A model not yet saved is treated as shared, which is what the other editor reads off a new workflow at the same moment.
• The referrer policy covers a refusal, which throws. The webform surface can refuse a party the step is not theirs to see, and the page is then built from the exception with the form array dropped, so the 403 was served from the same handle-bearing URL. The headers go on the exception, which core persists onto the page it builds. The dispatcher exit that had carried the policy since it was written now has a test.
• The dispatcher declared a narrower return type than it can answer with. InteractionController::routeToStep() promised a redirect or a render array, while InteractionInterface::respond() documents any Response; a contributed interaction answering with another kind would have raised a TypeError. The declaration now says what it hands on.
• Parameter naming. A member is camelCase in this project and a parameter or local is snake_case, and sixteen parameters had drifted. Seven were ours to name. The other nine implement modeler_api and core interfaces, where renaming breaks a named argument written against the interface. A sweep catches the next one, exempting a method marked inheritdoc, which is a rule rather than a list and so cannot go stale.
• The delegation listing is held to one read of the user table. Three accounts can appear on a row now that the author is named, and naming them a row at a time is the shape the inbox beside it had and lost. Five rows naming eleven accounts cost one read; a budget test refuses the growth.

Remaining tasks

None. Every linter the pipeline runs is clean, the impacted classes pass, and both manual lanes were played by hand so the next-major ones ran rather than reading as green while skipped.

User interface changes

Named per finding above. Two option lists lose the other tenants entries, the payment operations appear only for whoever can use them, and both personal delegation listings gain a Declared by column and the note. No new screens.

API changes

Pre-1.0, so a signature changes where the better shape needs it. DelegationInterface gains a required getCreatedById(), which is an addition to an interface marked api. InteractionController::routeToStep() widens its declared return type to what the interaction contract already promised. Delegation gains a created by base field, so reinstalling is the upgrade path and a row written before it names nobody. Three traits are added for rules that were being restated: the tenant a settings subform is edited in, the tenant limit shared by two views filters, and the no-referrer policy.

AI-Generated: Yes (Claude Code was used to run this audit, to draft this issue summary, and to write the fixes and their tests. I reviewed the findings against the source myself before posting, and every test was confirmed to fail against the unfixed code.)