[GH-ISSUE #1546] security: is_local_mode() concern ...and potential string issues #655

New issue

Closed

opened 2026-10-27 21:20:29 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-27 21:20:29 +00:00

Owner

Originally created by @fat-tire on GitHub (Feb 14, 2025).
Original GitHub issue: https://github.com/Comfy-Org/ComfyUI-Manager/issues/1546

Hey there! I'm thinking Line 59 of manager_server may have some issues:

is_local_mode = args.listen.startswith('127.') or args.listen.startswith('local.')

What I think this is supposed to do

Determine if comfyui is running locally so that is_allowed_security_level() knows which security levels to permit.

Why this check may not be so great

Checking the first couple characters of the listen argument is not always reliable. Running comfyui with --listen 127.someRandomDomain.com or local.myNonLocalHost.com would pass the tests, even though they likely shouldn't. And localhost would fail the test even though it should probably pass.

I'm not actually sure if this is meant to be a test of local-as-in-the-same-machine or local-as-in-within-the-same-local-network, but some routers will add/resolve the "local." prefix to any machine on the LAN. That's okay?

There are probably better, more specific ways to do this if you're checking if the host is the localhost. But if you mean to check if it's running on another computer on the same local network (which the local. prefix usually indicates), you can do that more explicitly as well maybe by resolving it to an IP and seeing if it's on the same subnet (?) or some other way.

Bare lower-case strings for security_settings config

Also, is_allowed_security_level() is using hardcoded lower-case string comparisons that may run into typo issues or capitalization issues not only in the config file edited by users but in codebase in the future as these strings are hand-typed repeatedly.

manager_core's read_config() uses .lower() to do string comparisons for disable_unsecure_features but not for security_level. It should, shouldn't it?

It looks like the values stored by configparser retain their caseness (though the keys themselves will become lowercase. ) You can verify this by changing the security_level in config.ini to Strong with a capital S and then:

>>> import configparser
>>> config = configparser.ConfigParser()
>>> config.read('config.ini')
['config.ini']
>>> config['default']['security_level']
'Strong'

So at the very least maybe avoid some potential mistakes if a user capitalizes by converting their setting to lowercase:

security_level = default_conf['security_level'] if 'security_level' in default_conf else 'normal'

security_level = default_conf['security_level'].lower() if 'security_level' in default_conf else 'normal'

Perhaps instead of typing the strings "strong", "normal", everywhere, you may want to use a single "security" class file with the various security levels/strings defined once and then refer to them consistently throughout the program with the class to avoid such situations (and de-duplicate the repeated strings all over the place). It would also mean you could change the strings in a single place and it would be reflected everywhere.

Hope this all makes sense. Nice work with this, and cheers!

Originally created by @fat-tire on GitHub (Feb 14, 2025). Original GitHub issue: https://github.com/Comfy-Org/ComfyUI-Manager/issues/1546 Hey there! I'm thinking [Line 59 of manager_server](https://github.com/ltdrdata/ComfyUI-Manager/blob/main/glob/manager_server.py#L59) may have some issues: ```python is_local_mode = args.listen.startswith('127.') or args.listen.startswith('local.') ``` ## What I think this is supposed to do Determine if comfyui is running locally so that `is_allowed_security_level()` knows which security levels to permit. ## Why this check may not be so great Checking the first couple characters of the listen argument is not always reliable. Running comfyui with `--listen 127.someRandomDomain.com` or `local.myNonLocalHost.com` would pass the tests, even though they likely shouldn't. And `localhost` would fail the test even though it should probably pass. I'm not actually sure if this is meant to be a test of local-as-in-the-same-machine or local-as-in-within-the-same-local-network, but some routers will add/resolve the "local." prefix to *any* machine on the LAN. That's okay? There are probably [better, more specific](https://stackoverflow.com/questions/47885832/determine-if-host-is-localhost) ways [to do this](https://stackoverflow.com/questions/47885832/determine-if-host-is-localhost) if you're checking if the host is the localhost. But if you mean to check if it's running on another computer on the same local network (which the `local.` prefix usually indicates), you can do that more explicitly as well maybe by resolving it to an IP and seeing if it's on the same subnet (?) or some other way. ## Bare lower-case strings for security_settings config Also, `is_allowed_security_level()` is using hardcoded lower-case string comparisons that may run into typo issues or capitalization issues not only in the config file edited by users but in codebase in the future as these strings are hand-typed repeatedly. `manager_core`'s [read_config()](https://github.com/ltdrdata/ComfyUI-Manager/blob/main/glob/manager_core.py#L1581) uses `.lower()` to do string comparisons for `disable_unsecure_features` but not for `security_level`. It should, shouldn't it? It looks like the values stored by `configparser` retain their caseness (though the keys themselves will become lowercase. ) You can verify this by changing the `security_level` in `config.ini` to **Strong** with a capital **S** and then: ```python >>> import configparser >>> config = configparser.ConfigParser() >>> config.read('config.ini') ['config.ini'] >>> config['default']['security_level'] 'Strong' ``` So at the very least maybe avoid some potential mistakes if a user capitalizes by converting their setting to lowercase: `security_level = default_conf['security_level'] if 'security_level' in default_conf else 'normal'` to `security_level = default_conf['security_level'].lower() if 'security_level' in default_conf else 'normal'` Perhaps instead of typing the strings "strong", "normal", everywhere, you may want to use a single "security" class file with the various security levels/strings defined once and then refer to them consistently throughout the program with the class to avoid such situations (and de-duplicate the repeated strings all over the place). It would also mean you could change the strings in a single place and it would be reflected everywhere. Hope this all makes sense. Nice work with this, and cheers!

bsakaguchi closed this issue

2026-10-27 21:20:29 +00:00

bsakaguchi commented

2026-10-27 21:20:30 +00:00

Author

Owner

@ltdrdata commented on GitHub (Feb 15, 2025):

This is patched.

@ltdrdata commented on GitHub (Feb 15, 2025): This is patched.

bsakaguchi referenced this issue

2026-10-27 21:31:28 +00:00

[PR #655] [MERGED] Update custom-node-list.json #1449