Skip to content

fix: replace innerHTML with textContent across core widgets for security#7757

Merged
walterbender merged 3 commits into
sugarlabs:masterfrom
NAME-ASHWANIYADAV:fix/jseditor-innerhtml-to-textcontent
Jul 9, 2026
Merged

fix: replace innerHTML with textContent across core widgets for security#7757
walterbender merged 3 commits into
sugarlabs:masterfrom
NAME-ASHWANIYADAV:fix/jseditor-innerhtml-to-textcontent

Conversation

@NAME-ASHWANIYADAV

@NAME-ASHWANIYADAV NAME-ASHWANIYADAV commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

This PR replaces multiple usages of innerHTML with safer DOM manipulation methods (textContent and createElement) across four core files (jseditor.js, turtles.js, palette.js, and toolbar-ui.js). These changes mitigate potential Cross-Site Scripting (XSS) risks by avoiding unnecessary HTML parsing for plain text updates (such as Material Icons and Unicode characters).

PR Category

  • Bug Fix - Fixes a bug or incorrect behavior
  • Feature - Adds new functionality
  • Performance - Improves performance (load time, memory, rendering, etc.)
  • Tests - Adds or updates test coverage
  • Documentation - Updates to docs, comments, or README
  • Chore / Refactor - Maintenance, cleanup, or refactoring with no behavior change
  • CI/CD - Changes to CI/CD workflows and automation

Changes Made

  • jseditor.js: Replaced innerHTML with textContent for   and console toggle arrows.
  • turtles.js: Replaced innerHTML with textContent when updating the Material Icons "menu" text.
  • palette.js: Swapped innerHTML for textContent on toggle arrows and refactored a complex innerHTML table header cell to use document.createElement.
  • toolbar-ui.js: Replaced innerHTML with textContent for plain text string assignments. Refactored icon tags (<i> and <img>) to use document.createElement(), and used node.cloneNode() instead of innerHTML to duplicate the theme select icon.

Testing Performed

  • Verified UI rendering for JS editor console arrows.
  • Verified turtle widget menu icons render correctly using textContent for Material Icons.
  • Ran npx jest js/widgets/__tests__/jseditor.test.js successfully (54/54 passed).
  • Ran npx jest js/__tests__/turtles.test.js successfully (45/45 passed).
  • Verified linting and Prettier checks passed completely.

Checklist

  • I have tested these changes locally and they work as expected.
  • I have added/updated tests that prove the effectiveness of these changes.
  • I have updated the documentation to reflect these changes, if applicable.
  • I have followed the project's coding style guidelines.
  • I have run npm run lint and npx prettier --check . with no errors.
  • I have addressed the code review feedback from the previous submission, if applicable.
  • I have enabled "Allow edits from maintainers" (required for auto-rebase; this only affects the PR branch, not your fork).

Additional Notes for Reviewers

These changes follow the same security improvements recently made in other widgets. Using textContent for Material Icon strings is fully supported by the CSS font-family implementation and avoids triggering the browser's HTML parser entirely.

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed area/javascript Changes to JS source files labels Jul 7, 2026
@github-actions github-actions Bot added the area/tests Changes to test files label Jul 7, 2026
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.81%. Comparing base (338362b) to head (319393f).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
js/palette.js 66.66% 2 Missing ⚠️
js/toolbar-ui.js 95.83% 1 Missing ⚠️
js/turtles.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7757      +/-   ##
==========================================
+ Coverage   56.80%   56.81%   +0.01%     
==========================================
  Files         172      172              
  Lines       57683    57701      +18     
==========================================
+ Hits        32767    32784      +17     
- Misses      24916    24917       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NAME-ASHWANIYADAV NAME-ASHWANIYADAV force-pushed the fix/jseditor-innerhtml-to-textcontent branch from e141ca7 to 319393f Compare July 7, 2026 20:11
@Ashutoshx7

Copy link
Copy Markdown
Member

@walterbender lgtm

@walterbender walterbender merged commit 7d3d648 into sugarlabs:master Jul 9, 2026
13 checks passed
rakshityadav1868 pushed a commit to rakshityadav1868/musicblocks that referenced this pull request Jul 9, 2026
…ity (sugarlabs#7757)

* fix: replace innerHTML with textContent in jseditor.js

* fix: replace innerHTML with textContent in turtles.js, palette.js, and toolbar-ui.js

* test: update toolbar and palette tests to match textContent refactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

4 participants