fix: sanitize LoRA paths and enable dynamic loading #1156
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:
|
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 |
MateusGPe
changed the title
- 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
MateusGPe
left a comment
There was a problem hiding this comment.
The problems that were identified have been resolved, I believe.
| LOG_DEBUG("%s", default_gen_params.to_string().c_str()); | ||
|
|
||
| sd_ctx_params_t sd_ctx_params = ctx_params.to_sd_ctx_params_t(false, false, false); | ||
| ctx_params.lora_apply_mode = LORA_APPLY_AT_RUNTIME; |
There was a problem hiding this comment.
I mentioned it as a simple comment, so it may have been overlooked: I believe this should follow the command-line flag (maybe keeping LORA_APPLY_AT_RUNTIME as a default).
wbruna
left a comment
wbruna
left a comment
There was a problem hiding this comment.
Again, sanitize_lora_path seems to me more complex than needed.
If we don't want to support any absolute LoRA path at all, it'd be enough (after excluding an empty path) to canonicalize first, then forbid paths either absolute or starting with .. (the canonicalization will ensure all relevant .. elements will be effectively moved to the beginning).
(disclaimer: I have a Unix background. Weird Windows stuff like c:../directory could very well fall under the cracks; I don't know. But I'd appreciate some comments, if that's the case 🙂)
OTOH, the canonicalization+comparison with lora_model_dir is needed if we intend to support absolute paths for LoRAs as long as the final path falls under lora_model_dir - which could be better from a backward-compatibility POV. But in this case, neither .. elements nor absolute paths would demand special handling.
2 participants
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.