From c698953b91f685caec8a41638aa0d5b760b839ed Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Sat, 24 Oct 2026 20:47:50 +0200 Subject: [PATCH] Stop masked values from landing on new nodes and edges The AI chat and MCP see condition literals and other redacted config as "***". When that value is copied onto a node or edge with a new id, there is no stored original to restore it from, and "***" used to be saved as a literal. A condition like status == "***" then never matches. The chat merge now adds a note for every such mask so the user sees it in the review. The MCP patcher rejects the whole operation with the key and item id, because MCP saves without a review step. A mask on a named secret key keeps the existing "set it manually" handling. The assistant prompt now says that "***" only stands for the value under the same id. Fixes #582 Co-Authored-By: Claude Opus 5.5 --- docs/mcp-server.md | 4 +- src/NodePilot.Ai/Prompts/assistant-system.md | 5 +- src/NodePilot.Ai/WorkflowDefinitionMerge.cs | 20 +++++- .../Mapping/WorkflowDefinitionPatcher.cs | 42 +++++++++---- src/NodePilot.Mcp/Tools/WorkflowEditTools.cs | 8 ++- .../content/de/mcp-server.md | 4 +- .../content/en/mcp-server.md | 3 +- .../WorkflowDefinitionMergeTests.cs | 55 ++++++++++++++++ .../Mapping/WorkflowDefinitionPatcherTests.cs | 62 +++++++++++++++++++ 9 files changed, 182 insertions(+), 21 deletions(-) diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 17137d9d..89f1877f 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -104,7 +104,9 @@ before retrying a start whose response was lost. - On `publish_workflow`/`update_workflow_definition`/`apply_workflow_patch`, real secrets are **restored from the stored version** by node id (including masks nested in condition/case arrays) — the agent's `***` never overwrites them, and a - secret the agent invents on a new node is rejected (masked + noted). + secret the agent invents on a new node is rejected (masked + noted). A `***` with no stored + value behind it (for example a condition literal copied onto a new edge id) rejects the whole + operation, because it would otherwise be saved as the literal `***`. - Credentials carry no password field; secret global values arrive masked. Create/update accept secrets write-only. diff --git a/src/NodePilot.Ai/Prompts/assistant-system.md b/src/NodePilot.Ai/Prompts/assistant-system.md index 88311a1a..612543ec 100644 --- a/src/NodePilot.Ai/Prompts/assistant-system.md +++ b/src/NodePilot.Ai/Prompts/assistant-system.md @@ -50,7 +50,10 @@ Rules: never rewrite or reconstruct it. Never invent a secret value and never set one — if a change would require a secret (a password, API key, webhook secret, connection string), leave it as `***` and tell the user in your reply to set it manually on the - node afterwards. + node afterwards. A `***` only stands for the value stored under the same node or edge + `id`. Never copy a `***` onto a new node or edge (a new `id`), for example when you + replace one edge with two: there is nothing to restore, so `***` would be saved + literally. Ask the user for the real value instead. 4. Keep the definition structurally valid: unique ids, every edge `source`/`target` referencing a real node, the trigger preserved unless explicitly told otherwise. diff --git a/src/NodePilot.Ai/WorkflowDefinitionMerge.cs b/src/NodePilot.Ai/WorkflowDefinitionMerge.cs index ab79481a..03591e72 100644 --- a/src/NodePilot.Ai/WorkflowDefinitionMerge.cs +++ b/src/NodePilot.Ai/WorkflowDefinitionMerge.cs @@ -122,6 +122,8 @@ private static void MergeObject(JsonObject target, JsonObject? source, List notes) + { + if (WorkflowSecretKeys.SecretConfigKeys.Contains(key)) + { + notes.Add($"Secret '{key}' bitte manuell am Node setzen — die KI darf keine Secrets vergeben."); + return; + } + notes.Add($"'{key}' von '{itemId ?? "?"}' enthält '***' ohne gespeicherten Originalwert — bitte den echten Wert im Designer setzen, sonst wird '***' wörtlich gespeichert."); + } + // Null and the empty string count as "no secret set". private static bool HasValue(JsonNode? node) => node is not null && !(node is JsonValue value && value.TryGetValue(out string? text) && string.IsNullOrEmpty(text)); @@ -206,9 +220,9 @@ private static void MergeNestedArray( MergeNestedArray(targetArray, sourceItem as JsonArray, notes, itemId, key); break; case JsonValue targetValue when targetValue.TryGetValue(out string? value) - && value == SecretMask - && sourceItem is not null: - target[index] = sourceItem.DeepClone(); + && value == SecretMask: + if (sourceItem is not null) target[index] = sourceItem.DeepClone(); + else NoteMaskWithoutOriginal(key, itemId, notes); break; } } diff --git a/src/NodePilot.Mcp/Mapping/WorkflowDefinitionPatcher.cs b/src/NodePilot.Mcp/Mapping/WorkflowDefinitionPatcher.cs index 04d28b5b..32239147 100644 --- a/src/NodePilot.Mcp/Mapping/WorkflowDefinitionPatcher.cs +++ b/src/NodePilot.Mcp/Mapping/WorkflowDefinitionPatcher.cs @@ -114,8 +114,9 @@ private static JsonArray MergeFullArray(JsonElement proposed, string arrayName, { if (item.ValueKind != JsonValueKind.Object || JsonNode.Parse(item.GetRawText()) is not JsonObject obj) continue; JsonObject? source = null; - if (IdOf(obj) is { } id) originalById.TryGetValue(id, out source); - MergeObject(obj, source, notes); + var id = IdOf(obj); + if (id is not null) originalById.TryGetValue(id, out source); + MergeObject(obj, source, notes, id); result.Add(obj); } return result; @@ -137,7 +138,7 @@ private static void UpsertById(List list, JsonObject proposed, List< ?? throw new ArgumentException("upsert item must have a non-empty string 'id'."); var existing = list.FirstOrDefault(x => IdOf(x) == id); var merged = (JsonObject)proposed.DeepClone(); - MergeObject(merged, existing, notes); + MergeObject(merged, existing, notes, id); if (existing is null) list.Add(merged); else @@ -146,7 +147,7 @@ private static void UpsertById(List list, JsonObject proposed, List< // Backfill untouched fields from the original + protect secret keys (adapted from the API // merge). - private static void MergeObject(JsonObject target, JsonObject? source, List notes) + private static void MergeObject(JsonObject target, JsonObject? source, List notes, string? itemId) { if (source is not null) foreach (var (key, sourceVal) in source) @@ -166,6 +167,8 @@ private static void MergeObject(JsonObject target, JsonObject? source, List notes) + { + if (WorkflowSecretKeys.SecretConfigKeys.Contains(key)) + { + notes.Add($"Secret '{key}' must be set manually — the agent may not assign secret values."); + return; + } + throw new ArgumentException( + $"'{key}' on '{itemId ?? "?"}' is '***' but has no stored value to restore. Masked values cannot be copied onto new nodes or edges; send the real value instead. Nothing was saved."); + } + // Null and the empty string count as "no secret set". private static bool HasValue(JsonNode? node) => node is not null && !(node is JsonValue value && value.TryGetValue(out string? text) && string.IsNullOrEmpty(text)); @@ -217,7 +234,8 @@ private static bool IsMask(JsonNode? node) // Nested condition/case arrays have no stable item identity. If a redacted array still // contains a mask, retain the whole original array so a reorder cannot attach a secret to the // wrong case. Otherwise recurse by index to keep named-secret protection for nested objects. - private static void MergeNestedArray(JsonArray target, JsonArray? source, List notes) + private static void MergeNestedArray( + JsonArray target, JsonArray? source, List notes, string? itemId, string key) { if (source is not null && ContainsMask(target)) { @@ -234,15 +252,15 @@ private static void MergeNestedArray(JsonArray target, JsonArray? source, List CreateWorkflow( RequireFullDefinition(definition); // No original to MergeFull against an empty def masks any secrets the agent set on new // nodes. - var merged = WorkflowDefinitionPatcher.MergeFull(EmptyDefinition, definition); + WorkflowDefinitionPatcher.PatchResult merged; + try { merged = WorkflowDefinitionPatcher.MergeFull(EmptyDefinition, definition); } + catch (ArgumentException ex) { throw new McpException(ex.Message); } var mergedElement = JsonSerializer.SerializeToElement(merged.Definition); var validation = WorkflowDefinitionStructuralValidator.Validate(mergedElement); if (!validation.IsValid) @@ -295,7 +297,9 @@ private async Task SaveFullAsync( RequireFullDefinition(definition); var wf = await ResolveAsync(idOrName, ct); var current = ParseDef(wf.DefinitionJson); - var merged = WorkflowDefinitionPatcher.MergeFull(current, definition); // restore secrets from current + WorkflowDefinitionPatcher.PatchResult merged; + try { merged = WorkflowDefinitionPatcher.MergeFull(current, definition); } // restore secrets from current + catch (ArgumentException ex) { throw new McpException(ex.Message); } var mergedElement = JsonSerializer.SerializeToElement(merged.Definition); var validation = WorkflowDefinitionStructuralValidator.Validate(mergedElement); if (!validation.IsValid) diff --git a/src/nodepilot-docs-ui/content/de/mcp-server.md b/src/nodepilot-docs-ui/content/de/mcp-server.md index 6400896d..40ff6fbe 100644 --- a/src/nodepilot-docs-ui/content/de/mcp-server.md +++ b/src/nodepilot-docs-ui/content/de/mcp-server.md @@ -81,7 +81,9 @@ Transport ist **stdio** (Streamable HTTP ist als spätere Option vorgesehen). Wi - **Secrets:** Workflow-Definitionen werden vor Tool-Output redigiert (`secret`/`apiKey`/`password`/ `authToken`/`bearer`/`connectionString` → `***`). Bei `publish`/`update`/`apply_workflow_patch` werden echte Secrets per Node-ID aus der gespeicherten Version wiederhergestellt — das `***` des - Agenten überschreibt nie einen echten Wert. Credentials/Globals geben Secrets nie aus. + Agenten überschreibt nie einen echten Wert. Ein `***` auf einer neuen Node- oder Edge-ID hat kein + Original, deshalb wird der Aufruf abgelehnt und nichts gespeichert. Credentials/Globals geben + Secrets nie aus. - **Annotations:** Read-Tools `readOnly`, gated Tools `destructive`, enable/disable/lock `idempotent`. `execute_workflow` ist **nicht idempotent**: ein erneuter Aufruf startet eine weitere Ausführung. Einen unklar beantworteten Start zuerst über die Ausführungen prüfen. diff --git a/src/nodepilot-docs-ui/content/en/mcp-server.md b/src/nodepilot-docs-ui/content/en/mcp-server.md index 41a78fd5..045d53c9 100644 --- a/src/nodepilot-docs-ui/content/en/mcp-server.md +++ b/src/nodepilot-docs-ui/content/en/mcp-server.md @@ -78,7 +78,8 @@ The transport is **stdio** (streamable HTTP is planned as a later option). Windo - **Secrets:** workflow definitions are redacted before tool output (`secret`/`apiKey`/`password`/ `authToken`/`bearer`/`connectionString` → `***`). On `publish`/`update`/`apply_workflow_patch`, real secrets are restored by node ID from the stored version — the agent's `***` never overwrites a real - value. Credentials/globals never emit secrets. + value. A `***` on a new node or edge ID has nothing to restore, so the call is rejected and nothing + is saved. Credentials/globals never emit secrets. - **Annotations:** read tools are `readOnly`, gated tools `destructive`, and enable/disable/lock are `idempotent`. `execute_workflow` is **not idempotent**: calling it again starts another run. Check executions before retrying a start whose response was lost. diff --git a/tests/NodePilot.Ai.Tests/WorkflowDefinitionMergeTests.cs b/tests/NodePilot.Ai.Tests/WorkflowDefinitionMergeTests.cs index e54c3d54..67d43065 100644 --- a/tests/NodePilot.Ai.Tests/WorkflowDefinitionMergeTests.cs +++ b/tests/NodePilot.Ai.Tests/WorkflowDefinitionMergeTests.cs @@ -419,4 +419,59 @@ public void Merge_MaskedCaseWithOmittedKeys_AddsNoNote() .GetValue().Should().Be("prod"); result.Notes.Should().BeEmpty(); } + + // The AI replaced edge-1 by two new edges and copied its masked literal operand onto both. + private const string EdgeSplitOriginal = """ + { "nodes": [], "edges": [{ "id": "edge-1", "source": "a", "target": "b", "data": { + "conditionExpression": { "type": "comparison", "left": { "kind": "variable", "value": "status" }, + "op": "==", "right": { "kind": "literal", "value": "ready" } } } }] } + """; + + [Fact] + public void Merge_MaskedLiteralCopiedOntoNewEdge_KeepsMaskAndNotes() + { + var proposed = Parse(""" + { "nodes": [], "edges": [ + { "id": "edge-2", "source": "a", "target": "j", "data": { "conditionExpression": { + "type": "comparison", "left": { "kind": "variable", "value": "status" }, + "op": "==", "right": { "kind": "literal", "value": "***" } } } }, + { "id": "edge-3", "source": "a", "target": "k", "data": { "conditionExpression": { + "type": "group", "op": "AND", "children": [{ "type": "comparison", + "left": { "kind": "variable", "value": "status" }, + "op": "!=", "right": { "kind": "literal", "value": "***" } }] } } } + ] } + """); + + var result = WorkflowDefinitionMerge.Merge(Parse(EdgeSplitOriginal), proposed); + + result.Notes.Should().HaveCount(2); + result.Notes[0].Should().Contain("'value'").And.Contain("'edge-2'").And.Contain("***"); + result.Notes[1].Should().Contain("'value'").And.Contain("'edge-3'"); + } + + [Fact] + public void Merge_MaskedValueInNewArrayItem_Notes() + { + var proposed = Parse(""" + { "nodes": [{ "id": "new-node", "type": "activity", "data": { "activityType": "runScript", + "config": { "arguments": ["-Force", "***"] } } }], "edges": [] } + """); + + var result = WorkflowDefinitionMerge.Merge(Parse("""{ "nodes": [], "edges": [] }"""), proposed); + + result.Notes.Should().ContainSingle().Which.Should().Contain("'arguments'").And.Contain("'new-node'"); + } + + [Fact] + public void Merge_MaskedNamedSecretOnNewNode_NotesManualSetting() + { + var proposed = Parse(""" + { "nodes": [{ "id": "new-node", "type": "activity", "data": { "activityType": "restApi", + "config": { "password": "***" } } }], "edges": [] } + """); + + var result = WorkflowDefinitionMerge.Merge(Parse("""{ "nodes": [], "edges": [] }"""), proposed); + + result.Notes.Should().ContainSingle().Which.Should().Contain("Secret 'password'"); + } } diff --git a/tests/NodePilot.Mcp.Tests/Mapping/WorkflowDefinitionPatcherTests.cs b/tests/NodePilot.Mcp.Tests/Mapping/WorkflowDefinitionPatcherTests.cs index a3679b39..65bac2ef 100644 --- a/tests/NodePilot.Mcp.Tests/Mapping/WorkflowDefinitionPatcherTests.cs +++ b/tests/NodePilot.Mcp.Tests/Mapping/WorkflowDefinitionPatcherTests.cs @@ -321,4 +321,66 @@ public void MergeFull_MaskedLiteralsInsideNestedArrays_RestoreOriginalConditions merged["edges"]![0]!["data"]!["conditionExpression"]!["children"]![0]! ["left"]!["value"]!.GetValue().Should().Be("left-secret"); } + + private const string EdgeWithLiteral = """ + {"nodes":[],"edges":[{"id":"edge-1","source":"a","target":"b","data":{"conditionExpression":{ + "type":"comparison","left":{"kind":"variable","value":"status"},"op":"==", + "right":{"kind":"literal","value":"ready"}}}}]} + """; + + [Fact] + public void MergeFull_MaskedLiteralCopiedOntoNewEdge_Throws() + { + // edge-1 replaced by edge-2, which carries the mask the agent saw on edge-1. + var proposed = E(""" + {"nodes":[],"edges":[{"id":"edge-2","source":"a","target":"j","data":{"conditionExpression":{ + "type":"comparison","left":{"kind":"variable","value":"status"},"op":"==", + "right":{"kind":"literal","value":"***"}}}}]} + """); + + var act = () => WorkflowDefinitionPatcher.MergeFull(E(EdgeWithLiteral), proposed); + + act.Should().Throw().WithMessage("*'value' on 'edge-2'*no stored value*"); + } + + [Fact] + public void Apply_UpsertNewEdgeWithMaskedGroupOperand_Throws() + { + var ops = WorkflowDefinitionPatcher.ParseOps(E(""" + [{"op":"upsertEdge","edge":{"id":"edge-2","source":"a","target":"j","data":{"conditionExpression":{ + "type":"group","op":"AND","children":[{"type":"comparison", + "left":{"kind":"variable","value":"status"},"op":"!=","right":{"kind":"literal","value":"***"}}]}}}}] + """)); + + var act = () => WorkflowDefinitionPatcher.Apply(E(EdgeWithLiteral), ops); + + act.Should().Throw().WithMessage("*'value' on 'edge-2'*"); + } + + [Fact] + public void Apply_UpsertNewNodeWithMaskedArrayItem_Throws() + { + var ops = WorkflowDefinitionPatcher.ParseOps(E(""" + [{"op":"upsertNode","node":{"id":"new-node","type":"activity","data":{"activityType":"runScript", + "config":{"arguments":["-Force","***"]}}}}] + """)); + + var act = () => WorkflowDefinitionPatcher.Apply(E("""{"nodes":[],"edges":[]}"""), ops); + + act.Should().Throw().WithMessage("*'arguments' on 'new-node'*"); + } + + [Fact] + public void Apply_UpsertNewNodeWithMaskedNamedSecret_KeepsMaskWithNote() + { + var ops = WorkflowDefinitionPatcher.ParseOps(E(""" + [{"op":"upsertNode","node":{"id":"new-node","type":"activity","data":{"activityType":"restApi", + "config":{"password":"***"}}}}] + """)); + + var result = WorkflowDefinitionPatcher.Apply(E("""{"nodes":[],"edges":[]}"""), ops); + + result.Definition["nodes"]![0]!["data"]!["config"]!["password"]!.GetValue().Should().Be("***"); + result.Notes.Should().ContainSingle().Which.Should().Contain("Secret 'password'"); + } }