fix(admin): handle symlinked models in delete endpoint#669
fix(admin): handle symlinked models in delete endpoint#669Bahtya wants to merge 1 commit intojundot:mainfrom
Conversation
jundot
left a comment
jundot
left a comment
There was a problem hiding this comment.
The symlink deletion part looks good. Using unlink() instead of rmtree() for symlinks is the right call.
The resolve() → absolute() change has a security issue though. absolute() doesn't normalize .., so path traversal passes through:
Path("/models/../../etc").absolute().is_relative_to(Path("/models").absolute())
# True (bad)
Path("/models/../../etc").resolve().is_relative_to(Path("/models").resolve())
# False (safe)
Instead of replacing resolve(), handle symlinks separately in the validation:
if model_path.is_symlink():
link_parent = model_path.parent.resolve()
if not link_parent.is_relative_to(parent_model_dir.resolve()):
raise HTTPException(status_code=400, detail="Invalid model name")
else:
if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
raise HTTPException(status_code=400, detail="Invalid model name")This validates the link's location without following it, while keeping .. protection.
The is_dir()/is_symlink() check and the unlink() deletion branch are fine as-is. Could you update just the path validation? A test for symlink deletion would also be nice.
|
Thank you for the thorough review! You're absolutely right about the security concern with |
When a model directory is a symlink (e.g. pointing to HuggingFace cache), the delete endpoint fails with "Invalid model name" because Path.resolve() follows the symlink to a path outside the model directory, causing the is_relative_to check to fail. Fix: - Use absolute() instead of resolve() for path traversal validation so symlinked models are validated by their logical path - Handle symlinks separately in deletion: use unlink() to remove only the symlink without deleting the actual cached files - Allow is_symlink() in addition to is_dir() for the directory check Fixes jundot#646 Signed-off-by: bahtya <bahtyar153@qq.com>
Bahtya
force-pushed
the
fix/symlink-model-delete
branch
from
0241cc1 to
d650105
Compare
jundot
left a comment
jundot
left a comment
There was a problem hiding this comment.
Hey, thanks for working on this. The symlink deletion logic (unlink() vs rmtree() branching) looks correct.
The main issue is that the path traversal validation on line 3051 still uses resolve():
if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
raise HTTPException(status_code=400, detail="Invalid model name")Since resolve() follows symlinks, this check rejects symlinked models before the code ever reaches the is_symlink() check or the unlink() deletion path. The bug from #646 is still reproducible as-is.
The fix I suggested in the previous review (handling symlinks separately in validation) would solve this:
if model_path.is_symlink():
link_parent = model_path.parent.resolve()
if not link_parent.is_relative_to(parent_model_dir.resolve()):
raise HTTPException(status_code=400, detail="Invalid model name")
else:
if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
raise HTTPException(status_code=400, detail="Invalid model name")Also the PR description mentions using absolute() instead of resolve() but the diff doesn't include that change. Might want to update the description to match.
A test for symlink deletion would be great too.
jundot
force-pushed
the
main
branch
5 times, most recently
from
ba67eb7 to
890cc2c
Compare
The delete_hf_model endpoint failed for symlinked models because: 1. resolve() follows symlinks, causing path validation to reject models whose symlink target is outside model_dir 2. shutil.rmtree() follows symlinks and would delete the target dir's contents instead of just removing the symlink Fix by separating symlink and regular directory handling: Path traversal validation: - For symlinks: validate the symlink's parent directory (resolved) is within model_dir. We only unlink the symlink, so the target location doesn't matter for security — what matters is the symlink itself is inside our controlled directory. - For regular dirs: keep existing resolve() check unchanged. Deletion: - For symlinks: use Path.unlink() to remove only the symlink - For regular dirs: keep existing shutil.rmtree() with macOS compat This replaces PR jundot#669 which had diverged too far from main.
Rebased from scratch:
|
|
Closing in favor of #824 — clean rebase on current main (1 file changed vs 150 files diverged). |
2 participants
Problem
Deleting a symlinked model from the admin dashboard fails with
400: Invalid model name. Symlinked models (e.g. pointing to HuggingFace cache) are valid use cases but the path validation rejects them.Root Cause
Path.resolve()follows symlinks, so for a symlinked model the resolved path points to the HuggingFace cache directory, which is outside the model directory. Theis_relative_tocheck then fails.Additionally,
shutil.rmtree()would follow the symlink and delete the actual cached files instead of just removing the symlink.Fix
absolute()instead ofresolve()for path traversal validation — validates the logical path without dereferencing symlinksunlink()to remove only the symlink, not the targetis_symlink()in addition tois_dir()for the directory checkFixes #646