Access, tenancy and credential boundaries the audit found open

Fixes all seven findings on #3621290.

The tenant resolvers had no order

TenantContext takes the first resolver that answers, and its constructor docblock promises they arrive "in priority order". Neither shipped resolver declared a priority, so the order was whatever the container happened to build — module registration order, weight then name, which put orchestra_domain ahead of orchestra_server_api for no reason anybody chose.

On a site running both connectors, an OAuth consumer bound to tenant A that called a hostname bound to tenant B was served in tenant B: it listed, read, started, signalled and wrote variables on another tenant's instances. ConsumerTenantResolver's documented fail-closed rule for an unbound or dangling binding never ran, because the resolver was never reached.

The consumer's own binding now outranks the request's host (100 against 0). TenantResolverInterface states the scale so a contributed resolver knows where it lands, and now says out loud what was previously silent: a resolver answering an id no tenant matches is treated as a deferral, so a binding that must never widen to the default tenant has to throw, as the consumer resolver does. That closes the "unknown tenant id silently becomes the default" finding at the same seam.

Pages that carry a capability handle, and the two that let it out

/interaction/comment?handle=… carries a signed capability token — whoever holds it can complete the step, from anywhere, until it expires. The dispatcher has sent Referrer-Policy: no-referrer since it was written; this page was added later and went without, so any off-site link or resource on it handed a live token to a third party. Counting the pages is what kept going wrong here — the trait's old docblock called the dispatcher "three routes" when it is two, with three exits — so the rule is stated by the handle now, not by a list.

The policy was a private method on InteractionController, which is why nothing outside that controller could use it. Lifted to NoReferrerTrait and applied to both of the comment page's exits — the refusal matters as much as the form, since it is reached with the same URL and so carries the same handle.

The same hole again, on the webform. That surface attaches the policy to the form it returns — but it can also refuse a party the step is not theirs to see, and a refusal throws. The page is then built from the exception, the form array is dropped along with the policy on it, and the 403 is served from the same URL, still carrying the handle. The page that says the link is not yours was the one passing it on.

A surface that throws never holds a response to attach to, so it puts the headers on the exception: HttpException takes them as its fourth argument and core persists them onto the page it builds (DefaultExceptionHtmlSubscriber says so in as many words, and the JSON and final subscribers do the same).

A kernel.response subscriber reaches the refusal too, and is the obvious move — but it would fire on every response the site sends in order to place one header on one 403, and an early exit inside a global dispatch is not a narrow fix: the dispatch has already happened. The exception carries it instead, which costs nothing anywhere else.

NoReferrerTrait now names the policy once, in noReferrerHeaders(), and offers it in the two shapes a surface needs: attachNoReferrer() for a render array, which is a value the caller reassigns, and withNoReferrer() for a response, altered in place. Splitting that union out of the applier is what let PHPStan see through to routeToStep(), whose declared RedirectResponse|array had been narrower all along than the Response|array that InteractionInterface::respond() promises — a third-party interaction answering with any other Response would have hit a TypeError in the dispatcher. The declaration now says what it hands on.

The rest

• Workflow Views filter listed every tenant's workflow labels. The query was tenant-scoped either way, so the dropdown disclosed the names while returning nothing for them. Given the same limit_to_tenant option as the Status filter beside it, defaulting to scoped, plus the orchestra_tenant cache context — without it an exposed form cached under one tenant's domain is replayed under another's with the first tenant's names in it.
• Payment operations were offered to everyone who could open the instance trace, but the route behind them is administer-only, so a tenant-scoped operator got two buttons that answered 403. Gated on the route's own access rather than broadened, because the narrower permission is deliberate.
• Delegation attribution. The permission advertises declaring cover for somebody already unreachable, and neither of the two people such an arrangement binds could see who made it or why: no author was stored at all, and the note — whose field description promises it is "for whoever reads the list later" — was rendered only on the administrative list, the one page neither of them can open.

The subprocess child picker listed every tenant's workflows

WorkflowExecutor::start() refuses a definition that does not apply to the tenant the run would happen in, so a subprocess node naming a child from elsewhere can only dead-letter its first token. The picker listed every workflow on the site regardless, so that configuration was one click away and the list itself named what other tenants have.

The list is now scoped to the rule the engine already enforces, and a selection that breaks it is refused while editing, so the problem is reported to whoever can fix it instead of to a token. The child already stored stays on the list whatever tenant it belongs to, so opening the node shows what it names rather than resetting to none and quietly saving a pass-through. Note which way the rule falls for a shared parent: it runs in whichever tenant starts it, so the only child guaranteed to be reachable wherever it lands is a shared one.

PluginFormInterface hands a settings subform no context, which is what stopped this being scoped when it was written: node features get a $context carrying the edited tenant (Status.php uses it for exactly this), task-type settings get nothing. EditedTenantTrait carries that tenant across the subform boundary, and says what a caller does without it: leave the list unscoped rather than empty it. The boundary is the engine's; this is the guard-rail in front of it.

Both editors, not one. The Modeler API hands the owner the id of the model being edited; orchestra_modeler's owner dropped it and built every task's settings on a bare form state, so on the BPMN.io canvas — the primary authoring surface — the child select listed every tenant's workflow labels and let one be saved. The id is carried through now.

Only the list, on that surface: the Modeler API validates a plugin's settings on a form state of its own making, which carries no model, so the refusal cannot fire there. The handbook says which editor does what rather than claiming both refuse.

A model that is not saved yet has nothing stored to read a tenant off. A new workflow starts shared, so the canvas says so and offers only shared children — the same answer the workflow form reads off it at that moment. A model that is not new and still would not load says nothing about its tenant, and stating none leaves the list unscoped rather than guessing at it.

Tests, each run red against the unfixed code

Test	Unfixed result
TenantResolverPriorityTest (both cases)	array has the key 'priority' failed on orchestra_domain, then null is greater than null
CommentInteractionTest::testTheCommentPageDoesNotLeakItsHandleInTheReferer	array contains ['Referrer-Policy', 'no-referrer'] failed
CommentInteractionTest::testTheRefusalPageAlsoSendsNoReferer	the same, on the refusal exit
StateStatusViewsTest::testWorkflowFilterOptionsAreScopedToTheTenant	stops on the option's own default: null is true failed, so the filter is not scoped at all
StateStatusViewsTest::testWorkflowFilterCacheability	array contains 'orchestra_tenant' failed
PaymentOperationsTest::testOperationsAreNotOfferedToSomeoneWhoCannotUseThem	array does not have the key 'payment_paid' failed — it was offered
SubprocessTest::testChildOptionsAreScopedToTheEditedTenant	array does not contain 'other_child' failed — it was listed
SubprocessTest::testCrossTenantChildIsRefusedWhileEditing	two arrays are identical failed — no error raised, for either the scoped parent or the shared one
TaskConfigRoundTripTest::testTheCanvasScopesSettingsToTheEditedTenant	array does not have the key 'other_child' failed for a saved model; the new-model half is pinned against a targeted revert
HandlerViewGateTest::testTheRefusedPageDoesNotHandTheHandleOn	two arrays are identical failed — the refusal carried no headers
DelegationAccessTest::testListingDeclaresThePeopleItNames	array contains 'user:4' failed — the author was named on the page but not tagged

Every message above is the one the lane actually printed, read back off the run rather than written from memory.

The lane was also simulated locally, reverting every production file to 1.x and keeping the tests: nine of the eleven classes go red, with no fatals. The two that pass do so for stated reasons. DomainTenantResolverTest is the contract test below. InteractionAccessTest::testTheSignalRedirectSendsNoReferer is new coverage for behaviour that predates this branch — the dispatcher has sent the policy since it was written, and nothing pinned it, which is how the comment page beside it came to be added without one; it is red against a targeted revert of that one call, not against a whole-diff revert that restores the original line.

DelegationAccessTest::testTheListingNamesEveryPartyFromOneRead is a guard of the same kind, and the one thing here that is measured rather than argued. 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 this had and lost in #3621294, where the preload gathered only assignees and a page of delegated work paid a user load per row. Measured through the controller: five rows naming eleven accounts cost one read of the user table. Dropping the author back out of the preload takes it to six, which is what the budget refuses. It passes against a whole-diff revert, where there is no author to look up at all — it guards the next change, not this one.

Duplication was measured the same way rather than asserted: a clone detector run over the branch and over 1.x finds no clone site overlapping a line this branch adds. The other two paths it touches were measured too — the Workflow filter resolves the tenant once outside its loop and pays nothing at all when the limit is off, and the canvas's workflow read is absorbed by the config static cache (one read across three panel builds).

TenantResolverPriorityTest reads the shipped *.services.yml files, so it fails whether or not a site has both connectors installed, and catches a resolver added later that forgets a priority. It pins the numbers, not what they buy, so DomainTenantResolverTest registers a stub resolver at 100 and asserts it answers ahead of the host. That one is a contract test, not a red-to-green one: it is deliberately not in the table above, because it passes with the fix reverted. What it guards is the wiring, that TenantContext still consumes a !tagged_iterator and does not reorder it; strip the priority off the stub's own tag and it fails. The delegation case is a functional test, left to CI as this module's other functional tests are.

Verified

• phpcs on the module root at CI's extension list: Drupal and DrupalPractice, 829 files, 0 errors 0 warnings each
• PHPStan level 5: no errors, nothing suppressed and no baseline entry added
• scripts/check-translations.php: exit 0, every string this branch adds carries its French
• cspell against the CI config plus the project word list: no word new to this branch
• Both manual lanes were played on the head pipeline and read job by job: composer (next major), so phpunit (next major) and phpstan (next major) actually ran rather than reading as green while skipped, and test-only changes, where every case in the table goes red naming its own assertion, with no fatals. A test that reaches production code the fix adds would fatal there instead of failing, which proves nothing, so none of these do.
• Impacted classes green: StateStatusViewsTest, PaymentOperationsTest, CommentInteractionTest, DelegationAccessTest, DelegationVisibilityTest, DelegationResolverTest, SubprocessTest, DomainTenantResolverTest, TenantResolverPriorityTest, TaskConfigRoundTripTest, HandlerViewGateTest

Parameter naming, and a sweep so it stays fixed

A member is camelCase here and a parameter or local is snake_case, which is what tells a reader which of the two they are looking at. This branch had added one drift of its own — the modeler owner's private helper copied the camelCase from the public method above it, whose names come from modeler_api — and fixing that turned up sixteen across the module.

Seven were ours to name. The audience plugins and the views field take theirs and hand them straight to a parent that promotes them, so the parent is right to keep camelCase and the child is not. Constructor promotion is where the distinction goes quiet: WorkflowUninstallValidator holds three lines that look identical, two promoted properties and a third that is a plain parameter named after the property it feeds.

The other nine stay. They implement modeler_api's ModelerInterface and ModelOwnerInterface, and core's LockBackendInterface; renaming a parameter there breaks a named argument written against the interface and leaves the docblock disagreeing with the signature.

ParameterNamingTest catches the next one. It exempts a method marked {@inheritdoc} — a rule rather than an allow-list, so it cannot go stale — and reads tokens rather than matching text, because promotion is a visibility keyword in the signature and nothing else. Proved both ways: putting a parameter back names it with its line, and stripping an {@inheritdoc} exposes the parameter that marker was carrying. It walks through ShippedFilesTrait rather than its own iterator, which is where the knowledge lives that a sweep must never start at the module root — d.o CI builds the docroot inside the checkout, so a walk from there reads core as if it were this project.

Smaller corrections, none of them behavioural

• The tenant-limit trait took its checkbox description untyped, the only parameter in orchestra_views without a type, and both call sites pass a translatable. Typed TranslatableMarkup, which also keeps a plain untranslated string out of a label whoever configures the view reads.
• The two filters that now share that trait declared getCacheContexts() differently, one typed and one not, inside the same commit range. Core's parent declares no return type, so narrowing is legal and both classes are final.
• Gating the payment operations turned two flat blocks into a loop with a ternary picking the label — the one part that actually differs — so the dedup read worse than what it replaced and split the two translatable strings apart. The loop is keyed by outcome with the titles as its values now. Beside it, the delegation list asked each row for its author three times where one local does.

Note on the base, and on the @api surface

Rebased onto 1.x as it stands with #3621292, #3621294, #3621296 and #3621291 merged. One conflict along the way, in DelegationUiTest, where 1.x added a test and this branch added another at the same spot; both are kept.

Nothing 1.x has brought needs a fix here to extend to it, checked each time rather than assumed: the forms it added carry no handle and no tenant-scoped list, the views filters it changed are BooleanOperator rather than option lists, SubprocessTask is still the only task-type settings form naming a tenant-scoped entity, and the helpers it extracted (OrchestraLoggerTrait, DelegationListRedirectTrait, ReloadPaymentTrait) are none of them something this branch restates — it adds no log site and no reload site at all.

DelegationInterface is @api and gains a required getCreatedById(): int. Only Delegation implements it in tree, so nothing here breaks; a third-party implementor would. Pre-1.0, so the interface is broken rather than widened around.

Note on the new base field

Delegation gains a created_by base field. Pre-1.0, so no update hook: reinstalling is the upgrade path, and a row written before it reads as None rather than claiming an author it does not have.

Note on views already using the Workflow filter

A view's cacheability is exported config, and <=11.3 installs it verbatim, so the orchestra_tenant context the filter now declares reaches a saved view only when that view is re-saved. No shipped view uses this filter, so nothing in the module is affected; a site that built its own view on it should re-save it once after updating.