Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshot: don't walk directories without read permissions #573

Conversation

BoboTiG
Copy link
Collaborator

@BoboTiG BoboTiG commented Jun 11, 2019

Original patch by Joshua Skelton (@joshuaskelly) on issue #408.

@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 11, 2019

@rrzaripov, do you mind reviewing 🙏 ?

@BoboTiG BoboTiG force-pushed the fix-issue-408-do-not-walk-folder-without-read-perm branch 3 times, most recently from e9c84d5 to dea9a51 Compare June 11, 2019 22:49
@rrzaripov
Copy link
Contributor

@BoboTiG I was tested this patch under Windows.
For example, next directory structure exists c:\tmp\A\B\C. All directories for simplifying empty. While testing we will disabling access to B.

Python 2.7

>>> from watchdog.utils import dirsnapshot
>>> dirsnapshot.DirectorySnapshot("c:\\tmp\\A")
{'c:\\tmp\\A': StatResult(st_dev=2195292832L, st_ino=6473924464939757L, st_mode=16895, st_mtime=1560413735L),
'c:\\tmp\\A\\B': StatResult(st_dev=2195292832L, st_ino=15199648742969916L, st_mode=16895, st_mtime=1560413742L), 
'c:\\tmp\\A\\B\\C': StatResult(st_dev=2195292832L, st_ino=5348024558097312L, st_mode=16895, st_mtime=1560413739L),}
>>> # Disable access to C:\tmp\A\B
>>> dirsnapshot.DirectorySnapshot("c:\\tmp\\A")
{'c:\\tmp\\A': StatResult(st_dev=2195292832L, st_ino=6473924464939757L, st_mode=16895, st_mtime=1560413735L)}

Python 3.6

>>> from watchdog.utils import dirsnapshot
>>> dirsnapshot.DirectorySnapshot("c:\\tmp\\A")
{'c:\\tmp\\A': os.stat_result(st_mode=16895, st_ino=6473924464939757, st_dev=2195292832, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1560413735, st_mtime=1560413735, st_ctime=1560411735),
'c:\\tmp\\A\\B': os.stat_result(st_mode=16895, st_ino=15199648742969916, st_dev=2195292832, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1560413742, st_mtime=1560413742, st_ctime=1560413306), 
'c:\\tmp\\A\\B\\C': os.stat_result(st_mode=16895, st_ino=5348024558097312, st_dev=2195292832, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1560413739, st_mtime=1560413739, st_ctime=1560413739)}
>>> # Disable access to C:\tmp\A\B
>>> dirsnapshot.DirectorySnapshot("c:\\tmp\\A")
{'c:\\tmp\\A': os.stat_result(st_mode=16895, st_ino=6473924464939757, st_dev=2195292832, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1560413735, st_mtime=1560413735, st_ctime=1560411735),
'c:\\tmp\\A\\B': os.stat_result(st_mode=16895, st_ino=0, st_dev=0, st_nlink=0, st_uid=0, st_gid=0, st_size=0, st_atime=1560413742, st_mtime=1560413742, st_ctime=1560413306)}

In both cases we not get content of B directory, without exceptions, this is great and I think #408 may be closed after merging this patch. But on Python 3.6 snapshot of directory A contains directory B. I think this is just Python 2 specificity, and just need to keep in mind.

@BoboTiG BoboTiG force-pushed the fix-issue-408-do-not-walk-folder-without-read-perm branch from dea9a51 to 191108d Compare June 13, 2019 09:42
@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 13, 2019

This is great, thank you very much for the review and test 💪

@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 13, 2019

I should add a test, this is somewhat crucial :)

@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 13, 2019

Test added + some refactoring.

* Rework DirectorySnapshot to allow monkeypatching .walk();
* Added repr(DirectorySnapshotDiff) to ease catchng changes.
@BoboTiG BoboTiG force-pushed the fix-issue-408-do-not-walk-folder-without-read-perm branch from efea5d4 to 9b493ce Compare June 13, 2019 13:52
@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 13, 2019

I will merge if you are still OK @rrzaripov.

@rrzaripov
Copy link
Contributor

@BoboTiG I'm checkout latest version of branch, test passed, all looks good for merging!

@BoboTiG BoboTiG merged commit ecaa927 into gorakhargosh:master Jun 14, 2019
@BoboTiG BoboTiG deleted the fix-issue-408-do-not-walk-folder-without-read-perm branch June 14, 2019 12:24
@joshuaskelly
Copy link

joshuaskelly commented Jun 14, 2019

@BoboTiG Thanks for driving this and getting it merged back in! 🎉🏆

@BoboTiG
Copy link
Collaborator Author

BoboTiG commented Jun 14, 2019

Thansk to you for the original patch ;)

CCP-Aporia pushed a commit to CCP-Aporia/watchdog that referenced this pull request Aug 13, 2020
…sh#573)

Original patch by Joshua Skelton (@joshuaskelly) on issue gorakhargosh#408.

* Add test + code refactoring

    - Rework DirectorySnapshot to allow monkeypatching .walk();
    - Added repr(DirectorySnapshotDiff) to ease catchng changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants