Add User Reporting System for Suspicious Bug Reports #5250
Conversation
…-suspicious-bug-reports
|
👋 Hi @sidd190! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-actions
bot
added
migrations
WalkthroughIntroduces an issue reporting system allowing authenticated users to report suspicious bug reports. Includes a new IssueReport model, database migration, four URL routes, six view functions for handling report submission and admin management, admin UI templates, user-facing report modal, and navigation updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| reason = request.POST.get("reason", "other") | ||
| description = request.POST.get("description", "") | ||
|
|
||
| if not description.strip(): | ||
| return JsonResponse({"status": "error", "message": "Please provide a description"}, status=400) | ||
|
|
||
| # Create the report | ||
| report = IssueReport.objects.create( | ||
| reporter=request.user, reported_issue=issue, reason=reason, description=description | ||
| ) |
There was a problem hiding this comment.
Bug: The report_issue function has a race condition where concurrent requests can bypass the duplicate check, causing an unhandled IntegrityError when creating a report.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
A race condition exists in the report_issue function. The code first checks if an IssueReport from a user on a specific issue already exists, and if not, it proceeds to create one. However, this check and creation are not atomic. If two concurrent requests from the same user for the same issue are processed, both can pass the initial existence check. The first request will successfully create the report. The second request's attempt to create a duplicate report will violate the unique_together database constraint on the IssueReport model, raising an unhandled IntegrityError. This causes the server to return a 500 error page instead of the expected JSON error response, leading to a poor user experience and breaking frontend error handling.
💡 Suggested Fix
Wrap the IssueReport.objects.create() call in a try...except IntegrityError block. In the except block, return a JsonResponse with an appropriate error message and a 400 status code, similar to the check for an existing report. This ensures that the race condition is handled gracefully without causing a server error.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: website/views/issue.py#L287-L300
Potential issue: A race condition exists in the `report_issue` function. The code first
checks if an `IssueReport` from a user on a specific issue already exists, and if not,
it proceeds to create one. However, this check and creation are not atomic. If two
concurrent requests from the same user for the same issue are processed, both can pass
the initial existence check. The first request will successfully create the report. The
second request's attempt to create a duplicate report will violate the `unique_together`
database constraint on the `IssueReport` model, raising an unhandled `IntegrityError`.
This causes the server to return a 500 error page instead of the expected JSON error
response, leading to a poor user experience and breaking frontend error handling.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6874317
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
website/templates/issue.html (1)
285-317: Consider using the existing notification system instead ofalert().The rest of the codebase uses
$.notify()for user feedback (seedeleteIssuefunction at line 268). Usingalert()here creates an inconsistent UX.function submitReport() { const reason = document.getElementById('reportReason').value; const description = document.getElementById('reportDescription').value; if (!description.trim()) { - alert('Please provide a description for your report.'); + $.notify('Please provide a description for your report.', { + style: "custom", + className: "danger" + }); return; } const formData = new FormData(); formData.append('reason', reason); formData.append('description', description); formData.append('csrfmiddlewaretoken', '{{ csrf_token }}'); fetch(`/report_issue/{{ object.id }}/`, { method: 'POST', body: formData, credentials: 'same-origin' }) .then(response => response.json()) .then(data => { if (data.status === 'success') { - alert(data.message); + $.notify(data.message, { + style: "custom", + className: "success" + }); closeReportModal(); } else { - alert(data.message || 'Failed to submit report'); + $.notify(data.message || 'Failed to submit report', { + style: "custom", + className: "danger" + }); } }) .catch(error => { console.error('Error:', error); - alert('Failed to submit report. Please try again.'); + $.notify('Failed to submit report. Please try again.', { + style: "custom", + className: "danger" + }); }); }website/templates/admin/issue_reports.html (2)
74-75: Potential XSS vector with inline JavaScript onclick handlers.While
escapejsescapes for JavaScript strings, passing user content directly to inline handlers can be risky. Consider usingdata-*attributes and attaching event listeners in JavaScript instead.-<button onclick="showReportDetails({{ report.id }}, '{{ report.description|escapejs }}', '{{ report.admin_notes|escapejs }}', '{{ report.status }}')" +<button data-report-id="{{ report.id }}" + data-description="{{ report.description|escapejs }}" + data-admin-notes="{{ report.admin_notes|escapejs }}" + data-status="{{ report.status }}" + onclick="showReportDetails(this)" class="text-indigo-600 hover:text-indigo-900 dark:text-indigo-400 dark:hover:text-indigo-300 mr-3"> View </button>Then update the JavaScript function:
function showReportDetails(element) { currentReportId = element.dataset.reportId; document.getElementById('reportDetailsDescription').textContent = element.dataset.description; document.getElementById('adminNotes').value = element.dataset.adminNotes; document.getElementById('reportStatus').value = element.dataset.status; document.getElementById('reportDetailsModal').classList.remove('hidden'); }
1-6: Missing admin access check in template.While the view enforces admin permissions, adding a defensive check in the template provides defense-in-depth. This template extends
base.htmlbut doesn't verify admin access at the template level.Consider wrapping the content in an admin check:
{% if request.user.is_superuser or request.user.is_staff %} <!-- existing content --> {% else %} <p>Access denied</p> {% endif %}website/templates/admin/issue_specific_reports.html (2)
23-24: Addrelattributes to external link to avoid reverse‑tabnabbingThe issue URL is opened in a new tab with
target="_blank"but withoutrel="noopener noreferrer", which can allow the target page to control the admin window. Addrel="noopener noreferrer"on this anchor.
71-82: JS modal wiring is generally safe; consider decoupling hard‑coded path and aligning CSRF with backendThe modal plumbing and use of
escapejs+.textContentlook good from an XSS perspective. Two improvements to consider:
- The update URL is hard‑coded as
/issue-reports/${currentReportId}/update/. If the URL pattern or prefix changes, this breaks silently. Prefer deriving it from{% url %}or putting the base path in adata-*attribute.- You append
csrfmiddlewaretokentoFormData, which is correct for Django, but the backend view (update_report_status) is currently marked@csrf_exempt. Either removecsrf_exemptand rely on CSRF protection or drop the unused token; keeping both aligned will avoid confusion.Also applies to: 107-112, 161-203
website/views/issue.py (2)
307-323: Admin reports overview looks fine; consider pagination and filtering laterThe admin overview correctly restricts access to staff/superusers and uses
select_relatedfor related objects. For large numbers of reports, loadingIssueReport.objects.all()without pagination or basic filtering (e.g., status) may become slow and hard to use, but that can be addressed incrementally if needed.
368-385: Per‑issue admin reports view is correct; consider pagination if counts grow
view_issue_specific_reportscorrectly:
- Enforces staff/superuser access.
- Uses
get_object_or_404(Issue, id=issue_id).- Filters
IssueReportbyreported_issueandselect_relatedfor the fields used in the template.- Provides
pending_countconsistent with the main reports view.For very report-heavy issues you may want pagination or ordering tweaks later, but functionally this is fine.
website/models.py (1)
1592-1625: IssueReport model aligns well with usage; consider adding indexes for common queriesThe model design matches how it’s used:
reporter/reported_issueFKs andunique_togetherenforce “one report per user per issue”.REPORT_REASONSandSTATUS_CHOICEScover the flows implemented in the views and templates.reviewed_by/reviewed_at/admin_notessupport the admin audit workflow.related_name="reports"onreported_issueis consistent with filters in the views.Given you frequently filter by
reported_issueand sometimes by(reported_issue, status), you might want to add an index (or composite index) on these fields to keep queries fast as the table grows:class IssueReport(models.Model): @@ admin_notes = models.TextField(blank=True, help_text="Admin notes about this report") class Meta: unique_together = ("reporter", "reported_issue") # Prevent duplicate reports from same user - ordering = ["-created"] + ordering = ["-created"] + indexes = [ + models.Index(fields=["reported_issue"], name="issuereport_issue_idx"), + models.Index(fields=["reported_issue", "status"], name="issuereport_issue_status_idx"), + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
blt/urls.py(3 hunks)website/migrations/0261_add_issue_reporting_system.py(1 hunks)website/models.py(1 hunks)website/templates/admin/issue_reports.html(1 hunks)website/templates/admin/issue_specific_reports.html(1 hunks)website/templates/includes/admin_sidenav.html(1 hunks)website/templates/includes/issue_right_info.html(1 hunks)website/templates/issue.html(2 hunks)website/views/issue.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
website/views/issue.py (1)
website/models.py (11)
Issue(590-739)IssueReport(1592-1625)save(74-77)save(277-289)save(1400-1427)save(1535-1538)save(1763-1766)save(1881-1895)save(1986-2008)save(2763-2766)save(3280-3290)
blt/urls.py (1)
website/views/issue.py (4)
report_issue(275-304)update_report_status(327-351)view_issue_reports(308-322)view_issue_specific_reports(369-385)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (10)
website/templates/includes/admin_sidenav.html (1)
10-16: LGTM!The new navigation item follows the existing sidenav pattern correctly. The conditional highlighting and URL linking are properly implemented.
website/templates/issue.html (2)
188-199: LGTM!The Report Issue button is correctly shown only to authenticated users who are not the issue owner. This prevents self-reporting while allowing legitimate reports.
207-243: LGTM!The modal structure is clean with appropriate form elements. The reason dropdown matches the model's
REASON_CHOICESfrom the migration.website/migrations/0261_add_issue_reporting_system.py (1)
15-37: LGTM!The migration is well-structured with appropriate:
- CASCADE delete on reporter and reported_issue (reports should be removed when source entities are deleted)
- SET_NULL on reviewed_by (preserves audit trail even if admin account is removed)
- unique_together constraint preventing duplicate reports
- Sensible default status of 'pending'
website/templates/includes/issue_right_info.html (1)
28-57: Verify context variables are passed from IssueView.The template uses
reports_countandpending_reports_count, but these must be passed from the view's context. EnsureIssueView.get_context_data()includes these variables for the template to render correctly.blt/urls.py (1)
593-596: Verify the ID type and authentication requirements for the report_issue view.Unable to determine whether the ID parameter should be
<int:id>or\w+without confirming the actual data type used by the Issue model. If Issue IDs are indeed integers, converting to<int:id>would improve type safety and consistency with the other admin URLs. Additionally, confirm whether thereport_issueview requires explicit@login_requireddecorator or if the request.user check provides sufficient authentication protection for your use case.website/views/issue.py (4)
354-366: Helper functions for report counts are straightforward and consistent with model
get_issue_reports_countandget_issue_pending_reports_countare simple, readable helpers and use thereported_issueFK consistently with theIssueReportmodel. No issues here; any heavier usage patterns can be optimized later if needed.
1888-1892: Admin‑only report counters inIssueViewcontext are appropriateThe new
reports_countandpending_reports_countare only computed for authenticated staff/superusers, avoiding overhead for regular users. Using the helper functions keeps the logic centralized and consistent with the admin views.
273-305: Verify CSRF and reason validation in report_issue view
report_issueis marked@csrf_exemptwhile performing a state-changing action for authenticated users. Remove@csrf_exemptand either use a standard POST form with{% csrf_token %}or ensure AJAX requests send the CSRF token in the header.Additionally,
reasonis taken directly fromrequest.POSTwithout validating againstIssueReport.REPORT_REASONSchoices. Validate thereasonvalue against the model's choices to reject unexpected values.Consider adding
@require_POSTto explicitly enforce POST-only access.
325-352: Remove@csrf_exemptfromupdate_report_statusand restrict to POSTThis endpoint mutates report state (calls
report.save()) and is only guarded by session authentication plus a staff/superuser check. The@csrf_exemptdecorator disables Django's built-in CSRF protection, creating a vulnerability where a CSRF attack against a logged-in staff member could silently modify report statuses and notes without their knowledge.Recommended:
- Remove
@csrf_exemptand rely on Django's CSRF middleware protection- Add
@require_POSTdecorator to make the HTTP contract explicit and prevent accidental state mutations from GET requests- Ensure the calling template includes
{% csrf_token %}in the form orX-CSRFTokenheader in AJAX requestsThe status validation and audit-field updates (
reviewed_by,reviewed_at) are appropriately implemented.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
github-actions
bot
added
tests: passed
|
💬 Reminder: Unresolved Conversations Hi @sidd190! This pull request has 1 unresolved conversation that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
github-actions
bot
added
last-active: 1d
1 participant
Summary
Implements a comprehensive reporting system that allows users to report suspicious bug reports and provides admins with efficient tools to review and manage these reports.
Problem
Fixes #2229
Previously, there was no mechanism for the community to flag potentially fraudulent, spam, or inappropriate bug reports. This made it difficult to maintain content quality and required manual monitoring by administrators.
Solution
Key Features
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.