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

Added input validation to OpenFile and CreateFile methods #322 #324

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

TheBaronOfDubstep
Copy link

Recreated the pull request to ensure I have only verified commits and all code style checks will succeed

@TheBaronOfDubstep
Copy link
Author

@LiorBanai As I do not know whether or not this nuget is cross platform, I will not introduce a binding to kernel32.dll. This code is ready for merge

@LiorBanai
Copy link
Owner

Thanks! . I'll review it. And yes it is cross platform but we can check the running platform and bind only on windows.

@TheBaronOfDubstep
Copy link
Author

Thanks! . I'll review it. And yes it is cross platform but we can check the running platform and bind only on windows.

Sounds great. I'll just read up on the compiler macros for that, and I'll send you another that will compile. I have no idea why this one won't.

@TheBaronOfDubstep
Copy link
Author

@LiorBanai new version is up and should compile better :)

@TheBaronOfDubstep
Copy link
Author

@LiorBanai Again, sorry for the commit spam. Tried to stay within guidelines of the static code analysis, but it promotes outdated code standards (for c# 4) and advocates against current best practices (for c# 7)

@LiorBanai
Copy link
Owner

@TheBaronOfDubstep Don't worry about the static code analysis.. it is indeed need revising...
I plan to add editorconfig not far away.

@LiorBanai
Copy link
Owner

AS long ad the code compiled and unit tests passes that is what important.
any chance you can add some specific tests?

@TheBaronOfDubstep
Copy link
Author

I can add tests for the input sanitation, but I am not sure if the tests will run if I include the kernel32.dll call in them

bug: file referenced in path needs to exist for shortpath to work. Check and exceptions added.
@TheBaronOfDubstep
Copy link
Author

@LiorBanai Tests are added for the added functionality of this PR.

@LiorBanai
Copy link
Owner

@TheBaronOfDubstep I'll review it this weekend.
Thanks! :)

@LiorBanai
Copy link
Owner

@TheBaronOfDubstep Thanks

@LiorBanai LiorBanai merged commit 46ba393 into LiorBanai:main Sep 15, 2023
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.

2 participants