Potential fix for code scanning alert no. 1: Shell command built from environment values#29
Potential fix for code scanning alert no. 1: Shell command built from environment values#29Dargon789 wants to merge 2 commits into
Conversation
… environment values Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors Hardhat helper scripts to avoid constructing shell commands from untrusted strings by switching to execFile/structured arguments and passing environment variables via options, eliminating shell redirection and interpolation while preserving task behavior. Sequence diagram for updated flatten flow using execFilesequenceDiagram
participant Dev as DevScript
participant flatten as flatten
participant execShellCommand as execShellCommand
participant cp as child_process_execFile
participant npx as npx_hardhat
Dev->>flatten: call flatten(filePath)
activate flatten
flatten->>execShellCommand: execShellCommand(cmd="npx", args=["hardhat","flatten",filePath])
activate execShellCommand
execShellCommand->>cp: execFile("npx", args, callback)
activate cp
cp->>npx: run hardhat flatten filePath
npx-->>cp: flattened Solidity source (stdout)
deactivate cp
cp-->>execShellCommand: callback(error, stdout, stderr)
execShellCommand->>execShellCommand: log stderr if present
execShellCommand-->>flatten: Promise resolved with stdout
deactivate execShellCommand
flatten->>flatten: process flattenedSource
flatten->>flatten: remove pragmas and comments
flatten->>flatten: append cleaned source to output file
flatten-->>Dev: completion
deactivate flatten
Sequence diagram for deployOnFork task with structured execSync callsequenceDiagram
actor User as Developer
participant Hardhat as HardhatCLI_TaskRunner
participant deployTask as deployOnFork_action
participant cpSync as child_process_execSync
participant npx as npx_hardhat
participant deployScript as deploy_on_fork_js
User->>Hardhat: run task deployOnFork --contract-names
Hardhat->>deployTask: invoke setAction handler(args)
activate deployTask
deployTask->>deployTask: build contractNames string
deployTask->>cpSync: execSync("npx", ["hardhat","run","./scripts/utils/deploy-on-fork.js","--network","fork"], options{stdio, shell, env{CONTRACTS: contractNames}})
activate cpSync
cpSync->>npx: spawn process with env.CONTRACTS
activate npx
npx->>deployScript: run with network fork and env.CONTRACTS
activate deployScript
deployScript-->>npx: deployment completed
deactivate deployScript
npx-->>cpSync: exit code
deactivate npx
cpSync-->>deployTask: return control or throw on error
deactivate cpSync
deployTask-->>Hardhat: task finished
deactivate deployTask
Hardhat-->>User: show deployment logs
Class diagram for updated Hardhat helper and task modulesclassDiagram
class HardhatTasksFunctions {
+getInput(text)
+execShellCommand(cmd, args)
+flatten(filePath)
}
class ChildProcessModule {
+exec(command, callback)
+execFile(command, args, callback)
}
class FileSystemModule {
+readFileSync(path)
+writeFileSync(path, data, options)
}
class HardhatTasksModule {
+deployOnFork(args)
}
class ChildProcessSyncModule {
+execSync(command, args, options)
}
HardhatTasksFunctions --> ChildProcessModule : uses execFile via execShellCommand
HardhatTasksFunctions --> FileSystemModule : reads and writes flattened files
HardhatTasksModule --> ChildProcessSyncModule : uses execSync
HardhatTasksModule --> HardhatTasksFunctions : same overall behavior of tasks using helpers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical code scanning alert by enhancing the security of external command execution within the project's scripts. It systematically replaces vulnerable patterns, such as constructing shell commands with interpolated strings and relying on shell redirection, with safer alternatives like Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request modifies shell execution commands in hardhat-tasks.js and hardhat-tasks-functions.js to mitigate command injection vulnerabilities by using execFile instead of exec where possible and passing arguments as an array. However, the fix in hardhat-tasks.js is incomplete and introduces a regression because it uses the wrong signature for execSync, and the shell: true option is still enabled. The reviewer also suggested documenting the args parameter in execShellCommand with a jsdoc comment and using a more appropriate logging level for stderr in hardhat-tasks-functions.js.
|
Deployment failed with the following error: Learn More: https://vercel.com/dargon789-forge?upgradeToPro=build-rate-limit |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Dargon789, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
1 participant
Potential fix for https://github.com/Dargon789/defisaver-v3-contracts/security/code-scanning/1
In general, to fix this kind of issue you should avoid passing dynamically constructed strings that contain untrusted or uncontrolled data to
exec/execSyncwith a shell. Instead, invoke the underlying binary directly usingexecFile/execFileSync(or similar) and pass dynamic parts as separate arguments, or at least properly quote/escape them. This prevents the shell from interpreting special characters in filenames or user input.For this codebase, the best targeted fix is:
execShellCommandinscripts/hardhat-tasks-functions.jsto useexecFileinstead ofexec, and have it accept a command and an argument array instead of a single shell command string.flatten(filePath)function, to callexecShellCommandwith:cmd = 'npx'args = ['hardhat', 'flatten', filePath]Then, instead of using shell redirection (
>), read fromstdoutreturned byexecFileand write the captured flattened content into the destination file withfs.writeFileSync. This avoids the shell completely.scripts/hardhat-tasks.js, adjust thedeployOnForktask to:execFileSyncwith commandnpx, args['hardhat', 'run', './scripts/utils/deploy-on-fork.js', '--network', 'fork'], and provide theCONTRACTSenvironment variable via theenvoption, rather than embedding it in a shell string.This keeps
shell: truebut no longer relies on the shell to parse a composite string; environment is passed structurally via Node.Concretely:
In
scripts/hardhat-tasks-functions.js:execFileto thechild_processimport.execShellCommand(cmd)implementation with one that callsexecFile(cmd, args, ...), whereargsis an array.flatten(filePath)so it builds thatcmdandargs, calls the newexecShellCommand, and uses the returned stdout as the flattened source instead of relying on the shell redirection operator.In
scripts/hardhat-tasks.js:const cmd = ...andexecSync(cmd, { stdio: 'inherit', shell: true });to instead callexecSync('npx', args, options)withenvthat includesCONTRACTS: contractNames.No new methods beyond these small adjustments are required; the rest of the code that consumes
flattenandexecShellCommandcan stay the same, as their external behavior (flattening and writing a file; executing commands and returning output) is preserved.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Harden script execution around Hardhat tasks to avoid constructing shell commands from untrusted environment values and rely on structured exec calls instead.
Bug Fixes:
Enhancements: