Conversation
|
For the curious, GPT-5 prompt used to generate this version: I think it one-shotted it (cursory review still). With a little follow-up to get rid of |
184fe42 to
2ad52ee
Compare
71dc66b to
bf3fddc
Compare
| const prefix = sanitizeAffix(this.fileOptions.prefix); | ||
| const postfix = sanitizeAffix(this.fileOptions.postfix); | ||
| const filename = `${prefix}${id}${postfix}`; | ||
| const candidate = path.resolve(baseDir, filename); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
| const prefix = sanitizeAffix(this.fileOptions.prefix); | ||
| const postfix = sanitizeAffix(this.fileOptions.postfix); | ||
| const filename = `${prefix}${id}${postfix}`; | ||
| const candidate = path.resolve(baseDir, filename); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Copilot Autofix
AI 9 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| const editorProcess = spawnSync( | ||
| this.editor.bin, | ||
| this.editor.args.concat([this.tempFile]), | ||
| { stdio: 'inherit' }, | ||
| ); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, we should ensure that the editor process is not spawned via a shell, and that the binary and arguments are not interpreted by the shell. This is achieved by explicitly setting the shell option to false in both spawnSync and spawn calls. Additionally, we should validate the editor binary to ensure it is a simple executable name or absolute path, and not a shell command or script with special characters. The changes should be made in the launchEditor and launchEditorAsync methods in packages/external-editor/src/index.ts. No changes are needed in the example file.
Required changes:
- In
launchEditorandlaunchEditorAsync, addshell: falseto the options object. - Optionally, add a simple validation for
this.editor.binto ensure it does not contain spaces or shell metacharacters (e.g.,;,|,&,>,<,`,$, etc.). - No new imports are required.
| @@ -207,2 +207,6 @@ | ||
| try { | ||
| // Validate editor binary for safety | ||
| if (/[;&|><`$]/.test(this.editor.bin)) { | ||
| throw new LaunchEditorError(new Error('Unsafe editor binary detected')); | ||
| } | ||
| const editorProcess = spawnSync( | ||
| @@ -210,3 +214,3 @@ | ||
| this.editor.args.concat([this.tempFile]), | ||
| { stdio: 'inherit' }, | ||
| { stdio: 'inherit', shell: false }, | ||
| ); | ||
| @@ -220,2 +224,6 @@ | ||
| try { | ||
| // Validate editor binary for safety | ||
| if (/[;&|><`$]/.test(this.editor.bin)) { | ||
| throw new LaunchEditorError(new Error('Unsafe editor binary detected')); | ||
| } | ||
| const editorProcess = spawn( | ||
| @@ -223,3 +231,3 @@ | ||
| this.editor.args.concat([this.tempFile]), | ||
| { stdio: 'inherit' }, | ||
| { stdio: 'inherit', shell: false }, | ||
| ); |
| const editorProcess = spawn( | ||
| this.editor.bin, | ||
| this.editor.args.concat([this.tempFile]), | ||
| { stdio: 'inherit' }, | ||
| ); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, we should ensure that the editor binary and its arguments are always passed as separate parameters to spawn and spawnSync, and never as a single string that could be interpreted by a shell. Additionally, we should sanitize the values obtained from environment variables (EDITOR, VISUAL) to prevent them from being shell commands or containing unsafe characters. The best way to do this is to parse the editor command into a binary and arguments, and reject or sanitize any values that appear to be shell commands (e.g., containing ;, &&, |, or starting with sh -c). We should also ensure that the spawn and spawnSync calls do not use shell: true.
The main changes should be made in the determineEditor method in packages/external-editor/src/index.ts, where the editor command is parsed from environment variables. We should add validation to reject unsafe values and ensure that only a binary and arguments are used. If an unsafe value is detected, we should fall back to a safe default editor.
| @@ -138,3 +138,3 @@ | ||
| private determineEditor() { | ||
| const editor = process.env['VISUAL'] | ||
| let editor = process.env['VISUAL'] | ||
| ? process.env['VISUAL'] | ||
| @@ -146,2 +146,9 @@ | ||
|
|
||
| // Sanitize editor command: reject if it contains shell metacharacters | ||
| const unsafePattern = /[;&|`$><]/; | ||
| if (unsafePattern.test(editor)) { | ||
| // Fallback to safe default | ||
| editor = process.platform.startsWith('win') ? 'notepad' : 'vim'; | ||
| } | ||
|
|
||
| const editorOpts = splitStringBySpace(editor).map((piece: string) => |
bf3fddc to
50682f0
Compare
50682f0 to
15e1011
Compare
Rel #1802
TODOs
Follow-ups (other PR)
instanceofis error prone)