feat: add evidences to the audit action plan export#3924
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a read-only Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/core/serializers.py`:
- Line 1385: The serializer is calling the Evidence.filename as a function which
will raise; update all occurrences in the evidence attachment handling to access
the property (remove the parentheses) — e.g., inside the SerializerMethodField
handler (evidence_attachments / the method that builds attachments) replace
evidence.filename() with evidence.filename and similarly fix every occurrence in
the block that builds attachment dicts (the segments around the 1402-1415 and
1438 usages) so they read the filename property directly.
In `@backend/core/views.py`:
- Around line 11712-11721: The exported evidence text and filenames are written
raw into CSV/XLSX cells and need spreadsheet-formula escaping: create a small
helper like escape_spreadsheet_formula(value) that returns the original string
unless it starts with one of the formula-triggering characters ('=', '+', '-',
'@'), in which case it returns the string prefixed with a single quote ('). Then
apply this helper to each evidence.get("str") and evidence.get("filename")
before joining (the expressions that build the "\n".join(...) for
item.get("evidences", []) and item.get("evidence_attachments", [])), and also
update the analogous block around lines 11765-11774 so all exported
user-controlled cell values are escaped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81387276-dc2e-4d5e-8bdc-0c50a527b030
📒 Files selected for processing (2)
backend/core/serializers.pybackend/core/views.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/core/serializers.py (1)
1402-1415:⚠️ Potential issue | 🔴 CriticalFix
filenameproperty access to prevent export-time crash.Line [1405] calls
evidence.filename(); this should be property access (evidence.filename). As written, it can raise at runtime and break action-plan export serialization.Proposed fix
def get_evidence_attachments(self, obj): attachments = [] for evidence in obj.evidences.all(): - filename = evidence.filename() + filename = evidence.filename if filename: attachments.append( { "id": str(evidence.id), "str": str(evidence), "filename": filename, } ) return attachments#!/bin/bash set -euo pipefail # Verify how `filename` is defined on Evidence/EvidenceRevision model(s) rg -n -C3 --type=py 'class\s+Evidence\b|class\s+EvidenceRevision\b|@property|def\s+filename\b' backend/core/models.py # Verify remaining callable usages in serializers rg -nP --type=py '\bevidence\.filename\s*\(' backend/core/serializers.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/core/serializers.py` around lines 1402 - 1415, In get_evidence_attachments (serializer method) change the callable access evidence.filename() to property access evidence.filename to avoid raising at export-time; update the code that assigns filename = evidence.filename() to filename = evidence.filename, keep the rest of the loop intact and ensure any other occurrences in backend/core/serializers.py using evidence.filename(...) are similarly converted to property access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/core/serializers.py`:
- Around line 1402-1415: In get_evidence_attachments (serializer method) change
the callable access evidence.filename() to property access evidence.filename to
avoid raising at export-time; update the code that assigns filename =
evidence.filename() to filename = evidence.filename, keep the rest of the loop
intact and ensure any other occurrences in backend/core/serializers.py using
evidence.filename(...) are similarly converted to property access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bef65f6e-91df-4cd9-b822-2ecb759dd553
📒 Files selected for processing (2)
backend/core/serializers.pybackend/core/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/core/views.py
…intuitem/ciso-assistant-community into SUP-1175-add-applied-controls-to-proofs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/core/views.py`:
- Around line 11715-11727: The exported requirement strings from
requirement_assessments and covered_requirements are not being sanitized,
leaving a CSV/XLSX formula-injection risk; update both occurrences where you
currently do "\n".join([ra.get("str") for ra in
item.get("requirement_assessments")]) and the similar covered_requirements join
to call self.escape_spreadsheet_cell(...) for each entry (and likewise ensure
covered_requirements uses escape_spreadsheet_cell on each requirement string),
matching how evidences and evidence_attachments are escaped; keep the
newline-joining behavior but wrap each get("str")/requirement value with
self.escape_spreadsheet_cell to close the gap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
nas-tabchiche
changed the title
|
Not sure of the usefulness of having the evidence filename in the csv exports for the action plan. Maybe we should consider a zip export, like we do for audit exports. Thoughts @ab-smith? |
As a client feature request, I though it was probably useful (at least for some users) to have them but I don't know what other information than only the name and the attachment are that important to be exported in the audit's action plan. |
|
I just re-read the ticket, and it's unclear whether we are talking about the name of the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/core/views.py`:
- Around line 10260-10264: The current escape_spreadsheet_cell implementation
misses values with leading whitespace and doesn't reuse existing hardening
logic; replace its body to delegate to the existing escape_excel_formula helper
so it benefits from its None handling, string coercion and leading-whitespace
stripping; locate the staticmethod escape_spreadsheet_cell and have it
call/return escape_excel_formula(value) (or the equivalent public helper)
instead of doing the manual first-char check against ("=", "+", "-", "@").
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecff7b31-1d2a-4147-9134-834baf2ddf70
📒 Files selected for processing (2)
backend/core/serializers.pybackend/core/views.py
|
consider something like |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/views.py (1)
10787-10850:⚠️ Potential issue | 🟠 MajorApply IAM filtering before exporting evidence metadata.
These two actions now write
item["evidences"]anditem["evidence_attachments"]straight into the export, but this path only checks that the user can read the audit. Unless the serializer is already filtering by readableEvidenceIDs, any audit reader can learn evidence names and attachment filenames for linked controls they are not allowed to inspect. Please intersect the exported evidence lists withRoleAssignment.get_accessible_object_ids(..., Evidence)or move that filtering intoComplianceAssessmentActionPlanSerializerso CSV/XLSX stay aligned with normal object-level IAM behavior.Based on learnings, only aggregate analytics endpoints in
backend/core/views.pyintentionally skip IAM filtering for controls/evidence; object-level exports should remain filtered.Also applies to: 10869-10907
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/core/views.py` around lines 10787 - 10850, The export currently writes item["evidences"] and item["evidence_attachments"] without IAM filtering; restrict these lists to only evidence the requesting user can access by intersecting their IDs with RoleAssignment.get_accessible_object_ids(request.user, Evidence) (or move that filtering into ComplianceAssessmentActionPlanSerializer so CSV/XLSX output matches normal object-level IAM). Locate the export loop in views.py that iterates serializer.data and, before joining evidence strings/filenames, filter each evidence/attachment by presence in the accessible IDs set (apply same fix for the other occurrence around the 10869-10907 block) so only permitted evidence names/filenames are written.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/core/views.py`:
- Around line 10787-10850: The export currently writes item["evidences"] and
item["evidence_attachments"] without IAM filtering; restrict these lists to only
evidence the requesting user can access by intersecting their IDs with
RoleAssignment.get_accessible_object_ids(request.user, Evidence) (or move that
filtering into ComplianceAssessmentActionPlanSerializer so CSV/XLSX output
matches normal object-level IAM). Locate the export loop in views.py that
iterates serializer.data and, before joining evidence strings/filenames, filter
each evidence/attachment by presence in the accessible IDs set (apply same fix
for the other occurrence around the 10869-10907 block) so only permitted
evidence names/filenames are written.
3 participants
This PR adds the applied controls' evidences and their attachments to the document when exporting the audit's action plan.
How to test
Summary by CodeRabbit