fix: add HTML escaping to sanitize=False computed Html fields#140
fix: add HTML escaping to sanitize=False computed Html fields#140gonzalesedwin1123 merged 6 commits into19.0from
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security of the application by introducing HTML escaping for dynamically generated content in various computed HTML fields. The primary goal is to mitigate potential Cross-Site Scripting (XSS) risks by ensuring that user-controlled or dynamic data rendered as HTML is properly sanitized, thereby preventing malicious script injection and improving overall data integrity and user safety. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces HTML escaping using markupsafe.escape to mitigate potential Cross-Site Scripting (XSS) vulnerabilities. The changes apply to various HTML fields in spp_audit_log.py, change_request.py, and preview_changes_wizard.py, ensuring dynamic content is properly sanitized before rendering. The review feedback indicates that while the current changes are a positive step, a comprehensive audit is still needed. Several other methods in spp_change_request_v2/models/change_request.py (e.g., _compute_review_documents_html, _compute_stage_banner_html, _compute_required_documents_html, _render_comparison_table, and _render_action_summary) still contain unescaped dynamic or user-provided data, posing continued XSS risks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #140 +/- ##
==========================================
+ Coverage 71.06% 71.34% +0.27%
==========================================
Files 925 932 +7
Lines 54704 54975 +271
==========================================
+ Hits 38876 39220 +344
+ Misses 15828 15755 -73
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request addresses a stored XSS vulnerability across the spp_audit and spp_change_request_v2 modules by implementing HTML escaping for dynamic content rendered within computed HTML fields. The markupsafe.escape function is now used to sanitize various fields, including audit log data, change request banners, document lists, registrant summaries, and preview HTML. Comprehensive unit tests have been added to both modules to verify the effectiveness of the HTML escaping. The review suggests enhancing the robustness of these new XSS tests by explicitly asserting the presence of escaped HTML entities, in addition to checking for the absence of unescaped ones.
…tml fields Escape dynamic values with markupsafe.escape() before inserting into f-string HTML to prevent stored XSS in audit logs, change request previews, and registrant summaries. Fixes #50
Verify that computed Html fields properly escape XSS payloads in dynamic values (script tags, img onerror, etc.) for audit logs, change request previews, registrant summaries, and preview wizard.
Address Gemini review: add html_escape() to _compute_stage_banner_html, _compute_required_documents_html, _compute_review_documents_html, _render_comparison_table, and _render_action_summary. Also fix ruff-format issues in test files.
spp_audit 19.0.2.0.0 → 19.0.2.0.1 spp_change_request_v2 19.0.2.0.2 → 19.0.2.0.3
gonzalesedwin1123
force-pushed
the
fix/html-escape-sanitize-false-fields
branch
from
29acaf9 to
aeb197f
Compare
… tests
Add assertIn("<script>") to two tests that only checked absence
of raw <script> tags, confirming the escaped data is actually present.
1 participant
Summary
markupsafe.escape()in computedfields.Htmlthat build HTML via f-strings, preventing stored XSSspp_audit: Escape values indata_htmlandparent_data_htmlaudit log tablesspp_change_request_v2: Escape registrant name/ID/address inregistrant_summary_html, and field diff values inpreview_html(both model and wizard)Fixes #50
Test plan
spp_audittests pass (19/19)spp_change_request_v2tests pass (286/286)