Skip to content

docs: verify threat models for completeness and fix silent gaps#1330

Open
spoorcc wants to merge 1 commit into
mainfrom
claude/threat-models-security-docs-59i5v1
Open

docs: verify threat models for completeness and fix silent gaps#1330
spoorcc wants to merge 1 commit into
mainfrom
claude/threat-models-security-docs-59i5v1

Conversation

@spoorcc

@spoorcc spoorcc commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Cross-checking the threat catalog, responses, controls, and generated
reports against each other and the codebase surfaced several gaps:

  • DFT-06 (command injection) and DFT-34 (long-lived stored credential)
    had documented responses and mitigating controls (C-006/C-007,
    C-015/C-017, C-042) but never appeared in any rendered threat table:
    DFT-06's condition no longer matches once parameterized subprocess
    input is modelled, and pytm's resolve() never evaluates threats
    against Data assets (it iterates TM._elements only). Render such
    responses from the threat catalog so every documented risk decision
    stays visible, and fail generation when a finding has no response or
    a response names an unknown threat ID.
  • Asset IDs A-09/A-10 meant different assets in the two models (Remote
    VCS Server / Archive HTTP Server in the usage model, Winget repo /
    WINGET_TOKEN PAT in the supply-chain model). Renumber the Winget
    assets to A-28/A-29 so every asset ID is unique across the doc set.
  • Gap control IDs C-018/C-019/C-020/C-035 cited by the usage model were
    defined nowhere; add an 'Identified Gaps' section to the security
    page so each reserved ID resolves to a description.
  • Fix the SBOM cross-reference in the usage model (A-04 -> A-03) and
    the stale OSCAL version (1.1.2 -> 1.2.2) in security.rst and
    security/README.md.
  • Regenerate both threat-model reports (also picks up the stale
    C-045/C-036 control-row ordering in the usage report).

Summary by CodeRabbit

  • New Features

    • Added clearer coverage for documented security risks, including previously unrepresented threat entries in generated reports.
  • Bug Fixes

    • Improved consistency in supply-chain security references and asset identifiers across threat-model documentation.
    • Fixed a documentation gap where some addressed threats could be omitted from rendered security tables.
  • Documentation

    • Updated security and supply-chain docs to reflect the latest OSCAL version and refreshed compliance references.
    • Clarified runtime and SBOM usage notes with updated source references.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@spoorcc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22f2f337-9137-4860-88ee-18e534738a05

📥 Commits

Reviewing files that changed from the base of the PR and between c4baf59 and c389926.

📒 Files selected for processing (9)
  • doc/explanation/security.rst
  • doc/explanation/threat_model_supply_chain.rst
  • doc/explanation/threat_model_usage.rst
  • security/README.md
  • security/dfetch.component-definition.json
  • security/tm_controls_data.py
  • security/tm_render.py
  • security/tm_supply_chain.py
  • security/tm_usage.py

Walkthrough

This PR renumbers Winget-related asset identifiers from A-09/A-10 to A-28/A-29 across the pytm threat model source files, generated threat model documentation, and control asset lists. It adds a new DFT-06 command-injection threat entry and DFT-34 credential-persistence threat, adds coverage validation logic (_validate_threat_coverage) and a _CatalogRow fallback in tm_render.py to ensure documented threats without pytm findings still render, and updates OSCAL references from 1.1.2 to 1.2.2 along with component-definition version metadata and a new reserved control IDs documentation section.

Changes

Threat model updates

Layer / File(s) Summary
Threat coverage validation and rendering
security/tm_render.py
Adds _CatalogRow dataclass and _validate_threat_coverage() to detect mismatches between findings and responses; _render_threat_rows() now includes catalog-only threats in rendered output.
Winget asset ID renumbering in model data
security/tm_supply_chain.py, security/tm_controls_data.py, security/tm_usage.py
Renames Winget PAT and repository asset IDs from A-09/A-10 to A-28/A-29 across assets, dataflows, threat responses, and the allowlist; updates SBOM cross-reference and DFT-06 target.
Generated supply-chain threat model docs
doc/explanation/threat_model_supply_chain.rst
Updates diagrams, asset identification, dataflows, and threats tables to A-28/A-29; adds DFT-06 and DFT-34 threat entries.
Generated usage threat model docs
doc/explanation/threat_model_usage.rst
Updates SBOM asset note, adds DFT-06 threat row, repositions C-045 control entry.

OSCAL compliance documentation update

Layer / File(s) Summary
OSCAL version and metadata
doc/explanation/security.rst, security/README.md, security/dfetch.component-definition.json
Updates OSCAL version references from 1.1.2 to 1.2.2 and adjusts component-definition version/last-modified metadata.
Reserved control IDs gaps section
doc/explanation/security.rst
Adds a new "Identified Gaps (Reserved Control IDs)" section listing C-018, C-019, C-020, C-035.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Generator as tm_render.py
  participant Catalog as TM._threats
  participant Validator as _validate_threat_coverage
  participant Table as Rendered Threat Table

  Generator->>Catalog: build threat catalog from TM._threats
  Generator->>Validator: validate(findings, responses)
  alt mismatch detected
    Validator-->>Generator: raise ValueError
  else coverage OK
    Validator-->>Generator: pass
    Generator->>Table: populate rows from findings (representative map)
    Generator->>Table: insert _CatalogRow for responses without findings
  end
Loading

Possibly related PRs

  • dfetch-org/dfetch#1182: Introduced the original pytm threat model modules and control/asset data that this PR renumbers and extends.
  • dfetch-org/dfetch#1263: Prior update to the Winget publishing workflow and associated threat/control wiring for the same assets touched here.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: validating threat-model completeness and fixing missing rendered gaps.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/threat-models-security-docs-59i5v1

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@security/tm_render.py`:
- Around line 178-197: The early return in tm_render’s findings handling skips
_validate_threat_coverage() and prevents response-only rows from being rendered.
In the tm_render flow, move coverage validation before the empty-findings exit
and only return immediately when both tm.findings and responses are empty. Keep
the existing representative-building logic in tm_render so response threat IDs
not present in findings still get catalog-backed rows and unknown IDs still
trigger validation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 709dfd35-5fa0-4670-9c26-7e6764cd5156

📥 Commits

Reviewing files that changed from the base of the PR and between 8c8a595 and c4baf59.

📒 Files selected for processing (9)
  • doc/explanation/security.rst
  • doc/explanation/threat_model_supply_chain.rst
  • doc/explanation/threat_model_usage.rst
  • security/README.md
  • security/dfetch.component-definition.json
  • security/tm_controls_data.py
  • security/tm_render.py
  • security/tm_supply_chain.py
  • security/tm_usage.py

Comment thread security/tm_render.py
@spoorcc spoorcc force-pushed the claude/threat-models-security-docs-59i5v1 branch from c4baf59 to be98353 Compare July 6, 2026 19:25
Cross-checking the threat catalog, responses, controls, and generated
reports against each other and the codebase surfaced several gaps:

- DFT-06 (command injection) and DFT-34 (long-lived stored credential)
  had documented responses and mitigating controls (C-006/C-007,
  C-015/C-017, C-042) but never appeared in any rendered threat table:
  DFT-06's condition no longer matches once parameterized subprocess
  input is modelled, and pytm's resolve() never evaluates threats
  against Data assets (it iterates TM._elements only). Render such
  responses from the threat catalog so every documented risk decision
  stays visible, and fail generation when a finding has no response or
  a response names an unknown threat ID.
- Asset IDs A-09/A-10 meant different assets in the two models (Remote
  VCS Server / Archive HTTP Server in the usage model, Winget repo /
  WINGET_TOKEN PAT in the supply-chain model). Renumber the Winget
  assets to A-28/A-29 so every asset ID is unique across the doc set.
- Gap control IDs C-018/C-019/C-020/C-035 cited by the usage model were
  defined nowhere; add an 'Identified Gaps' section to the security
  page so each reserved ID resolves to a description.
- Fix the SBOM cross-reference in the usage model (A-04 -> A-03) and
  the stale OSCAL version (1.1.2 -> 1.2.2) in security.rst and
  security/README.md.
- Regenerate both threat-model reports (also picks up the stale
  C-045/C-036 control-row ordering in the usage report).
@spoorcc spoorcc force-pushed the claude/threat-models-security-docs-59i5v1 branch from be98353 to c389926 Compare July 6, 2026 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants