-
Notifications
You must be signed in to change notification settings - Fork 3
Feat ew2 #11
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
base: master
Are you sure you want to change the base?
Conversation
| const homeDeno = path.resolve(os.homedir(), '.deno/bin/deno'); | ||
| function checkDeno(command: string) { | ||
| return new Promise((resolve, reject) => { | ||
| exec(`${command} --version`, (err) => { |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
absolute path
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, we should avoid constructing the shell command dynamically and instead use execFile or execFileSync to pass the command and its arguments separately. This approach ensures that the shell does not interpret the command string, thus preventing command injection vulnerabilities.
Specifically, we need to:
- Replace the use of
execwithexecFilein thecheckDenofunction. - Modify the
checkDenofunction to pass the command and its arguments separately.
-
Copy modified line R1 -
Copy modified line R27
| @@ -1,2 +1,2 @@ | ||
| import { exec, execSync } from 'child_process'; | ||
| import { execFile, execSync } from 'child_process'; | ||
| import os from 'os'; | ||
| @@ -26,3 +26,3 @@ | ||
| return new Promise((resolve, reject) => { | ||
| exec(`${command} --version`, (err) => { | ||
| execFile(command, ['--version'], (err) => { | ||
| if (err) { |
| const p = path.join(__dirname, './install'); | ||
| const installCommand = `sh ${p}/installEw2.sh ${manifest.version}`; | ||
| try { | ||
| execSync(installCommand, { stdio: 'inherit', env: { ...process.env } }); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, we should avoid using execSync with a concatenated string that includes untrusted data. Instead, we can use execFileSync which allows us to pass command arguments as an array, thus avoiding the risk of command injection. This approach ensures that the command and its arguments are properly separated and handled securely by the operating system.
We need to modify the installEw2 function to use execFileSync instead of execSync. We will also need to import execFileSync from the child_process module.
-
Copy modified line R6 -
Copy modified line R118 -
Copy modified line R120
| @@ -5,3 +5,3 @@ | ||
| import https from 'https'; | ||
| import { execSync } from 'child_process'; | ||
| import { execFileSync } from 'child_process'; | ||
| import logger from '../libs/logger.js'; | ||
| @@ -117,5 +117,5 @@ | ||
| const p = path.join(__dirname, './install'); | ||
| const installCommand = `sh ${p}/installEw2.sh ${manifest.version}`; | ||
| const installScript = path.join(p, 'installEw2.sh'); | ||
| try { | ||
| execSync(installCommand, { stdio: 'inherit', env: { ...process.env } }); | ||
| execFileSync('sh', [installScript, manifest.version], { stdio: 'inherit', env: { ...process.env } }); | ||
| const md5 = await calculateFileMD5(EW2BinPath); |
| const workerRes = await fetch( | ||
| `http://${localUpstream ? localUpstream : host}${url}`, | ||
| { | ||
| method, | ||
| headers: { | ||
| ...headers, | ||
| 'x-er-context': | ||
| 'eyJzaXRlX2lkIjogIjYyMjcxODQ0NjgwNjA4IiwgInNpdGVfbmFtZSI6ICJjb21wdXRlbHguYWxpY2RuLXRlc3QuY29tIiwgInNpdGVfcmVjb3JkIjogIm1vY2hlbi1uY2RuLmNvbXB1dGVseC5hbGljZG4tdGVzdC5jb20iLCAiYWxpdWlkIjogIjEzMjI0OTI2ODY2NjU2MDgiLCAic2NoZW1lIjoiaHR0cCIsICAiaW1hZ2VfZW5hYmxlIjogdHJ1ZX0=', | ||
| 'x-er-id': 'a.bA' | ||
| }, | ||
| body: req.method === 'GET' ? undefined : req, | ||
| agent: new HttpProxyAgent(`http://127.0.0.1:${ew2Port}`) | ||
| } | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the SSRF vulnerability:
- Restrict the use of
host: Replace the user-providedreq.headers.hostwith a fixed, pre-approved value or restrict it to an allowlist of trusted hosts. - Sanitize
url: Ensurereq.urldoes not contain malicious input (e.g., path traversal or query manipulation). - Use a known-safe base for URLs: Construct the URL using a hardcoded base or a validated and sanitized alternative.
The best fix is to introduce an allowlist of trusted hosts and safely construct the URL using validated components. This ensures the application only communicates with pre-approved endpoints, mitigating SSRF risks.
-
Copy modified lines R164-R166 -
Copy modified line R184
| @@ -161,8 +161,9 @@ | ||
| } | ||
| } | ||
| try { | ||
| const host = req.headers.host; | ||
| const url = req.url; | ||
| const trustedHosts = ['allowed-host1.com', 'allowed-host2.com']; | ||
| const host = trustedHosts.includes(req.headers.host || '') ? req.headers.host : 'default-host.com'; | ||
| const url = new URL(req.url || '/', `http://${host}`); | ||
| const method = req.method; | ||
| const headers = Object.entries(req.headers).reduce( | ||
| (acc: Record<string, string | undefined>, [key, value]) => { | ||
| @@ -180,7 +181,7 @@ | ||
| // @ts-ignore | ||
| const localUpstream = global.localUpstream; | ||
| const workerRes = await fetch( | ||
| `http://${localUpstream ? localUpstream : host}${url}`, | ||
| `http://${localUpstream ? localUpstream : url.hostname}${url.pathname}${url.search}`, | ||
| { | ||
| method, | ||
| headers: { |
No description provided.