A code audit surfaced one functional bug, two config-schema gaps, and two low-severity security-hardening items. This issue fixes them together (each small and self-contained) with regression coverage. Written so another contributor can pick it up and verify independently.

1. Bug (functional) - task result ignores result_scope, always written token-scoped

Files: src/WorkflowExecutor.php, methods recordResult() (~line 826) and recordOutcome() (~line 860).

Cause: resultTarget() returns [?string name, VariableScope scope]. Both callers destructured the second element into a variable named "scoped" and used it as a boolean (scoped ? token : NULL). VariableScope is a PHP backed enum, and every enum case is truthy, so the ternary always took the token branch. VariableResolver::setVariable() treats a non-NULL token as a token-scoped write.

Effect: a node configured result_scope: instance had its result variable (and its message/reason variable in recordOutcome) written token-scoped anyway. Token-scoped variables are visible only to the writing branch and its descendants, so the result was invisible to sibling branches and dropped when a synchronizing join re-parented the continuing token. Affects any task recording a result via this path, including the ECA event task (modules/orchestra_eca/src/Plugin/TaskType/EcaEventTask.php) and all outcome-exception results/messages.

Fix: compare the enum explicitly, matching the pattern already correct in resumeParked() (~line 725): scope === VariableScope::Token ? token : NULL. Renamed the destructured var from "scoped" to "scope". No import change needed (same namespace).

2. Config schema - missing interaction settings schema

The node schema in config/schema/orchestra.schema.yml types interaction settings as orchestra.interaction.[%parent.plugin] (~line 258) with no wildcard fallback. Two shipped plugins had no schema, so any workflow using them fails strict config-schema validation (enforced by core in Kernel tests and on config save):

  • orchestra.interaction.comment - plugin modules/orchestra_interaction/src/Plugin/Interaction/CommentInteraction.php, settings required (bool), comment_variable (string). Added to modules/orchestra_interaction/config/schema/orchestra_interaction.schema.yml.
  • orchestra.interaction.entity_form - plugin modules/orchestra_content/src/Plugin/Interaction/EntityInteraction.php, settings form_operation (string), attachment_key (string). orchestra_content had no config/schema dir; added modules/orchestra_content/config/schema/orchestra_content.schema.yml.

3. Security (low) - capability token leak via Referer

File: modules/orchestra_interaction/src/Controller/InteractionController.php, methods step() and signal().

The public dispatcher routes (/interaction/{orchestra_instance} and /interaction/{orchestra_instance}/signal/{outcome}) are gated by a signed capability token in the "token" query arg (see InteractionToken). Since the token sits in the URL, any off-site link/resource on the rendered page would leak it via the Referer header. Added a private withNoReferrer() helper attaching Referrer-Policy: no-referrer to both render-array responses (#attached[http_header]) and redirect responses. Same-site navigation, which the token still authorizes, is unaffected.

4. Security (low) - client base URL allowed cleartext

File: modules/orchestra_client/src/Form/SettingsForm.php.

The base URL was a plain url element, so http:// was accepted; the OAuth client secret and bearer token travel there. Added validateForm() requiring https, excepting loopback hosts (localhost, 127.0.0.1, ::1, *.localhost) for local dev. Validation is form-only, so programmatic config writes (and existing tests) are unaffected.

Reviewed, intentionally not changed

  • subprocess retry node setting riding the orchestra.node_setting.* string fallback is correct: node config is a free map stored as strings by design and cast on read (SubprocessTask::retryLimit does an int cast). Declaring it integer would create a validation failure.
  • Remote API error messages in ApiController.php are a deliberate contract for authenticated, tenant-scoped consumers (not stack traces; 500s already sanitized).
  • Instance-scoped token 30-day TTL / non-revocation is a documented design trade-off; the Referer hardening addresses the main leak vector.

Tests

Added two Kernel tests to tests/src/Kernel/EngineOutcomeTest.php: testOutcomeResultHonorsInstanceScope() and testOutcomeResultHonorsTokenScope(), asserting the stored variable owning-token column is NULL for instance scope and non-NULL for token scope (new variableTokenId() helper). Pre-existing tests asserted only value-by-name and did not catch the bug. outcomeWorkflow() gained a result_scope param (defaults to instance).

Verify

  • EngineOutcomeTest: new tests pass with the fix, the instance-scope one fails if WorkflowExecutor is reverted.
  • phpcs (Drupal + DrupalPractice) and php -l clean on all changed files.

Issue fork orchestra-3610442

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

  • mably committed f3001e4d on 1.x
    fix: #3610442 Fix outcome result_scope bug, add missing interaction...
mably’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.