From fd3faed9c29d5f57d270c372e621832006d342ce Mon Sep 17 00:00:00 2001 From: Hacks4Snacks Date: Mon, 27 Jul 2026 14:45:28 -0500 Subject: [PATCH] support user edits of threat title + categories --- docs/cli-reference.md | 27 ++- docs/studio-guide.md | 10 ++ .../ThreatGenerator.cs | 137 ++++++++++++++- src/ThreatModelForge.Cli/McpAuthoringTools.cs | 10 +- src/ThreatModelForge.Cli/ThreatsCommand.cs | 37 +++- .../Authoring/AuthoringService.cs | 47 ++++- .../Authoring/EditThreatRequest.cs | 13 ++ src/ThreatModelForge.Engine/EngineService.cs | 2 + .../TmForgeJsonFormat.cs | 17 +- src/ThreatModelForge.Studio/src/App.css | 7 + .../src/dfd/Editor.tsx | 9 + .../src/dfd/ThreatsPanel.test.tsx | 45 +++++ .../src/dfd/ThreatsPanel.tsx | 42 +++++ .../ThreatTextEditingTest.cs | 166 ++++++++++++++++++ .../ThreatsCommandTest.cs | 56 ++++++ 15 files changed, 608 insertions(+), 17 deletions(-) create mode 100644 test/ThreatModelForge.Api.Tests/ThreatTextEditingTest.cs diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 333cb33..ccd4acd 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -712,7 +712,7 @@ lookup when needed. `--remove` deletes a manual or stale entry. Each operation t ```text tmforge threats --add --title --category [--id ] [--scope ] [--state ] [--priority

] [--mitigation ] [--description ] -tmforge threats --edit [--state ] [--priority

] [--mitigation ] [--description ] [--note ] +tmforge threats --edit [--title ] [--category ] [--state ] [--priority

] [--mitigation ] [--description ] [--note ] tmforge threats --remove tmforge threats --remove-stale [--force] [--rules ] ``` @@ -721,7 +721,9 @@ tmforge threats --remove-stale [--force] [--rules ] | --- | --- | | `--add` | Author a **manual threat** the rules do not detect. `--category` is a STRIDE category (`Spoofing` / `Tampering` / `Repudiation` / `InformationDisclosure` / `DenialOfService` / `ElevationOfPrivilege`); `--scope` is an element or flow id (omit for a model-wide threat). Manual threats are keyed in the reserved `manual:` namespace and do not implicitly persist generated threats. | | `--id ` | Key the threat yourself instead of taking a generated id, so it can be referenced from a ticket or control catalogue and the same authoring command can be re-run. Letters, digits, `-`, `_`, and `.`, up to 128 characters; the `manual:` prefix is added if you omit it. Re-using an existing id is an error — use `--edit` to change that threat. | -| `--edit ` | Change a threat's `--state` (`Open` / `NeedsInvestigation` / `Mitigated` / `Accepted`), `--priority` (`Critical` / `High` / `Medium` / `Low`), `--mitigation`, `--description`, or `--note`. Works on rule-derived and manual threats. | +| `--edit ` | Change a threat's `--title`, `--state` (`Open` / `NeedsInvestigation` / `Mitigated` / `Accepted`), `--priority` (`Critical` / `High` / `Medium` / `Low`), `--mitigation`, `--description`, or `--note`. Works on rule-derived and manual threats. `--category` applies to **manual threats only**. | +| `--title ` | Retitle a threat. On a rule-derived threat this is an **override**: the rule keeps detecting the threat and keeps its identity, but the register shows your wording. Pass an empty title to drop the override and restore the rule's. | +| `--category ` | Set a **manual** threat's STRIDE category. Refused for a rule-derived threat, whose category is the rule's conclusion rather than an author's opinion. | | `--remove ` | Delete a **manual** threat, or a **stale** one. A threat the rules still produce is refused: removing it would only bring it back on the next run — accept or edit it instead. | | `--remove-stale` | Delete every stale entry at once. Entries carrying triage are **kept and listed**; `--force` discards them too. Refuses outright if any entry's rule was not part of the run, naming the rules so you can re-run with `--rules` rather than lose a real finding. | @@ -729,10 +731,31 @@ tmforge threats --remove-stale [--force] [--rules ] tmforge threats app.tm7 --add --title "Admin actions are unlogged" --category Repudiation --scope --priority High tmforge threats app.tm7 --add --id unlogged-admin-actions --title "Admin actions are unlogged" --category Repudiation tmforge threats app.tm7 --edit --state Mitigated --description "Handled by the mesh" +tmforge threats app.tm7 --edit --title "Unauthenticated ledger write" # override the rule's wording +tmforge threats app.tm7 --edit --title "" # and hand it back to the rule tmforge list threats app.tm7 # see the register and each entry's standing tmforge threats app.tm7 --remove-stale # clear leftovers; triaged ones are kept and listed ``` +#### Who owns which field + +A threat has two kinds of text on it and they answer to different people. **Detection** — the rule +that fired, the category it concluded, and the threat's identity — belongs to the rule set, so a +re-run can be trusted to say the same thing. **Judgement** — title, state, priority, description, +mitigation, and note — belongs to the author. + +Title sits deliberately on the author's side of that line. Rule wording is written to be precise +about a class of problem, not about your system, and a reviewer reading the register is better served +by "Unauthenticated ledger write" than by a sentence naming a generic data flow. Overriding it +changes nothing a later run depends on: the rule still fires, the threat keeps its id, and the +analysis document, SARIF fingerprints, and register key are all unmoved. Clearing the override +restores the rule's wording, so the edit is reversible. + +Category does not, and that asymmetry is the point. A generated threat's category is what the +analysis concluded; letting an author change it would record a claim the rules do not support, and +the register would no longer mean one thing. The edit is refused rather than ignored, so nobody is +left believing it took effect. A manual threat has no rule behind it, so its author owns both. + #### Clearing stale entries Applying a generation result never deletes, so triage survives every re-run. The cost is that an diff --git a/docs/studio-guide.md b/docs/studio-guide.md index 9aa5783..8711967 100644 --- a/docs/studio-guide.md +++ b/docs/studio-guide.md @@ -124,10 +124,20 @@ configuration, so the loader reports that rather than pretending a local pack to **Analyze** also returns the model's **STRIDE threat register** — the threat-bearing findings framed as threats, grouped by category. Each threat is **editable inline**: click **Edit** to set its +- **title** — your wording in place of the rule's; clear it to restore the rule's; - **state** — Open, Needs investigation, Mitigated, or Accepted (accepting reveals a justification field); - **priority** — Critical / High / Medium / Low; - **description** and **mitigation** notes. +The **category** control is disabled for a rule-derived threat, and says so. Its category is what the +analysis concluded, not an opinion held separately from the rules — editing it would record a claim +the rule set does not support. A manual threat has no rule behind it, so its author owns its category +as well as its title. + +Retitling a rule threat changes nothing a later run depends on: the rule keeps detecting it, and the +threat keeps the identity that the analysis document, SARIF fingerprints, and the `.tm7` register key +are all built on. + Click **+ Add threat** to author a **manual threat** the rules do not detect: give it a title, a STRIDE category, and a scope — a specific element or flow, or model-wide. Manual threats are badged **Manual** and can be **deleted**; rule-derived threats cannot. diff --git a/src/ThreatModelForge.Analysis/ThreatGenerator.cs b/src/ThreatModelForge.Analysis/ThreatGenerator.cs index 8ec93be..29da631 100644 --- a/src/ThreatModelForge.Analysis/ThreatGenerator.cs +++ b/src/ThreatModelForge.Analysis/ThreatGenerator.cs @@ -256,9 +256,9 @@ public static Threat AddManual( } ///

- /// Edits a threat's author-owned fields in place: its lifecycle state, priority, description, - /// mitigation, and state note. Only the non-null arguments are applied. Works on both - /// rule-derived and manually-authored threats found in the register. + /// Edits a threat's author-owned fields in place: its lifecycle state, priority, title, + /// description, mitigation, category, and state note. Only the non-null arguments are applied. + /// Works on both rule-derived and manually-authored threats found in the register. /// /// The model containing the threat. /// The threat's register key, interaction key, or numeric id. @@ -267,7 +267,20 @@ public static Threat AddManual( /// The new description, or to leave it unchanged (empty clears it). /// The new mitigation, or to leave it unchanged (empty clears it). /// The new state note / justification, or to leave it unchanged. + /// + /// The new title, or to leave it unchanged. Empty clears an override on a + /// generated threat, restoring the title its rule produces. + /// + /// + /// The new category for a manually authored threat, or to leave it + /// unchanged. + /// /// if a matching threat was found and edited; otherwise . + /// + /// was supplied for a rule-derived threat, whose category belongs to + /// the rule that detected it. Callers that surface errors to a person should check first rather + /// than rely on the exception text. + /// public static bool Edit( ThreatModel model, string threatId, @@ -275,7 +288,9 @@ public static bool Edit( string? priority, string? description, string? mitigation, - string? note) + string? note, + string? title = null, + string? category = null) { if (model == null) { @@ -293,11 +308,29 @@ public static bool Edit( return false; } + bool manual = ManualThreatId.IsManual(threat.InteractionKey); + if (category != null && !manual) + { + throw new ArgumentException( + "The category of a generated threat belongs to the rule that detected it.", + nameof(category)); + } + if (state.HasValue) { threat.State = state.Value; } + if (title != null) + { + ApplyTitle(threat, title, manual); + } + + if (category != null) + { + threat.UserThreatCategory = category; + } + if (!string.IsNullOrWhiteSpace(priority)) { threat.Priority = priority; @@ -622,6 +655,8 @@ private static void UpdateThreat(Threat existing, GeneratedThreat threat) { bool priorityOverridden = HasPriorityOverride(existing, threat); string? overriddenPriority = priorityOverridden ? existing.Priority : null; + bool titleOverridden = HasTitleOverride(existing); + string? overriddenTitle = titleOverridden ? existing.Title : null; existing.TypeId = threat.RuleId; existing.SourceGuid = threat.SourceGuid; existing.TargetGuid = threat.TargetGuid; @@ -629,7 +664,7 @@ private static void UpdateThreat(Threat existing, GeneratedThreat threat) existing.DrawingSurfaceGuid = threat.DiagramGuid; existing.InteractionKey = threat.Id; existing.InteractionString = threat.InteractionString; - existing.Title = threat.Title; + existing.Title = overriddenTitle ?? threat.Title; existing.Priority = overriddenPriority ?? threat.Priority; existing.UserThreatCategory = threat.ThreatCategory.Name; existing.Properties = BuildProperties(threat); @@ -638,12 +673,18 @@ private static void UpdateThreat(Threat existing, GeneratedThreat threat) existing.Properties["PriorityOverride"] = "true"; } + if (titleOverridden) + { + existing.Properties["TitleOverride"] = "true"; + } + existing.ModifiedAt = DateTime.UtcNow; } private static void HydrateThreat(Threat existing, GeneratedThreat threat) { bool priorityOverridden = HasPriorityOverride(existing, threat); + bool titleOverridden = HasTitleOverride(existing); existing.TypeId = threat.RuleId; existing.SourceGuid = threat.SourceGuid; existing.TargetGuid = threat.TargetGuid; @@ -651,7 +692,7 @@ private static void HydrateThreat(Threat existing, GeneratedThreat threat) existing.DrawingSurfaceGuid = threat.DiagramGuid; existing.InteractionKey = threat.Id; existing.InteractionString = threat.InteractionString; - existing.Title = FirstNonEmpty(existing.Title, threat.Title); + existing.Title = titleOverridden ? existing.Title : FirstNonEmpty(existing.Title, threat.Title); existing.Priority = priorityOverridden ? existing.Priority : threat.Priority; existing.UserThreatCategory = threat.ThreatCategory.Name; @@ -669,6 +710,11 @@ private static void HydrateThreat(Threat existing, GeneratedThreat threat) generatedProperties["PriorityOverride"] = "true"; } + if (titleOverridden) + { + generatedProperties["TitleOverride"] = "true"; + } + existing.Properties = generatedProperties; } @@ -682,6 +728,11 @@ private static Dictionary BuildProperties(GeneratedThreat threat ["Rule"] = threat.RuleId, ["CategoryId"] = threat.ThreatCategory.Id, ["GeneratedDefaultPriority"] = threat.Priority, + + // Stashed for the same reason as the default priority: it is the only way to tell an + // author's title from the rule's on a later run, including one where the title was + // changed in another tool that cannot write tmforge's override marker. + ["GeneratedDefaultTitle"] = threat.Title, }; if (threat.Stride.HasValue) { @@ -730,11 +781,85 @@ private static bool HasPriorityOverride(Threat existing, GeneratedThreat generat StringComparison.OrdinalIgnoreCase); } + /// + /// Applies a title edit, recording that the title is now the author's. + /// + /// + /// Clearing a generated threat's title restores the rule's own wording from the stashed + /// default, which is what makes the edit reversible: an author who changes their mind gets the + /// rule's title back rather than an empty one. A manual threat has no rule to fall back to, so + /// its caller refuses a blank title before reaching here. + /// + /// The threat being edited. + /// The requested title; empty clears an override. + /// Whether the threat is manually authored. + private static void ApplyTitle(Threat threat, string title, bool manual) + { + if (title.Trim().Length > 0) + { + threat.Title = title; + if (!manual) + { + threat.Properties ??= new Dictionary(StringComparer.OrdinalIgnoreCase); + threat.Properties["TitleOverride"] = "true"; + } + + return; + } + + if (manual) + { + return; + } + + threat.Properties?.Remove("TitleOverride"); + if (threat.Properties?.TryGetValue("GeneratedDefaultTitle", out string? generated) == true && + !string.IsNullOrEmpty(generated)) + { + threat.Title = generated; + } + } + + /// + /// Reports whether an existing threat's title is the author's rather than the rule's. + /// + /// + /// Mirrors . The marker is the fast path for a title tmforge + /// applied itself; comparing against the stashed generated default is what catches a title + /// changed in another tool, which cannot write the marker. Without this, regeneration would + /// quietly restore the rule's wording over the author's. + /// + /// The threat already in the register. + /// when the title is author-owned. + private static bool HasTitleOverride(Threat existing) + { + if (existing.Properties?.TryGetValue("TitleOverride", out string? marker) == true && + string.Equals(marker, "true", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + if (string.IsNullOrWhiteSpace(existing.Title)) + { + return false; + } + + if (existing.Properties?.TryGetValue("GeneratedDefaultTitle", out string? previousDefault) == true) + { + return !string.Equals(existing.Title, previousDefault, StringComparison.Ordinal); + } + + // No stashed default means this entry predates the override contract. Claiming an override + // would freeze the rule's own wording as though an author had chosen it. + return false; + } + private static bool IsRuleOwnedProperty(string name) { return string.Equals(name, "Rule", StringComparison.OrdinalIgnoreCase) || string.Equals(name, "CategoryId", StringComparison.OrdinalIgnoreCase) || string.Equals(name, "GeneratedDefaultPriority", StringComparison.OrdinalIgnoreCase) || + string.Equals(name, "GeneratedDefaultTitle", StringComparison.OrdinalIgnoreCase) || string.Equals(name, "STRIDE", StringComparison.OrdinalIgnoreCase) || string.Equals(name, "References", StringComparison.OrdinalIgnoreCase); } diff --git a/src/ThreatModelForge.Cli/McpAuthoringTools.cs b/src/ThreatModelForge.Cli/McpAuthoringTools.cs index 84c9d8a..47c47f1 100644 --- a/src/ThreatModelForge.Cli/McpAuthoringTools.cs +++ b/src/ThreatModelForge.Cli/McpAuthoringTools.cs @@ -248,28 +248,34 @@ public static AuthoringResultDto AddThreat( /// The tmforge-json model to edit. /// The threat id (from the threats tool). /// The lifecycle state (Open, NeedsInvestigation, Mitigated, or Accepted). + /// The threat title. + /// The STRIDE category, for a manual threat only. /// The threat priority. /// The suggested mitigation. /// The threat description. /// A justification or state note. /// The edited model, or a blocking error. [McpServerTool(Name = "edit_threat")] - [Description("Edits a threat's lifecycle state, priority, mitigation, description, or justification. Use a threat id from the threats tool.")] + [Description("Edits a threat's title, lifecycle state, priority, mitigation, description, or justification. Use a threat id from the threats tool.")] public static AuthoringResultDto EditThreat( [Description("The tmforge-json model to edit.")] TmForgeModelDto model, [Description("The threat id (from the threats tool).")] string id, [Description("The lifecycle state: Open, NeedsInvestigation, Mitigated, or Accepted.")] string? state = null, + [Description("The title. On a generated threat this is an override; pass an empty title to restore the rule's wording.")] string? title = null, + [Description("The STRIDE category. Manual threats only: a generated threat's category belongs to the rule that detected it.")] string? category = null, [Description("The priority: Critical, High, Medium, or Low.")] string? priority = null, [Description("The suggested mitigation.")] string? mitigation = null, [Description("A description of the threat.")] string? description = null, [Description("A justification or state note (for example, why a risk is accepted).")] string? justification = null) { McpToolSupport.ValidateModel(model); - McpToolSupport.ValidateArguments(new[] { id, state, priority, mitigation, description, justification }); + McpToolSupport.ValidateArguments(new[] { id, state, title, category, priority, mitigation, description, justification }); return McpToolSupport.ValidateResult(AuthoringService.EditThreat(model, new EditThreatRequest { Id = id, State = state, + Title = title, + Category = category, Priority = priority, Mitigation = mitigation, Description = description, diff --git a/src/ThreatModelForge.Cli/ThreatsCommand.cs b/src/ThreatModelForge.Cli/ThreatsCommand.cs index cc1af07..3744379 100644 --- a/src/ThreatModelForge.Cli/ThreatsCommand.cs +++ b/src/ThreatModelForge.Cli/ThreatsCommand.cs @@ -225,12 +225,39 @@ private static int RunEdit( { string? stateArg = parsed.Get("state"); ThreatState? state = stateArg == null ? null : ThreatStateWire.Parse(stateArg); + + // An empty --edit value reaches the operation as a blank id, which it rejects by throwing. + // Caught here so a mistyped flag prints a message instead of a stack trace. + if (string.IsNullOrWhiteSpace(editId)) + { + Console.Error.WriteLine("--edit needs a threat id. Use 'tmforge list threats " + input + "' to find one."); + return 1; + } + if (!TryCanonicalPriority(parsed.Get("priority"), out string? priority)) { Console.Error.WriteLine("--priority must be one of: " + ThreatPriorities.Describe() + "."); return 1; } + // Checked here rather than left to the exception so the message reads for a person, which + // is the same reason --add validates its id before calling through. + string? title = parsed.Get("title"); + string? category = parsed.Get("category"); + bool manual = ManualThreatId.IsManual(editId); + if (category != null && !manual) + { + Console.Error.WriteLine( + "--category applies only to a manual threat. A generated threat's category belongs to the rule that detected it."); + return 1; + } + + if (manual && title != null && title.Trim().Length == 0) + { + Console.Error.WriteLine("--title cannot be empty for a manual threat; there is no generated title to fall back to."); + return 1; + } + if (!ThreatGenerator.Edit( model, editId, @@ -238,7 +265,9 @@ private static int RunEdit( priority, parsed.Get("description"), parsed.Get("mitigation"), - parsed.Get("note"))) + parsed.Get("note"), + title, + category)) { Console.Error.WriteLine("Threat not found: " + editId + ". Use 'tmforge list threats " + input + "' to find its id."); return 1; @@ -457,14 +486,16 @@ private static void PrintUsage() Console.Error.WriteLine("Usage:"); Console.Error.WriteLine(" tmforge threats [--write] [--json] [--rules ] "); Console.Error.WriteLine(" tmforge threats --add --title --category [--id ] [--scope ] [--state ] [--priority

] [--mitigation ] [--description ] [--json] "); - Console.Error.WriteLine(" tmforge threats --edit [--state ] [--priority

] [--mitigation ] [--description ] [--note ] [--json] "); + Console.Error.WriteLine(" tmforge threats --edit [--title ] [--category ] [--state ] [--priority

] [--mitigation ] [--description ] [--note ] [--json] "); Console.Error.WriteLine(" tmforge threats --remove [--json] "); Console.Error.WriteLine(" tmforge threats --remove-stale [--force] [--rules ] [--json] "); Console.Error.WriteLine(); Console.Error.WriteLine("--write persist the generated threats into the model's register (preserves prior triage)."); Console.Error.WriteLine("--add author a manual threat; --category is a STRIDE category (Spoofing / Tampering / Repudiation / InformationDisclosure / DenialOfService / ElevationOfPrivilege)."); Console.Error.WriteLine("--id the threat's id, so you can reference it elsewhere and re-run the same authoring safely; letters, digits, '-', '_', '.' (a 'manual:' prefix is added if you omit it). Defaults to a generated id."); - Console.Error.WriteLine("--edit change a threat's state (Open / NeedsInvestigation / Mitigated / Accepted), priority, mitigation, description, or note."); + Console.Error.WriteLine("--edit change a threat's title, state (Open / NeedsInvestigation / Mitigated / Accepted), priority, mitigation, description, or note."); + Console.Error.WriteLine(" --title on a generated threat is an override; pass an empty title to restore the rule's wording."); + Console.Error.WriteLine(" --category applies only to a manual threat: a generated threat's category is owned by its rule."); Console.Error.WriteLine("--remove delete a manual threat, or one that has gone stale (a threat the rules still produce would regenerate; accept or edit it instead)."); Console.Error.WriteLine("--remove-stale delete every stale entry — stored threats whose rule no longer fires. Entries carrying triage are kept and listed; --force discards them too."); Console.Error.WriteLine(); diff --git a/src/ThreatModelForge.Engine/Authoring/AuthoringService.cs b/src/ThreatModelForge.Engine/Authoring/AuthoringService.cs index 3c82103..66861d9 100644 --- a/src/ThreatModelForge.Engine/Authoring/AuthoringService.cs +++ b/src/ThreatModelForge.Engine/Authoring/AuthoringService.cs @@ -262,6 +262,31 @@ public static AuthoringResultDto EditThreat(TmForgeModelDto? model, EditThreatRe int index = overlay.FindIndex(entry => string.Equals(entry.Id, request.Id, StringComparison.OrdinalIgnoreCase)); ThreatStateDto existing = index >= 0 ? overlay[index] : new ThreatStateDto { Id = request.Id }; bool manual = existing.Manual == true || ThreatStateWire.IsManualKey(request.Id); + + // A generated threat's category is the rule's conclusion about what kind of threat this is, + // not an opinion the author holds separately. Refusing the edit keeps the category meaning + // one thing; silently ignoring it would let an author believe they had changed it. + if (request.Category != null && !manual) + { + return new AuthoringResultDto + { + Success = false, + Error = "The category of a generated threat belongs to the rule that detected it and cannot be edited. " + + "Only a manually authored threat has an author-owned category.", + }; + } + + // Clearing a title returns a generated threat to the one its rule produces. A manual threat + // has no rule to fall back to, so clearing its title would leave it with none at all. + if (manual && request.Title != null && request.Title.Trim().Length == 0) + { + return new AuthoringResultDto + { + Success = false, + Error = "A manually authored threat needs a title; there is no generated title to fall back to.", + }; + } + ThreatStateDto updated = new ThreatStateDto { Id = request.Id, @@ -271,8 +296,8 @@ public static AuthoringResultDto EditThreat(TmForgeModelDto? model, EditThreatRe Priority = priority ?? existing.Priority, Description = request.Description ?? existing.Description, Mitigation = request.Mitigation ?? existing.Mitigation, - Category = existing.Category, - Title = existing.Title, + Category = request.Category ?? existing.Category, + Title = Resolve(request.Title, existing.Title), ElementIds = existing.ElementIds, }; if (index >= 0) @@ -316,6 +341,24 @@ public static AuthoringResultDto RemoveThreat(TmForgeModelDto? model, RemoveThre private static bool TryCanonicalPriority(string? value, out string? priority) => ThreatPriorities.TryCanonicalize(value, out priority); + ///

+ /// Resolves an optional text edit. leaves the field alone; a blank value + /// clears it, which is how an author removes an override rather than replacing it with + /// whitespace; anything else is the new value. + /// + /// The requested value. + /// The value already recorded. + /// The value to record. + private static string? Resolve(string? requested, string? existing) + { + if (requested == null) + { + return existing; + } + + return requested.Trim().Length == 0 ? null : requested; + } + private static TmForgeModelDto WithThreats(TmForgeModelDto source, List overlay) { return new TmForgeModelDto diff --git a/src/ThreatModelForge.Engine/Authoring/EditThreatRequest.cs b/src/ThreatModelForge.Engine/Authoring/EditThreatRequest.cs index e78785a..2792ae7 100644 --- a/src/ThreatModelForge.Engine/Authoring/EditThreatRequest.cs +++ b/src/ThreatModelForge.Engine/Authoring/EditThreatRequest.cs @@ -12,6 +12,19 @@ public sealed class EditThreatRequest /// Gets the new lifecycle state, or to leave it unchanged. public string? State { get; init; } + /// + /// Gets the new title, or to leave it unchanged. An empty string clears an + /// override on a generated threat, returning it to the title its rule produces. + /// + public string? Title { get; init; } + + /// + /// Gets the new category, or to leave it unchanged. Only a manually + /// authored threat has an author-owned category; a generated threat's category belongs to the + /// rule that detected it, and editing it is refused rather than silently ignored. + /// + public string? Category { get; init; } + /// Gets the new priority, or to leave it unchanged. public string? Priority { get; init; } diff --git a/src/ThreatModelForge.Engine/EngineService.cs b/src/ThreatModelForge.Engine/EngineService.cs index e6467d1..284db8b 100644 --- a/src/ThreatModelForge.Engine/EngineService.cs +++ b/src/ThreatModelForge.Engine/EngineService.cs @@ -1296,6 +1296,8 @@ private static void ApplyOverlayEdits(ThreatModel model, IReadOnlyList(StringComparer.OrdinalIgnoreCase); + threat.Properties["TitleOverride"] = "true"; } if (!string.IsNullOrEmpty(entry.Description)) diff --git a/src/ThreatModelForge.Formats/TmForgeJsonFormat.cs b/src/ThreatModelForge.Formats/TmForgeJsonFormat.cs index c819132..4fdc07e 100644 --- a/src/ThreatModelForge.Formats/TmForgeJsonFormat.cs +++ b/src/ThreatModelForge.Formats/TmForgeJsonFormat.cs @@ -444,9 +444,22 @@ private static (Guid Source, Guid Target, Guid Flow) ResolveScope(IReadOnlyList< bool markerlessTriagedPriority = threat.State != ThreatState.AutoGenerated && !string.IsNullOrEmpty(threat.Priority) && threat.Properties?.ContainsKey("GeneratedDefaultPriority") != true; + + // A retitled threat is edited even when nothing else about it changed. Without this a + // title override is dropped here and the next regeneration restores the rule's wording, + // so the edit survives in the tool's own file but not through the canonical format. + bool explicitTitleOverride = threat.Properties?.TryGetValue("TitleOverride", out string? titleMarker) == true && + string.Equals(titleMarker, "true", StringComparison.OrdinalIgnoreCase); + bool generatedTitleDiffers = + threat.Properties?.TryGetValue("GeneratedDefaultTitle", out string? generatedTitle) == true && + !string.IsNullOrEmpty(threat.Title) && + !string.Equals(threat.Title, generatedTitle, StringComparison.Ordinal); + bool titleOverride = explicitTitleOverride || generatedTitleDiffers; + bool edited = threat.State != ThreatState.AutoGenerated || !string.IsNullOrEmpty(threat.UserThreatDescription) - || priorityOverride; + || priorityOverride + || titleOverride; if (!manual && !edited) { continue; @@ -459,7 +472,7 @@ private static (Guid Source, Guid Target, Guid Flow) ResolveScope(IReadOnlyList< Justification = NullIfEmpty(threat.StateInformation), Manual = manual ? true : (bool?)null, Category = manual ? NullIfEmpty(threat.UserThreatCategory) : null, - Title = manual ? NullIfEmpty(threat.Title) : null, + Title = manual || titleOverride ? NullIfEmpty(threat.Title) : null, Description = NullIfEmpty(threat.UserThreatDescription), Mitigation = manual ? MitigationOf(threat) : null, Priority = manual || priorityOverride || markerlessTriagedPriority ? NullIfEmpty(threat.Priority) : null, diff --git a/src/ThreatModelForge.Studio/src/App.css b/src/ThreatModelForge.Studio/src/App.css index fd5b309..c37097f 100644 --- a/src/ThreatModelForge.Studio/src/App.css +++ b/src/ThreatModelForge.Studio/src/App.css @@ -1239,6 +1239,13 @@ margin-bottom: 8px; } +/* Explains why a control is disabled, or what clearing a field does, next to the control itself. */ +.threat-edit-hint { + font-size: 11px; + color: var(--muted); + margin: -4px 0 8px; +} + .threat-desc { font-size: 11px; color: var(--muted); diff --git a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx index 553b161..751fa36 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx @@ -1174,6 +1174,13 @@ export function Editor() { if (edit.mitigation !== undefined) { next.mitigation = edit.mitigation; } + if (edit.title !== undefined) { + // An empty title clears the override, so the engine falls back to the rule's wording. + next.title = edit.title === '' ? undefined : edit.title; + } + if (edit.category !== undefined) { + next.category = edit.category; + } return [...prev.filter((t) => t.id !== threat.id), next]; }); setThreats((prev) => @@ -1186,6 +1193,8 @@ export function Editor() { priority: edit.priority ?? t.priority, description: edit.description ?? t.description, mitigation: edit.mitigation ?? t.mitigation, + title: edit.title === undefined || edit.title === '' ? t.title : edit.title, + category: edit.category ?? t.category, } : t, ), diff --git a/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.test.tsx b/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.test.tsx index 140671b..60c4388 100644 --- a/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.test.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.test.tsx @@ -194,6 +194,51 @@ describe('ThreatsPanel', () => { }); }); + it('edits a threat: retitling a generated threat emits the title and leaves its category alone', () => { + const { onEditThreat } = renderPanel({ threats: [THREATS[0]] }); + + fireEvent.click(screen.getByRole('button', { name: 'Edit' })); + fireEvent.change(screen.getByLabelText('Title'), { target: { value: 'Spoofed client identity' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(onEditThreat).toHaveBeenCalledWith(THREATS[0], { + state: 'Open', + title: 'Spoofed client identity', + }); + }); + + it("does not let a generated threat's category be edited, and says why", () => { + renderPanel({ threats: [THREATS[0]] }); + + fireEvent.click(screen.getByRole('button', { name: 'Edit' })); + + expect(screen.getByLabelText('Category')).toBeDisabled(); + expect(screen.getByText(/belongs to the rule that detected this threat/i)).toBeInTheDocument(); + }); + + it("lets a manual threat's category be edited, since no rule owns it", () => { + const manual = threat({ id: 'manual:byo', ruleId: '', manual: true, category: 'Spoofing', title: 'Stolen laptop' }); + const { onEditThreat } = renderPanel({ threats: [manual] }); + + fireEvent.click(screen.getByRole('button', { name: 'Edit' })); + expect(screen.getByLabelText('Category')).toBeEnabled(); + fireEvent.change(screen.getByLabelText('Category'), { target: { value: 'Tampering' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(onEditThreat).toHaveBeenCalledWith(manual, expect.objectContaining({ category: 'Tampering' })); + }); + + it('clearing the title of a generated threat emits an empty title, which restores the rule wording', () => { + const retitled = threat({ id: 'client:TM1001', ruleId: 'TM1001', title: 'An author wrote this' }); + const { onEditThreat } = renderPanel({ threats: [retitled] }); + + fireEvent.click(screen.getByRole('button', { name: 'Edit' })); + fireEvent.change(screen.getByLabelText('Title'), { target: { value: '' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(onEditThreat).toHaveBeenCalledWith(retitled, { state: 'Open', title: '' }); + }); + it('shows a non-open state badge and the justification for an accepted threat', () => { const accepted = threat({ id: 'client:TM1023', diff --git a/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.tsx b/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.tsx index 0707fd3..34cfa12 100644 --- a/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/ThreatsPanel.tsx @@ -59,6 +59,8 @@ export interface ThreatScopeOption { /** An author edit to a threat's owned fields. `state` is always carried; the rest are sparse. */ export interface ThreatEdit { state: ThreatLifecycleState; + title?: string; + category?: string; priority?: string; description?: string; mitigation?: string; @@ -114,13 +116,21 @@ function severityClassForPriority(priority: string | undefined): string { /** The inline editor for a single threat's author-owned fields. */ function ThreatEditor({ threat, onSave, onCancel }: { threat: Threat; onSave: (edit: ThreatEdit) => void; onCancel: () => void }) { const [state, setState] = useState(threat.state); + const [title, setTitle] = useState(threat.title ?? ''); const [priority, setPriority] = useState(threat.priority ?? 'Medium'); const [description, setDescription] = useState(threat.description ?? ''); const [mitigation, setMitigation] = useState(threat.mitigation ?? ''); const [justification, setJustification] = useState(threat.justification ?? ''); + const [category, setCategory] = useState(threat.category ?? 'Spoofing'); function save() { const edit: ThreatEdit = { state }; + if (title !== (threat.title ?? '')) { + edit.title = title; + } + if (threat.manual && category !== (threat.category ?? '')) { + edit.category = category; + } if (priority !== (threat.priority ?? 'Medium')) { edit.priority = priority; } @@ -138,6 +148,38 @@ function ThreatEditor({ threat, onSave, onCancel }: { threat: Threat; onSave: (e return (
{ event.preventDefault(); save(); }}> + + {threat.manual ? null : ( +

+ Clear the title to restore the wording its rule produces. +

+ )} + + {threat.manual ? null : ( +

+ The category belongs to the rule that detected this threat, so it is not editable here. +

+ )}