Skip to content

Update fix for potential XSS on /view #8384

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

Merged
merged 2 commits into from
Jun 2, 2025

Conversation

jessegonyou
Copy link
Contributor

@jessegonyou jessegonyou commented Jun 2, 2025

This PR is an extension of #6035. This PR uses mimetypes to add more restricted filetypes to prevent from being served, since mimetypes are what browsers use to determine how to serve files. It also adds XHTML documents to the list of restricted filetypes.

Reproduction steps

  1. Create an XHTML file titled test.xhtml in the ComfyUI/output folder with the following contents:
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="en">
    <head>
        <meta charset="utf-8" />
        <meta name="viewport" content="width=device-width" />
        <title>Test XHTML document</title>
    </head>
    <body>
        <p>Test XHTML document.</p>
    </body>
    <script>
        alert("Hello, world!");
    </script>
</html>
  1. Run ComfyUI
  2. Navigate to http://localhost:8188/api/view?filename=test.xhtml&type=output&subfolder= in any web browser
  3. The page is loaded in the current main branch of ComfyUI, while in the patched version, it is downloaded instead.

Note

As per #6035, you must clear your browser cache whenever you are switching between ComfyUI instances while testing this patch.

This commit uses mimetypes to add more restricted filetypes to prevent from being served, since mimetypes are what browsers use to determine how to serve files.
Fixed a typo that prevented the program from running
Copy link
Collaborator

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

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

LGTM, but cannot test currently.

I believe the linked issue in the PR description was intended to be:

@comfyanonymous comfyanonymous merged commit 4f4f1c6 into comfyanonymous:master Jun 2, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants