Skip to content

Scanning 2#4045

Closed
girichinna27 wants to merge 3 commits into
unslothai:mainfrom
girichinna27:scanning-2
Closed

Scanning 2#4045
girichinna27 wants to merge 3 commits into
unslothai:mainfrom
girichinna27:scanning-2

Conversation

@girichinna27
Copy link
Copy Markdown

No description provided.

root and others added 2 commits February 13, 2026 11:24
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @girichinna27, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a security measure by implementing a whitelist for dynamic module imports within unsloth/import_fixes.py. This change aims to prevent potential arbitrary code execution by ensuring that only explicitly approved modules from vllm.lora.ops.triton_ops can be loaded, thereby enhancing the overall security posture of the application.

Highlights

  • Security Enhancement: Implemented a whitelist for module imports in unsloth/import_fixes.py to restrict dynamic loading to a predefined set of safe modules.
  • Arbitrary Code Execution Prevention: Introduced validation logic that checks if a module's path is in the allowed_modules whitelist before attempting to import it, logging a warning and skipping non-whitelisted modules.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • unsloth/import_fixes.py
    • Added a allowed_modules set to define whitelisted module paths.
    • Implemented a check before importlib.import_module to ensure the module path is whitelisted.
    • Added a warning log for attempts to import non-whitelisted modules.
Activity
  • No human activity has been recorded on this pull request since its creation.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a security check to validate module paths before dynamic importing. While the intention is good, the implementation introduces code duplication by creating a separate whitelist that mirrors a hardcoded dictionary. My review focuses on removing this redundancy to improve maintainability, as the check is currently superfluous.

Comment thread unsloth/import_fixes.py
Comment on lines +1078 to +1091

# Whitelist of allowed module paths to prevent arbitrary code execution
allowed_modules = {
"vllm.lora.ops.triton_ops.lora_expand_op",
"vllm.lora.ops.triton_ops.lora_shrink_op",
"vllm.lora.ops.triton_ops.fused_moe_lora_op",
}

for name, path in consumer_modules.items():
# Validate module path against whitelist before importing
if path not in allowed_modules:
logger.warning(
f"Unsloth: Skipping import of non-whitelisted module: {path}"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

While adding a whitelist for module imports is a good security practice, the current implementation introduces redundancy. The allowed_modules set is a manual copy of the values from the consumer_modules dictionary. This creates a maintenance burden, as any changes to consumer_modules must be manually duplicated in allowed_modules, which is error-prone.

Since consumer_modules is hardcoded and not derived from any external input, it already acts as a whitelist. The additional check is currently redundant. I suggest removing the allowed_modules set and the validation check to avoid this duplication.

Suggested change
# Whitelist of allowed module paths to prevent arbitrary code execution
allowed_modules = {
"vllm.lora.ops.triton_ops.lora_expand_op",
"vllm.lora.ops.triton_ops.lora_shrink_op",
"vllm.lora.ops.triton_ops.fused_moe_lora_op",
}
for name, path in consumer_modules.items():
# Validate module path against whitelist before importing
if path not in allowed_modules:
logger.warning(
f"Unsloth: Skipping import of non-whitelisted module: {path}"
)
for name, path in consumer_modules.items():

@Datta0
Copy link
Copy Markdown
Collaborator

Datta0 commented Feb 14, 2026

Hey @girichinna27 when you raise a PR, we appreciate if you can add a description to better assess the changes and review

@girichinna27
Copy link
Copy Markdown
Author

girichinna27 commented Feb 14, 2026

@Datta0

My tool: [AI-Guardian](https://ai-rem-demo.remediation.opsmx.net/ ]), provides an automated security vulnerability detection and remediation for your applications. Using the same I have raised this PR.

Please find the fix details here:
`Semgrep Rule Fix

Rule ID: subprocess-shell-true
Rule Message: Found 'subprocess' function 'Popen' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
File Path: unsloth/chat_templates.py
Line: 2694

`

danielhanchen added a commit that referenced this pull request Feb 16, 2026
* Fix security-regression fallout in chat templates and PDL patching

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Drop security regression test files from PR scope

* Apply suggestion from @danielhanchen

---------

Co-authored-by: Daniel Hanchen <danielhanchen@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@danielhanchen
Copy link
Copy Markdown
Member

Closing this PR as superseded by #4062, which was merged on 2026-02-16 and consolidates/corrects the security-related changes from #4042, #4044, and #4045 while preserving runtime behavior.

@danielhanchen danielhanchen mentioned this pull request Feb 18, 2026
abiswas-realadvice pushed a commit to abiswas-realadvice/unsloth that referenced this pull request May 14, 2026
…nslothai#4045 (unslothai#4062)

* Fix security-regression fallout in chat templates and PDL patching

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Drop security regression test files from PR scope

* Apply suggestion from @danielhanchen

---------

Co-authored-by: Daniel Hanchen <danielhanchen@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

3 participants