Skip to content

fix: replace innerHTML with safer DOM APIs#137

Open
yuvrajangadsingh wants to merge 2 commits intoRocketChat:masterfrom
yuvrajangadsingh:fix/innerhtml-to-textcontent
Open

fix: replace innerHTML with safer DOM APIs#137
yuvrajangadsingh wants to merge 2 commits intoRocketChat:masterfrom
yuvrajangadsingh:fix/innerhtml-to-textcontent

Conversation

@yuvrajangadsingh
Copy link
Copy Markdown

Replaces all innerHTML assignments in application code (src/index.js and admin/src/index.js) with safer alternatives:

  • textContent where the content is plain text (total counts, repo names)
  • createElement + appendChild where HTML structure is needed (anchor tags, remove buttons)
  • table.deleteRow() loop instead of table.innerHTML = table.rows[0].innerHTML to clear table rows

10 instances fixed across 2 files. 2 remaining innerHTML hits are in vendored third-party code (admin/src/assets/layer/layer.js and admin/src/assets/layer/mobile/layer.js) and left untouched.

Closes #136

Server-side e2e tests pass (node --test, 2/2). Frontend changes are not covered by existing tests. Not tested locally in the browser.

AI disclosure: AI-assisted

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 30, 2026

CLA assistant check
All committers have signed the CLA.

@Sing-Li
Copy link
Copy Markdown
Member

Sing-Li commented Mar 30, 2026

@yuvrajangadsingh thanks

Server-side e2e tests pass (node --test, 2/2). Frontend changes are not covered by existing tests. Not tested locally in the browser.

Please DO PERFORM these tests that the agent indicated to be necessary - MANUALLY and supply the screen captures accordingly in the PR.

ALSO

2 remaining innerHTML hits are in vendored third-party code (admin/src/assets/layer/layer.js and admin/src/assets/layer/mobile/layer.js) and left untouched.

Wouldn't these two still expose the application to the SAME risk that you pointed out in #136 ???? If so, what good is this PR 🤔

@yuvrajangadsingh
Copy link
Copy Markdown
Author

yuvrajangadsingh commented Mar 31, 2026

tested locally. ran the app with node src/app.js, verified both pages.

leaderboard page:

leaderboard

  • table renders correctly with avatars, clickable username links, Open PRs / Merged PRs / Issues columns
  • totals row shows correct counts
  • footer links (homepage | GitHub) render as separate <a> elements built with DOM APIs
  • refreshTable() clears old rows using deleteRow() loop, repopulates cleanly

admin panel:

admin

  • login works
  • contributor list renders with "Total: 3" counter
  • all usernames appear as proper <a> links
  • each row has a working "Remove" button built with createElement/textContent
  • Include/Exclude Repos, Start Date, Refresh Interval controls all render and function

regarding the 2 layer.js innerHTML instances:

checked both files. they are safe:

  1. desktop layer.js: t[0].innerHTML = "" inside close/cleanup. empties a popup node before removal. hardcoded empty string, no data flows in.

  2. mobile layer.js:

    • s.innerHTML = ... builds popup HTML from the library own config (shade, CSS classes, animation). only external value is username from layer.confirm(), sourced from server config / GitHub API, not free-form user input.
    • n.innerHTML = "" same cleanup pattern, hardcoded empty string.

these are vendored third-party code (layui layer.js v3.1.1 and mobile v2.0.0). the innerHTML calls consume internal config and hardcoded strings, not browser user input. modifying minified vendored code creates maintenance burden with no security benefit.

yuvrajangadsingh added a commit to yuvrajangadsingh/Opensource-Contribution-Leaderboard that referenced this pull request Mar 31, 2026
@yuvrajangadsingh
Copy link
Copy Markdown
Author

@Sing-Li screenshots and vendored code analysis added above. let me know if you want me to dig deeper into the layer.js instances or if the explanation checks out.

@Sing-Li
Copy link
Copy Markdown
Member

Sing-Li commented Apr 3, 2026

@yuvrajangadsingh very good work. Please resolve conflict and test one more time manually after. Please use the data file https://github.com/RocketChat/Opensource-Contribution-Leaderboard/blob/master/contrib/rocketchat/gsoc/2025/gsoc2025final.json when testing.

Switch innerHTML assignments to textContent where content is plain text,
and use createElement/appendChild where HTML structure is needed.

Closes RocketChat#136
@yuvrajangadsingh yuvrajangadsingh force-pushed the fix/innerhtml-to-textcontent branch from 0df6896 to aabcbfd Compare April 3, 2026 07:30
@yuvrajangadsingh
Copy link
Copy Markdown
Author

conflicts resolved, rebased on latest master.

tested again with the gsoc 2025 data file (251 contributors):

  • leaderboard page loads, table renders with all contributors
  • admin panel loads, login works, contributor management works
  • all API endpoints working (/api/data, /api/config, /api/stats, /api/rank, /api/contributor)
  • zero innerHTML in app source or rendered output
  • only expected error: GitHub Token is not correct from refresh.js since no AUTH_TOKEN was set, doesn't affect display

ready for merge whenever you are @Sing-Li

@yuvrajangadsingh
Copy link
Copy Markdown
Author

yuvrajangadsingh commented Apr 3, 2026

screenshots with gsoc2025final.json (251 contributors):

leaderboard:
leaderboard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

potential XSS vectors and code quality findings

3 participants