Fix permission handling in _setup_cookie_directory and add tests for …#191
Fix permission handling in _setup_cookie_directory and add tests for …#191
Conversation
…custom path scenarios. #190
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates cookie-directory setup to normalise user paths, use os.makedirs for creation, and enforce permissions via os.chmod and a temporary umask. Adds optional default Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.4)tests/conftest.pytests/test_base.pytests/test_cmdline.py
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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a multi-user permission issue on Linux systems where the first user creates /tmp/pyicloud with restrictive permissions, preventing other users from accessing it. The fix separates directory creation from permission setting by using mkdir() followed by explicit chmod() calls, setting the shared topdir to 0o777 permissions while maintaining 0o700 for user-specific directories.
Changes:
- Modified
_setup_cookie_directoryto use separatemkdir()andchmod()calls instead ofmkdir(path, mode)for more reliable permission setting - Set topdir (
/tmp/pyicloud) permissions to 0o777 for multi-user access - Added comprehensive unit tests covering various directory existence scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pyicloud/base.py | Refactored directory permission handling to use explicit chmod calls; added chmod import; set topdir to 0o777 for multi-user access |
| tests/test_base.py | Added 6 comprehensive test cases covering custom paths, default paths, directory existence scenarios, and tilde expansion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not path.exists(topdir): | ||
| mkdir(topdir, 0o777) | ||
| mkdir(topdir) | ||
| chmod(topdir, 0o777) |
There was a problem hiding this comment.
The fix only sets 0o777 permissions on topdir when it's newly created (line 128-130), but doesn't fix permissions if the directory already exists with incorrect permissions (line 128 condition). If user1 initially created the directory with restrictive permissions before this fix was deployed, user2 will still encounter permission errors even after upgrading to this version. To handle the upgrade scenario where existing directories have incorrect permissions, chmod should be called on topdir unconditionally (move chmod(topdir, 0o777) outside the 'if not path.exists(topdir)' block), similar to how chmod is called unconditionally for _cookie_directory at line 134.
| chmod(topdir, 0o777) | |
| chmod(topdir, 0o777) |
| mkdir(topdir) | ||
| chmod(topdir, 0o777) |
There was a problem hiding this comment.
There is a potential race condition between mkdir and chmod calls. If two users simultaneously create the topdir, the second user's mkdir will fail. Consider using exist_ok parameter or catching the FileExistsError exception to handle this scenario gracefully. Additionally, the chmod should be called even if mkdir fails due to concurrent creation to ensure proper permissions.
| if not path.exists(topdir): | ||
| mkdir(topdir, 0o777) | ||
| mkdir(topdir) | ||
| chmod(topdir, 0o777) |
There was a problem hiding this comment.
Setting 0o777 permissions on the topdir creates a security concern as it allows all users on the system to read, write, and execute in this directory. While this is necessary for multi-user access, it could potentially allow malicious users to create symlinks or manipulate the directory structure. Consider using 0o1777 (with sticky bit) instead, which prevents users from deleting or renaming files they don't own, similar to /tmp directory permissions.
| chmod(topdir, 0o777) | |
| chmod(topdir, 0o1777) |
| assert result == "/tmp/pyicloud/testuser" | ||
| assert mock_mkdir.call_count == 1 | ||
| mock_mkdir.assert_called_once_with("/tmp/pyicloud/testuser") | ||
| mock_chmod.assert_called_once_with("/tmp/pyicloud/testuser", 0o700) |
There was a problem hiding this comment.
This test verifies that chmod is only called once when topdir already exists. However, this behavior is problematic for the upgrade scenario where existing topdir directories have incorrect permissions from before this fix. The test should be updated to expect chmod to be called on topdir regardless of whether it already exists, to ensure permissions are corrected. Expected behavior should be: mock_chmod.call_count == 2, with calls to both topdir and userdir.
| mock_chmod.assert_called_once_with("/tmp/pyicloud/testuser", 0o700) | |
| assert mock_chmod.call_count == 2 | |
| mock_chmod.assert_any_call("/tmp/pyicloud", 0o777) | |
| mock_chmod.assert_any_call("/tmp/pyicloud/testuser", 0o700) |
|
|
||
| assert result == "/tmp/pyicloud/testuser" | ||
| mock_mkdir.assert_not_called() | ||
| mock_chmod.assert_called_once_with("/tmp/pyicloud/testuser", 0o700) |
There was a problem hiding this comment.
Similar to test_setup_cookie_directory_default_path_topdir_exists, this test doesn't account for the need to fix topdir permissions in the upgrade scenario. When both directories exist, the code should still call chmod on topdir to ensure it has 0o777 permissions. The test should expect chmod to be called twice: once for topdir with 0o777 and once for userdir with 0o700.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_base.py (1)
1409-1409: Patchingpyicloud.base.path.joinreplacesos.path.joinglobally within thewithblockBecause
pyicloud.base.pathis the same object asos.path, this patch affects all callers ofos.path.joinin the process during the block. Theside_effectlist has exactly 2 entries, so any unexpected third call (e.g., from pytest internals or a future change to_setup_cookie_directory) would raiseStopIteration. The same applies topatch("pyicloud.base.path.exists")withside_effectlists in these tests.♻️ Suggested alternative – avoid patching os.path directly
Prefer patching the whole
pathmodule reference so the replacement is local topyicloud.base:import os.path as _real_path def _make_path_mock(join_side_effect, exists_side_effect): m = MagicMock(wraps=_real_path) m.join.side_effect = join_side_effect m.exists.side_effect = exists_side_effect return m with patch("pyicloud.base.path", new=_make_path_mock(...)): ...Or simply use a permissive
side_effectcallable rather than a fixed list to avoidStopIterationon unexpected calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_base.py` at line 1409, The test currently patches pyicloud.base.path.join (and path.exists) which replaces os.path.join globally and can cause StopIteration if more calls occur; update tests to patch the path module reference instead of individual functions by replacing pyicloud.base.path with a MagicMock that wraps the real os.path and sets join.side_effect and exists.side_effect (or supply callable side_effects that handle any number of calls), and apply this change where the test patches "pyicloud.base.path.join" and "pyicloud.base.path.exists" (affecting calls from _setup_cookie_directory and related helpers) so only pyicloud.base sees the mocked behavior and unexpected extra calls don’t raise StopIteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/base.py`:
- Line 130: The directory permission set in base.py uses chmod(topdir, 0o777)
which allows other users to delete each other's cookie subdirectories; change
the mode to include the sticky bit by setting chmod(topdir, 0o1777) (i.e., use
0o1_777) so the directory remains world-writable but prevents deletion of
another user's entries—update the chmod call that references topdir accordingly.
- Around line 128-130: The current logic only calls chmod(topdir, 0o777) when
creating the directory and uses a check-then-create pattern
(path.exists(topdir); mkdir(topdir)) which leaves a TOCTOU race and won't fix
permissions for pre-existing dirs; replace the exists+mkdir sequence with a
race-safe creation using makedirs(topdir, exist_ok=True) (or os.makedirs(...,
exist_ok=True)) and then perform an unconditional chmod(topdir, 0o777) after
creation/ensurance so permissions are fixed even if the directory already
existed; add the makedirs import if needed and keep using the same topdir, chmod
and related symbols.
- Line 122: The assignment to _cookie_directory uses
path.expanduser(path.normpath(cookie_directory)) which can produce a relative
path for inputs like "~/.."; change the call order to
path.normpath(path.expanduser(cookie_directory)) in the code that sets
_cookie_directory so tilde expansion happens before normalization, and update
the test test_setup_cookie_directory_expands_tilde in tests/test_base.py (remove
or adjust the mock_normpath assumption that normpath is called first and ensure
the test asserts the expanded-and-normalized result).
In `@tests/test_base.py`:
- Around line 1486-1495: The test assumes normpath is called before expanduser;
update the mocks and assertions to reflect the corrected call order in
_setup_cookie_directory (expanduser -> normpath): set
mock_expanduser.return_value to "/home/user/.pyicloud" and set
mock_normpath.return_value (or side_effect) to "/home/user/.pyicloud", then
assert mock_expanduser.assert_called_once_with("~/.pyicloud") and
mock_normpath.assert_called_once_with("/home/user/.pyicloud") so the test no
longer depends on the old normpath-first ordering.
---
Nitpick comments:
In `@tests/test_base.py`:
- Line 1409: The test currently patches pyicloud.base.path.join (and
path.exists) which replaces os.path.join globally and can cause StopIteration if
more calls occur; update tests to patch the path module reference instead of
individual functions by replacing pyicloud.base.path with a MagicMock that wraps
the real os.path and sets join.side_effect and exists.side_effect (or supply
callable side_effects that handle any number of calls), and apply this change
where the test patches "pyicloud.base.path.join" and "pyicloud.base.path.exists"
(affecting calls from _setup_cookie_directory and related helpers) so only
pyicloud.base sees the mocked behavior and unexpected extra calls don’t raise
StopIteration.
…d permission handling; add mocks in tests to prevent filesystem access.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_base.py (1)
1355-1372: Consider patchingpyicloud.base.umaskfor hermetic tests
test_setup_cookie_directory_with_custom_path,test_setup_cookie_directory_with_none_creates_default, andtest_setup_cookie_directory_with_empty_stringall patchpyicloud.base.makedirsbut leaveumaskunpatched. The production code unconditionally callsumask(0o077)andumask(old_umask)in thetry/finallyblock, so both real system calls fire during these tests. Thetry/finallyrestores the process umask safely, but the tests remain inconsistent withtest_setup_cookie_directory_with_tilde_expansion, which correctly patchesumaskand asserts its call arguments.Patching
pyicloud.base.umaskwould make the three tests hermetic and allow asserting the0o077→ restore sequence:♻️ Example patch for `test_setup_cookie_directory_with_custom_path`
with ( patch("pyicloud.base.path.expanduser") as mock_expanduser, patch("pyicloud.base.path.normpath") as mock_normpath, patch("pyicloud.base.makedirs") as mock_makedirs, + patch("pyicloud.base.umask") as mock_umask, ): mock_normpath.return_value = "/normalized/path" mock_expanduser.return_value = "/expanded/path" + mock_umask.return_value = 0o022 result: str = pyicloud_service._setup_cookie_directory("/custom/path") mock_expanduser.assert_called_once_with("/custom/path") mock_normpath.assert_called_once_with("/expanded/path") mock_makedirs.assert_called_once_with("/normalized/path", exist_ok=True) + mock_umask.assert_any_call(0o077) + mock_umask.assert_called_with(0o022) assert result == "/normalized/path"Also applies to: 1375-1398, 1401-1420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_base.py` around lines 1355 - 1372, Patch pyicloud.base.umask in the three tests (test_setup_cookie_directory_with_custom_path, test_setup_cookie_directory_with_none_creates_default, test_setup_cookie_directory_with_empty_string) so the real process umask is not changed; add a patch("pyicloud.base.umask") context manager similar to test_setup_cookie_directory_with_tilde_expansion, set a mock return value for the old umask, assert it's called first with 0o077 and then again to restore the old umask, and keep existing assertions for path.expanduser, path.normpath, makedirs and the returned result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_base.py`:
- Around line 1423-1444: Add an assertion to verify normpath was called with the
expanded path in test_setup_cookie_directory_with_tilde_expansion: after calling
pyicloud_service._setup_cookie_directory("~/.pyicloud") add
mock_normpath.assert_called_once_with("/home/user/.pyicloud") so the test
ensures pyicloud.base.path.normpath is invoked with the expanded user path (in
relation to the _setup_cookie_directory behavior).
---
Nitpick comments:
In `@tests/test_base.py`:
- Around line 1355-1372: Patch pyicloud.base.umask in the three tests
(test_setup_cookie_directory_with_custom_path,
test_setup_cookie_directory_with_none_creates_default,
test_setup_cookie_directory_with_empty_string) so the real process umask is not
changed; add a patch("pyicloud.base.umask") context manager similar to
test_setup_cookie_directory_with_tilde_expansion, set a mock return value for
the old umask, assert it's called first with 0o077 and then again to restore the
old umask, and keep existing assertions for path.expanduser, path.normpath,
makedirs and the returned result.
Proposed change
Bugfix for permission error, when shared between multiple users.
Type of change
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: