Conversation
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
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 integrates a custom Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
![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
このプルリクエストでは、外部の concurrently パッケージに依存しない、新しい wb concurrently コマンドが追加されています。これにより、複数のコマンドを並列実行できるようになります。また、既存のスクリプトで YARN concurrently を使用している箇所が、新しい YARN wb concurrently に置き換えられています。全体的に良い変更だと思いますが、concurrently コマンドの --raw オプションについて、1点フィードバックがあります。
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストでは、外部ツールへの依存を減らすための新しい wb concurrently コマンドが導入されており、素晴らしい追加機能です。しかし、セキュリティの観点から、プロジェクトのスクリプト内での concurrently コマンドの呼び出し方法にコマンドインジェクションの脆弱性があります。特に、BaseScripts および HttpServerScripts の複数のメソッドが、変数を文字列に補間し、二重引用符で囲む際に適切なエスケープ処理を行っていないため、攻撃者がコマンドライン引数に影響を与えることができる場合（例：CI/CD環境）、任意のシェルコマンドを実行される可能性があります。この脆弱性に対処するためには、補間されるすべての変数に対して堅牢なシェルエスケープメカニズムを実装するか、複雑なシェルコマンドを文字列として構築することを完全に避けることを強く推奨します。実装は子プロセスやシグナルの処理など堅牢で、既存のスクリプトも新しいコマンドを使用するように更新されており、主要なシナリオをカバーするテストも追加されていますが、堅牢性とテストの正確性を向上させるための提案もいくつかあります。
| return `YARN wb concurrently --kill-others-on-fail | ||
| "${this.startDevProtected(project, argv)}" | ||
| "${this.waitAndOpenApp(project)}"`; |
There was a problem hiding this comment.
The code constructs shell commands by interpolating variables into strings that are subsequently executed. Specifically, it wraps commands in double quotes but does not escape any double quotes or other shell-sensitive characters that might be present in the variables (e.g., this.startDevProtected(project, argv)). If an attacker can influence these variables—for instance, through command-line arguments like normalizedArgsText or targets—they can break out of the quoted string and execute arbitrary shell commands. This is particularly risky in CI/CD environments where arguments might be derived from untrusted sources such as branch names or PR metadata.
| return `YARN wb concurrently --kill-others-on-fail | ||
| "${this.startProductionProtected(project, argv)}" | ||
| "${this.waitAndOpenApp(project)}"`; |
There was a problem hiding this comment.
Similar to the issue in startDev, the startProduction method constructs a shell command using unescaped variables wrapped in double quotes, leading to potential command injection.
| && YARN wb concurrently --kill-others-on-fail | ||
| "${dockerScripts.stopAndStart(project, argv.normalizedDockerOptionsText ?? '', argv.normalizedArgsText ?? '')}" | ||
| "${this.waitAndOpenApp(project)}"`; |
There was a problem hiding this comment.
The startDocker method constructs a shell command using unescaped variables wrapped in double quotes, leading to potential command injection.
| await checkAndKillPortProcess(project.env.PORT, project); | ||
| // Use empty NODE_ENV to avoid "production" mode in some frameworks like Blitz.js. | ||
| return `NODE_ENV="" YARN concurrently --kill-others --raw --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; | ||
| return `NODE_ENV="" YARN wb concurrently --kill-others --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; |
There was a problem hiding this comment.
The testStart method constructs a shell command using unescaped variables wrapped in double quotes, leading to potential command injection.
| return `YARN wb concurrently --kill-others --success first | ||
| "${startCommand} && exit 1" | ||
| "wait-on -t 600000 -i 2000 http-get://127.0.0.1:${port} | ||
| && ${playwrightCommand}${suffix}"`; |
There was a problem hiding this comment.
The testE2EProtected method constructs a shell command using unescaped variables (startCommand, playwrightCommand, suffix) wrapped in double quotes, leading to potential command injection.
| return `YARN wb concurrently --kill-others --success first | ||
| "${startCommand} && exit 1" | ||
| "wait-on -t 600000 -i 2000 http-get://127.0.0.1:${port} | ||
| && vitest run ${testTarget} --color --passWithNoTests --allowOnly${bailOption}${suffix}"`; |
There was a problem hiding this comment.
The testE2EProtected method constructs a shell command using unescaped variables (startCommand, testTarget, bailOption, suffix) wrapped in double quotes, leading to potential command injection.
| const exitCode = await runConcurrently({ | ||
| commands, | ||
| cwd: project.dirPath, | ||
| env: project.env, | ||
| killOthers: argv.killOthers ?? false, | ||
| killOthersOnFail: argv.killOthersOnFail ?? false, | ||
| success: argv.success, | ||
| }); | ||
| process.exit(exitCode); |
There was a problem hiding this comment.
現在の実装では、runConcurrentlyが例外をスローした場合（例えば、プロセスの起動に失敗した場合など）、未処理のPromiseリジェクションが発生し、プロセスがクラッシュする可能性があります。堅牢性を高めるために、try...catchブロックでrunConcurrentlyの呼び出しを囲み、エラーを適切にハンドリングして終了コード1でプロセスを終了させることをお勧めします。
try {
const exitCode = await runConcurrently({
commands,
cwd: project.dirPath,
env: project.env,
killOthers: argv.killOthers ?? false,
killOthersOnFail: argv.killOthersOnFail ?? false,
success: argv.success,
});
process.exit(exitCode);
} catch (error) {
console.error(chalk.red(error instanceof Error ? error.message : String(error)));
process.exit(1);
}| success: 'all', | ||
| }); | ||
|
|
||
| expect(exitCode).not.toBe(0); |
There was a problem hiding this comment.
テストのアサーションがexpect(exitCode).not.toBe(0)となっていますが、より厳密にすることが可能です。このテストケースでは、最初のコマンドが終了コード1で失敗するため、runConcurrentlyは1を返すことが期待されます。アサーションをexpect(exitCode).toBe(1)のように、具体的な期待される終了コードに変更することで、テストの正確性が向上します。
| expect(exitCode).not.toBe(0); | |
| expect(exitCode).toBe(1); |
exKAZUu
force-pushed
the
feat/wb-concurrently-command
branch
from
98a443a to
85d25bb
Compare
1 participant
Summary
wb concurrentlycommand to run multiple commands in parallel without externalconcurrentlyYARN concurrentlyusage in wb-generated scripts withYARN wb concurrentlyVerification