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

Mark OSX home directory as a posix path #867

Merged
merged 4 commits into from
Oct 3, 2024
Merged

Conversation

twiggler
Copy link
Contributor

@twiggler twiggler commented Sep 27, 2024

This fixes an issue where the home directory could not be found when running dissect on windows.

@pyrco also helped

Closes #866

@twiggler twiggler changed the title Marl OSX home directory as a posix path Mark OSX home directory as a posix path Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.26%. Comparing base (38f84b2) to head (53ac69c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #867   +/-   ##
=======================================
  Coverage   76.26%   76.26%           
=======================================
  Files         312      312           
  Lines       26788    26789    +1     
=======================================
+ Hits        20430    20431    +1     
  Misses       6358     6358           
Flag Coverage Δ
unittests 76.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@twiggler twiggler force-pushed the fix/make-homedir-osx-posix branch 2 times, most recently from 1b2c3b1 to 5119ff9 Compare September 27, 2024 16:21
@twiggler twiggler self-assigned this Sep 27, 2024
@twiggler twiggler requested a review from Miauwkeru September 27, 2024 16:23
@twiggler twiggler force-pushed the fix/make-homedir-osx-posix branch from 361569b to 62deea1 Compare September 27, 2024 18:26
@@ -25,7 +25,7 @@ def test_unix_bsd_osx_os(target_osx_users, fs_osx):

assert dissect_user.name == "_dissect"
assert dissect_user.passwd == "*"
assert dissect_user.home == "/var/empty"
assert str(dissect_user.home) == "/var/empty"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary if home is an instance of TargetPath.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be needed either way as the flow path __eq__ can deal with directly comparing against strings.

Copy link
Contributor Author

@twiggler twiggler Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pyrco @JSCU-CNI The point is to bypass the overriden __eq__ method

By comparing to the string representation when the test is running under windows, I am ensuring that the home directory is stored in posix_format; when it is not, this assertion will fail because the string representation would contain backslashes. The __eq__ would pass, even it is a windows_format.

Effectively, this guards against regressions, where the home path would be interpreted as a windows path when running under windows.

I have added a comment to the code to clarify.

Alternatively, i can add an explicit type assertion for posix_path. But perhaps it is better to check behavior than types.

Copy link
Contributor Author

@twiggler twiggler Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Added comment)

@pyrco @JSCU-CNI The right-hand term of the or operator https://github.com/fox-it/flow.record/blob/1701dcf6ff77d99923ef2523cb258fc75ade68e1/flow/record/fieldtypes/__init__.py#L698

makes the comparison through __eq__ more lenient than directly comparing to the string representation.

i.e. more concretely, windows_path('/Users/dissect') == '/Users/dissect',
while str(windows_path('/Users/dissect')) == '\\Users\\dissect'

@twiggler twiggler requested a review from pyrco September 30, 2024 12:00
@twiggler twiggler force-pushed the fix/make-homedir-osx-posix branch 2 times, most recently from ce0d672 to e61ac3c Compare September 30, 2024 14:08
@twiggler twiggler requested a review from JSCU-CNI September 30, 2024 14:17
Comment on lines 28 to 31
# When the test is running under windows, comparing to the string representation ensures
# that the home directory is stored in posix format.
# The __eq__ operator of the path class is too lenient.
assert str(dissect_user.home) == "/var/empty"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# When the test is running under windows, comparing to the string representation ensures
# that the home directory is stored in posix format.
# The __eq__ operator of the path class is too lenient.
assert str(dissect_user.home) == "/var/empty"
assert dissect_user.home == "/var/empty"

The flow.record paths were explicitly designed so we could do this in tests. No need for casts or comments.

Copy link
Contributor

@pyrco pyrco Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an explicit type check for posix_path to prevent regressions is merited as that is what it should be by definition.

Copy link
Contributor Author

@twiggler twiggler Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added an assertion for posix_path.

In the long term, I am still slightly unsatisfied by our current solution :)

  • I think the original assertion assert dissect_user.home == "/var/empty" should have caught the bug on windows this pr fixes.
  • Related, but I think adding type checks cause the test to become unnecessarily coupled to implementation details (in this case posix_path). Ideally, I think we should check for behavior. So I slightly prefer the str cast and the comparison to the string representation.

But perhaps this is only the first time we have a bug like this. I have made a note for future reference. Maybe we can discuss at a later time in person.

@twiggler twiggler requested a review from pyrco October 1, 2024 06:17
@twiggler twiggler force-pushed the fix/make-homedir-osx-posix branch from d48bdfa to 53ac69c Compare October 3, 2024 09:11
@twiggler twiggler merged commit 6d0ddd0 into main Oct 3, 2024
18 checks passed
@twiggler twiggler deleted the fix/make-homedir-osx-posix branch October 3, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Parallels child detection
3 participants