Merge pull request #140 from OpenSPP/fix/html-escape-sanitize-false-fields

fix: add HTML escaping to sanitize=False computed Html fields

Author: gonzalesedwin1123

spp_audit/README.rst

@@ -154,6 +154,8 @@ Changelog
   all records, not just the first
 - refactor: use ``isinstance(value, Markup)`` instead of fragile
   ``str(type(...))`` comparison in all audit decorator methods
+- fix: add HTML escaping to computed ``data_html`` and
+  ``parent_data_html`` fields to prevent stored XSS (#50)
 
 19.0.2.0.0
 ~~~~~~~~~~

spp_audit/models/spp_audit_log.py

@@ -2,6 +2,7 @@
 import logging
 
 from dateutil import tz
+from markupsafe import escape as html_escape
 
 from odoo import _, api, fields, models
 from odoo.exceptions import UserError
@@ -164,7 +165,7 @@ def _compute_data_html(self):
             for line in rec._get_content():
                 row = ""
                 for item in line:
-                    row += f"<td>{item}</td>"
+                    row += f"<td>{html_escape(str(item))}</td>"
                 tbody += f"<tr>{row}</tr>"
             tbody = f"<tbody>{tbody}</tbody>"
             rec.data_html = f'<table class="o_list_view table table-condensed table-striped">{thead}{tbody}</table>'
@@ -206,7 +207,7 @@ def _compute_parent_data_html(self):
             for line in rec._parent_get_content():
                 row = ""
                 for item in line:
-                    row += f"<td>{item}</td>"
+                    row += f"<td>{html_escape(str(item))}</td>"
                 tbody += f"<tr>{row}</tr>"
             tbody = f"<tbody>{tbody}</tbody>"
             rec.parent_data_html = (

spp_audit/readme/HISTORY.md

@@ -3,6 +3,7 @@
 - fix: use @api.model_create_multi for audit_create to support Odoo 19 create overrides (#138)
 - fix: Markup sanitization in audit_write and audit_unlink now handles all records, not just the first
 - refactor: use `isinstance(value, Markup)` instead of fragile `str(type(...))` comparison in all audit decorator methods
+- fix: add HTML escaping to computed `data_html` and `parent_data_html` fields to prevent stored XSS (#50)
 
 ### 19.0.2.0.0
 

spp_audit/static/description/index.html

@@ -524,6 +524,8 @@ <h1>19.0.2.0.1</h1>
 all records, not just the first</li>
 <li>refactor: use <tt class="docutils literal">isinstance(value, Markup)</tt> instead of fragile
 <tt class="docutils literal"><span class="pre">str(type(...))</span></tt> comparison in all audit decorator methods</li>
+<li>fix: add HTML escaping to computed <tt class="docutils literal">data_html</tt> and
+<tt class="docutils literal">parent_data_html</tt> fields to prevent stored XSS (#50)</li>
 </ul>
 </div>
 <div class="section" id="section-2">

spp_audit/tests/__init__.py

@@ -1,2 +1,3 @@
 from . import test_audit_backend
+from . import test_html_escaping
 from . import test_spp_audit_rule

spp_audit/tests/test_html_escaping.py

@@ -0,0 +1,90 @@
+from odoo.tests.common import TransactionCase
+
+
+class TestAuditHtmlEscaping(TransactionCase):
+    """Tests that audit log HTML fields properly escape dynamic values."""
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.model_partner = cls.env["ir.model"].search([("model", "=", "res.partner")], limit=1)
+        cls.audit_rule = cls.env["spp.audit.rule"].search([("model_id", "=", cls.model_partner.id)], limit=1)
+        if not cls.audit_rule:
+            cls.audit_rule = cls.env["spp.audit.rule"].create(
+                {
+                    "name": "Test Rule",
+                    "model_id": cls.model_partner.id,
+                    "is_log_create": True,
+                    "is_log_write": True,
+                    "is_log_unlink": False,
+                }
+            )
+
+    def _create_audit_log(self, old_vals, new_vals):
+        """Create an audit log record with given old/new values."""
+        data = repr({"old": old_vals, "new": new_vals})
+        return self.env["spp.audit.log"].create(
+            {
+                "audit_rule_id": self.audit_rule.id,
+                "user_id": self.env.uid,
+                "model_id": self.model_partner.id,
+                "res_id": 1,
+                "method": "write",
+                "data": data,
+            }
+        )
+
+    def test_data_html_escapes_script_tags(self):
+        """Verify data_html escapes <script> in field values."""
+        xss_payload = '<script>alert("xss")</script>'
+        log = self._create_audit_log(
+            old_vals={"name": "Safe Name"},
+            new_vals={"name": xss_payload},
+        )
+        html = log.data_html
+        self.assertNotIn("<script>", html)
+        self.assertIn("&lt;script&gt;", html)
+
+    def test_data_html_escapes_html_entities(self):
+        """Verify data_html escapes angle brackets and ampersands."""
+        log = self._create_audit_log(
+            old_vals={"name": "Before <b>bold</b>"},
+            new_vals={"name": "After <img src=x onerror=alert(1)>"},
+        )
+        html = log.data_html
+        self.assertNotIn("<img ", html)
+        self.assertIn("&lt;img ", html)
+        self.assertNotIn("<b>bold</b>", html)
+        self.assertIn("&lt;b&gt;bold&lt;/b&gt;", html)
+
+    def test_data_html_renders_table_structure(self):
+        """Verify data_html still produces valid table structure."""
+        log = self._create_audit_log(
+            old_vals={"name": "Old"},
+            new_vals={"name": "New"},
+        )
+        html = log.data_html
+        self.assertIn("<table", html)
+        self.assertIn("<thead>", html)
+        self.assertIn("<tbody>", html)
+        self.assertIn("<td>", html)
+
+    def test_parent_data_html_escapes_script_tags(self):
+        """Verify parent_data_html escapes <script> in field values."""
+        xss_payload = '<script>alert("xss")</script>'
+        parent_model = self.env["ir.model"].search([("model", "=", "res.partner")], limit=1)
+        log = self.env["spp.audit.log"].create(
+            {
+                "audit_rule_id": self.audit_rule.id,
+                "user_id": self.env.uid,
+                "model_id": self.model_partner.id,
+                "res_id": 1,
+                "method": "write",
+                "data": repr({"old": {"name": "Safe"}, "new": {"name": xss_payload}}),
+                "parent_model_id": parent_model.id,
+                "parent_res_ids_str": "1",
+            }
+        )
+        html = log.parent_data_html
+        self.assertNotIn("<script>", html)
+        self.assertIn("&lt;script&gt;", html)

spp_change_request_v2/README.rst

@@ -853,6 +853,12 @@ Before declaring a new CR type complete:
 Changelog
 =========
 
+19.0.2.0.3
+~~~~~~~~~~
+
+- fix: add HTML escaping to all computed Html fields with
+  ``sanitize=False`` to prevent stored XSS (#50)
+
 19.0.2.0.2
 ~~~~~~~~~~
 

spp_change_request_v2/__manifest__.py

@@ -1,6 +1,6 @@
 {
     "name": "OpenSPP Change Request V2",
-    "version": "19.0.2.0.2",
+    "version": "19.0.2.0.3",
     "sequence": 50,
     "category": "OpenSPP",
     "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention",

spp_change_request_v2/models/change_request.py

@@ -380,11 +380,11 @@ def _compute_target_type_message(self):
     @api.depends("name", "request_type_id", "registrant_id")
     def _compute_stage_banner_html(self):
         for rec in self:
-            cr_ref = rec.name or ""
-            cr_type = rec.request_type_id.name if rec.request_type_id else ""
+            cr_ref = html_escape(rec.name or "")
+            cr_type = html_escape(rec.request_type_id.name) if rec.request_type_id else ""
             html = f'<span class="fw-bold">{cr_ref}</span><span class="text-muted mx-2">|</span><span>{cr_type}</span>'
             if rec.registrant_id:
-                registrant = rec.registrant_id.name or ""
+                registrant = html_escape(rec.registrant_id.name or "")
                 html += (
                     f'<span class="text-muted mx-2">|</span>'
                     f'<i class="fa fa-user me-1 text-muted"></i>'
@@ -422,14 +422,11 @@ def _compute_required_documents_html(self):
             uploaded_types = rec.document_ids.mapped("document_type_id").filtered(lambda c: c)
             items = []
             for doc_type in required:
+                escaped_name = html_escape(doc_type.display_name)
                 if doc_type in uploaded_types:
-                    items.append(
-                        f'<li class="text-success"><i class="fa fa-check-circle me-1"></i>{doc_type.display_name}</li>'
-                    )
+                    items.append(f'<li class="text-success"><i class="fa fa-check-circle me-1"></i>{escaped_name}</li>')
                 else:
-                    items.append(
-                        f'<li class="text-danger"><i class="fa fa-times-circle me-1"></i>{doc_type.display_name}</li>'
-                    )
+                    items.append(f'<li class="text-danger"><i class="fa fa-times-circle me-1"></i>{escaped_name}</li>')
 
             rec.required_documents_html = (
                 '<div class="mb-3">'
@@ -508,8 +505,8 @@ def _compute_review_documents_html(self):
             html.append("<tbody>")
 
             for doc in rec.document_ids:
-                doc_name = doc.name or ""
-                doc_type = doc.document_type_id.display_name if doc.document_type_id else ""
+                doc_name = html_escape(doc.name or "")
+                doc_type = html_escape(doc.document_type_id.display_name) if doc.document_type_id else ""
                 uploaded = doc.create_date.strftime("%Y-%m-%d") if doc.create_date else ""
                 html.append(
                     f"<tr>"
@@ -541,19 +538,20 @@ def _compute_registrant_summary_html(self):
                 html_parts.append('<i class="fa fa-users fa-lg text-primary me-2"></i>')
             else:
                 html_parts.append('<i class="fa fa-user fa-lg text-primary me-2"></i>')
-            html_parts.append(f"<strong>{reg.name}</strong>")
+            html_parts.append(f"<strong>{html_escape(reg.name or '')}</strong>")
             html_parts.append("</div>")
 
             # ID badge
             if hasattr(reg, "spp_id") and reg.spp_id:
-                html_parts.append(f'<div class="mb-2"><span class="badge bg-secondary">ID: {reg.spp_id}</span></div>')
+                escaped_id = html_escape(reg.spp_id)
+                html_parts.append(f'<div class="mb-2"><span class="badge bg-secondary">ID: {escaped_id}</span></div>')
 
             # Address
             address_parts = []
             if reg.street:
-                address_parts.append(reg.street)
+                address_parts.append(html_escape(reg.street))
             if reg.city:
-                address_parts.append(reg.city)
+                address_parts.append(html_escape(reg.city))
             if address_parts:
                 html_parts.append(
                     f'<div class="text-muted small mb-2">'
@@ -1073,7 +1071,7 @@ def _generate_preview_html(self):
         action_label = action_labels.get(action, action.replace("_", " ").title())
         html_parts.append(
             f'<div class="mb-3 d-flex align-items-center">'
-            f'<span class="badge bg-primary me-2">{action_label}</span>'
+            f'<span class="badge bg-primary me-2">{html_escape(action_label)}</span>'
             f"</div>"
         )
 
@@ -1085,7 +1083,7 @@ def _generate_preview_html(self):
             for key, value in changes.items():
                 if key.startswith("_"):
                     continue
-                display_key = key.replace("_", " ").title()
+                display_key = html_escape(key.replace("_", " ").title())
 
                 # Handle dict with old/new structure
                 if isinstance(value, dict) and "new" in value:
@@ -1095,16 +1093,16 @@ def _generate_preview_html(self):
                     if old_val is None or old_val is False or old_val == "":
                         old_display = '<span class="text-muted">—</span>'
                     else:
-                        old_display = str(old_val)
+                        old_display = html_escape(str(old_val))
                     # Format new value
                     if new_val is None or new_val is False or new_val == "":
                         new_display = '<span class="text-muted">—</span>'
                     else:
-                        new_display = f"<strong>{new_val}</strong>"
+                        new_display = f"<strong>{html_escape(str(new_val))}</strong>"
                     display_value = f"{old_display} → {new_display}"
                 elif isinstance(value, list):
                     if value:
-                        display_value = "<br/>".join(str(v) for v in value)
+                        display_value = "<br/>".join(html_escape(str(v)) for v in value)
                     else:
                         display_value = '<span class="text-muted">Not set</span>'
                 elif value is None or value is False or value == "":
@@ -1114,7 +1112,7 @@ def _generate_preview_html(self):
                     # Only True reaches here (False caught above)
                     display_value = '<span class="badge text-bg-success">Yes</span>'
                 else:
-                    display_value = str(value)
+                    display_value = html_escape(str(value))
 
                 html_parts.append(f"<tr><td><strong>{display_key}</strong></td><td>{display_value}</td></tr>")
 
@@ -1167,7 +1165,7 @@ def _render_comparison_table(self, changes, header=None):
         """Render a three-column comparison table for field-mapping CR types."""
         html = []
         if header:
-            html.append(f"<h4>{header}</h4>")
+            html.append(f"<h4>{html_escape(header)}</h4>")
         html.append('<table class="table table-sm table-bordered mb-0" style="width:100%">')
         html.append(
             "<thead><tr>"
@@ -1182,7 +1180,7 @@ def _render_comparison_table(self, changes, header=None):
             if key.startswith("_"):
                 continue
             # Use key as-is if it contains spaces (human-readable), otherwise convert
-            display_key = key if " " in key else key.replace("_", " ").title()
+            display_key = html_escape(key if " " in key else key.replace("_", " ").title())
 
             if isinstance(value, dict) and "old" in value:
                 old_val = value.get("old")
@@ -1220,7 +1218,7 @@ def _render_action_summary(self, action, changes, header=None):
         html = []
 
         if header:
-            html.append(f"<h4>{header}</h4>")
+            html.append(f"<h4>{html_escape(header)}</h4>")
 
         if not changes:
             html.append('<p class="text-muted mb-0"><i class="fa fa-info-circle me-2"></i>No details to display.</p>')
@@ -1233,7 +1231,7 @@ def _render_action_summary(self, action, changes, header=None):
         for key, value in changes.items():
             if key.startswith("_"):
                 continue
-            display_key = key if " " in key else key.replace("_", " ").title()
+            display_key = html_escape(key if " " in key else key.replace("_", " ").title())
             display_value = self._format_review_value(value)
             html.append(f'<tr><td class="bg-light"><strong>{display_key}</strong></td><td>{display_value}</td></tr>')
 

spp_change_request_v2/readme/HISTORY.md

@@ -1,3 +1,7 @@
+### 19.0.2.0.3
+
+- fix: add HTML escaping to all computed Html fields with `sanitize=False` to prevent stored XSS (#50)
+
 ### 19.0.2.0.2
 
 - fix: fix batch approval wizard line deletion (#130)