Update main.star #2019
Update main.star #2019
Conversation
1.Error checking and logging: Added error checks for command executions with meaningful error messages. This makes debugging easier and helps identify failure points more clearly. 2.Avoiding code duplication: Created the run_command function to avoid repeating code for command execution and error handling, simplifying the structure and making future modifications easier. 3.Wallet handling: Moved wallet type handling into a separate get_wallet_command function. This makes it easier to extend for new wallet types in the future. 4.Safe access to deployment data: Used .get() method to safely access dictionary values in deployment to prevent KeyError if a key is missing. 5.String constants for file paths and commands: Moved frequently used strings such as file paths and commands into constants for better maintainability and to avoid potential errors if these values need to be changed in the future. 6.Security improvements: Introduced the use of shlex.quote to safely handle user input (like RPC URLs and wallet values) and prevent command injection vulnerabilities. Signed-off-by: futreall <[email protected]>
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey team, happy to make any necessary adjustments or improvements if needed. Let me know if there's anything you'd like me to tweak or clarify! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| contract_path = "{}/{}".format(SOURCE_DIR_PATH, contracts_path) if contracts_path else SOURCE_DIR_PATH | ||
|
|
||
| if dependency_type == "local": | ||
| # Run shell script | ||
| plan.exec( | ||
| service_name = foundry_service.name, | ||
| recipe = ExecRecipe( | ||
| command = ["/bin/sh", "-c", "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path)], | ||
| ), | ||
| ) | ||
| run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path), "Local dependency execution failed") | ||
| elif dependency_type == "git": | ||
| plan.exec( | ||
| service_name = foundry_service.name, | ||
| recipe = ExecRecipe( | ||
| command = ["/bin/sh", "-c", "cd {} && sh {}".format(contract_path, dependency_path)], | ||
| ), | ||
| ) | ||
| run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, dependency_path), "Git dependency execution failed") | ||
|
|
||
| if script_path: | ||
| result = plan.exec( | ||
| service_name = foundry_service.name, | ||
| recipe = ExecRecipe( | ||
| command = ["/bin/sh", "-c", "cd {} && forge build".format(contract_path)], | ||
| ), | ||
| ) | ||
| plan.verify(result["code"], "==", 0) | ||
| # Logging the build and deployment process | ||
| plan.print("Running deployment script: {}".format(script_path)) | ||
| run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed") |
There was a problem hiding this comment.
Sanitize contracts_path when used in shell commands
The contracts_path variable is derived from deployment data and used in shell commands without proper sanitization. This could lead to command injection vulnerabilities.
Apply shlex.quote to contracts_path when constructing shell commands:
contract_path = "{}/{}".format(SOURCE_DIR_PATH, contracts_path) if contracts_path else SOURCE_DIR_PATH
+contract_path_quoted = shlex.quote(contract_path)
- run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed")
+ run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path_quoted), "Forge build failed")
...
- "cd {} && {}".format(contract_path, TRANSACTION_FILE_CMD)
+ "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD)
...
- "cat {}/{}".format(contract_path, OUTPUT_FILE),
+ "cat {}/{}".format(contract_path_quoted, OUTPUT_FILE),Also applies to: 83-89, 96-96, 105-105
| # Logging the build and deployment process | ||
| plan.print("Running deployment script: {}".format(script_path)) | ||
| run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed") |
There was a problem hiding this comment.
Sanitize script_path and contract_name to prevent command injection
The script_path and contract_name variables are used in shell commands without sanitization. This could lead to vulnerabilities if they contain malicious input.
Apply shlex.quote to script_path and contract_name when constructing shell commands:
plan.print("Running deployment script: {}".format(script_path))
+script_path_quoted = shlex.quote(script_path)
+contract_name_quoted = shlex.quote(contract_name)
- run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed")
+ run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path_quoted), "Forge build failed")
...
script_output = exec_on_service(
plan,
foundry_service.name,
- "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
- contract_path,
- script_path,
- contract_name,
+ "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
+ contract_path_quoted,
+ script_path_quoted,
+ contract_name_quoted,
rpc_url,
wallet_command,
OUTPUT_FILE
),
)Also applies to: 83-89
| "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format( | ||
| contract_path, | ||
| script_path, | ||
| contract_name, | ||
| rpc_url, | ||
| wallet_command, | ||
| OUTPUT_FILE |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability by refactoring command construction
The command string used in the forge script execution is lengthy and complex. Refactoring it can enhance readability and maintainability.
Refactor the command construction as follows:
script_command = "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
contract_path_quoted,
script_path_quoted,
contract_name_quoted,
rpc_url,
wallet_command,
OUTPUT_FILE
)
# Then use the command in exec_on_service or run_command
run_command(
plan,
foundry_service.name,
script_command,
"Forge script execution failed",
)|
|
||
| def run(plan, deployment = {}): | ||
| def run(plan, deployment={}): |
There was a problem hiding this comment.
Avoid mutable default arguments
In the run function, using a mutable default argument like {} for deployment can lead to unexpected behaviors because the default mutable object is shared across all calls to the function.
Apply this diff to fix the issue:
-def run(plan, deployment={}):
+def run(plan, deployment=None):
+ if deployment is None:
+ deployment = {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def run(plan, deployment={}): | |
| def run(plan, deployment=None): | |
| if deployment is None: | |
| deployment = {} |
| dependency_path = dependency.get("path", "") | ||
| plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR) |
There was a problem hiding this comment.
Check for missing dependency_path before use
The dependency_path is retrieved with a default empty string but used in commands. If it's empty, the command may fail.
Add a check to ensure dependency_path is provided when a dependency is specified:
dependency_path = dependency.get("path", "")
+if not dependency_path:
+ fail("Dependency path not specified for dependency type '{}'".format(dependency_type))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dependency_path = dependency.get("path", "") | |
| plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR) | |
| dependency_path = dependency.get("path", "") | |
| if not dependency_path: | |
| fail("Dependency path not specified for dependency type '{}'".format(dependency_type)) | |
| plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR) |
| contract_name = deployment.get("contract_name", "") | ||
| rpc_url = shlex.quote(deployment.get("rpc_url", "")) |
There was a problem hiding this comment.
Check for missing contract_name and rpc_url before use
The contract_name and rpc_url are retrieved with default empty strings but used in commands. If either is empty, the command may fail or behave unexpectedly.
Add checks to ensure contract_name and rpc_url are provided:
contract_name = deployment.get("contract_name", "")
+if not contract_name:
+ fail("Contract name not specified in deployment")
rpc_url = shlex.quote(deployment.get("rpc_url", ""))
+if not deployment.get("rpc_url"):
+ fail("RPC URL not specified in deployment")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contract_name = deployment.get("contract_name", "") | |
| rpc_url = shlex.quote(deployment.get("rpc_url", "")) | |
| contract_name = deployment.get("contract_name", "") | |
| if not contract_name: | |
| fail("Contract name not specified in deployment") | |
| rpc_url = shlex.quote(deployment.get("rpc_url", "")) | |
| if not deployment.get("rpc_url"): | |
| fail("RPC URL not specified in deployment") |
| if wallet_type == "private_key": | ||
| return "--private-key {}".format(shlex.quote(wallet["value"])) | ||
| else: |
There was a problem hiding this comment.
Add error handling for missing wallet["value"]
The code assumes that wallet["value"] exists, which may raise a KeyError if the key is missing. It's important to check for the presence of this key before using it.
Add a check to ensure wallet["value"] is present:
def get_wallet_command(wallet):
wallet_type = wallet["type"]
if wallet_type == "private_key":
+ if "value" not in wallet:
+ fail("Wallet value not specified for private_key type")
return "--private-key {}".format(shlex.quote(wallet["value"]))
else:
fail("Wallet type {} not supported.".format(wallet_type))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if wallet_type == "private_key": | |
| return "--private-key {}".format(shlex.quote(wallet["value"])) | |
| else: | |
| if wallet_type == "private_key": | |
| if "value" not in wallet: | |
| fail("Wallet value not specified for private_key type") | |
| return "--private-key {}".format(shlex.quote(wallet["value"])) | |
| else: |
| exec_output = exec_on_service( | ||
| plan, | ||
| foundry_service.name, | ||
| "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"]) |
There was a problem hiding this comment.
Sanitize transaction_file_details["output"] to prevent command injection
The transaction_file_details["output"] is used in a shell command without sanitization. This could be exploited if the output contains malicious content.
Apply shlex.quote to transaction_file_details["output"] when constructing shell commands:
exec_output = exec_on_service(
plan,
foundry_service.name,
- "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"])
+ "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"]))
)Committable suggestion was skipped due to low confidence.
| run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path), "Local dependency execution failed") | ||
| elif dependency_type == "git": | ||
| plan.exec( | ||
| service_name = foundry_service.name, | ||
| recipe = ExecRecipe( | ||
| command = ["/bin/sh", "-c", "cd {} && sh {}".format(contract_path, dependency_path)], | ||
| ), | ||
| ) | ||
| run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, dependency_path), "Git dependency execution failed") |
There was a problem hiding this comment.
Secure dependency_path to prevent command injection
The dependency_path variable is used in shell commands without proper sanitization. This could lead to command injection vulnerabilities if dependency_path contains malicious input.
Apply shlex.quote to dependency_path when constructing shell commands:
if dependency_type == "local":
- run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path), "Local dependency execution failed")
+ run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, shlex.quote(dependency_path)), "Local dependency execution failed")
elif dependency_type == "git":
- run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, dependency_path), "Git dependency execution failed")
+ run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, shlex.quote(dependency_path)), "Git dependency execution failed")Committable suggestion was skipped due to low confidence.
| # Executing forge script | ||
| script_output = exec_on_service( | ||
| plan, | ||
| foundry_service.name, | ||
| "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > output.json ".format( | ||
| "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format( | ||
| contract_path, | ||
| script_path, | ||
| contract_name, | ||
| rpc_url, | ||
| wallet_command, | ||
| OUTPUT_FILE | ||
| ), | ||
| ) | ||
|
|
||
| exec_on_service( | ||
| plan, | ||
| foundry_service.name, | ||
| "cat {}/output.json ".format(contract_path), | ||
| "cat {}/{}".format(contract_path, OUTPUT_FILE), | ||
| ) | ||
|
|
||
| if script_path: | ||
| # Get the forge script output in a output.json file and grep from it | ||
| transaction_file = "grep 'Transactions saved to' output.json | awk -F': ' '{print $2}'" | ||
| plan.print("transaction_file", transaction_file) | ||
|
|
||
| transaction_file_details = exec_on_service(plan, foundry_service.name, "cd {} && {}".format(contract_path, transaction_file)) | ||
|
|
||
| # Fetch and check the transaction file | ||
| plan.print("Retrieving transaction file...") | ||
| transaction_file_details = exec_on_service( | ||
| plan, | ||
| foundry_service.name, | ||
| "cd {} && {}".format(contract_path, TRANSACTION_FILE_CMD) | ||
| ) | ||
|
|
||
| if not transaction_file_details["output"]: | ||
| fail("Transaction file not found.") | ||
| exec_output = exec_on_service(plan, foundry_service.name, "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"])) | ||
|
|
||
| exec_output = exec_on_service( | ||
| plan, | ||
| foundry_service.name, | ||
| "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"]) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify command execution and error handling
Multiple commands are executed using exec_on_service without consistent error handling. To maintain consistency and ensure proper error handling, consider using run_command instead of exec_on_service where appropriate.
Refactor the code to use run_command for command execution that requires error checks:
-# Executing forge script
-script_output = exec_on_service(
- plan,
- foundry_service.name,
- "cd {} && forge script ...".format(...),
-)
+run_command(
+ plan,
+ foundry_service.name,
+ "cd {} && forge script ...".format(...),
+ "Forge script execution failed",
+)
...
-transaction_file_details = exec_on_service(
- plan,
- foundry_service.name,
- "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD)
-)
+transaction_file_details = run_command(
+ plan,
+ foundry_service.name,
+ "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD),
+ "Transaction file retrieval failed",
+)
...
-exec_output = exec_on_service(
- plan,
- foundry_service.name,
- "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"]))
-)
+exec_output = run_command(
+ plan,
+ foundry_service.name,
+ "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"])),
+ "Failed to read transaction file",
+)Committable suggestion was skipped due to low confidence.
1.Error checking and logging: Added error checks for command executions with meaningful error messages. This makes debugging easier and helps identify failure points more clearly.
2.Avoiding code duplication: Created the run_command function to avoid repeating code for command execution and error handling, simplifying the structure and making future modifications easier.
3.Wallet handling: Moved wallet type handling into a separate get_wallet_command function. This makes it easier to extend for new wallet types in the future.
4.Safe access to deployment data: Used .get() method to safely access dictionary values in deployment to prevent KeyError if a key is missing.
5.String constants for file paths and commands: Moved frequently used strings such as file paths and commands into constants for better maintainability and to avoid potential errors if these values need to be changed in the future.
6.Security improvements: Introduced the use of shlex.quote to safely handle user input (like RPC URLs and wallet values) and prevent command injection vulnerabilities.
Summary by CodeRabbit