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

Add Ruff dependencies and warning message when custom nodes are using eval or exec calls #218

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

yoland68
Copy link
Member

@yoland68 yoland68 commented Dec 20, 2024

Implemented security checks using Ruff to identify specific violations (S102, S307) in Python files.

Test against reactor node:

(comfycli-dev) ➜  comfyui-reactor-node git:(main) comfy node publish --token="fdsafdsa"
Validating node configuration...
Running security checks...
Security warnings found:
r_basicsr/utils/options.py:77:16: S307 Use of possibly insecure function;
consider using `ast.literal_eval`
   |
75 |     # list
76 |     if value.startswith('['):
77 |         return eval(value)
   |                ^^^^^^^^^^^ S307
78 |     # str
79 |     return value
   |

r_basicsr/utils/options.py:128:13: S102 Use of `exec` detected
    |
126 |             eval_str += '=value'
127 |             # using exec function
128 |             exec(eval_str)
    |             ^^^^ S102
129 |
130 |     opt['auto_resume'] = args.auto_resume
    |

r_facelib/detection/yolov5face/models/yolo.py:188:13: S307 Use of possibly
insecure function; consider using `ast.literal_eval`
    |
186 |     layers, save, c2 = [], [], ch[-1]  # layers, savelist, ch out
187 |     for i, (f, n, m, args) in enumerate(d["backbone"] + d["head"]):  #
from, number, module, args
188 |         m = eval(m) if isinstance(m, str) else m  # eval strings
    |             ^^^^^^^ S307
189 |         for j, a in enumerate(args):
190 |             try:
    |

r_facelib/detection/yolov5face/models/yolo.py:191:27: S307 Use of possibly
insecure function; consider using `ast.literal_eval`
    |
189 |         for j, a in enumerate(args):
190 |             try:
191 |                 args = eval(a) if isinstance(a, str) else a  # eval
strings
    |                           ^^^^^^^ S307
192 |             except:
193 |                 pass
    |

scripts/r_masking/core.py:110:15: S307 Use of possibly insecure function;
consider using `ast.literal_eval`
    |
108 |     }
109 |     code = f'lambda _cls, {arg_list}: _tuple_new(_cls, ({arg_list}))'
110 |     __new__ = eval(code, namespace)
    |               ^^^^^^^^^^^^^^^^^^^^^ S307
111 |     __new__.__name__ = '__new__'
112 |     __new__.__doc__ = f'Create new instance of {typename}({arg_list})'
    |

Found 5 errors.

We will soon disable exec and eval, so this will be an error soon.
Publishing node version...
{'personal_access_token': 'fdsafdsa', 'node': {'id': 'comfyui-reactor-node', 'description': 'The Fast and Simple Face Swap Extension Node for ComfyUI, based on ReActor SD-WebUI Face Swap Extension', 'icon': '', 'name': 'comfyui-reactor-node', 'license': '{"file": "LICENSE"}', 'repository': 'https://github.com/Gourieff/comfyui-reactor-node'}, 'node_version': {'version': '0.5.2-a2', 'dependencies': ['insightface==0.7.3', 'onnx>=1.14.0', 'opencv-python>=4.7.0.72', 'numpy==1.26.3', 'segment_anything', 'albumentations>=1.4.16', 'ultralytics']}}
{'Failed to publish node version: 400 {"message":"Invalid personal access
token"}\n'}

Implemented security checks using Ruff to identify specific violations (S102, S307) in Python files.
Replaced the previous security check implementation with a subprocess call to 'ruff' for better error handling and output. Added tests to verify behavior on security violations, missing 'ruff' installation, and successful publishing with a token. This enhances the reliability and maintainability of the publish process.
Updated the security check implementation in the publish command to use 'ruff' with an '--exit-zero' flag, changing the output handling to display warnings instead of errors. Adjusted the test cases to reflect these changes, ensuring that security warnings are correctly asserted. This improves the clarity of security feedback during the publishing process.
@yoland68 yoland68 merged commit ae9b0aa into main Dec 20, 2024
10 of 12 checks passed
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.

1 participant