Workflows: credentials inside a Conditional action's child actions are stored and returned in clear
- Dominant language
- C#
- Stars
- 6
- Forks
- 7
- Avg merge
- 4h 42m
- Merged PRs (30d)
- 307
Description
A Conditional action carries its child actions as JSON strings in its `ThenActions` and `ElseActions` parameters. Nothing that protects a credential looks inside them, so a child's `Secret`, `ApiKey`, `Password` or `Token` is stored in clear, copied onto every run in clear, and returned by the API in clear.
Pre-existing. #862 did not cause it: it widened encryption to every credential-named parameter, but only at the top level, the same place `Secret` was encrypted before.
## Where it happens
- `WebhookSigning.ProtectSecrets` walks `workflow.Actions[*].Parameters` only. `ThenActions` and `ElseActions` are not credential-named, so the JSON string holding the child parameters is saved untouched, and the startup migration skips it for the same reason.
- `ConditionalAction.RunAsync` deserialises the branch (`ConditionalAction.cs` around line 80) and calls `plugin.RunAsync(childAction.Parameters, ...)` (around line 119) with the child parameters exactly as stored. The runner's `WebhookSigning.UnprotectCredentials` only sees the parent's parameters.
- `WorkflowActionResponse.From` (`WorkflowResponse.cs` around line 55) builds the response with `WebhookSigning.WithoutSecret(a.Parameters)`, which drops credential-named keys at the top level only and returns `ThenActions` and `ElseActions` whole, child credentials included, to anyone who can read workflows.
- A child Webhook with a `Secret` can never send. Its Secret reaches `WebhookAction` in clear, `ISecretProtector.Unprotect` returns null, and the action refuses permanently with "not protected". A child with a plaintext `ApiKey` does work, which is exactly the value sitting in clear at rest.
## Fix direction
1. On protect (create and the startup migration): parse `ThenActions` and `ElseActions` on a Conditional, run the same credential encryption over each child's parameters (recursively, for a Conditional inside a branch), and write the JSON back. A branch that is not valid JSON is left as it is.
2. On read: `WorkflowActionResponse` drops credential-named keys inside the child JSON the same way it does at the top level, and says whether each child has a secret set.
3. Before running a child: `ConditionalAction` decrypts the child's credentials the way the runner does for a top-level action (everything but `Secret`, which Webhook decrypts itself), and fails the child with a message naming the parameter, never the value, when one will not decrypt.
## Done when
- A workflow created with a Conditional whose child has a `Secret` and an `ApiKey` stores neither in clear (asserted on the raw stored JSON), and a workflow stored that way before the fix is rewritten by the startup migration.
- `GET /api/workflows` and the create response contain neither value.
- The child Webhook signs with the original secret, and a child custom action receives the plaintext `ApiKey`, each proven by a test that fails before the fix.
Contributor guide
Research direction
Start with WebhookSigning.ProtectSecrets and UnprotectCredentials, then read ConditionalAction.cs around the branch deserialization and WorkflowResponse.cs around WorkflowActionResponse.From. Run the existing workflow and Conditional action tests if available. Done means recursive child credentials are protected during creation and migration, omitted from API responses, and child Webhook and custom actions receive the expected values without exposing them.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- api, backend, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100