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

Hardening suggestions for PorySuite / plugins/expansion/1.8.1 #14

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

pixeebot[bot]
Copy link

@pixeebot pixeebot bot commented Apr 3, 2024

I've reviewed the recently opened PR (13 - Update pokeemerald_expansion plugin to v0.1.1) and have identified some area(s) that could benefit from additional hardening measures.

These changes should help prevent potential security vulnerabilities and improve overall code quality.

Thank you for your consideration!

docs | feedback
Powered by: pixeebot

@pixeebot pixeebot bot requested a review from jschoeny April 3, 2024 01:49
subprocess.run(["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
subprocess.run(["osascript", "-e", 'tell app "Terminal" to activate'])
safe_command.run(subprocess.run, ["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
safe_command.run(subprocess.run, ["osascript", "-e", 'tell app "Terminal" to activate'])
elif sys.platform == "win32":
command = f"docker run --rm -i -t -v porysuite_{self.project_dir_name}:/root/projects/{self.project_dir_name} " \
f"--workdir /root/projects/{self.project_dir_name} porysuiteimage"
Copy link
Author

Choose a reason for hiding this comment

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

Set shell keyword argument to False

elif sys.platform == "win32":
command = f"docker run --rm -i -t -v porysuite_{self.project_dir_name}:/root/projects/{self.project_dir_name} " \
f"--workdir /root/projects/{self.project_dir_name} porysuiteimage"
subprocess.run(["start", "cmd", "/k", command], shell=True)
safe_command.run(subprocess.run, ["start", "cmd", "/k", command], shell=False)
else:
command = f"docker run --rm -i -t --mount type=bind,source={self.project_dir},target={project_dir_container} " \
f"--workdir '{project_dir_container}' porysuiteimage"
Copy link
Author

Choose a reason for hiding this comment

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

Set shell keyword argument to False

@@ -522,13 +523,13 @@ def try_open_terminal(self):
if sys.platform == "darwin":
command = f"docker run --rm -i -t --mount type=bind,source=\\\"{self.project_dir}\\\",target=\\\"{project_dir_container}\\\" " \
f"--workdir '{project_dir_container}' porysuiteimage"
Copy link
Author

Choose a reason for hiding this comment

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

Replaces subprocess.{func} with more secure safe_command library functions.

@@ -522,13 +523,13 @@ def try_open_terminal(self):
if sys.platform == "darwin":
command = f"docker run --rm -i -t --mount type=bind,source=\\\"{self.project_dir}\\\",target=\\\"{project_dir_container}\\\" " \
f"--workdir '{project_dir_container}' porysuiteimage"
subprocess.run(["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
subprocess.run(["osascript", "-e", 'tell app "Terminal" to activate'])
safe_command.run(subprocess.run, ["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
Copy link
Author

Choose a reason for hiding this comment

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

Replaces subprocess.{func} with more secure safe_command library functions.

subprocess.run(["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
subprocess.run(["osascript", "-e", 'tell app "Terminal" to activate'])
safe_command.run(subprocess.run, ["osascript", "-e", f'tell app "Terminal" to do script "{command}"'])
safe_command.run(subprocess.run, ["osascript", "-e", 'tell app "Terminal" to activate'])
elif sys.platform == "win32":
command = f"docker run --rm -i -t -v porysuite_{self.project_dir_name}:/root/projects/{self.project_dir_name} " \
f"--workdir /root/projects/{self.project_dir_name} porysuiteimage"
Copy link
Author

Choose a reason for hiding this comment

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

Replaces subprocess.{func} with more secure safe_command library functions.

elif sys.platform == "win32":
command = f"docker run --rm -i -t -v porysuite_{self.project_dir_name}:/root/projects/{self.project_dir_name} " \
f"--workdir /root/projects/{self.project_dir_name} porysuiteimage"
subprocess.run(["start", "cmd", "/k", command], shell=True)
safe_command.run(subprocess.run, ["start", "cmd", "/k", command], shell=False)
else:
command = f"docker run --rm -i -t --mount type=bind,source={self.project_dir},target={project_dir_container} " \
f"--workdir '{project_dir_container}' porysuiteimage"
Copy link
Author

Choose a reason for hiding this comment

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

Replaces subprocess.{func} with more secure safe_command library functions.

@@ -3,3 +3,6 @@ platformdirs~=4.1.0
Unidecode~=1.3.7
docker~=7.0.0
lark~=1.1.9
security==1.2.1 \
Copy link
Author

Choose a reason for hiding this comment

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

This library holds security tools for protecting Python API calls.

License: MITOpen SourceMore facts

@jschoeny jschoeny changed the base branch from plugins/expansion/1.8.1 to dev April 3, 2024 05:15
@jschoeny jschoeny changed the base branch from dev to plugins/expansion/1.8.1 April 3, 2024 05:15
@jschoeny
Copy link
Owner

jschoeny commented Apr 3, 2024

Not relevant to PR #13. Will review after #13 has been merged.

@jschoeny jschoeny marked this pull request as draft April 3, 2024 05:17
Base automatically changed from plugins/expansion/1.8.1 to dev April 5, 2024 01:14
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.

1 participant