coderabbitai[bot]
commented
2 weeks ago Walkthrough
This update modifies how list attributes are handled in HTML content within the CKEditor5's HTML support package. Specifically, it adjusts the condition checks for attribute inheritance in nested lists and introduces a test case to ensure attributes from ancestor lists are not incorrectly applied to descendant lists.
Changes
| File Path | Change Summary |
|---|---|
.../ckeditor5-html-support/src/integrations/list.ts |
Modified condition in viewToModelListAttributeConverter to correctly handle list attributes. |
.../ckeditor5-html-support/tests/integrations/list.js |
Added a test case to verify correct attribute handling in nested lists. |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
| Ensure correct model construction during upcasting for nested lists (#16227) | ✅ | |
| Prevent post-fixer from firing unnecessarily after loading initial data due to attribute mishandling (#16227) | ✅ | |
| Refactor the list attribute handling to avoid revisiting model list items multiple times (#16227) | ❓ | The provided changes address the immediate bug, but it's unclear if a broader refactoring was considered or applied. |
The changes appear to directly address the main issues with attribute handling in nested lists, ensuring a more stable and accurate data model upon loading. However, the extent of any additional refactoring to optimize the process further is not specified, which might still be an area for future improvement.
Recent Review Details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**Commits
Files that changed from the base of the PR and between 97e41cd9e76d23fdc10de8936d584eb9f2a3dea6 and a933122024793562312d93f5a8b2e4fa0f135dce.Files selected for processing (2)
* packages/ckeditor5-html-support/src/integrations/list.ts (1 hunks) * packages/ckeditor5-html-support/tests/integrations/list.js (1 hunks)Additional Context Used
AST-based Instructions (4)
packages/ckeditor5-html-support/tests/integrations/list.js (4)
[hint] 351-351: Detected a call to replace or replaceAll in an attempt to HTML escape the string `JSON.stringify( listAttributes )`. Manually sanitizing input through a manually built list can be circumvented in many situations, and it's better to use a well known sanitization library such as `sanitize-html` or `DOMPurify`. Context: JSON.stringify( listAttributes ).replaceAll( '"', '"' ) Note: [OWASP A07:2017] https://owasp.org/www-project-top-ten/2017/A07_2017-Cross-Site_Scripting_(XSS).html [OWASP A03:2021] https://owasp.org/www-project-top-ten/2021/A03_2021-Injection.html [CWE-79] https://cwe.mitre.org/data/definitions/79.html [REFERENCES] - https://www.npmjs.com/package/dompurify - https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html --- [hint] 362-362: Detected a call to replace or replaceAll in an attempt to HTML escape the string `JSON.stringify( listItemAtributes )`. Manually sanitizing input through a manually built list can be circumvented in many situations, and it's better to use a well known sanitization library such as `sanitize-html` or `DOMPurify`. Context: JSON.stringify( listItemAtributes ).replaceAll( '"', '"' ) Note: [OWASP A07:2017] https://owasp.org/www-project-top-ten/2017/A07_2017-Cross-Site_Scripting_(XSS).html [OWASP A03:2021] https://owasp.org/www-project-top-ten/2021/A03_2021-Injection.html [CWE-79] https://cwe.mitre.org/data/definitions/79.html [REFERENCES] - https://www.npmjs.com/package/dompurify - https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html --- [hint] 869-869: Detected a call to replace or replaceAll in an attempt to HTML escape the string `JSON.stringify( { attributes: liAttributes } )`. Manually sanitizing input through a manually built list can be circumvented in many situations, and it's better to use a well known sanitization library such as `sanitize-html` or `DOMPurify`. Context: JSON.stringify( { attributes: liAttributes } ).replaceAll( '"', '"' ) Note: [OWASP A07:2017] https://owasp.org/www-project-top-ten/2017/A07_2017-Cross-Site_Scripting_(XSS).html [OWASP A03:2021] https://owasp.org/www-project-top-ten/2021/A03_2021-Injection.html [CWE-79] https://cwe.mitre.org/data/definitions/79.html [REFERENCES] - https://www.npmjs.com/package/dompurify - https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html --- [hint] 1329-1329: Detected a call to replace or replaceAll in an attempt to HTML escape the string `JSON.stringify( { attributes: listAttributes } )`. Manually sanitizing input through a manually built list can be circumvented in many situations, and it's better to use a well known sanitization library such as `sanitize-html` or `DOMPurify`. Context: JSON.stringify( { attributes: listAttributes } ).replaceAll( '"', '"' ) Note: [OWASP A07:2017] https://owasp.org/www-project-top-ten/2017/A07_2017-Cross-Site_Scripting_(XSS).html [OWASP A03:2021] https://owasp.org/www-project-top-ten/2021/A03_2021-Injection.html [CWE-79] https://cwe.mitre.org/data/definitions/79.html [REFERENCES] - https://www.npmjs.com/package/dompurify - https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
Additional comments not posted (2)
packages/ckeditor5-html-support/src/integrations/list.ts (1)
`219-219`: The updated condition in `viewToModelListAttributeConverter` correctly prevents inappropriate attribute inheritance in nested lists.packages/ckeditor5-html-support/tests/integrations/list.js (1)
`649-685`: The new test case effectively verifies that attributes from ancestor lists are not applied to nested lists, aligning with the PR's objectives.
Suggested merge commit message (convention)
Other (html-support): GHS list integration will no longer create a redundant operation to clean attributes in case of nested lists. Closes #16227.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.
Summary by CodeRabbit