Update escaping rules in io's rmRF (#828)

* Better Handling of escaping in rmrf
actions · Jun 7, 2021 · c9af6bb · c9af6bb
1 parent bf4ce74
commit c9af6bb
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 3 deletions.
diff --git a/packages/io/__tests__/io.test.ts b/packages/io/__tests__/io.test.ts
@@ -556,6 +556,45 @@ describe('rmRF', () => {
       await assertNotExists(symlinkFile)
       await assertNotExists(outerDirectory)
     })
+  } else {
+    it('correctly escapes % on windows', async () => {
+      const root: string = path.join(getTestTemp(), 'rmRF_escape_test_win')
+      const directory: string = path.join(root, '%test%')
+      await io.mkdirP(root)
+      await io.mkdirP(directory)
+      const oldEnv = process.env['test']
+      process.env['test'] = 'thisshouldnotresolve'
+
+      await io.rmRF(directory)
+      await assertNotExists(directory)
+      process.env['test'] = oldEnv
+    })
+
+    it('Should throw for invalid characters', async () => {
+      const root: string = path.join(getTestTemp(), 'rmRF_invalidChar_Windows')
+      const errorString =
+        'File path must not contain `*`, `"`, `<`, `>` or `|` on Windows'
+      await expect(io.rmRF(path.join(root, '"'))).rejects.toHaveProperty(
+        'message',
+        errorString
+      )
+      await expect(io.rmRF(path.join(root, '<'))).rejects.toHaveProperty(
+        'message',
+        errorString
+      )
+      await expect(io.rmRF(path.join(root, '>'))).rejects.toHaveProperty(
+        'message',
+        errorString
+      )
+      await expect(io.rmRF(path.join(root, '|'))).rejects.toHaveProperty(
+        'message',
+        errorString
+      )
+      await expect(io.rmRF(path.join(root, '*'))).rejects.toHaveProperty(
+        'message',
+        errorString
+      )
+    })
   }
 
   it('removes symlink folder with missing source using rmRF', async () => {

diff --git a/packages/io/src/io-util.ts b/packages/io/src/io-util.ts
@@ -166,3 +166,8 @@ function isUnixExecutable(stats: fs.Stats): boolean {
     ((stats.mode & 64) > 0 && stats.uid === process.getuid())
   )
 }
+
+// Get the path of cmd.exe in windows
+export function getCmdPath(): string {
+  return process.env['COMSPEC'] ?? `cmd.exe`
+}
diff --git a/packages/io/src/io.ts b/packages/io/src/io.ts
@@ -5,6 +5,7 @@ import {promisify} from 'util'
 import * as ioUtil from './io-util'
 
 const exec = promisify(childProcess.exec)
+const execFile = promisify(childProcess.execFile)
 
 /**
  * Interface for cp/mv options
@@ -117,11 +118,24 @@ export async function rmRF(inputPath: string): Promise<void> {
   if (ioUtil.IS_WINDOWS) {
     // Node doesn't provide a delete operation, only an unlink function. This means that if the file is being used by another
     // program (e.g. antivirus), it won't be deleted. To address this, we shell out the work to rd/del.
+
+    // Check for invalid characters
+    // https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
+    if (/[*"<>|]/.test(inputPath)) {
+      throw new Error(
+        'File path must not contain `*`, `"`, `<`, `>` or `|` on Windows'
+      )
+    }
     try {
+      const cmdPath = ioUtil.getCmdPath()
       if (await ioUtil.isDirectory(inputPath, true)) {
-        await exec(`rd /s /q "${inputPath}"`)
+        await exec(`${cmdPath} /s /c "rd /s /q "%inputPath%""`, {
+          env: {inputPath}
+        })
       } else {
-        await exec(`del /f /a "${inputPath}"`)
+        await exec(`${cmdPath} /s /c "del /f /a "%inputPath%""`, {
+          env: {inputPath}
+        })
       }
     } catch (err) {
       // if you try to delete a file that doesn't exist, desired result is achieved
@@ -149,7 +163,7 @@ export async function rmRF(inputPath: string): Promise<void> {
     }
 
     if (isDir) {
-      await exec(`rm -rf "${inputPath}"`)
+      await execFile(`rm`, [`-rf`, `${inputPath}`])
     } else {
       await ioUtil.unlink(inputPath)
     }