From 244536d0550f79dad50994038bfe990fb6ca9237 Mon Sep 17 00:00:00 2001 From: charles-fineman-bot Date: Mon, 31 Aug 2026 00:48:27 +0000 Subject: [PATCH 1/3] feat: carry the review guidance a threat model needs to be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A threat model that lists threats is not yet a document a reviewer can work through. Three collections close that gap, chosen from the four Kivaar carries that #7 did not specify: - criticality[] records what each priority level means for this project, with worked examples. priority is a schema-owned vocabulary, but where its boundaries fall is a project's judgement about blast radius. A model that ranks threats without recording that judgement leaves its most consequential field unfalsifiable — a reader cannot tell a miscalibrated ranking from an honest disagreement about the scale. - top_abuse_path_links names the abuse paths to read first. Deliberately not derived from priority: editorial order is a judgement, and a list that repeats every critical threat has made none. - focus_paths[] says where in the repository a threat actually lives, and which threats make each location worth reading. No other collection carries that link: components describe what the system is made of and planned_evidence describes what will test it, but neither tells a reviewer what to read. Quantitative policy is configuration, not schema rule — min_criticality_ examples, min_top_abuse_paths, max_top_abuse_paths, and a require_criticality_for_every_priority coverage switch — because the right numbers differ between a small service and a platform. Each bound defaults to zero meaning unbounded. A negative bound, or a maximum below its own minimum, is rejected at load time rather than surfacing later as a document rejection nothing can satisfy. quality_checks is deliberately absent, and the schema documentation says why with the mapping attached: a model's own completeness self-assessment duplicates what this tool proves — coverage across seven collections, requirement resolution, required assumptions and open questions — and a hand-maintained claim can go stale and contradict the validator that ran beside it. What remains are process notes about how a review was run, which belong in the pull request that changed the model. That is schema-or-nothing rather than schema-versus-template, which the issue framed as a third option it is not: unknown members are rejected, so a collection absent from the schema cannot be in a document at all, and a consumer template has nothing to render. New rules are mutation-tested rather than merely written: dropping the level enum, the headline-list minimum, the focus-path validation, and the calibration completeness rule each make the corresponding test fail. Closes #9 --- CHANGELOG.md | 40 ++++ docs/config.md | 26 ++- docs/schema-threat-model.md | 82 ++++++++ internal/policy/config.go | 58 +++++- internal/policy/config_test.go | 21 ++ internal/render/threats/default.md.tmpl | 27 ++- internal/render/threats/threats.go | 32 +++ internal/render/threats/threats_test.go | 85 ++++++++ internal/threats/model.go | 28 +++ internal/threats/validate.go | 98 ++++++++++ internal/threats/validate_test.go | 153 +++++++++++++++ testdata/config.json | 8 +- testdata/threats.json | 249 +++++++++++++++++++----- testdata/threats.md | 38 ++++ 14 files changed, 887 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 681cc5f..f92a793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,30 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). drift apart on what a safe URI looks like, and `check.RepoRelativePath` for the threat model's repository-relative references. + +- **Threat-model review guidance.** Three collections that help a reader + navigate a model rather than describe the system + ([#9](https://github.com/sofired/tracedoc/issues/9)): + + - `criticality[]` — what each `priority` level means for this project, + with worked examples. `priority` is schema-owned, but where its + boundaries fall is a project's judgement; recording it is what makes a + ranking reviewable rather than merely asserted. + - `top_abuse_path_links` — the abuse paths to read first, as declared + threat IDs. Deliberately not derived from `priority`: editorial order + is a judgement, and repeating every `critical` threat makes none. + - `focus_paths[]` — where in the repository a threat actually lives, + with the threats that make each location worth reading. No other + collection carries the link from a threat to an artifact. + +- Configuration: `threat_model.limits` (`min_criticality_examples`, + `min_top_abuse_paths`, `max_top_abuse_paths`), each defaulting to `0` + meaning unbounded, and an eighth coverage switch, + `require_criticality_for_every_priority`. Quantitative policy is + configuration rather than schema rule, because the right numbers differ + between a small service and a platform. A negative bound, or a maximum + below its own minimum, is rejected when the configuration loads. + ### Fixed - **Control characters are now rejected in every validated string, as @@ -116,6 +140,22 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). break out of its code span and inject Markdown — including a link to a destination the `reference_hosts` allowlist would have refused. + +### Notes + +- **No `quality_checks` collection**, deliberately. A model's own + completeness self-assessment duplicates what this tool already proves — + entry-point, boundary, asset, flow, control, risk and evidence coverage, + requirement resolution, and required assumptions and open questions are + each enforced — and a hand-maintained claim can go stale and contradict + the validator beside it. `docs/schema-threat-model.md` carries the + mapping. The claims that remain are process notes about how a review was + run, which belong in the pull request that changed the model. + + This is schema-or-nothing rather than schema-versus-template: unknown + members are rejected, so a collection absent from the schema cannot be in + a document for a consumer template to render. + ## 0.1.0 - 2026-08-02 Initial release. The requirements-matrix mechanics were externalized from diff --git a/docs/config.md b/docs/config.md index 4063cbe..ac43a17 100644 --- a/docs/config.md +++ b/docs/config.md @@ -53,7 +53,13 @@ Example: "require_entry_point_coverage": true, "require_control_coverage": true, "require_risk_coverage": true, - "require_evidence_per_threat": true + "require_evidence_per_threat": true, + "require_criticality_for_every_priority": true + }, + "limits": { + "min_criticality_examples": 2, + "min_top_abuse_paths": 3, + "max_top_abuse_paths": 10 }, "render": { "source_name": "threats.json", @@ -116,6 +122,7 @@ no section is exit `2`. | `evidence_statuses` | non-empty list of allowed `planned_evidence[].status` values | | `reference_hosts` | optional list of lowercase multi-label DNS names; hosts an external reference may use | | `coverage` | boolean switches for the declared-entity coverage rules; see below | +| `limits` | quantitative bounds on the review-guidance collections; see below | | `render` | presentation strings; see below | These four vocabularies are a project's own workflow labels. The ones the @@ -139,9 +146,26 @@ the reproducible default. rejects, and what "analysed" means, is specified once in [schema-threat-model.md](schema-threat-model.md#coverage). +`require_criticality_for_every_priority` is the eighth switch: it demands a +`criticality` entry for each of the four `priority` levels. + Each switch defaults to `false` when omitted, so a project can adopt the document type first and tighten coverage as the model fills in. +`limits`: + +| Member | Bounds | +| -------------------------- | --------------------------------------------- | +| `min_criticality_examples` | worked examples required per `criticality` entry | +| `min_top_abuse_paths` | fewest entries in `top_abuse_path_links` | +| `max_top_abuse_paths` | most entries in `top_abuse_path_links` | + +Each defaults to `0`, which disables that bound. Negative values are +rejected, as is a maximum below its own minimum — a bound no document can +satisfy is a configuration error worth catching at load time rather than as +a puzzling rejection later. The schema says what these collections are; how +much of them a project expects is its own call. + ### `render` (per section) | Member | Used for | diff --git a/docs/schema-threat-model.md b/docs/schema-threat-model.md index d093903..d73dc9d 100644 --- a/docs/schema-threat-model.md +++ b/docs/schema-threat-model.md @@ -38,6 +38,9 @@ will demonstrate it is handled and who carries whatever risk remains. | `planned_evidence` | array | non-empty; see below | | `observability` | array | required, may be empty; see below | | `threats` | array | non-empty; see below | +| `criticality` | array | required, may be empty; see below | +| `top_abuse_path_links` | array | required, may be empty; declared threat IDs | +| `focus_paths` | array | required, may be empty; see below | | `supersessions` | array | required, may be empty; shared supersession rules with threat IDs | Every array member is **required to be present**, even when empty. `compare` @@ -350,6 +353,55 @@ present: - `accept`, `avoid`, and `transfer` each require `treatment_rationale`: all three record a decision not to build a control. +## `criticality[]` + +| Member | Type | Rules | +| ------------ | ------ | -------------------------------------------------- | +| `level` | string | a `priority` value (schema-owned); unique | +| `definition` | string | non-empty; what this level means for this project | +| `examples` | array | non-empty; worked examples at this level | + +`priority` is a schema-owned vocabulary, but what separates one level from +the next is a project's own judgement about blast radius. A model that +ranks threats without recording that judgement leaves its most consequential +field unfalsifiable: a reader cannot tell a miscalibrated ranking from an +honest disagreement about the scale. + +The level is the record's key, so there is no separate identifier — nothing +links to a calibration entry. Duplicated levels are rejected. + +Two bounded configuration switches govern how much calibration a project +expects: `require_criticality_for_every_priority` demands an entry for each +of the four levels, and `min_criticality_examples` sets a floor on +`examples`. Both are [configuration](config.md#threat_model), because the +right answer differs between a small service and a platform. + +## `top_abuse_path_links` + +A required array, possibly empty, of declared threat IDs: the abuse paths a +reviewer should follow first. Bounded by the configured +`min_top_abuse_paths` and `max_top_abuse_paths`. + +This is deliberately **not** derived from `priority`. "The paths to read +first" is an editorial judgement about narrative order, and a list that +merely repeats every `critical` threat has made no such judgement. A model +with fifteen threats and three genuinely instructive attack narratives +should say so. + +## `focus_paths[]` + +| Member | Type | Rules | +| -------------- | ------ | -------------------------------------------- | +| `path` | string | repository-relative, unique; same rules as a [reference path](#references) | +| `why` | string | non-empty; why this location deserves scrutiny | +| `threat_links` | array | non-empty; declared threat IDs | + +The model's link from a threat to where that threat actually lives. No +other collection carries it: `components` describe what the system is made +of and `planned_evidence` describes what will test it, but neither tells a +reviewer what to read. The path is the record's key, and as with every path +this schema carries, the tool never opens it. + ## Ownership `threats[]`, `controls[]`, and `planned_evidence[]` each carry an `owner` @@ -406,6 +458,36 @@ false positive worth rejecting: a threat that crosses the same boundary by a different route has not examined this surface, and accepting it would certify an entry point nobody reviewed. +## Why there is no `quality_checks` + +A threat model may be tempted to carry its own completeness self-assessment +— a list of claims like "all entry points are covered", each marked +complete. This schema deliberately has no such collection, because for the +claims worth making the tool already proves them, and a hand-maintained +claim can go stale and contradict the validator that ran beside it: + +| A claim of this kind | What already enforces it | +| -------------------- | ------------------------ | +| All entry points are covered | `require_entry_point_coverage` | +| Every trust boundary appears in a threat | `require_boundary_coverage` | +| Every asset, flow, control, and risk is analysed | the remaining [coverage switches](#coverage) | +| Every threat has planned evidence | `require_evidence_per_threat` | +| Requirement traceability resolves | `validate -requirements` | +| Assumptions and open questions are explicit | `assumptions` and `open_questions` are required members | + +The claims left over — "the review reflected a conversation with the +architect", "both deployment shapes were considered" — are process notes +about how a review was conducted rather than statements about the system. +They belong in the pull request that changed the model, where they can be +read against the diff. + +Note that this is a schema-or-nothing choice, not a schema-versus-template +one. Unknown members are rejected +([shared lexical contract](schema.md#shared-lexical-contract)), so a +collection absent from the schema cannot appear in a document at all, and a +consumer template has nothing to render. Anything a project needs to record +has to be a schema member. + ## Which vocabularies are schema-owned `likelihood`, `severity`, `priority`, `treatment`, and `decisions[].status` diff --git a/internal/policy/config.go b/internal/policy/config.go index 0064535..51988c6 100644 --- a/internal/policy/config.go +++ b/internal/policy/config.go @@ -84,6 +84,7 @@ type ThreatModelSection struct { EvidenceStatuses []string `json:"evidence_statuses"` ReferenceHosts []string `json:"reference_hosts"` Coverage Coverage `json:"coverage"` + Limits Limits `json:"limits"` Render Render `json:"render"` } @@ -92,13 +93,24 @@ type ThreatModelSection struct { // declared was actually analysed; a consumer building a model incrementally // can turn one off while the model is still being written. type Coverage struct { - RequireAssetCoverage bool `json:"require_asset_coverage"` - RequireBoundaryCoverage bool `json:"require_boundary_coverage"` - RequireFlowCoverage bool `json:"require_flow_coverage"` - RequireEntryPointCoverage bool `json:"require_entry_point_coverage"` - RequireControlCoverage bool `json:"require_control_coverage"` - RequireRiskCoverage bool `json:"require_risk_coverage"` - RequireEvidencePerThreat bool `json:"require_evidence_per_threat"` + RequireAssetCoverage bool `json:"require_asset_coverage"` + RequireBoundaryCoverage bool `json:"require_boundary_coverage"` + RequireFlowCoverage bool `json:"require_flow_coverage"` + RequireEntryPointCoverage bool `json:"require_entry_point_coverage"` + RequireControlCoverage bool `json:"require_control_coverage"` + RequireRiskCoverage bool `json:"require_risk_coverage"` + RequireEvidencePerThreat bool `json:"require_evidence_per_threat"` + RequireCriticalityForEveryPriority bool `json:"require_criticality_for_every_priority"` +} + +// Limits is the consumer's quantitative policy for the collections whose +// usefulness depends on how much of them there is. Omitting a member, or +// setting it to zero, disables that limit — the schema says what these +// collections are, and the project says how much of them it expects. +type Limits struct { + MinCriticalityExamples int `json:"min_criticality_examples"` + MinTopAbusePaths int `json:"min_top_abuse_paths"` + MaxTopAbusePaths int `json:"max_top_abuse_paths"` } // StandardSource declares where citations for one standard may point: either @@ -197,6 +209,12 @@ func (c *Config) ThreatsPolicy() (threats.Policy, error) { Controls: section.Coverage.RequireControlCoverage, Risks: section.Coverage.RequireRiskCoverage, Evidence: section.Coverage.RequireEvidencePerThreat, + Criticality: section.Coverage.RequireCriticalityForEveryPriority, + }, + Limits: threats.Limits{ + MinCriticalityExamples: section.Limits.MinCriticalityExamples, + MinTopAbusePaths: section.Limits.MinTopAbusePaths, + MaxTopAbusePaths: section.Limits.MaxTopAbusePaths, }, } return result, nil @@ -366,9 +384,35 @@ func (c *Config) validateThreatModelSection( seen[host] = struct{}{} } + validateLimits(add, section.Limits) validateRender(add, "threat_model.render", section.Render) } +// validateLimits rejects negative bounds and a max below its min. A limit +// that can never be satisfied is a configuration error worth catching at +// load time rather than as a puzzling document rejection later. +func validateLimits(add func(location, format string, args ...any), limits Limits) { + for _, bound := range []struct { + name string + value int + }{ + {"min_criticality_examples", limits.MinCriticalityExamples}, + {"min_top_abuse_paths", limits.MinTopAbusePaths}, + {"max_top_abuse_paths", limits.MaxTopAbusePaths}, + } { + if bound.value < 0 { + add("threat_model.limits."+bound.name, "expected a non-negative integer") + } + } + if limits.MaxTopAbusePaths > 0 && limits.MinTopAbusePaths > limits.MaxTopAbusePaths { + add( + "threat_model.limits.max_top_abuse_paths", + "must not be smaller than min_top_abuse_paths (%d)", + limits.MinTopAbusePaths, + ) + } +} + func (c *Config) compilePattern(errs *[]string, location, value string) *regexp.Regexp { add := func(format string, args ...any) { *errs = append(*errs, location+": "+fmt.Sprintf(format, args...)) diff --git a/internal/policy/config_test.go b/internal/policy/config_test.go index 8fdecd6..43ecd0a 100644 --- a/internal/policy/config_test.go +++ b/internal/policy/config_test.go @@ -61,6 +61,13 @@ func TestFixtureConfigLoads(t *testing.T) { len(threatsPolicy.ReferenceHosts) != 2 { t.Fatalf("unexpected threat-model vocabularies: %#v", threatsPolicy) } + if threatsPolicy.Limits != (threats.Limits{ + MinCriticalityExamples: 2, + MinTopAbusePaths: 2, + MaxTopAbusePaths: 10, + }) { + t.Fatalf("unexpected limits: %#v", threatsPolicy.Limits) + } if threatsPolicy.Coverage != (threats.Coverage{ Assets: true, Boundaries: true, @@ -69,6 +76,7 @@ func TestFixtureConfigLoads(t *testing.T) { Controls: true, Risks: true, Evidence: true, + Criticality: true, }) { t.Fatalf("unexpected coverage switches: %#v", threatsPolicy.Coverage) } @@ -300,6 +308,19 @@ func TestConfigRejections(t *testing.T) { want: "threat_model.reference_hosts[0]: expected a lowercase DNS host name", mutate: func(c *Config) { c.ThreatModel.ReferenceHosts[0] = "localhost" }, }, + { + name: "negative limit", + want: "threat_model.limits.min_criticality_examples: expected a non-negative integer", + mutate: func(c *Config) { c.ThreatModel.Limits.MinCriticalityExamples = -1 }, + }, + { + name: "top abuse path maximum below its minimum", + want: "threat_model.limits.max_top_abuse_paths: must not be smaller than min_top_abuse_paths (9)", + mutate: func(c *Config) { + c.ThreatModel.Limits.MinTopAbusePaths = 9 + c.ThreatModel.Limits.MaxTopAbusePaths = 3 + }, + }, { name: "duplicate reference host", want: `threat_model.reference_hosts[1]: duplicate host`, diff --git a/internal/render/threats/default.md.tmpl b/internal/render/threats/default.md.tmpl index 0230c3f..20c59d9 100644 --- a/internal/render/threats/default.md.tmpl +++ b/internal/render/threats/default.md.tmpl @@ -128,7 +128,23 @@ | --- | ---: | {{range .TreatmentCounts}}| `{{.Label}}` | {{.Value}} | {{end}} -## Assets +## Priority calibration + +{{if .Document.Criticality}}What each priority level means in this project, with worked examples. + +{{range .Document.Criticality}}**`{{.Level}}`** — {{prose .Definition}} + +{{range .Examples}}- {{prose .}} +{{end}} +{{end}}{{else}}No priority calibration recorded. +{{end}}## Start here + +{{if .TopAbusePaths}}The abuse paths a reviewer should follow first. + +{{range $index, $threat := .TopAbusePaths}}{{add1 $index}}. [`{{$threat.ID}}`](#{{anchorHref $threat.ID}}) — {{prose $threat.Title}} (`{{$threat.Priority}}`) +{{end}} +{{else}}No headline abuse paths recorded. +{{end}}## Assets | ID | Asset | Objective | Threats | | --- | --- | --- | ---: | @@ -277,7 +293,14 @@ **Controls:** {{inlineValues .ControlLinks}} {{end}}{{else}}No observability expectations recorded. -{{end}}## Supersessions +{{end}}## Where to look + +{{if .FocusPaths}}| Path | Why | Threats | +| --- | --- | --- | +{{range .FocusPaths}}| {{inlineCode .Path}} | {{table .Why}} | {{range $index, $threat := .Threats}}{{if $index}}, {{end}}[`{{$threat.ID}}`](#{{anchorHref $threat.ID}}){{end}} | +{{end}}{{else}}No focus paths recorded. +{{end}} +## Supersessions {{if .Document.Supersessions}}| Retired ID | Replacement IDs | Rationale | | --- | --- | --- | diff --git a/internal/render/threats/threats.go b/internal/render/threats/threats.go index dddceac..5ccc867 100644 --- a/internal/render/threats/threats.go +++ b/internal/render/threats/threats.go @@ -30,6 +30,16 @@ type view struct { Controls []control Evidence []evidence Sections []prioritySection + TopAbusePaths []threats.Threat + FocusPaths []focusPath +} + +// focusPath resolves each threat link to the threat itself, so the +// rendered reading list can show what a reviewer is being sent to read +// about rather than a bare list of identifiers. +type focusPath struct { + threats.FocusPath + Threats []threats.Threat } type count struct { @@ -272,6 +282,28 @@ func newView(doc threats.Document, options render.Options) view { Controls: evidenceControls[item.ID], }) } + // The curated headline list and the reading list both resolve their + // threat links against document order, so the rendered companion never + // depends on map iteration. + byID := make(map[string]threats.Threat, len(doc.Threats)) + for _, item := range doc.Threats { + byID[item.ID] = item + } + for _, id := range doc.TopAbusePaths { + if item, ok := byID[id]; ok { + result.TopAbusePaths = append(result.TopAbusePaths, item) + } + } + for _, item := range doc.FocusPaths { + linked := make([]threats.Threat, 0, len(item.ThreatLinks)) + for _, id := range item.ThreatLinks { + if threat, ok := byID[id]; ok { + linked = append(linked, threat) + } + } + result.FocusPaths = append(result.FocusPaths, focusPath{FocusPath: item, Threats: linked}) + } + for _, priority := range threats.PriorityOrder { if len(byPriority[priority]) == 0 { continue diff --git a/internal/render/threats/threats_test.go b/internal/render/threats/threats_test.go index a510a21..da5204b 100644 --- a/internal/render/threats/threats_test.go +++ b/internal/render/threats/threats_test.go @@ -478,3 +478,88 @@ func TestRiskIDAnchorsAreEscaped(t *testing.T) { t.Errorf("risk anchor is not escaped as expected; rendered:\n%s", rendered) } } + +// TestReviewGuidanceRendering covers the three collections that exist to +// help a reader navigate the model rather than to describe the system: the +// priority calibration, the curated headline list, and the reading list. +// Each resolves identifiers the renderer must not reorder. +func TestReviewGuidanceRendering(t *testing.T) { + doc := fixtureDocument(t) + rendered, err := Render(doc, fixtureOptions(), "") + if err != nil { + t.Fatalf("render review guidance: %v", err) + } + + for _, want := range []string{ + "## Priority calibration", + "## Start here", + "## Where to look", + // Calibration is emitted in the schema's priority order, not the + // document's, so a reader always sees the scale top-down. + "**`critical`**", + "**`low`**", + // The headline list keeps document order and resolves to titles. + "1. [`THRT-001`](#thrt-001)", + "2. [`THRT-002`](#thrt-002)", + // A focus path renders its own threat links. + "`diagrams/data-flow.md`", + } { + if !strings.Contains(rendered, want) { + t.Errorf("rendered Markdown does not contain %q", want) + } + } +} + +// TestReviewGuidanceEscaping keeps the new sections inside the same +// injection guarantees as the rest of the document. A focus path is +// consumer-authored free text emitted into a table cell and an inline code +// span, and a calibration definition is prose. +func TestReviewGuidanceEscaping(t *testing.T) { + doc := fixtureDocument(t) + doc.Criticality[0].Definition = "Compromise