fix: SHA-pin slsa_with_provenance action; explicit shell=False; fix C-009/C-024 doc errors#1278
Conversation
…-009/C-024 doc errors - Pin slsa-framework/source-actions/slsa_with_provenance to commit SHA dea965cdca5e0cb422bf7b2653c9d15f678ad01c (was tag v0.1.0, violating the C-009 commit-SHA pinning control for the most security-sensitive workflow) - Add explicit shell=False to subprocess.run() in cmdline.py; the default is False but the code did not state it, undermining the C-007 claim - Rename duplicate C-009 "Plaintext transport detection" (usage model) to C-045 to eliminate the control-ID conflict with C-009 "Actions commit-SHA pinning" (supply-chain model); update all references in compliance_data.py, compliance_track.rst, threat_model_usage.rst, control_register.rst - Rename C-024 from "secrets: inherit scope" to "Explicit secret forwarding" — the implementation deliberately avoids secrets: inherit and uses selective named forwarding; the old name was the inverse of the actual behaviour - Fix DFT-15 (VCS externals) accept rationale: the "Manifest under code review" assumption does not cover nested submodule/external URLs that come from the upstream repo, not dfetch.yaml; rebase on dfetch scope boundary assumption with explicit residual-risk statement - Add Risk Rating Methodology section to security.rst documenting the Sev/Risk scale (L/M/H/VH/C) aligned with BSI TR-03183-1 §5.3; add cross-reference from both threat model pages and update report_template - Clarify ECR-C SO.Updateability and ECR-M SO.SecureDataDeletion: add explanatory notes and gaps column text so "✓ Implemented" rows with no control listed are not misleading to conformity assessors - Update C-009 description from absolute "Every third-party Action" to "All third-party Actions" (accurate after the SHA-pin fix above) https://claude.ai/code/session_01CtU2HEmkrr4gBHvZMRT3U5
|
Warning Review limit reached
More reviews will be available in 50 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughPins the SLSA source-provenance GitHub Action to a commit SHA and adds ChangesSecurity Hardening (Code and Workflow)
Security Documentation and Compliance Data Refresh
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
🚥 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)
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 |
B603 is bandit's reminder-check for subprocess calls regardless of shell=False; the suppression acknowledges that cmd is list-form args constructed entirely from internal code, not from untrusted user input. Scoped to B603 only (was unscoped # nosec before). https://claude.ai/code/session_01CtU2HEmkrr4gBHvZMRT3U5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
security/tm_usage.py (1)
1366-1377:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the stale control ID in the DFT-26 note.
C-009now refers to commit-SHA pinning, so this note should point toC-045to stay aligned with the control register and generated docs.Diff
- "C-009 emits a visible warning immediately before the VCS command when a " + "C-045 emits a visible warning immediately before the VCS command when a "🤖 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 1366 - 1377, The DFT-26 definition contains a stale control ID reference in its note field. Replace the reference to "C-009" with "C-045" in the note text that describes the visible warning for plaintext VCS schemes. This ensures the note stays aligned with the current control register where C-009 now refers to commit-SHA pinning and C-045 is the correct control for this credential warning behavior.
🤖 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 `@doc/explanation/security.rst`:
- Around line 111-113: The Risk Rating Methodology section in
doc/explanation/security.rst (lines 111-113) lacks a dedicated reStructuredText
section label, causing existing cross-references to resolve to the page-level
security anchor instead of the section itself. Add a unique section label
(following reStructuredText convention) directly above the "Risk Rating
Methodology" heading in doc/explanation/security.rst. Then update the three
reference sites to point to this new section label instead of the generic
security anchor: in security/report_template.rst (lines 17-18), in
doc/explanation/threat_model_supply_chain.rst (lines 20-21), and in
doc/explanation/threat_model_usage.rst (lines 20-21). Ensure all three sibling
files use the same new section label name to maintain consistency.
---
Outside diff comments:
In `@security/tm_usage.py`:
- Around line 1366-1377: The DFT-26 definition contains a stale control ID
reference in its note field. Replace the reference to "C-009" with "C-045" in
the note text that describes the visible warning for plaintext VCS schemes. This
ensures the note stays aligned with the current control register where C-009 now
refers to commit-SHA pinning and C-045 is the correct control for this
credential warning behavior.
🪄 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: b8d7f337-9572-4e0f-a653-10de2b352b20
📒 Files selected for processing (11)
.github/workflows/source-provenance.ymldfetch/util/cmdline.pydoc/explanation/compliance_track.rstdoc/explanation/control_register.rstdoc/explanation/security.rstdoc/explanation/threat_model_supply_chain.rstdoc/explanation/threat_model_usage.rstsecurity/compliance_data.pysecurity/report_template.rstsecurity/tm_supply_chain.pysecurity/tm_usage.py
…09 in DFT-26 - Add .. _risk-rating-methodology: label above the section heading in security.rst so :ref: cross-references resolve to the section itself rather than the page-level anchor - Update all three reference sites to use <risk-rating-methodology>: security/report_template.rst, threat_model_supply_chain.rst, threat_model_usage.rst - Replace stale C-009 with C-045 in the DFT-26 note in tm_usage.py (C-009 is commit-SHA pinning; C-045 is plaintext transport detection) https://claude.ai/code/session_01CtU2HEmkrr4gBHvZMRT3U5
2 participants
dea965cdca5e0cb422bf7b2653c9d15f678ad01c (was tag v0.1.0, violating
the C-009 commit-SHA pinning control for the most security-sensitive
workflow)
is False but the code did not state it, undermining the C-007 claim
C-045 to eliminate the control-ID conflict with C-009 "Actions
commit-SHA pinning" (supply-chain model); update all references in
compliance_data.py, compliance_track.rst, threat_model_usage.rst,
control_register.rst
forwarding" — the implementation deliberately avoids secrets: inherit
and uses selective named forwarding; the old name was the inverse of
the actual behaviour
review" assumption does not cover nested submodule/external URLs that
come from the upstream repo, not dfetch.yaml; rebase on dfetch scope
boundary assumption with explicit residual-risk statement
Sev/Risk scale (L/M/H/VH/C) aligned with BSI TR-03183-1 §5.3; add
cross-reference from both threat model pages and update report_template
explanatory notes and gaps column text so "✓ Implemented" rows with no
control listed are not misleading to conformity assessors
"All third-party Actions" (accurate after the SHA-pin fix above)
https://claude.ai/code/session_01CtU2HEmkrr4gBHvZMRT3U5
Summary by CodeRabbit
Release Notes
Documentation
Chores