Access, tenant and credential checks that can be bypassed

Problem

Seven access-control defects found by reading every file in the module. Each is a way for a user to reach data or an action the configuration does not grant them. They are grouped because they share one shape: a check exists nearby, and the value that decides the outcome is not the value that was checked.

11 findings: 6 high, 3 med, 2 low.

High severity

The task screen takes its entity form operation from an unvalidated query arg, so ?op=delete gives a task candidate a working delete form after only an update access check

Where: modules/orchestra_content/src/Controller/EntityTaskController.php:81-85

op is a raw query value handed straight to EntityFormBuilder::getForm(), but the only entity-level gate is hardcoded to the update operation. Any form operation the entity type declares is therefore reachable under an update check. node declares 'delete' => NodeDeleteForm::class (core Node::$handlers['form']), and NodeDeleteForm extends ContentEntityDeleteForm to ContentEntityConfirmFormBase to ContentEntityForm, i.e. it is a ContentEntityFormInterface, so OrchestraContentFormHooks::formAlter() decorates it too: it relabels the confirm button to "Save" and appends a "Save and outcome" button per outcome, on a confirm form whose submit handler is ContentEntityDeleteForm::submitForm(), which calls $entity->delete(). The controller's own docblock ("Re-apply it here, so task candidacy alone cannot grant edit rights on content the viewer may not otherwise edit") is true only for op=edit; for every other operation the check is the wrong one.

What goes wrong: a user holds process orchestra tasks and edit any page content but not delete any page content, and is a candidate on a task carrying the entity_form interaction. The interaction redirects their browser to the task URL with op=edit, so the whole URL is in their address bar. They change it to op=delete, the controller passes the update check, and the delete form is returned. Pressing the relabeled button deletes content they have no delete permission for.

Untrusted process-variable text is pre-rendered into a body that Easy Email then token-renders again, so injected token syntax is evaluated

Where: modules/orchestra_easy_email/src/EventSubscriber/EasyEmailNotificationSubscriber.php:215-248

Orchestra resolves the orchestra token family first and writes the raw variable value into the subject and both bodies. Easy Email then runs its own token pass over the same strings twice more, so any token syntax a variable's value contained is evaluated by those later passes, including the easy_email family that Easy Email's own containsUnsafeTokens() and replaceUnsafeTokens() machinery exists to gate. Because Orchestra's pass runs before that check, injected text is indistinguishable from author-written template text at the point Easy Email decides what is unsafe.

What goes wrong: a webform interaction captures a public submitter's field into a process variable, and an operator places that variable's token in a mail template body, which the README documents and encourages. The submitter types an easy_email one-time-login or cancel-url token into the field. Orchestra inserts it literally, Easy Email's gate then finds it, splits the send per recipient and resolves it against that recipient. The notification mails each recipient a live login or account-cancellation URL for their own account, inside a legitimate-looking Orchestra email.

The operation resumer's access check is not tenant-scoped, so a client-supplied continuation handle resumes another tenant's step

Where: modules/orchestra_interaction_operation/src/Gateway/OperationResumer.php:73-87

This is the whole authorization on the continuation-handle path, and it never compares the token's tenant with the current one. Both siblings do: orchestra_ui/src/Access/OperationAccessCheck.php:88 refuses a token whose tenant differs from the current tenant, and the task twin delegates to WorkItemManager::checkActionAccess(), whose first branch is belongsToCurrentTenant() (src/WorkItemManager.php:620-622). AssignmentMatcher::matches() (src/AssignmentMatcher.php:369-375) intersects node candidates with the viewer's tokens, and viewer tokens are global. Nothing in that chain reads a tenant. OperationAccessCheck's own docblock states the three conditions separately, including "belongs to the current tenant", which is the proof the matcher does not cover it.

What goes wrong: the binding element is a Hidden webform element with prepopulate enabled, so its value is client-supplied, and handles are a bare sequential integer. A user holding a global role that matches a node's role audience in another tenant opens the form with that tenant's handle in the query string. The handler binds the submission to the other tenant's instance and advances its run. A cross-tenant read and write.

Only the delegate's right to act is ever checked; the delegator's never is, so cover outlives the delegator's own permission

Where: modules/orchestra_delegation/src/Form/DelegationForm.php:150-167 (creation) and src/WorkItemManager.php:619-635 (the live gate)

The form deliberately refuses a delegate who cannot process tasks, with a written reason. Nothing anywhere, not the form, the resolver, or the act-time gate, asks whether the delegator may act on their own work. The two delegation permissions are independent of process orchestra tasks, so the asymmetry is reachable both at creation and later by revocation: a delegator can lose that permission while their standing cover keeps working, each completion recorded on behalf of a person who may no longer act at all.

InstanceReference and InstanceTraceLink render a per-viewer access decision and declare no cacheability at all

Where: modules/orchestra_views/src/Plugin/views/field/InstanceReference.php:61-79 and InstanceTraceLink.php:56-71, both resolving through InstanceLinkFieldBase.php:179-185

Url::access() throws away all cacheability. The route it checks is gated by InstanceReadAccessCheck::access(), which returns cachePerUser() plus the work-item list tag, a per-uid tag, the instance, the matcher and the read-access config. So the column varies per uid, not per permission set, and goes stale when a step parks on somebody. DisplayPluginBase::calculateCacheMetadata() only merges handlers implementing CacheableDependencyInterface, so a display carrying these fields declares none of it. The same-module siblings all do declare it: InstanceActionLink, WorkItemHolder, and inbox_views' TaskActionLink and TaskOperations, which spell out the reason in a comment. Two handlers doing the work, two not.

What goes wrong: an operator builds a shared view with the Reference column and the default tag-based cache, with no per-user filter. The display's computed contexts contain no user context, so the first viewer's rows are cached and served to the next, who gets a live link to a run they may not read, or sees their own run rendered as dead plain text.

ResumeTokenSubscriber resumes a token in any tenant; both sibling resume paths confine to the active tenant

Where: src/EventSubscriber/ResumeTokenSubscriber.php:42-47

This is the @api, integration-neutral resume seam (src/Event/ResumeTokenEvent.php:12-17, docs/architecture.md:124-127) and it does no tenant check. The two other paths doing the same job both check: orchestra_eca/src/Plugin/Action/OrchestraResume.php:90-98 confines to the active tenant with a written reason, and orchestra_api/src/LocalOrchestraClient.php:79-84 rejects a token whose tenant differs from the current one. The event carries only an opaque token id, so the subscriber has everything it needs to apply the same guard and does not. Nothing in this repository dispatches the event today, so it is not exploitable as shipped; it is the documented seam an integration is told to use.

The rest

One line each: what it is, and where. The failing scenario and the fix for each are carried by its own commit.

• med A permission described as "reassign" also confers completing anyone's task. src/WorkItemManager.php:625-627, modules/orchestra_inbox/orchestra_inbox.permissions.yml:5-8
• med The delegation entity's access handler has no tenant gate, unlike every other Orchestra entity operation. modules/orchestra_delegation/src/DelegationAccessControlHandler.php:30-46
• med "Require a comment" is enforced from a user-editable query argument, so a visitor removes it from the URL. modules/orchestra_interaction/src/Form/CommentForm.php:125 and :152-158, minted at CommentInteraction.php:84
• low An unknown op value is an uncaught InvalidPluginDefinitionException, i.e. a 500 on a plain query-string edit. modules/orchestra_content/src/Controller/EntityTaskController.php:84-85
• low The anonymous account counts as "the assignee" on the two permission-free routes. modules/orchestra_inbox/src/Controller/InboxController.php:290-303, with orchestra_inbox.routing.yml:57-78

Remaining tasks

• Fix the findings above, one commit per finding where they are independent.
• Add or correct a test for every behaviour change.
• Run every linter and the affected test classes before pushing.

This issue summary was drafted with the assistance of an AI agent (Claude). The analysis and the wording were reviewed by me before posting, and accountability for the content is mine.