-
Notifications
You must be signed in to change notification settings - Fork 108
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
Nmap error handling #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TheSecEng!
Congratulations on your PR, I'm very excited about your contribution :) I have a few suggestions, mostly related to style, but that should be addressed before the PR can be merged 👍
Thanks a lot and let me know if you have any questions!
Removing Plurals as suggest @Ullaakut Co-Authored-By: Brendan Le Glaunec <brendan.le-glaunec@epitech.eu>
Removing Plurals as suggest @Ullaakut Co-Authored-By: Brendan Le Glaunec <brendan.le-glaunec@epitech.eu>
Removing Plurals as suggest @Ullaakut Co-Authored-By: Brendan Le Glaunec <brendan.le-glaunec@epitech.eu>
Updated from bool to struct{} as suggested. Co-Authored-By: Brendan Le Glaunec <brendan.le-glaunec@epitech.eu>
…nmap-error-handling
Updating to append a comment about function. Co-Authored-By: Brendan Le Glaunec <brendan.le-glaunec@epitech.eu>
All changes have been made...This is my first PR on a project ! What a learning experience. |
Hey @TheSecEng ! Well you did a great job for a first PR! 👏 I'll just checkout the branch and test it out on my machine for a few minutes, and then it will be merged :) Thanks again for your contribution friend! |
Ah, the unit tests need an update it seems! # github.com/Ullaakut/nmap [github.com/Ullaakut/nmap.test]
./xml_test.go:1069:24: not enough arguments in call to Parse
have ([]byte)
want ([]byte, []NmapError) If you are not familiar with Go unit tests, let me know and I'll handle it :) |
I am unfamiliar with those. I'd love to see what you have to change to make this work. As a learning experience. |
Okay so after looking into it, I think it's better for If we kept it, the way the tests would have been changed in this case would have simply been to pass a EDIT: I'll push a commit that makes those changes in a few minutes and we'll merge the PR :) |
Ahh you know, originally when I had this written out I had additional logic in the Parse() function. Since I removed that, you are right, its not needed. Commited. |
Added a NmapErrors Struct for holding errors encountered during the Nmap Scan. Since these errors are specific to nmap, we shouldn't cancel an entire multi target scan just because one host is unresolvable or causing issues.
Additionally, added a utils file that holds a RemoveDuplicates to remove Duplicate nmap errors. Nmap can under certain circumstances return duplicate errors.
This could potentially be further improved to remove duplicates of specific errors.
Example Usage: