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

Add console-based hex editors for binary formats #5252

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mohamedmostafadawood
Copy link

No description provided.

Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

mitmproxy/tools/console/master.py Show resolved Hide resolved
for editor in "hexedit", "hexyl":
if shutil.which(editor):
return editor
if os.name == "xxd":
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comparison. Can you explain?

Choose a reason for hiding this comment

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

I figured out the error :).
I will push the new code.

if os.name == "xxd":
return "xxd"
else:
return "ghex"
Copy link
Member

Choose a reason for hiding this comment

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

What happens if ghex is not installed?

def spawn_editor(self, data):
if strutils.is_mostly_bin(data):
isBinary = True
Copy link
Member

Choose a reason for hiding this comment

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

  • Please use snake_case for Python variables.
  • Is there a reason why you create a variable here instead of directly using strutils.is_mostly_bin later on?

Choose a reason for hiding this comment

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

I got it! Thanks.

mitmproxy/tools/console/master.py Show resolved Hide resolved
if shutil.which(editor):
return editor
else:
return "notepad"
Copy link
Member

Choose a reason for hiding this comment

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

This code is clearly untested as it contains a major logic bug. Please don't rely on me catching your errors but test your code yourself. Ideally by writing tests. :)

Choose a reason for hiding this comment

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

Okay, sir. I will add the required test and sorry for the inconvenience.

@mohamedmostafadawood
Copy link
Author

@mhils I have tested it and it is now working.
But, on Windows, I faced a problem related to that shutil.which() sometimes cannot locate a hex-editor.
I have made some research related to this problem and found this workaround. But, I think it is dependent on the user machine using the tool and if he has changed the PATHEXT as this workaround suggests or not.
So, what do you think will be the best solution for this to continue my work :) ?
Screenshot from 2022-04-14 21-32-54

@mohamedmostafadawood
Copy link
Author

@mhils I have tested it and it is now working. But, on Windows, I faced a problem related to that shutil.which() sometimes cannot locate a hex-editor. I have made some research related to this problem and found this workaround. But, I think it is dependent on the user machine using the tool and if he has changed the PATHEXT as this workaround suggests or not. So, what do you think will be the best solution for this to continue my work :) ? Screenshot from 2022-04-14 21-32-54

references:
https://stackoverflow.com/questions/56399696/shutil-which-not-finding-programs-without-appending-file-extension

https://stackoverflow.com/questions/61097406/jupyter-kernelspec-not-found-while-installing-iqsharp-however-it-exists-on-pat/

@@ -13,7 +13,7 @@
import threading

Copy link
Contributor

@rinsuki rinsuki May 19, 2022

Choose a reason for hiding this comment

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

It is need to use platform.system()

Suggested change
import platform

Comment on lines +129 to +137
for editor in "ghex", "hexedit", "bless", "wxhexeditor", "nano":
if shutil.which(editor):
return editor
if os.name == "nt":
for editor in "HxD", "WinVi", "Cygnus", "Frhed", "notepad":
if shutil.which(editor):
return editor
else:
return "vi"
Copy link
Contributor

Choose a reason for hiding this comment

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

bless command is completely different tool on macOS (in macOS, it is a tool to modify startup disk configuration / set volumes bootability), so it might be good to check Darwin or not.

also I think nano is not suitable for binary editor.

Suggested change
for editor in "ghex", "hexedit", "bless", "wxhexeditor", "nano":
if shutil.which(editor):
return editor
if os.name == "nt":
for editor in "HxD", "WinVi", "Cygnus", "Frhed", "notepad":
if shutil.which(editor):
return editor
else:
return "vi"
editors = ["ghex", "hexedit", "wxhexeditor"]
if platform.system() == "Darwin":
editors = ["hexf"] + editors
else:
editors += ["bless"]
if os.name == "nt":
editors += ["HxD", "WinVi", "Cygnus", "Frhed", "notepad"]
for editor in editors:
if shutil.which(editor):
return editor
return "vi"

(note: hexf command is shortcut to Hex Fiend.app https://github.com/HexFiend/HexFiend )

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.

None yet

3 participants