fix(backend): prevent path traversal in face search endpoint - #1415
fix(backend): prevent path traversal in face search endpoint#1415AbiramiR-27 wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe face-search endpoint restricts path inputs to allowed folders and ChangesFace Search Security and Input Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/routes/face_clusters.py (1)
262-279: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize before probing the filesystem.
Unauthorized existing paths return 403, while unauthorized nonexistent paths return 400, creating a filesystem-existence oracle. Call
is_safe_path(local_file_path)beforeos.path.isfile()so all disallowed paths consistently return 403.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routes/face_clusters.py` around lines 262 - 279, Reorder the validation in the face-cluster route so is_safe_path(local_file_path) executes before os.path.isfile(local_file_path). Preserve the existing 403 response for unsafe paths and the 400 response for safe paths that are not valid files, preventing filesystem existence from being revealed for unauthorized paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/routes/face_clusters.py`:
- Around line 45-58: Update the path containment logic in the folder-checking
function to use canonical paths: resolve both target_path and each allowed
folder, including temp_dir, with realpath() before commonpath comparison.
Preserve the existing allowed-folder iteration and return behavior while
ensuring symlink targets outside permitted roots are rejected.
In `@backend/tests/test_face_clusters.py`:
- Around line 420-434: Expand test_face_search_path_traversal_blocked to use an
actual parent-directory traversal such as /allowed/folder/../restricted/file.jpg
and assert it remains blocked with 403. Add a separate symlink-escape regression
test that mocks or creates a link inside the allowed folder pointing outside it,
then verifies canonicalized-path validation rejects the request.
---
Outside diff comments:
In `@backend/app/routes/face_clusters.py`:
- Around line 262-279: Reorder the validation in the face-cluster route so
is_safe_path(local_file_path) executes before os.path.isfile(local_file_path).
Preserve the existing 403 response for unsafe paths and the 400 response for
safe paths that are not valid files, preventing filesystem existence from being
revealed for unauthorized paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cdff1766-c663-410d-baf4-bf1bac2a023d
📒 Files selected for processing (2)
backend/app/routes/face_clusters.pybackend/tests/test_face_clusters.py
…losures in face-search (fixes AOSSIE-Org#1322)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/test_face_clusters.py (2)
472-485: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the exact path passed to face search.
assert_called_once()does not verify that the authorized path is the one forwarded downstream.- mock_perform.assert_called_once() + mock_perform.assert_called_once_with("/allowed/folder/family.jpg")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_face_clusters.py` around lines 472 - 485, Update test_face_search_safe_path_allowed to assert the exact arguments passed to mock_perform, including the authorized path /allowed/folder/family.jpg, rather than only asserting it was called once. Preserve the existing success response and folder authorization setup.Source: Path instructions
472-485: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCover the
temp_uploadsallowlist branch.The test covers registered folders but not the second permitted root used by
is_safe_path(). Add a permittedtemp_uploads/...case so that exception cannot regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_face_clusters.py` around lines 472 - 485, Extend test_face_search_safe_path_allowed to also exercise a path under the temp_uploads permitted root used by is_safe_path(), while retaining the existing registered-folder coverage and successful perform_face_search assertions. Ensure the temp_uploads case verifies a request is accepted and mock_perform is invoked.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_face_clusters.py`:
- Around line 427-446: Harden both blocked-path tests, including the
symlink-escape test, by mocking os.path.isfile to return True for the supplied
escaped paths and asserting mock_perform was not called. Keep the existing
path-resolution mocks and 403 assertions, ensuring the tests model an existing
file rejected by authorization rather than by file absence.
---
Outside diff comments:
In `@backend/tests/test_face_clusters.py`:
- Around line 472-485: Update test_face_search_safe_path_allowed to assert the
exact arguments passed to mock_perform, including the authorized path
/allowed/folder/family.jpg, rather than only asserting it was called once.
Preserve the existing success response and folder authorization setup.
- Around line 472-485: Extend test_face_search_safe_path_allowed to also
exercise a path under the temp_uploads permitted root used by is_safe_path(),
while retaining the existing registered-folder coverage and successful
perform_face_search assertions. Ensure the temp_uploads case verifies a request
is accepted and mock_perform is invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce5d4d18-c5d8-4199-9f1e-1429bfa3deed
📒 Files selected for processing (2)
backend/app/routes/face_clusters.pybackend/tests/test_face_clusters.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/routes/face_clusters.py
|
@rohan-pandeyy Could you please review this PR when you get a chance? |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/routes/face_clusters.py (1)
46-65: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEliminate the validation-to-use race.
real_targetis validated here, butface_tagging()later checks and passes the originalpayload.path. A caller able to modify an allowed directory can replace that path with a symlink after validation and beforeperform_face_search()opens it. Open the image through trusted directory descriptors with no-follow semantics and make the search consume that opened handle/bytes, rather than reopening the user-controlled pathname.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routes/face_clusters.py` around lines 46 - 65, Update the face_tagging/perform_face_search flow to eliminate the validation-to-use race: after is_safe_path validation, open the image via trusted allowed-directory file descriptors using no-follow semantics, then pass the opened handle or loaded bytes to perform_face_search instead of the original payload.path. Preserve rejection of paths outside registered folders and temp_uploads, and ensure the image is not reopened through the user-controlled pathname.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/app/routes/face_clusters.py`:
- Around line 46-65: Update the face_tagging/perform_face_search flow to
eliminate the validation-to-use race: after is_safe_path validation, open the
image via trusted allowed-directory file descriptors using no-follow semantics,
then pass the opened handle or loaded bytes to perform_face_search instead of
the original payload.path. Preserve rejection of paths outside registered
folders and temp_uploads, and ensure the image is not reopened through the
user-controlled pathname.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25064d07-ed48-42ab-9e0e-ad2f7797ddda
📒 Files selected for processing (2)
backend/app/routes/face_clusters.pybackend/tests/test_face_clusters.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/test_face_clusters.py
…ecurely with O_NOFOLLOW
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/models/FaceDetector.py (1)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the
detect_facesreturn contract.The method returns either
Noneor a result dictionary, but exposes no return type. Declare this contract to keep the new byte-based call path type-safe.Proposed fix
-from typing import Optional +from typing import Any, Dict, Optional ... def detect_faces( self, image_id: str, image_path: Optional[str] = None, forSearch: bool = False, image_bytes: Optional[bytes] = None, - ): + ) -> Optional[Dict[str, Any]]:As per path instructions, “Ensure proper use of type hints.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/models/FaceDetector.py` around lines 34 - 50, Update the detect_faces method signature to declare its return type as an optional result dictionary, preserving the existing None returns for missing or invalid images and dictionary results for successful detection.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/routes/face_clusters.py`:
- Around line 323-329: Update the file-opening logic in the route handler around
local_file_path to eliminate the authorization-to-open race: open the verified
allowed root as a directory descriptor, resolve each relative path component
using dir_fd and O_NOFOLLOW, and open the final component without reopening the
original request path. Verify the resulting descriptor is a regular file before
reading it, while preserving the existing canonical-target authorization checks.
In `@backend/tests/test_face_clusters.py`:
- Around line 494-495: Strengthen the tests in
backend/tests/test_face_clusters.py at lines 494-495 by asserting
perform_face_search receives exactly b"fakeimagebytes" and verifying os.open
includes O_NOFOLLOW when supported; at lines 508-514, decode the fixture and
assert perform_face_search receives the exact decoded bytes.
---
Nitpick comments:
In `@backend/app/models/FaceDetector.py`:
- Around line 34-50: Update the detect_faces method signature to declare its
return type as an optional result dictionary, preserving the existing None
returns for missing or invalid images and dictionary results for successful
detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32de94d5-a4d8-45eb-9cfc-17e9a1a6a1e1
📒 Files selected for processing (4)
backend/app/models/FaceDetector.pybackend/app/routes/face_clusters.pybackend/app/utils/faceSearch.pybackend/tests/test_face_clusters.py
1 participant
Addressed Issues:
Fixes #1322
Screenshots/Recordings:
N/A (This is a backend security fix).
Additional Notes:
This PR resolves a path traversal vulnerability in the
/face-searchendpoint. Previously, ifinput_typewas set topath, the endpoint would attempt to read and process any arbitrary file path provided by the user.Changes made in this PR:
is_safe_path()helper function inbackend/app/routes/face_clusters.pythat normalizes paths and validates that they are located inside one of the registered folders in the database or thetemp_uploadsfolder usingos.path.commonpath.is_safe_pathbefore processing paths in/face-search, returning a403 Forbiddenerror on violation.test_face_search_path_traversal_blockedandtest_face_search_safe_path_allowedunit tests inbackend/tests/test_face_clusters.pyto cover both cases.AI Usage Disclosure:
I have used the following AI models and tools: Antigravity coding assistant.
Checklist
Summary by CodeRabbit
image_bytes.