-
Notifications
You must be signed in to change notification settings - Fork 491
fix: sanitize LoRA paths and enable dynamic loading #1156
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
base: master
Are you sure you want to change the base?
Conversation
- Implement `sanitize_lora_path` in `SDGenerationParams` to prevent directory traversal attacks via LoRA tags in prompts. - Restrict LoRA paths to be relative and strictly within the configured LoRA directory (no subdirectories allowed, optional? drawback: users cannot organize their LoRAs into subfolders.). - Update server example to pass `lora_model_dir` to `process_and_check`, enabling LoRA extraction from prompts. - Force `LORA_APPLY_AT_RUNTIME` in the server to allow applying LoRAs dynamically per request without reloading the model.
|
Is this block optional or required? It has a clear disadvantage: users cannot organize their LoRA files into subfolders. // 3. The file must be directly in the lora directory, not in a subdirectory.
if (relative_path.has_parent_path() && !relative_path.parent_path().empty()) {
LOG_WARN("lora path in subdirectories is not allowed: %s", raw_path_str.c_str());
return false;
}Proposals:
|
examples/common/common.hpp
Outdated
| try_path += ext; | ||
| if (fs::exists(try_path)) { | ||
| final_path = try_path; | ||
| final_path = try_path.lexically_normal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize, I realized it's redundant, the code following this block does that:
const std::string key = final_path.lexically_normal().string();
Since we have a parameter controlling that, the server should follow it. But I think it'd be OK to have its value default to |
- Remove the restriction that LoRA models must be in the root of the LoRA directory, allowing them to be organized in subfolders. - Refactor the directory containment check to use `std::mismatch` instead of `lexically_relative` to verify the path is inside the allowed root. - Remove redundant `lexically_normal()` call when resolving file extensions.
MateusGPe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problems that were identified have been resolved, I believe.
examples/common/common.hpp
Outdated
| try_path += ext; | ||
| if (fs::exists(try_path)) { | ||
| final_path = try_path; | ||
| final_path = try_path.lexically_normal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize, I realized it's redundant, the code following this block does that:
const std::string key = final_path.lexically_normal().string();
sanitize_lora_pathinSDGenerationParamsto prevent directory traversal attacks via LoRA tags in prompts.lora_model_dirtoprocess_and_check, enabling LoRA extraction from prompts.LORA_APPLY_AT_RUNTIMEin the server to allow applying LoRAs dynamically per request without reloading the model and avoiding weight accumulation.