Add pytm threat models#1182
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds security/ with a 33-entry threat catalog, shared helpers/types, a Markdown report template, README (pinned pytm install), a pylint config tweak, and two pytm threat models (supply-chain and runtime-usage) that emit reports and diagrams. ChangesThreat Modeling Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub
participant CI as GitHub Actions
participant PyPI
participant Consumer
Developer->>GitHub: push/PR
GitHub->>CI: trigger workflow
CI->>PyPI: publish via OIDC (SBOM/attestation)
Consumer->>PyPI: pip install
sequenceDiagram
participant User
participant DFetch
participant VCS as Git/SVN
participant ArchiveServer
participant LocalFS
User->>DFetch: invoke with manifest
DFetch->>VCS: fetch source
DFetch->>ArchiveServer: download archive (http/https)
DFetch->>LocalFS: extract/apply patches/write SBOM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@security/README.md`:
- Around line 3-6: Update the installation instruction that currently pins pytm
to the git master branch to use a stable release; replace the mutable git URL
(`git+https://github.com/OWASP/pytm.git@master`) with a PyPI pinned version for
pytm (e.g., pytm==1.3.1) so installation is reproducible and stable—edit the
README entry that contains the pip install line referencing pytm to use the
explicit version.
In `@security/tm_supply_chain.py`:
- Around line 149-155: The setting gh_actions_workflow.controls.isHardened is
incorrectly set to True while the surrounding text states the runner only audits
egress; change gh_actions_workflow.controls.isHardened to False and update its
inline comment to reflect that the workflow runner is audit-only for egress (or
otherwise make the prose and boolean consistent), and ensure related flags like
gh_actions_workflow.controls.providesIntegrity and
gh_actions_workflow.controls.hasAccessControl remain accurate relative to that
corrected egress posture.
In `@security/tm_usage.py`:
- Around line 336-344: The df05 Dataflow currently conflates HTTP and HTTPS and
sets controls.isEncrypted = False, so HTTPS downloads will be reported as
unencrypted; split df05 into two Dataflow entries (e.g., df05_plain and
df05_tls) or similar named flows: keep the existing df05 as the plain HTTP
variant with protocol "HTTP" and controls.isEncrypted = False and update its
description to mention only non-TLS archives, then add a new Dataflow (e.g.,
df05_tls) with protocol "HTTPS", controls.isEncrypted = True and a description
noting TLS and any redirect behavior; ensure any references to df05 are adjusted
to the correct variant and preserve the original title/message string ("DF-05:
Archive download request") or suffix it to distinguish the two.
🪄 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: 08ab64b0-6cca-4e96-9884-75b2bd91e6c5
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
There was a problem hiding this comment.
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/README.md`:
- Around line 10-19: The README and module docstrings are inconsistent about
working directory: update both the README examples and the module docstrings in
tm_supply_chain.py and tm_usage.py to use a single repo-root invocation style by
referencing the template with a repo-root-relative path (e.g.,
security/report_template.md) and showing commands like python -m
security.tm_supply_chain --report security/report_template.md (and same for
tm_usage) so the --report template resolves correctly when run from the
repository root.
🪄 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: 3f5fb48a-1d00-4adb-aac6-5de938318d6c
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pyproject.toml`:
- Around line 170-171: Remove the pylint suppression for "pytm" under
[tool.pylint.typecheck] and instead declare pytm as an optional dependency in
pyproject.toml by adding it to the [project.optional-dependencies] group (e.g.,
a "security" or "docs" extra); update any README that instructs manual git
installs to reference pip install dfetch[security] (or the chosen extra name) so
the threat-model modules that import pytm can be installed declaratively and
tooling picks it up without the ignored-modules entry.
In `@security/threats.json`:
- Around line 147-156: The condition for this SSRF redirect threat incorrectly
depends on target.controls.isEncrypted; change it to key off missing
redirect-destination validation instead. Update the "condition" expression
(currently referencing target.controls.isHardened and
target.controls.isEncrypted and len(target.protocol) > 0) to remove the
isEncrypted check and require a control that represents redirect validation
being absent (e.g. target.controls.validatesRedirectDestination is False or
target.controls.validatesRedirects is False) while keeping the isHardened check
and the protocol length check so the condition becomes something like:
target.controls.isHardened is False and
target.controls.validatesRedirectDestination is False and len(target.protocol) >
0.
- Around line 168-169: Update the "example" value that currently contains the
literal token-shaped string
"https://ci-bot:ghp_XXXXXXXXXX@github.com/corp/private-lib.git" so it uses a
neutral placeholder (e.g.
"https://ci-bot:<TOKEN>@github.com/corp/private-lib.git" or similar) to avoid
triggering secret scanners; edit the JSON entry for the "example" key to
substitute the token segment with a placeholder and ensure surrounding
punctuation and escaping remain valid JSON.
In `@security/tm_supply_chain.py`:
- Around line 388-409: Several Control entries (C-012, C-013, C-015, C-017,
C-022, C-024) reference non-existent asset IDs (e.g., SA-02, SA-01, PA-05); open
security/tm_supply_chain.py, locate each Control(...) block for those IDs and
replace the assets=[...] lists with the actual asset IDs defined in this module
(use the canonical IDs declared earlier in this file), ensuring every asset
referenced exists so build_asset_controls_index() can attach controls to real
assets; after editing, run the indexing/unit test that exercises
build_asset_controls_index() to confirm controls appear in per-asset output.
- Around line 7-11: Update the example usage strings so the --report flag
includes its required template argument; replace occurrences like "python -m
security.tm_supply_chain --report" with "python -m security.tm_supply_chain
--report REPORT" (or a concrete template path) in the docstring(s) in
security/tm_supply_chain.py and also mirror the same fix in security/tm_usage.py
so the CLI examples accurately show the required REPORT/template parameter.
🪄 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: 25e37198-86a8-4d0a-8876-a5aa2a61a2ad
📒 Files selected for processing (8)
pyproject.tomlsecurity/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
security/tm_usage.py (1)
421-438:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winArchive download flows still conflate HTTP and HTTPS.
df05anddf06declareprotocol = "HTTP or HTTPS"withisEncrypted = False, mirroring the issue called out previously. Because the VCS flows were split intodf03_tls/df03_plainanddf04_tls/df04_plain, the same model-accuracy gap exists here: HTTPS archive downloads will be reported as plaintext, and HSTS/TLS-only mitigations applied only to TLS variants will not be visible per-flow. Split intodf05_tls/df05_plain(anddf06_tls/df06_plain) consistent with the VCS pattern.🤖 Prompt for 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. In `@security/tm_usage.py` around lines 421 - 438, Split the combined df05 and df06 flows into TLS and plain variants like the VCS pattern: create df05_tls and df05_plain (instances of Dataflow currently named df05) and df06_tls and df06_plain (instances of Dataflow currently named df06); for the _tls variants set protocol = "HTTPS", controls.isEncrypted = True and retain other fields (description, controls.providesIntegrity as before), and for the _plain variants set protocol = "HTTP", controls.isEncrypted = False and keep the existing plaintext-risk wording in the description; ensure names df05_tls/df05_plain and df06_tls/df06_plain replace df05/df06 so downstream references match.security/README.md (1)
13-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWorking-directory convention still differs from the module docstrings.
The README invokes the models from inside
security/(cd security+python -m tm_supply_chain), whiletm_supply_chain.pyandtm_usage.pydocstrings showpython -m security.tm_supply_chain(repo-root invocation). Pick one convention and apply it consistently; otherwise users following the docstrings from the repo root will hit aFileNotFoundErroronreport_template.md.🤖 Prompt for 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. In `@security/README.md` around lines 13 - 22, README and module docstrings disagree on working-directory invocation: README shows running modules from inside the security directory while tm_supply_chain.py and tm_usage.py docstrings show repo-root invocation (python -m security.tm_supply_chain). Update either the README examples or the module docstrings to use the same convention; specifically, choose repo-root invocation (python -m security.tm_supply_chain and python -m security.tm_usage) or change the docstrings to match running from the security package (python -m tm_supply_chain), and ensure the example commands reference the correct path to report_template.md so running the chosen invocation does not raise FileNotFoundError.
🤖 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/threats.json`:
- Line 152: The condition uses a nonexistent Controls attribute
target.controls.validatesRedirectDestination which will raise AttributeError
during tm.process(); fix by either changing the condition to an existing
Controls attribute (e.g., target.controls.sanitizesInput or
target.controls.validatesInput) in the condition string, or register the new
attribute on pytm.Controls before evaluation (extend or patch Controls to
include validatesRedirectDestination) and ensure this registration runs prior to
tm.process(); update the condition entry and/or the Controls extension so DFT-12
evaluates without error.
---
Duplicate comments:
In `@security/README.md`:
- Around line 13-22: README and module docstrings disagree on working-directory
invocation: README shows running modules from inside the security directory
while tm_supply_chain.py and tm_usage.py docstrings show repo-root invocation
(python -m security.tm_supply_chain). Update either the README examples or the
module docstrings to use the same convention; specifically, choose repo-root
invocation (python -m security.tm_supply_chain and python -m security.tm_usage)
or change the docstrings to match running from the security package (python -m
tm_supply_chain), and ensure the example commands reference the correct path to
report_template.md so running the chosen invocation does not raise
FileNotFoundError.
In `@security/tm_usage.py`:
- Around line 421-438: Split the combined df05 and df06 flows into TLS and plain
variants like the VCS pattern: create df05_tls and df05_plain (instances of
Dataflow currently named df05) and df06_tls and df06_plain (instances of
Dataflow currently named df06); for the _tls variants set protocol = "HTTPS",
controls.isEncrypted = True and retain other fields (description,
controls.providesIntegrity as before), and for the _plain variants set protocol
= "HTTP", controls.isEncrypted = False and keep the existing plaintext-risk
wording in the description; ensure names df05_tls/df05_plain and
df06_tls/df06_plain replace df05/df06 so downstream references match.
🪄 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: b7363add-0a51-43d4-80c0-73695e22cdf2
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/report_template.md`:
- Around line 5-7: The "## Dataflow Diagram - Level 0 DFD" heading is left empty
so generated reports show a blank section; update the template by either
removing that heading or inserting a diagram placeholder under the heading
(e.g., a comment/placeholder like "{{dataflow_diagram}}" or a short "Diagram to
be added" note) so the "## Dataflow Diagram - Level 0 DFD" section renders
meaningful content and does not appear empty in outputs; reference the headings
"## Dataflow Diagram - Level 0 DFD" and "## Dataflows" when making the change.
In `@security/threats.json`:
- Around line 146-156: The DFT-12 condition is too broad—replace the current
expression that uses target.controls.sanitizesInput and len(target.protocol) > 0
with a check that limits the threat to redirect-capable archive downloads;
specifically update the "condition" for SID "DFT-12" to require
target.controls.isHardened is False AND (target.controls.followsRedirects is
True OR target.flow == "archive-download" OR "archive" in target.type) AND
target.protocol in ["http","https"] (or target.capabilities.followRedirects if
that property exists) so only flows that actually follow HTTP redirects for
archive retrieval trigger this SSRF finding.
- Line 74: Threat condition checks reference
target.controls.usesParameterizedInput which isn't defined for all Process
targets and causes AttributeError during tm.process(); fix by ensuring every
Process target in security/tm_usage.py (e.g., dfetch_cli, svn_export_proc,
patch_apply, archive_extract) has a controls attribute with
usesParameterizedInput set (True/False), or alternatively add
usesParameterizedInput to your Controls subclass used by those Processes or
reuse an existing pytm control field consistently; update the Process
instantiation or Controls subclass so tm.process() can safely evaluate the
conditions in security/threats.json (DFT-06/DFT-15/DFT-26) without raising.
In `@security/tm_common.py`:
- Around line 75-83: The make_network_boundary helper currently hard-codes
runtime-specific text into the Boundary description which is reused across
threat models; change make_network_boundary to accept an optional description
argument (e.g., def make_network_boundary(description: Optional[str] = None) ->
Boundary) and set b.description to that value when provided, otherwise use a
short, model-agnostic default; then update callers (the threat model
constructors that call make_network_boundary) to pass model-specific
descriptions (one for runtime/client behavior, another for supply-chain report)
so each model controls its own wording.
🪄 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: 297661d0-71a9-46af-92cb-3703b57cb066
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
security/report_template.md (1)
5-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win"Dataflow Diagram - Level 0 DFD" section still renders empty.
There is still no diagram placeholder or content under the heading on Line 5, so generated reports continue to contain a blank section between the heading and "## Dataflows". Either remove the heading or add a reference to the rendered diagram (e.g.,
or a{tm.dfd}-style placeholder).🤖 Prompt for 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. In `@security/report_template.md` around lines 5 - 7, The "## Dataflow Diagram - Level 0 DFD" heading is empty and leaves a blank section in generated reports; update the "## Dataflow Diagram - Level 0 DFD" section in report_template.md (before the following "## Dataflows" heading) to either remove the heading or insert a diagram placeholder/reference such as an image link `` or the tooling placeholder `{tm.dfd}` so the rendered report includes the Level 0 DFD.security/tm_supply_chain.py (1)
14-20: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueSame
sys.pathmutation astm_usage.py.Identical concern as in
security/tm_usage.py: importing this module silently inserts the repo root intosys.path. Consider sharing a single helper or gating the insert on Sphinx loading.🤖 Prompt for 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. In `@security/tm_supply_chain.py` around lines 14 - 20, The module tm_supply_chain.py is mutating sys.path at import time by computing _repo_root and inserting it into sys.path (sys.path.insert(0, _repo_root)), which duplicates the same pattern from tm_usage.py; refactor by extracting the path-insertion logic into a shared helper function (e.g., ensure_repo_root_on_sys_path()) used by both modules or gate the mutation behind a Sphinx-specific condition so the insertion only happens when rendering docs; update tm_supply_chain.py to call the new helper instead of directly computing _repo_root and calling sys.path.insert.security/threats.json (2)
146-156:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDFT-12 still keys on
sanitizesInput, which is unrelated to redirect handling.The condition
target.controls.isHardened is False and target.controls.sanitizesInput is False and len(target.protocol) > 0will match any non-hardened dataflow whose target lacks input sanitisation and has any protocol set. Intm_usage.pythat includes the VCS flows (df03_*,df04_*) that do not follow HTTP redirects, so they will be incorrectly flagged as "SSRF via unvalidated HTTP redirect chain". Narrow this to the archive-download flows that actually follow redirects (e.g. introduce afollowsRedirects-style control ondf05_*/df06_*and key the condition on it, or scopetargetto those flow names).🤖 Prompt for 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. In `@security/threats.json` around lines 146 - 156, DFT-12's condition incorrectly keys on target.controls.sanitizesInput and thus flags VCS flows (df03_*, df04_*) that don't follow redirects; update the threat rule to only apply to flows that actually follow HTTP redirects by adding or using a redirect-specific control (e.g., target.controls.followsRedirects) or by scoping target to archive-download flows (df05_*, df06_*), and change the condition to: target.controls.isHardened is False and target.controls.followsRedirects is True and len(target.protocol) > 0 so only non-hardened flows that follow redirects are matched.
74-74:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
usesParameterizedInputis still referenced forProcesstargets that don't set it, leaving DFT-06/DFT-15/DFT-26 prone toAttributeError.DFT-06 (line 74), DFT-15 (line 191), and DFT-26 (line 334) all key on
target.controls.usesParameterizedInput, but insecurity/tm_usage.pyonlydfetch_cliandsvn_export_procassign this control —patch_apply(lines 147–160) andarchive_extract(lines 162–180) do not. Whentm.process()evaluates these conditions against everyProcess, accessing the unset attribute will raiseAttributeError(the attribute is not part of pytm'sControlsclass).Either set
usesParameterizedInputexplicitly on everyProcessinstance, restrict these conditions to a control that is universally set (e.g.isHardened), or registerusesParameterizedInputon aControlssubclass beforetm.process()runs.#!/bin/bash # Confirm which Process targets define usesParameterizedInput; non-coverage causes AttributeError. rg -nP '\b(Process|Datastore|Dataflow|ExternalEntity|Actor)\s*\(' security/tm_usage.py security/tm_supply_chain.py echo "---" rg -nP '\.controls\.usesParameterizedInput\s*=' security/tm_usage.py security/tm_supply_chain.pyDoes pytm Controls class at commit 279ed14aa13ea8f0b989717812fd4626bfcddf3d define an attribute `usesParameterizedInput`? List the supported attributes.Also applies to: 191-191, 334-334
🤖 Prompt for 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. In `@security/threats.json` at line 74, DFT-06/DFT-15/DFT-26 reference target.controls.usesParameterizedInput but many Process targets don't define that control (causing AttributeError in tm.process()); fix by ensuring every Process sets the attribute or by making the condition use a universally-present control. Concretely, either (A) add target.controls.usesParameterizedInput = False/True on all Process instances that currently lack it (e.g. for targets created by patch_apply and archive_extract), or (B) change the conditions for DFT-06/DFT-15/DFT-26 to check a control that all Processes have (like isHardened) or register usesParameterizedInput on a Controls subclass before calling tm.process(); update references in tm_usage (dfetch_cli, svn_export_proc, patch_apply, archive_extract) and ensure tm.process() runs after the attribute is present.security/tm_common.py (1)
75-83: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
make_network_boundarystill embeds runtime-only language in a shared helper.Lines 79–82 still describe behaviour that only applies to the runtime model ("manifest URLs", "VCS clients"). Since
tm_supply_chain.pyalso calls this factory, the supply-chain report will inherit verbatim text that is not accurate for that model. Accept an optionaldescriptionargument or split the helper so each model owns its wording.♻️ Proposed change
-def make_network_boundary() -> Boundary: +def make_network_boundary(description: str | None = None) -> Boundary: """Create the *Internet* trust boundary.""" b = Boundary("Internet") - b.description = ( - "All traffic crossing the local/remote boundary. TLS enforcement is the " - "responsibility of the OS and VCS clients; dfetch does not enforce HTTPS " - "on manifest URLs." - ) + b.description = description or ( + "All traffic crossing the local/remote boundary." + ) return b🤖 Prompt for 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. In `@security/tm_common.py` around lines 75 - 83, Update the shared factory make_network_boundary to avoid embedding runtime-only wording: change the signature of make_network_boundary (the function that returns a Boundary named "Internet") to accept an optional description parameter (e.g., description: Optional[str] = None) and use the provided description when present, otherwise fall back to the current default text; then update callers (such as tm_supply_chain.py and the runtime model caller) to pass model-appropriate wording so supply-chain reports don't inherit runtime-specific phrases. Ensure the Boundary instantiation (Boundary("Internet")) and attribute assignment (b.description) remain but are driven by the new parameter.
🤖 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/threats.json`:
- Line 178: The condition for DFT-14 incorrectly couples setuid preservation
risk to bounds checking; update the condition expression (currently
"target.controls.checksInputBounds is True and target.controls.isHardened is
False") to only test the relevant mitigation control (e.g.,
"target.controls.isHardened is False" or a new control like
"target.controls.stripsSetuidBits is False") so the rule for archive_extract
(DFT-14) fires based on whether setuid bits are stripped, not on
checksInputBounds.
In `@security/tm_supply_chain.py`:
- Line 47: TM.reset() is being called at module import time in
tm_supply_chain.py (and similarly in tm_usage.py), causing import side-effects
that wipe previously-built models and leave dangling pytm references; remove the
top-level TM.reset() and the model construction from the module body,
encapsulate the reset and all model-building logic inside a new build_model()
function (e.g., def build_model(): TM.reset(); ... construct model ...; return
model), and call build_model() only from an if __name__ == '__main__' block (or
from explicit test/setup code) so importing the module no longer mutates global
TM state.
In `@security/tm_usage.py`:
- Around line 21-23: The module unconditionally mutates sys.path by computing
_repo_root and inserting it, which can silently rewrite import state when run as
a module; update the top-level logic around _repo_root and sys.path.insert to
only modify sys.path when necessary (e.g., guard the insertion with a check like
__package__ == "" or another explicit flag used by the Sphinx loader), or move
the insertion into a Sphinx-only entrypoint; specifically change the block that
defines _repo_root and calls sys.path.insert to be conditional (use the existing
_repo_root symbol and the sys.path.insert call) so normal imports/tests do not
mutate sys.path at import time.
- Around line 147-180: The patch_apply and archive_extract Process objects do
not set controls.usesParameterizedInput, causing AttributeError when threat
rules (DFT-06/15/26) access it; update both Process instances (patch_apply and
archive_extract) to explicitly assign controls.usesParameterizedInput = False or
= True as appropriate for those operations so the threat evaluator always finds
the attribute (alternatively remove/guard the threat conditions that assume its
presence), ensuring consistent behavior when tm.process() iterates processes.
- Line 487: The assignment to df08.data currently sets a single Data object
(embedded_url_credential) but pytm expects Dataflow.data to be a list; change
the assignment so df08.data is a list containing embedded_url_credential (e.g.,
set df08.data = [embedded_url_credential]) so iteration over flow.data in the
threat processing (see loops that call getattr(flow, "data", [])) works
correctly.
---
Duplicate comments:
In `@security/report_template.md`:
- Around line 5-7: The "## Dataflow Diagram - Level 0 DFD" heading is empty and
leaves a blank section in generated reports; update the "## Dataflow Diagram -
Level 0 DFD" section in report_template.md (before the following "## Dataflows"
heading) to either remove the heading or insert a diagram placeholder/reference
such as an image link `` or the tooling placeholder
`{tm.dfd}` so the rendered report includes the Level 0 DFD.
In `@security/threats.json`:
- Around line 146-156: DFT-12's condition incorrectly keys on
target.controls.sanitizesInput and thus flags VCS flows (df03_*, df04_*) that
don't follow redirects; update the threat rule to only apply to flows that
actually follow HTTP redirects by adding or using a redirect-specific control
(e.g., target.controls.followsRedirects) or by scoping target to
archive-download flows (df05_*, df06_*), and change the condition to:
target.controls.isHardened is False and target.controls.followsRedirects is True
and len(target.protocol) > 0 so only non-hardened flows that follow redirects
are matched.
- Line 74: DFT-06/DFT-15/DFT-26 reference target.controls.usesParameterizedInput
but many Process targets don't define that control (causing AttributeError in
tm.process()); fix by ensuring every Process sets the attribute or by making the
condition use a universally-present control. Concretely, either (A) add
target.controls.usesParameterizedInput = False/True on all Process instances
that currently lack it (e.g. for targets created by patch_apply and
archive_extract), or (B) change the conditions for DFT-06/DFT-15/DFT-26 to check
a control that all Processes have (like isHardened) or register
usesParameterizedInput on a Controls subclass before calling tm.process();
update references in tm_usage (dfetch_cli, svn_export_proc, patch_apply,
archive_extract) and ensure tm.process() runs after the attribute is present.
In `@security/tm_common.py`:
- Around line 75-83: Update the shared factory make_network_boundary to avoid
embedding runtime-only wording: change the signature of make_network_boundary
(the function that returns a Boundary named "Internet") to accept an optional
description parameter (e.g., description: Optional[str] = None) and use the
provided description when present, otherwise fall back to the current default
text; then update callers (such as tm_supply_chain.py and the runtime model
caller) to pass model-appropriate wording so supply-chain reports don't inherit
runtime-specific phrases. Ensure the Boundary instantiation
(Boundary("Internet")) and attribute assignment (b.description) remain but are
driven by the new parameter.
In `@security/tm_supply_chain.py`:
- Around line 14-20: The module tm_supply_chain.py is mutating sys.path at
import time by computing _repo_root and inserting it into sys.path
(sys.path.insert(0, _repo_root)), which duplicates the same pattern from
tm_usage.py; refactor by extracting the path-insertion logic into a shared
helper function (e.g., ensure_repo_root_on_sys_path()) used by both modules or
gate the mutation behind a Sphinx-specific condition so the insertion only
happens when rendering docs; update tm_supply_chain.py to call the new helper
instead of directly computing _repo_root and calling sys.path.insert.
🪄 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: cfcdb185-0dda-4ae6-8299-c465c67d7828
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_supply_chain.py`:
- Around line 739-742: The code dereferences sys.argv[_idx + 1] without
verifying a template path was provided, causing IndexError when "--report" is
last; modify the "--report" handling to validate that _idx + 1 < len(sys.argv)
and that sys.argv[_idx + 1] is not another flag (e.g., doesn’t start with "-")
before calling tm.resolve() and tm.report(...); if validation fails, exit early
with a clear usage error (raise SystemExit or print usage) instead of indexing,
referencing the existing variables and functions _idx, sys.argv, tm.resolve(),
and tm.report.
- Around line 273-281: Update the gh_actions_cache/A-08b and DF-18 descriptions
to acknowledge that cache-poisoning (DFT-28) is mitigated by ref-scoped cache
keys: explicitly state that build.yml includes `${{ github.ref_name }}` in both
`key` and `restore-keys`, which isolates PR and release caches per C-033, and
either remove the present-tense threat wording or reframe it as historical
context noting the implemented mitigation; ensure references to DFT-28 remain
but clearly mark it as mitigated by the ref-scoped key control.
In `@security/tm_usage.py`:
- Around line 60-66: The function currently instantiates b_archive,
archive_extract, svn_export_proc, local_vcs_cache, and audit_reports but only
returns b_dev and b_remote_vcs, leaving those elements and their flows out of
build_model(); update the return to include b_archive, archive_extract,
svn_export_proc, local_vcs_cache, and audit_reports, and add the corresponding
DataFlow connections (e.g., archive -> archive_extract, archive_extract ->
local_vcs_cache, svn_export_proc -> local_vcs_cache, local_vcs_cache ->
audit_reports, and audit_reports -> whatever consumer/send path exists) so
build_model() receives and can wire the archive/SVN/extraction/temp-cache/audit
report paths (apply same fix to the other similar blocks where those symbols are
instantiated).
- Around line 967-970: The code accesses sys.argv[_idx + 1] without ensuring an
argument follows "--report", causing IndexError; update the block that checks
for "--report" to verify that _idx + 1 < len(sys.argv) before indexing, and if
missing, call tm.resolve() then emit a clear usage/error message or raise
SystemExit instead of indexing; keep references to tm.resolve() and
tm.report(...) when implementing the guard so you still call tm.resolve() and
only call tm.report with the next argv entry when it exists.
🪄 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: 4fee2aee-20d2-4398-99b3-62b0378fc0db
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
security/tm_supply_chain.py (1)
359-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDF-18 and DF-19 still narrate cache-poisoning as unmitigated, contradicting C-033/A-08b.
A-08b (Line 278-283) and C-033 (Line 637-654) declare ref-scoped cache keys (
${{ github.ref_name }}) as an implemented control isolating PR and release caches. However, DF-18 still reads "Pull-request builds from forks can write to the same cache namespace as release builds, enabling cache-poisoning attacks (DFT-28, SLSA E6). Release builds should disable cache restore or use isolated cache keys.", and DF-19 still says "the release build may consume attacker-controlled artifacts". The generated report will be self-contradictory: the same model both asserts DFT-28 is open and that it is closed by C-033.Reframe DF-18/DF-19 to acknowledge the ref-scoped key mitigation (or remove the present-tense threat wording), consistent with the A-08b update.
🛠️ Suggested edit
df18.description = ( "GitHub Actions runner writes build cache entries (pip dependencies, gem packages, " "incremental build outputs) to the shared cache. " - "Pull-request builds from forks can write to the same cache namespace as " - "release builds, enabling cache-poisoning attacks (DFT-28, SLSA E6). " - "Release builds should disable cache restore or use isolated cache keys." + "Cache-poisoning from forked PRs (DFT-28, SLSA E6) is mitigated by ref-scoped " + "cache keys: build.yml includes ``${{ github.ref_name }}`` in both ``key`` and " + "``restore-keys`` (C-033), so PR and release caches are isolated per branch." ) df18.protocol = "HTTPS" df18.controls.isEncrypted = True + df18.controls.isHardened = True # ref-scoped cache keys per C-033 df19 = Dataflow(gh_actions_cache, gh_actions_runner, "DF-19: CI cache restore") df19.description = ( "GitHub Actions runner restores a previously written cache entry before " - "running build steps. If the restored entry was written by a less-privileged " - "workflow, the release build may consume attacker-controlled artifacts." + "running build steps. Release builds only restore entries whose keys include " + "the release tag ref (C-033), so a fork PR cannot pre-populate a cache slot " + "consumed by the release workflow." ) df19.protocol = "HTTPS" df19.controls.isEncrypted = True + df19.controls.isHardened = True # ref-scoped restore-keys per C-033🤖 Prompt for 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. In `@security/tm_supply_chain.py` around lines 359 - 376, DF-18 (df18) and DF-19 (df19) descriptions incorrectly state cache-poisoning as an unmitigated, present-tense threat contradicting the implemented ref-scoped cache-key control (A-08b / C-033); update the df18 and df19 description text to acknowledge that ref-scoped cache keys (e.g., ${{ github.ref_name }}) are implemented to isolate PR and release caches and thereby mitigate the cache-poisoning attack, and then note any residual risk only conditionally (e.g., "if ref-scoped keys are not used or misconfigured") or switch the wording to past/conditional tense so the narrative is consistent with the implemented control.security/tm_common.py (1)
14-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional
pytmimports to unblock lint/CI without changing dependency policy.Current hard imports fail in environments where
pytmis intentionally not installed, which is already breaking this PR’s pipeline (pylint E0401). Add a module-level import guard that preserves runtime behavior with a clear error when model execution is attempted.Suggested patch
-from pytm import Assumption, Boundary, Data, Element -from pytm.report_util import ReportUtils +try: + from pytm import Assumption, Boundary, Data, Element + from pytm.report_util import ReportUtils +except ImportError as exc: + _PYTM_IMPORT_ERROR = exc + + class _MissingPytm: + def __getattr__(self, _name: str) -> Any: + raise RuntimeError( + "pytm is required for threat-model execution. " + "Install it using security/README.md instructions." + ) from _PYTM_IMPORT_ERROR + + Assumption = Boundary = Data = Element = Any # type: ignore[assignment] + ReportUtils = _MissingPytm() # type: ignore[assignment]#!/bin/bash set -euo pipefail echo "Verify direct pytm imports are currently unguarded:" sed -n '1,40p' security/tm_common.py echo echo "Verify CI failure evidence mentions pylint import-error:" # If logs are stored in repo artifacts or docs, adjust this grep target accordingly. rg -n "E0401|Unable to import 'pytm'|pylint" .Based on learnings: do not add
pytmtopyproject.toml; keep installation guidance aligned withsecurity/README.md.🤖 Prompt for 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. In `@security/tm_common.py` around lines 14 - 15, The file imports pytm symbols (Assumption, Boundary, Data, Element, ReportUtils) at module level which breaks lint/CI when pytm is not installed; wrap those imports in a try/except ImportError block in security/tm_common.py, set a module-level flag like _HAS_PYTM or create lightweight dummy placeholders for the imported names that raise a clear RuntimeError when any model-execution function (e.g., functions or classes that use Assumption/Boundary/Data/Element/ReportUtils) is called; ensure normal behavior when pytm is present by keeping the real imports and raise an explicit error message only at runtime if someone tries to execute the model without pytm installed.
🤖 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/report_template.md`:
- Around line 62-63: The Assets table header row contains an extra trailing pipe
in the template string "Name|{{item.name}}|" which creates an unintended empty
third column; update the template so the header row uses "Name|{{item.name}}"
(remove the trailing pipe) wherever that snippet appears (the Assets section
header row) to ensure the table has the correct two columns.
In `@security/tm_supply_chain.py`:
- Around line 23-33: The import of pytm (from which TM, Actor, Boundary,
Classification, Data, Dataflow, Datastore, ExternalEntity, Process are imported)
triggers pylint E0401; fix by suppressing the import-error for pytm either by
adding the inline disable to that import (extend noqa/pylint disable to include
import-error) in the module(s) that import pytm (the line that imports TM,
Actor, ...), or preferably add pytm to the project's pylint configuration under
ignored-modules so all files importing pytm are covered centrally.
In `@security/tm_usage.py`:
- Around line 26-36: The pylint import error is raised for the optional pytm
import; update the import's pylint suppression to also disable import-error so
CI stops failing. Modify the import block that brings in TM, Actor, Boundary,
Classification, Data, Dataflow, Datastore, ExternalEntity, Process and add
import-error to the existing "# noqa: E402 # pylint:
disable=wrong-import-position" comment (e.g., change to "# noqa: E402 # pylint:
disable=wrong-import-position,import-error") so the linter ignores the missing
optional dependency.
---
Duplicate comments:
In `@security/tm_common.py`:
- Around line 14-15: The file imports pytm symbols (Assumption, Boundary, Data,
Element, ReportUtils) at module level which breaks lint/CI when pytm is not
installed; wrap those imports in a try/except ImportError block in
security/tm_common.py, set a module-level flag like _HAS_PYTM or create
lightweight dummy placeholders for the imported names that raise a clear
RuntimeError when any model-execution function (e.g., functions or classes that
use Assumption/Boundary/Data/Element/ReportUtils) is called; ensure normal
behavior when pytm is present by keeping the real imports and raise an explicit
error message only at runtime if someone tries to execute the model without pytm
installed.
In `@security/tm_supply_chain.py`:
- Around line 359-376: DF-18 (df18) and DF-19 (df19) descriptions incorrectly
state cache-poisoning as an unmitigated, present-tense threat contradicting the
implemented ref-scoped cache-key control (A-08b / C-033); update the df18 and
df19 description text to acknowledge that ref-scoped cache keys (e.g., ${{
github.ref_name }}) are implemented to isolate PR and release caches and thereby
mitigate the cache-poisoning attack, and then note any residual risk only
conditionally (e.g., "if ref-scoped keys are not used or misconfigured") or
switch the wording to past/conditional tense so the narrative is consistent with
the implemented control.
🪄 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: 8b91104c-20fc-4621-8d16-a9a4e6526ae2
📒 Files selected for processing (7)
security/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/threats.json`:
- Line 401: DFT-27 and DFT-29 use an overly broad condition
("target.controls.isHardened is False") that triggers on non-CI processes;
tighten the rule by requiring the target to be a CI/release pipeline as well —
change the condition to check both target.controls.isHardened is False AND
target.controls.isCiPipeline is True (or equivalent conjunction), and ensure
relevant supply-chain process definitions have target.controls.isCiPipeline set
to true so only CI/release processes are flagged; update the entries for DFT-27
and DFT-29 and add the isCiPipeline flag to the CI/release process objects where
appropriate.
- Line 446: DFT-30's condition is too broad; update the condition (the
"condition" value that currently reads "target.controls.providesIntegrity is
True") to specifically target hash-verification logic by adding a dedicated
control check such as "and target.controls.hashVerification is True" or "and
target.controls.enforcesHashAlgorithm is True" (or replace with whichever
project-standard flag exists for hash validation), so the rule only fires for
components that perform hash-based integrity checks rather than any
integrity-providing control.
🪄 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: ce139044-276f-40e4-9cfb-3363e0a51320
📒 Files selected for processing (8)
pyproject.tomlsecurity/README.mdsecurity/__init__.pysecurity/report_template.mdsecurity/threats.jsonsecurity/tm_common.pysecurity/tm_supply_chain.pysecurity/tm_usage.py
Contributor
Author
|
@coderabbitai resume |
Contributor
|
✅ Actions performedReviews resumed. Review triggered.
|
This was referenced May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features