fix: restrict user file access to current user only #5177
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR modifies the get_user_files_as_user function in backend/onyx/file_store/utils.py to change how user file access validation is handled. Previously, the function would raise a ValueError when a user attempted to access files they didn't own. The new implementation silently filters out unauthorized files and returns only those owned by the requesting user.
The change replaces error-throwing behavior with a filtering approach: instead of validating access permissions upfront and failing fast, the function now iterates through all retrieved files and only includes those where user_file.user_id == user_id in the returned list. This addresses a specific issue where the chat system was failing when users tried to access recent documents that included files from other users.
This function is part of the file store utilities layer and is likely called by chat-related endpoints when users request access to files. The modification represents a shift from strict access control enforcement to a more permissive approach that prioritizes system stability over immediate error feedback.
Confidence score: 2/5
- This PR introduces a potentially problematic change that silently masks authorization failures instead of properly handling them
- Score reflects concerns about security implications and the difficulty of debugging access control issues when errors are silently suppressed
- Pay close attention to
backend/onyx/file_store/utils.pyand consider if this change aligns with security requirements
1 file reviewed, 2 comments
| current_user_files = [] | ||
| for user_file in user_files: | ||
| # Note: if user_id is None, then all files should be None as well | ||
| # (since auth must be disabled in this case) | ||
| if user_file.user_id != user_id: | ||
| raise ValueError( | ||
| f"User {user_id} does not have access to file {user_file.id}" | ||
| ) | ||
| return user_files | ||
| if user_file.user_id == user_id: | ||
| current_user_files.append(user_file) | ||
|
|
||
| return current_user_files |
There was a problem hiding this comment.
logic: Silent filtering of unauthorized files could mask security issues. The original error-raising behavior provided explicit feedback when access control violations occurred. Consider logging when files are filtered out for security auditing.
| # Note: if user_id is None, then all files should be None as well | ||
| # (since auth must be disabled in this case) |
There was a problem hiding this comment.
logic: The comment about user_id being None when auth is disabled suggests this function handles both authenticated and unauthenticated scenarios, but the filtering logic doesn't account for this properly - it would filter out all files when user_id is None.
| # Note: if user_id is None, then all files should be None as well | |
| # (since auth must be disabled in this case) | |
| current_user_files = [] | |
| for user_file in user_files: | |
| # Note: if user_id is None, then all files should be None as well | |
| # (since auth must be disabled in this case) | |
| if user_file.user_id == user_id: | |
| current_user_files.append(user_file) | |
| return current_user_files |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 1 file. Review in cubic
* fix: restrict user file access to current user only * fix: enhance user file access control for recent folder
* fix: restrict user file access to current user only * fix: enhance user file access control for recent folder
Description
This pull request updates the logic in the
get_user_files_as_userfunction to improve how user file access is validated. Instead of raising an error when a file does not belong to the user, it now filters out files not owned by the user and only returns those the user has access to.Linear issue -> https://linear.app/danswer/issue/DAN-2201/trying-to-chat-with-recent-documents-fails
How Has This Been Tested?
Tested from UI
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Updated file access logic to ensure users only see files they own, filtering out files from other users.