fix: remove redundant htmlspecialchars from JSON API responses#94
fix: remove redundant htmlspecialchars from JSON API responses#94
Conversation
JSON encoding inherently prevents XSS — content is data, not rendered markup. The server-side HTML escaping created an unnecessary encode-then-decode roundtrip (server escapes → frontend _decodeEntities → raw HTML for CKEditor). The frontend already sanitizes via DOMParser before DOM insertion. Changes: - Remove htmlspecialchars() from AjaxController (chat, stream, tasks) - Remove htmlspecialchars() from CompleteResponse DTO - Remove _decodeEntities() from CowriterDialog.js frontend - Update all unit and E2E tests to expect raw content Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de> Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
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 refactors the application's XSS prevention strategy by removing redundant HTML escaping from JSON API responses on the backend. The change leverages the inherent XSS protection provided by JSON encoding and centralizes content sanitization to the frontend's Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Thanks for your first pull request! We are excited to have you contribute. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for solo maintainer project
This PR has passed all automated quality gates:
- ✅ Static analysis (PHPStan)
- ✅ Code style (PHP-CS-Fixer)
- ✅ Unit & functional tests
- ✅ Security scanning
- ✅ Dependency review
See SECURITY_CONTROLS.md for compensating controls documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
============================================
- Coverage 98.67% 98.66% -0.01%
Complexity 229 229
============================================
Files 14 14
Lines 1282 1277 -5
Branches 110 110
============================================
- Hits 1265 1260 -5
Misses 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 effectively addresses the redundant HTML escaping in JSON API responses by removing htmlspecialchars() calls from the PHP backend and the corresponding _decodeEntities() method from the JavaScript frontend. The updated approach correctly relies on json_encode for inherent XSS prevention in JSON data and delegates content sanitization for DOM insertion to the frontend. The comprehensive updates to unit and E2E tests confirm the new behavior, ensuring that raw content is now correctly transmitted and expected. This change simplifies the data flow and removes unnecessary encode/decode cycles, which is a significant improvement.
There was a problem hiding this comment.
Pull request overview
This PR removes server-side htmlspecialchars() escaping from cowriter JSON/SSE responses and updates the CKEditor integration and tests to work with raw (unescaped) content.
Changes:
- Stop HTML-escaping
content,model, andfinishReasoninAjaxControllerJSON and SSE responses. - Stop HTML-escaping
CompleteResponse::success()fields and update its documentation accordingly. - Remove the frontend
_decodeEntities()roundtrip and update unit/E2E tests to assert raw values.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Classes/Controller/AjaxController.php | Returns raw strings in JSON/SSE responses instead of HTML-escaped strings. |
| Classes/Domain/DTO/CompleteResponse.php | Removes escaping from the success factory and updates DTO docs. |
| Resources/Public/JavaScript/Ckeditor/CowriterDialog.js | Removes entity decoding and sanitizes preview directly from raw content. |
| Tests/Unit/Controller/AjaxControllerTest.php | Updates expectations to raw values; adjusts SSE assertions. |
| Tests/Unit/Domain/DTO/CompleteResponseTest.php | Updates assertions/provider to expect raw values. |
| Tests/E2E/CowriterWorkflowTest.php | Updates E2E assertions to accept raw “malicious” payload strings. |
Comments suppressed due to low confidence (2)
Tests/E2E/CowriterWorkflowTest.php:188
- The assertion is fine, but the comment is inaccurate: JSON encoding is not inherently XSS-safe. XSS depends on how the client renders the decoded string. Please adjust the comment to avoid implying that transport as JSON provides protection; instead reference the actual sinks in this extension that sanitize/escape before DOM insertion (or state that consumers must do so).
// Assert: Raw content returned in JSON (JSON encoding is inherently XSS-safe;
// frontend sanitizes via DOMParser before DOM insertion)
$data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
Tests/Unit/Controller/AjaxControllerTest.php:816
- The comment “JSON encoding handles safety” is misleading here as well: JSON encoding ensures valid JSON inside the SSE payload, but it doesn’t make the decoded
contentsafe to render as HTML. Suggest rephrasing to reflect that safety depends on downstream rendering (sanitization/escaping at the sink).
// Raw content in SSE data (JSON encoding handles safety)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix misleading "JSON encoding prevents XSS" docblocks — XSS prevention is the frontend's responsibility via DOMParser sanitization - Update integration and E2E tests to expect raw content instead of HTML-escaped entities (missed in initial commit) Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de> Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
There was a problem hiding this comment.
Automated approval for solo maintainer project
This PR has passed all automated quality gates:
- ✅ Static analysis (PHPStan)
- ✅ Code style (PHP-CS-Fixer)
- ✅ Unit & functional tests
- ✅ Security scanning
- ✅ Dependency review
See SECURITY_CONTROLS.md for compensating controls documentation.
Summary
Remove redundant
htmlspecialchars()encoding from all JSON API responses. JSON encoding (json_encode) inherently prevents XSS — content is transmitted as data, not rendered markup. The frontend already sanitizes viaDOMParserbefore DOM insertion.The old pattern created an unnecessary encode→decode roundtrip:
htmlspecialchars()escapes<,>,&,',"_decodeEntities()reverses the escaping via a hidden textarea_sanitizeHtml()sanitizes before DOM insertionNow the flow is simply:
_sanitizeHtml()sanitizes before DOM insertionType of Change
Related Issues
Checklist
make lintand it passesmake testand all tests passTest Plan
_decodeEntities)Files Changed
Classes/Controller/AjaxController.phphtmlspecialchars()from chat, stream, tasks responsesClasses/Domain/DTO/CompleteResponse.phphtmlspecialchars()fromsuccess()factoryResources/Public/JavaScript/Ckeditor/CowriterDialog.js_decodeEntities()method and usageTests/Unit/Controller/AjaxControllerTest.phpTests/Unit/Domain/DTO/CompleteResponseTest.phpTests/E2E/CowriterWorkflowTest.php