Repository: compiler-explorer/compiler-explorer
Pull request #7202 (2024-12-07 22:40:14)

Reimplement shell splitting
Takes the bones of shell-split and makes it handle lots of situations without throwing exceptions, and using modern-ish typescript/javascript stuff.

Adds lots more tests too.

Removes unneeded package dependencies.

Changes from before: escaping a pipe character `moo \\# hello` would drop the `# hello` part (?) that is, I updated the test that did:
```ts
expect(splitArguments('hello \\#veryfancy etc')).toEqual(['hello', '\\']);
```
to be the more sane expectation of `['hello', '\\#veryfancy']`.

Everything else I could think of (unterminated strings, escapes etc) I kept the same. And added tests for.

Hopefully addresses #7201 without breaking any reasonable use of the code (though @partouf raises fair concerns about this.)


Patch:
diff --git a/package-lock.json b/package-lock.json
index 437bf17d3..d5ea7c9a8 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -59,7 +59,6 @@
         "scroll-into-view-if-needed": "^3.1.0",
         "semver": "^7.6.3",
         "serve-favicon": "^2.5.0",
-        "shell-quote": "^1.8.1",
         "systemd-socket": "0.0.0",
         "tar-stream": "^3.1.7",
         "temp": "^0.9.4",
@@ -95,7 +94,6 @@
         "@types/request": "^2.48.12",
         "@types/response-time": "^2.3.8",
         "@types/serve-favicon": "^2.5.7",
-        "@types/shell-quote": "^1.7.5",
         "@types/temp": "^0.9.4",
         "@types/underscore": "^1.13.0",
         "@types/url-join": "^4.0.3",
@@ -4495,12 +4493,6 @@
         "@types/send": "*"
       }
     },
-    "node_modules/@types/shell-quote": {
-      "version": "1.7.5",
-      "resolved": "https://registry.npmjs.org/@types/shell-quote/-/shell-quote-1.7.5.tgz",
-      "integrity": "sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==",
-      "dev": true
-    },
     "node_modules/@types/sinon": {
       "version": "17.0.3",
       "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-17.0.3.tgz",
@@ -13818,14 +13810,6 @@
         "node": ">=8"
       }
     },
-    "node_modules/shell-quote": {
-      "version": "1.8.1",
-      "resolved": "https://registry.npmjs.org/shell-quote/-/shell-quote-1.8.1.tgz",
-      "integrity": "sha512-6j1W9l1iAs/4xYBI1SYOVZyFcCis9b4KCLQ8fgAGG07QvzaRLVVRQvAy85yNmmZSjYjg4MWh4gNvlPujU/5LpA==",
-      "funding": {
-        "url": "https://github.com/sponsors/ljharb"
-      }
-    },
     "node_modules/side-channel": {
       "version": "1.0.6",
       "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.0.6.tgz",
diff --git a/package.json b/package.json
index d34a6795d..1556dd21d 100644
--- a/package.json
+++ b/package.json
@@ -68,7 +68,6 @@
     "scroll-into-view-if-needed": "^3.1.0",
     "semver": "^7.6.3",
     "serve-favicon": "^2.5.0",
-    "shell-quote": "^1.8.1",
     "systemd-socket": "0.0.0",
     "tar-stream": "^3.1.7",
     "temp": "^0.9.4",
@@ -104,7 +103,6 @@
     "@types/request": "^2.48.12",
     "@types/response-time": "^2.3.8",
     "@types/serve-favicon": "^2.5.7",
-    "@types/shell-quote": "^1.7.5",
     "@types/temp": "^0.9.4",
     "@types/underscore": "^1.13.0",
     "@types/url-join": "^4.0.3",
diff --git a/shared/common-utils.ts b/shared/common-utils.ts
index d63318e1c..c7c50c48e 100644
--- a/shared/common-utils.ts
+++ b/shared/common-utils.ts
@@ -22,8 +22,6 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
-import {parse as quoteParse} from 'shell-quote';
-
 export function isString(x: any): x is string {
     return typeof x === 'string' || x instanceof String;
 }
@@ -50,6 +48,7 @@ export function intersection<V>(a: V[], b: V[]): V[] {
 export function remove<U, V extends U>(arr: U[], v: V) {
     return arr.filter(item => item !== v) as Exclude<U, V extends null | undefined ? V : never>[];
 }
+
 // https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAMwK4DsDGMCWEVysAWwgDdgAeAVQBo4A1OUGYFAEwGc4KA+ACgEMoUAFycA2gF0axEbQCUcAN4AoOKrzAYSKLgFQAdAkwAbJlB6YmBOAF4ucC4TgBCa9bjF5fDgFEQaI0gs5NR0DCBMrBwoSEZGcAA+cKhBhijALHAA-KEiaaRQXBIA3EoAvkpKQf4CwHBoOGzwuiI8jVCYKADmCXDRsbLFFfUojXAgNupEpPyCNH1GsiUA9EtqcAB6mUMN8ACeE-hTwDNQNABECBAQZ4tKK2ubFUA
 
 // For Array.prototype.sort
@@ -75,6 +74,7 @@ const EscapeMap = {
     '`': '&#x60;',
 };
 const EscapeRE = new RegExp(`(?:${Object.keys(EscapeMap).join('|')})`, 'g');
+
 export function escapeHTML(text: string) {
     return text.replace(EscapeRE, str => EscapeMap[str as keyof typeof EscapeMap]);
 }
@@ -102,10 +102,110 @@ export function addDigitSeparator(n: string, digitSeparator: string, chunkSize:
     return splitIntoChunks(n, chunkSize).join(digitSeparator);
 }
 
-export function splitArguments(options = ''): string[] {
-    // escape hashes first, otherwise they're interpreted as comments
-    const escapedOptions = options.replace(/#/g, '\\#');
-    return quoteParse(escapedOptions)
-        .map((x: any) => (typeof x === 'string' ? x : (x.pattern as string)))
-        .filter(Boolean);
+class ArgSplitter {
+    private rest: string;
+    private position: number;
+    private inDoubleQuotes: boolean;
+    private readonly results: string[];
+
+    constructor(str: string) {
+        this.rest = str.trim();
+        this.position = 0;
+        this.inDoubleQuotes = false;
+        this.results = [];
+    }
+
+    private addResult(result: string) {
+        if (this.results[this.position] === undefined) {
+            this.results[this.position] = result;
+        } else {
+            this.results[this.position] += result;
+        }
+    }
+
+    private handleEscapeCharacter() {
+        if (this.rest.length < 2) {
+            this.addResult('\\');
+        } else {
+            if (this.rest.charAt(1) !== '\n') {
+                this.addResult(this.rest.charAt(1));
+            }
+        }
+        this.rest = this.rest.slice(2);
+    }
+
+    private handleSingleQuotes() {
+        let quotePos = this.rest.indexOf("'", 1);
+        if (quotePos === -1) quotePos = this.rest.length;
+        this.addResult(this.rest.slice(1, quotePos));
+        this.rest = this.rest.slice(quotePos + 1);
+    }
+
+    private handleDoubleQuotes() {
+        const match = /["\\]/.exec(this.rest);
+        const matchEnd = match ? match.index : this.rest.length;
+
+        this.addResult(this.rest.slice(0, matchEnd));
+
+        if (match && match[0] === '\\') {
+            this.rest = this.rest.slice(matchEnd);
+            this.handleEscapeCharacter();
+        } else {
+            this.rest = this.rest.slice(matchEnd + 1);
+            this.inDoubleQuotes = false;
+        }
+    }
+
+    private handleWhitespace() {
+        const wsMatch = /^\s+/.exec(this.rest);
+        if (wsMatch) {
+            this.rest = this.rest.slice(wsMatch[0].length);
+            ++this.position;
+        }
+    }
+
+    private handleNonDoubleQuote(nextMatch: RegExpExecArray) {
+        this.addResult(this.rest.slice(0, nextMatch.index));
+        this.rest = this.rest.slice(nextMatch.index);
+
+        switch (nextMatch[0]) {
+            case '\\':
+                this.handleEscapeCharacter();
+                break;
+            case "'":
+                this.handleSingleQuotes();
+                break;
+            case '"':
+                this.rest = this.rest.slice(1);
+                this.inDoubleQuotes = true;
+                break;
+            default:
+                break;
+        }
+    }
+
+    public exec(): string[] {
+        while (this.rest) {
+            if (this.inDoubleQuotes) {
+                this.handleDoubleQuotes();
+                continue;
+            }
+
+            this.handleWhitespace();
+
+            const nextMatch = /[\s'"\\]/.exec(this.rest);
+            if (!nextMatch) {
+                this.addResult(this.rest);
+                this.rest = '';
+                continue;
+            }
+
+            this.handleNonDoubleQuote(nextMatch);
+        }
+        return this.results;
+    }
+}
+
+export function splitArguments(str = ''): string[] {
+    return new ArgSplitter(str).exec();
 }


Test patch:
diff --git a/test/common-utils-tests.ts b/test/common-utils-tests.ts
index 6fcd97970..1c21da901 100644
--- a/test/common-utils-tests.ts
+++ b/test/common-utils-tests.ts
@@ -59,6 +59,12 @@ describe('digit separator', () => {
 });
 
 describe('argument splitting', () => {
+    it('should handle empty strings', () => {
+        expect(splitArguments('')).toEqual([]);
+    });
+    it('should handle purely whitespace strings', () => {
+        expect(splitArguments('    ')).toEqual([]);
+    });
     it('should handle normal things', () => {
         expect(splitArguments('-hello --world etc --std=c++20')).toEqual(['-hello', '--world', 'etc', '--std=c++20']);
     });
@@ -78,13 +84,86 @@ describe('argument splitting', () => {
         expect(splitArguments("--hello '-world etc'")).toEqual(['--hello', '-world etc']);
     });
 
-    it('should handle cheekyness part 1', () => {
-        /* eslint-disable no-useless-escape */
+    it('should handle mismatched doublequoted args', () => {
+        expect(splitArguments('--hello "-world etc')).toEqual(['--hello', '-world etc']);
+    });
+
+    it('should handle mismatched singlequoted args', () => {
+        expect(splitArguments("--hello '-world etc")).toEqual(['--hello', '-world etc']);
+    });
+
+    it('should handle hashes at the beginning of words', () => {
         expect(splitArguments('hello #veryfancy etc')).toEqual(['hello', '#veryfancy', 'etc']);
-        /* eslint-enable no-useless-escape */
     });
 
-    it('should handle cheekyness part 2', () => {
-        expect(splitArguments('hello \\#veryfancy etc')).toEqual(['hello', '\\']);
+    it('should handle empty arguments', () => {
+        expect(splitArguments('nothing "" is lost \'\'')).toEqual(['nothing', '', 'is', 'lost', '']);
+    });
+
+    it('should handle weird characters', () => {
+        expect(splitArguments('hello | moose | \\n < yay & \n\t\nmoo')).toEqual([
+            'hello',
+            '|',
+            'moose',
+            '|',
+            'n',
+            '<',
+            'yay',
+            '&',
+            'moo',
+        ]);
+    });
+
+    it('should handle escaped backslashes', () => {
+        expect(splitArguments('hello \\\\ moose')).toEqual(['hello', '\\', 'moose']);
+    });
+
+    it('should handle mismatched backslashes', () => {
+        expect(splitArguments('hello \\')).toEqual(['hello', '\\']);
+        expect(splitArguments('hello world\\')).toEqual(['hello', 'world\\']);
+    });
+    it('should handle mismatched backslashes in quotes', () => {
+        expect(splitArguments('hello "\\')).toEqual(['hello', '\\']);
+    });
+
+    it('should handle escaped quotes', () => {
+        expect(splitArguments('hi \\"mum\\"')).toEqual(['hi', '"mum"']);
+        expect(splitArguments('hi "\\"mum\\""')).toEqual(['hi', '"mum"']);
+        expect(splitArguments("hi \\'mum\\'")).toEqual(['hi', "'mum'"]);
+    });
+
+    it('should do something sane in the face of escaped quotes in quotes', () => {
+        // Not really valid input, bash sees this as a bunch of abutting single-quotes, unterminated.
+        // But this makes as much sense as anything and so I've added it to lock in the behaviour.
+        expect(splitArguments("hi '\\'mum\\''")).toEqual(['hi', "\\mum'"]);
+    });
+
+    it('should handle abutting quotes', () => {
+        expect(splitArguments('well "hello""there"')).toEqual(['well', 'hellothere']);
+        expect(splitArguments('well "hello"\'there\'')).toEqual(['well', 'hellothere']);
+    });
+
+    it('should ignore escaped hashes etc', () => {
+        expect(splitArguments('hello \\#veryfancy etc')).toEqual(['hello', '#veryfancy', 'etc']);
+    });
+
+    it('should handle multiple spaces', () => {
+        expect(splitArguments('hello    world')).toEqual(['hello', 'world']);
+    });
+
+    it('should handle escaped spaces', () => {
+        expect(splitArguments('hello\\ there')).toEqual(['hello there']);
+    });
+
+    it('should handle mixed quotes and spaces', () => {
+        expect(splitArguments('"hello \'world\' there"')).toEqual(["hello 'world' there"]);
+    });
+
+    it('should handle quotes containing spaces', () => {
+        expect(splitArguments('hello "   " world')).toEqual(['hello', '   ', 'world']);
+    });
+
+    it('should handle multiple escapes in quotes', () => {
+        expect(splitArguments('"hello \\"world\\" \\\\"')).toEqual(['hello "world" \\']);
     });
 });