diff --git a/mcp_zammad/models.py b/mcp_zammad/models.py index 29d0a4fb..04ddb2fe 100644 --- a/mcp_zammad/models.py +++ b/mcp_zammad/models.py @@ -269,8 +269,12 @@ class TicketCreate(StrictBaseModel): @field_validator("title", "article_body") @classmethod def sanitize_html(cls, v: str) -> str: - """Escape HTML to prevent XSS attacks.""" - return html.escape(v) + """Escape HTML to prevent XSS attacks. + + quote=False: title and the initial article are plain text (no content_type choice here), + sent/stored verbatim, so quotes and apostrophes must not become '/" entities. + """ + return html.escape(v, quote=False) class TicketUpdate(StrictBaseModel): @@ -341,7 +345,10 @@ class ArticleCreate(StrictBaseModel): def sanitize_body(self) -> "ArticleCreate": """Sanitize body content according to content type.""" if self.content_type == "text/plain": - self.body = html.escape(self.body) + # quote=False: plain text is sent/stored verbatim (e.g. in outbound emails), so quotes + # and apostrophes must not be turned into '/" entities. Still neutralize + # <, >, & in case a downstream renderer treats the body as HTML despite the content type. + self.body = html.escape(self.body, quote=False) else: self.body = self._sanitize_html_body(self.body) return self diff --git a/tests/test_models.py b/tests/test_models.py index f9761bea..b1d4971e 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -26,7 +26,7 @@ def test_html_sanitization_in_title(self): customer="test@example.com", article_body="Test body", ) - assert ticket.title == "<script>alert('XSS')</script>" + assert ticket.title == "<script>alert('XSS')</script>" def test_html_sanitization_in_body(self): """Test that HTML is escaped in article body.""" @@ -36,7 +36,7 @@ def test_html_sanitization_in_body(self): customer="test@example.com", article_body="Bold and ", ) - assert ticket.article_body == "<b>Bold</b> and <script>alert('XSS')</script>" + assert ticket.article_body == "<b>Bold</b> and <script>alert('XSS')</script>" def test_field_length_limits(self): """Test that field length limits are enforced.""" @@ -74,12 +74,17 @@ class TestArticleCreate: """Test ArticleCreate model validation.""" def test_html_sanitization_in_body(self): - """Test that HTML is escaped in article body.""" + """Test that HTML is escaped in article body, but quotes/apostrophes are left intact. + + text/plain bodies are sent verbatim (e.g. in outbound emails), so quotes must not + become '/" entities. <, >, & are still escaped, which already neutralizes + the tag regardless of the quote style used inside it. + """ article = ArticleCreate( ticket_id=123, body="
Click me
", ) - assert article.body == "<div onclick='alert()'>Click me</div>" + assert article.body == "<div onclick='alert()'>Click me</div>" def test_ticket_id_validation(self): """Test that ticket_id must be positive.""" diff --git a/tests/test_server.py b/tests/test_server.py index e5e7e7be..2354d433 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -627,6 +627,9 @@ def test_add_article_content_type_validation() -> None: plain_article = ArticleCreate(ticket_id=1, body="

plain

", content_type="text/plain") assert plain_article.body == "<p>plain</p>" + apostrophe_article = ArticleCreate(ticket_id=1, body="we've got it, you'd agree", content_type="text/plain") + assert apostrophe_article.body == "we've got it, you'd agree" + with pytest.raises(ValidationError, match="content_type"): ArticleCreate(ticket_id=1, body="test", content_type="application/json") # type: ignore[arg-type]