[bug] XSS use of ace.min.js

[daniel-lewis-ab]

Describe the bug

Hi, I'm working on a project that involves examining a bunch of the ComfyUI nodes' JS.

In comfyui_shared.js:417 and note_plus.js you use a loadScript() to load ace.min.js into the script. NotePlus node wasn't visible in utils. I've disabled the remote script import for our purposes but just wanted to point out the state it was found in. This may be an out of date version issue that's already been fixed in your main? Regardless, the external path thing is something that would probably be best localized for security reasons for most uses.

web/extern/shodown.js being deleted seems to not cause any errors at a cursory glance, and I'm not sure why it's there? Solving this riddle is flagged as one of my remaining issues before we can include your node in a large project.

Reproduction

The affected code is note_plus.js

Expected behavior

I would hope to see the ace.min.js call either localized to directory or removed altogether to prevent the possibility of the remote server being used as a vector of attack to other people's machines/browser experience/servers.

Operating System

Linux

Comfy Mode

Other (online services, containers etc..)

Console output

No response

Additional context

No response

[melMass]

Good point yes it makes sense since I do for other externals.
Ace is used solely for the editing part of note plus and showdown is for rendering makrdown from note_plus and from the upcoming documentation widget

[daniel-lewis-ab]

#167

As mentioned, test before pulling.

[melMass]

#167

As mentioned, test before pulling.

Thanks, given the nature of the PR (huge minified code..) I will do it myself.
I think it's also missing all the utility files (from cdn they are linked properly)
I will quickly look into it now, and if there are no blockers it should be solved soon

[daniel-lewis-ab]

Fantastic.

…

On Tue, Apr 2, 2024 at 1:46 AM Mel Massadian ***@***.***> wrote: #167 <#167> As mentioned, test before pulling. Thanks, given the nature of the PR (huge minified code..) I will do it myself. I think it's also missing all the utility files (from cdn they are linked properly) I will quickly look into it now, and if there are no blockers it should be solved soon — Reply to this email directly, view it on GitHub <#166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC36LN7N34NIKSPZZHMV3KDY3JO5RAVCNFSM6AAAAABFOVEHK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGMYDEMJTGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[melMass]

It was way more involving than planned since comfy preloads all the scripts in the web folder.
The trick was to add an extra static path to the server!
Let me know if it solves your security concerns

[daniel-lewis-ab]

Yeah, that should work. Now it isn't technically XSS exposed, and if we scan the file it'll tell us what we need to know.

…

On Tue, Apr 2, 2024 at 9:25 AM Mel Massadian ***@***.***> wrote: It was way more involving than planned since comfy preloads all the scripts in the web folder. The trick was to add an extra static path to the server! Let me know if it solves your security concerns — Reply to this email directly, view it on GitHub <#166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC36LN44J7UWTLAWWCFZOHDY3LEX7AVCNFSM6AAAAABFOVEHK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZSGM2TSNBYGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>