Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't use innerHTML to populate drag item (DH-18645) #2378

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Mar 3, 2025

  • We were using innerHTML with the text of the name of the file to create the drag placeholder content
  • Instead, inject the name using innerText so it is escaped properly
  • Tested by naming a file <img src=q onerror=prompt(1)>.py, and then attempting to move it. It no longer triggered the popup.

- We were using `innerHTML` with the text of the name of the file to create the drag placeholder content
- Instead, inject the name using `innerText` so it is escaped properly
- Tested by naming a file `<img src=q onerror=prompt(1)>.py`, and then attempting to move it. It no longer triggered the popup.
@mofojed mofojed requested review from mattrunyon and Copilot March 3, 2025 21:13
@mofojed mofojed self-assigned this Mar 3, 2025
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 46.79%. Comparing base (652212e) to head (16b46ae).

Files with missing lines Patch % Lines
packages/file-explorer/src/FileList.tsx 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2378      +/-   ##
==========================================
- Coverage   46.80%   46.79%   -0.01%     
==========================================
  Files         710      710              
  Lines       39226    39229       +3     
  Branches     9975     9791     -184     
==========================================
  Hits        18358    18358              
- Misses      20814    20860      +46     
+ Partials       54       11      -43     
Flag Coverage Δ
unit 46.79% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR addresses a potential XSS vulnerability by replacing the use of innerHTML with innerText when creating the drag placeholder content in the file explorer.

  • Replaces innerHTML assignment with safe DOM manipulation using createElement and innerText
  • Ensures proper escaping of file names in the drag placeholder to mitigate script injection attacks

Reviewed Changes

File Description
packages/file-explorer/src/FileList.tsx Updates drag placeholder creation to use innerText for escaping

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@mofojed mofojed merged commit d44370b into deephaven:main Mar 3, 2025
11 checks passed
@mofojed mofojed deleted the DH-18645-fix-file-list branch March 3, 2025 21:40
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants