From ada1eee64852bca2ac252e50a263fe8374355f9d Mon Sep 17 00:00:00 2001 From: Michal Dorner Date: Sat, 12 Dec 2020 21:02:07 +0100 Subject: [PATCH 1/2] Simplify shell escaping - escape chars instead of quoting whole string --- __tests__/shell-escape.test.ts | 21 +++++++++++++-------- dist/index.js | 7 +++---- src/shell-escape.ts | 8 +++----- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/__tests__/shell-escape.test.ts b/__tests__/shell-escape.test.ts index 232cdd7..d03669a 100644 --- a/__tests__/shell-escape.test.ts +++ b/__tests__/shell-escape.test.ts @@ -1,16 +1,21 @@ import shellEscape from '../src/shell-escape' -test('simple path escaped', () => { - expect(shellEscape('file')).toBe("'file'") +test('simple filename should not be modified', () => { + expect(shellEscape('file.txt')).toBe('file.txt') }) -test('path with space is wrapped with single quotes', () => { - expect(shellEscape('file with space')).toBe("'file with space'") +test('directory separator should be preserved and not escaped', () => { + expect(shellEscape('path/to/file.txt')).toBe('path/to/file.txt') }) -test('path with quote is divided into quoted segments and escaped quote', () => { - expect(shellEscape("file'with quote")).toBe("'file'\\''with quote'") +test('spaces should be escaped with backslash', () => { + expect(shellEscape('file with space')).toBe('file\\ with\\ space') }) -test('path with leading quote does not have double quotes at beginning', () => { - expect(shellEscape("'file-leading-quote")).toBe("\\''file-leading-quote'") + +test('quotes should be escaped with backslash', () => { + expect(shellEscape('file\'with quote"')).toBe('file\\\'with\\ quote\\"') +}) + +test('$variables sould be escaped', () => { + expect(shellEscape('$var')).toBe('\\$var') }) diff --git a/dist/index.js b/dist/index.js index ea54600..c0b7926 100644 --- a/dist/index.js +++ b/dist/index.js @@ -15222,12 +15222,11 @@ module.exports = require("fs"); "use strict"; -// Credits to https://github.com/xxorax/node-shell-escape Object.defineProperty(exports, "__esModule", { value: true }); +// Uses easy safe set of characters which can be left unescaped to keep it readable. +// Every other character will be backslash-escaped function shellEscape(value) { - return `'${value.replace(/'/g, "'\\''")}'` - .replace(/^(?:'')+/g, '') // unduplicate single-quote at the beginning - .replace(/\\'''/g, "\\'"); // remove non-escaped single-quote if there are enclosed between 2 escaped + return value.replace(/([^a-zA-Z0-9,._+:@%/-])/gm, '\\$1'); } exports.default = shellEscape; diff --git a/src/shell-escape.ts b/src/shell-escape.ts index 6dfe46d..643b7cc 100644 --- a/src/shell-escape.ts +++ b/src/shell-escape.ts @@ -1,7 +1,5 @@ -// Credits to https://github.com/xxorax/node-shell-escape - +// Uses easy safe set of characters which can be left unescaped to keep it readable. +// Every other character will be backslash-escaped export default function shellEscape(value: string): string { - return `'${value.replace(/'/g, "'\\''")}'` - .replace(/^(?:'')+/g, '') // unduplicate single-quote at the beginning - .replace(/\\'''/g, "\\'") // remove non-escaped single-quote if there are enclosed between 2 escaped + return value.replace(/([^a-zA-Z0-9,._+:@%/-])/gm, '\\$1') } From e4d886f50334b309621f18374f57aef30a4b38db Mon Sep 17 00:00:00 2001 From: Michal Dorner Date: Sun, 13 Dec 2020 21:07:47 +0100 Subject: [PATCH 2/2] Provide `shell` and `escape` options when formatting matching files --- .../workflows/pull-request-verification.yml | 9 +-- README.md | 14 ++-- __tests__/shell-escape.test.ts | 64 +++++++++++++++---- action.yml | 7 +- dist/index.js | 40 +++++++++--- src/main.ts | 8 ++- src/shell-escape.ts | 29 ++++++++- 7 files changed, 124 insertions(+), 47 deletions(-) diff --git a/.github/workflows/pull-request-verification.yml b/.github/workflows/pull-request-verification.yml index 66e001c..27627a9 100644 --- a/.github/workflows/pull-request-verification.yml +++ b/.github/workflows/pull-request-verification.yml @@ -119,15 +119,12 @@ jobs: - name: Print 'deleted_files' run: echo ${{steps.filter.outputs.deleted_files}} - name: filter-test - # only single quotes are supported in GH action literal - # single quote needs to be escaped with single quote - # '''add.txt''' resolves to string 'add.txt' if: | steps.filter.outputs.added != 'true' || steps.filter.outputs.deleted != 'true' || steps.filter.outputs.modified != 'true' || steps.filter.outputs.any != 'true' - || steps.filter.outputs.added_files != '''add.txt''' - || steps.filter.outputs.modified_files != '''LICENSE''' - || steps.filter.outputs.deleted_files != '''README.md''' + || steps.filter.outputs.added_files != 'add.txt' + || steps.filter.outputs.modified_files != 'LICENSE' + || steps.filter.outputs.deleted_files != 'README.md' run: exit 1 diff --git a/README.md b/README.md index 0bb10be..145ecc0 100644 --- a/README.md +++ b/README.md @@ -66,18 +66,12 @@ For more scenarios see [examples](#examples) section. # What's New +- Improved listing of matching files with `list-files: shell` and `list-files: escape` options - Support local changes - Fixed retrieval of all changes via Github API when there are 100+ changes - Paths expressions are now evaluated using [picomatch](https://github.com/micromatch/picomatch) library - Support workflows triggered by any event - Fixed compatibility with older (<2.23) versions of git -- Support for tag pushes and tags as a base reference -- Fixes for various edge cases when event payload is incomplete - - Supports local execution with [act](https://github.com/nektos/act) -- Fixed behavior of feature branch workflow: - - Detects only changes introduced by feature branch. Later modifications on base branch are ignored -- Filter by type of file change: - - Optionally consider if file was added, modified or deleted For more information see [CHANGELOG](https://github.com/actions/checkout/blob/main/CHANGELOG.md) @@ -128,8 +122,10 @@ For more information see [CHANGELOG](https://github.com/actions/checkout/blob/ma # Enables listing of files matching the filter: # 'none' - Disables listing of matching files (default). # 'json' - Matching files paths are formatted as JSON array. - # 'shell' - Matching files paths are escaped and space-delimited. - # Output is usable as command line argument list in linux shell. + # 'shell' - Space delimited list usable as command line argument list in linux shell. + # If needed it uses single or double quotes to wrap filename with unsafe characters. + # 'escape'- Space delimited list usable as command line argument list in linux shell. + # Backslash escapes every potentially unsafe character. # Default: none list-files: '' diff --git a/__tests__/shell-escape.test.ts b/__tests__/shell-escape.test.ts index d03669a..e706dfb 100644 --- a/__tests__/shell-escape.test.ts +++ b/__tests__/shell-escape.test.ts @@ -1,21 +1,57 @@ -import shellEscape from '../src/shell-escape' +import {escape, shellEscape} from '../src/shell-escape' -test('simple filename should not be modified', () => { - expect(shellEscape('file.txt')).toBe('file.txt') +describe('escape() backslash escapes every character except subset of definitely safe characters', () => { + test('simple filename should not be modified', () => { + expect(escape('file.txt')).toBe('file.txt') + }) + + test('directory separator should be preserved and not escaped', () => { + expect(escape('path/to/file.txt')).toBe('path/to/file.txt') + }) + + test('spaces should be escaped with backslash', () => { + expect(escape('file with space')).toBe('file\\ with\\ space') + }) + + test('quotes should be escaped with backslash', () => { + expect(escape('file\'with quote"')).toBe('file\\\'with\\ quote\\"') + }) + + test('$variables should be escaped', () => { + expect(escape('$var')).toBe('\\$var') + }) }) -test('directory separator should be preserved and not escaped', () => { - expect(shellEscape('path/to/file.txt')).toBe('path/to/file.txt') -}) +describe('shellEscape() returns human readable filenames with as few escaping applied as possible', () => { + test('simple filename should not be modified', () => { + expect(shellEscape('file.txt')).toBe('file.txt') + }) -test('spaces should be escaped with backslash', () => { - expect(shellEscape('file with space')).toBe('file\\ with\\ space') -}) + test('directory separator should be preserved and not escaped', () => { + expect(shellEscape('path/to/file.txt')).toBe('path/to/file.txt') + }) -test('quotes should be escaped with backslash', () => { - expect(shellEscape('file\'with quote"')).toBe('file\\\'with\\ quote\\"') -}) + test('filename with spaces should be quoted', () => { + expect(shellEscape('file with space')).toBe("'file with space'") + }) -test('$variables sould be escaped', () => { - expect(shellEscape('$var')).toBe('\\$var') + test('filename with spaces should be quoted', () => { + expect(shellEscape('file with space')).toBe("'file with space'") + }) + + test('filename with $ should be quoted', () => { + expect(shellEscape('$var')).toBe("'$var'") + }) + + test('filename with " should be quoted', () => { + expect(shellEscape('file"name')).toBe("'file\"name'") + }) + + test('filename with single quote should be wrapped in double quotes', () => { + expect(shellEscape("file'with quote")).toBe('"file\'with quote"') + }) + + test('filename with single quote and special characters is split and quoted/escaped as needed', () => { + expect(shellEscape("file'with $quote")).toBe("file\\''with $quote'") + }) }) diff --git a/action.yml b/action.yml index 76938e8..d1321d6 100644 --- a/action.yml +++ b/action.yml @@ -22,8 +22,11 @@ inputs: description: | Enables listing of files matching the filter: 'none' - Disables listing of matching files (default). - 'json' - Matching files paths are serialized as JSON array. - 'shell' - Matching files paths are escaped and space-delimited. Output is usable as command line argument list in linux shell. + 'json' - Serialized as JSON array. + 'shell' - Space delimited list usable as command line argument list in linux shell. + If needed it uses single or double quotes to wrap filename with unsafe characters. + 'escape'- Space delimited list usable as command line argument list in linux shell. + Backslash escapes every potentially unsafe character. required: true default: none initial-fetch-depth: diff --git a/dist/index.js b/dist/index.js index c0b7926..953026b 100644 --- a/dist/index.js +++ b/dist/index.js @@ -4641,9 +4641,6 @@ var __importStar = (this && this.__importStar) || function (mod) { __setModuleDefault(result, mod); return result; }; -var __importDefault = (this && this.__importDefault) || function (mod) { - return (mod && mod.__esModule) ? mod : { "default": mod }; -}; Object.defineProperty(exports, "__esModule", { value: true }); const fs = __importStar(__webpack_require__(747)); const core = __importStar(__webpack_require__(470)); @@ -4651,7 +4648,7 @@ const github = __importStar(__webpack_require__(469)); const filter_1 = __webpack_require__(235); const file_1 = __webpack_require__(258); const git = __importStar(__webpack_require__(136)); -const shell_escape_1 = __importDefault(__webpack_require__(751)); +const shell_escape_1 = __webpack_require__(751); async function run() { try { const workingDirectory = core.getInput('working-directory', { required: false }); @@ -4819,14 +4816,16 @@ function serializeExport(files, format) { switch (format) { case 'json': return JSON.stringify(fileNames); + case 'escape': + return fileNames.map(shell_escape_1.escape).join(' '); case 'shell': - return fileNames.map(shell_escape_1.default).join(' '); + return fileNames.map(shell_escape_1.shellEscape).join(' '); default: return ''; } } function isExportFormat(value) { - return value === 'none' || value === 'shell' || value === 'json'; + return value === 'none' || value === 'shell' || value === 'json' || value === 'escape'; } run(); @@ -15223,12 +15222,33 @@ module.exports = require("fs"); "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); -// Uses easy safe set of characters which can be left unescaped to keep it readable. -// Every other character will be backslash-escaped -function shellEscape(value) { +exports.shellEscape = exports.escape = void 0; +// Backslash escape every character except small subset of definitely safe characters +function escape(value) { return value.replace(/([^a-zA-Z0-9,._+:@%/-])/gm, '\\$1'); } -exports.default = shellEscape; +exports.escape = escape; +// Returns filename escaped for usage as shell argument. +// Applies "human readable" approach with as few escaping applied as possible +function shellEscape(value) { + if (value === '') + return value; + // Only safe characters + if (/^[a-zA-Z0-9,._+:@%/-]+$/m.test(value)) { + return value; + } + if (value.includes("'")) { + // Only safe characters, single quotes and white-spaces + if (/^[a-zA-Z0-9,._+:@%/'\s-]+$/m.test(value)) { + return `"${value}"`; + } + // Split by single quote and apply escaping recursively + return value.split("'").map(shellEscape).join("\\'"); + } + // Contains some unsafe characters but no single quote + return `'${value}'`; +} +exports.shellEscape = shellEscape; /***/ }), diff --git a/src/main.ts b/src/main.ts index 6464367..176dafc 100644 --- a/src/main.ts +++ b/src/main.ts @@ -6,9 +6,9 @@ import {Webhooks} from '@octokit/webhooks' import {Filter, FilterResults} from './filter' import {File, ChangeStatus} from './file' import * as git from './git' -import shellEscape from './shell-escape' +import {escape, shellEscape} from './shell-escape' -type ExportFormat = 'none' | 'json' | 'shell' +type ExportFormat = 'none' | 'json' | 'shell' | 'escape' async function run(): Promise { try { @@ -201,6 +201,8 @@ function serializeExport(files: File[], format: ExportFormat): string { switch (format) { case 'json': return JSON.stringify(fileNames) + case 'escape': + return fileNames.map(escape).join(' ') case 'shell': return fileNames.map(shellEscape).join(' ') default: @@ -209,7 +211,7 @@ function serializeExport(files: File[], format: ExportFormat): string { } function isExportFormat(value: string): value is ExportFormat { - return value === 'none' || value === 'shell' || value === 'json' + return value === 'none' || value === 'shell' || value === 'json' || value === 'escape' } run() diff --git a/src/shell-escape.ts b/src/shell-escape.ts index 643b7cc..25bb940 100644 --- a/src/shell-escape.ts +++ b/src/shell-escape.ts @@ -1,5 +1,28 @@ -// Uses easy safe set of characters which can be left unescaped to keep it readable. -// Every other character will be backslash-escaped -export default function shellEscape(value: string): string { +// Backslash escape every character except small subset of definitely safe characters +export function escape(value: string): string { return value.replace(/([^a-zA-Z0-9,._+:@%/-])/gm, '\\$1') } + +// Returns filename escaped for usage as shell argument. +// Applies "human readable" approach with as few escaping applied as possible +export function shellEscape(value: string): string { + if (value === '') return value + + // Only safe characters + if (/^[a-zA-Z0-9,._+:@%/-]+$/m.test(value)) { + return value + } + + if (value.includes("'")) { + // Only safe characters, single quotes and white-spaces + if (/^[a-zA-Z0-9,._+:@%/'\s-]+$/m.test(value)) { + return `"${value}"` + } + + // Split by single quote and apply escaping recursively + return value.split("'").map(shellEscape).join("\\'") + } + + // Contains some unsafe characters but no single quote + return `'${value}'` +}