Updated modules/processors/frame/core.py to fix security vulnerability [python.lang.security.audit.non-literal-import.non-literal-import] #1285
Conversation
…-modules-processors-frame-core.py
Reviewer's GuideThis PR secures dynamic imports in modules/processors/frame/core.py by introducing a whitelist-driven safe_import_module function and replacing direct importlib.import_module calls with it; reviewers should also flag and consolidate redundant definitions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @kira-offgrid - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
safe_import_module calls itself recursively (link)
-
There are multiple duplicated definitions of ALLOWED_MODULES and safe_import_module; consolidate them into a single definition at the top.
-
The safe_import_module function currently calls itself recursively—replace that with importlib.import_module to actually load the module.
-
load_frame_processor_module should handle the None case returned by safe_import_module to avoid unexpected attribute checks on None.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 2 other issues
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if module_name in ALLOWED_MODULES: | ||
| return safe_import_module(module_name) |
There was a problem hiding this comment.
suggestion (bug_risk): safe_import_module calls itself recursively
This causes infinite recursion; use importlib.import_module(module_name) instead of calling safe_import_module.
| if module_name in ALLOWED_MODULES: | |
| return safe_import_module(module_name) | |
| if module_name in ALLOWED_MODULES: | |
| return importlib.import_module(module_name) |
| @@ -1,12 +1,129 @@ | |||
| import sys | |||
| import importlib | |||
| from typing import Set, Optional | |||
|
|
|||
| # Define a whitelist of allowed modules that can be dynamically imported | |||
There was a problem hiding this comment.
suggestion: Duplicate whitelist and function definitions
Consolidate the repeated whitelist and safe_import_module definitions into a single implementation to simplify maintenance and avoid confusion.
| @@ -20,7 +137,7 @@ | |||
|
|
|||
| def load_frame_processor_module(frame_processor: str) -> Any: | |||
| try: | |||
| frame_processor_module = importlib.import_module(f'modules.processors.frame.{frame_processor}') | |||
| frame_processor_module = safe_import_module(f'modules.processors.frame.{frame_processor}') | |||
There was a problem hiding this comment.
suggestion (bug_risk): Handle None return from safe_import_module
Check frame_processor_module for None before using it to avoid an AttributeError.
| frame_processor_module = safe_import_module(f'modules.processors.frame.{frame_processor}') | |
| frame_processor_module = safe_import_module(f'modules.processors.frame.{frame_processor}') | |
| if frame_processor_module is None: | |
| sys.exit(f"Failed to load frame processor module: modules.processors.frame.{frame_processor}") |
| else: | ||
| # Log the attempt to import a non-whitelisted module | ||
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | ||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
| else: | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None |
| else: | ||
| # Log the attempt to import a non-whitelisted module | ||
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | ||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
| else: | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None |
| else: | ||
| # Log the attempt to import a non-whitelisted module | ||
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | ||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
| else: | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None |
| else: | ||
| # Log the attempt to import a non-whitelisted module | ||
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | ||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
| else: | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None | |
| # Log the attempt to import a non-whitelisted module | |
| print(f"Warning: Attempted to import non-whitelisted module: {module_name}") | |
| return None |
Context and Purpose:
Summary by Sourcery
Mitigate a security vulnerability in dynamic module imports by enforcing a whitelist and replacing direct importlib calls with a safe import wrapper
Bug Fixes:
Enhancements: