From 6b3e1bb7b9e3322db7748be80948a6cf42f786c2 Mon Sep 17 00:00:00 2001 From: Eric Cornelissen Date: Fri, 27 Dec 2024 21:40:29 +0100 Subject: [PATCH] Filter out expressions that are certainly safe Update the expressions matcher logic to not complain about expressions that can be proven to be safe because they consist exclusively of literals[1] and/or functions[2] whose output does not contain any of the input values (i.e. predicate functions as well as `hashFiles`). The choice to match twice, once against the input with safe expressions removed and once against the actual input, may seem unnecessarily bad from a performance ponit of view. However, if the stripped input does contain a problem, we want to get the matches on the original input to present to the user. The performance impact is minimized by using the `Regexp.Find` method which will return on the first match. -- 1. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#literals 2. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#functions -- Signed-off-by: Eric Cornelissen --- matchers.go | 37 +++++ matchers_test.go | 363 ++++++++++++++++++++++++++++++++++++----------- 2 files changed, 317 insertions(+), 83 deletions(-) diff --git a/matchers.go b/matchers.go index a3f48c4..d4bbf93 100644 --- a/matchers.go +++ b/matchers.go @@ -16,6 +16,7 @@ package ades import ( + "bytes" "regexp" ) @@ -40,6 +41,10 @@ var allExprRegExp = regexp.MustCompile(`\$\{\{.*?\}\}`) type allExprMatcher struct{} func (m allExprMatcher) FindAll(v []byte) [][]byte { + if allExprRegExp.Find(stripSafe(v)) == nil { + return nil + } + return allExprRegExp.FindAll(v, len(v)) } @@ -48,5 +53,37 @@ var conservativeExprRegExp = regexp.MustCompile(`\$\{\{.+?(github\.event\.issue\ type conservativeExprMatcher struct{} func (m conservativeExprMatcher) FindAll(v []byte) [][]byte { + if conservativeExprRegExp.Find(stripSafe(v)) == nil { + return nil + } + return conservativeExprRegExp.FindAll(v, len(v)) } + +var ( + leading = `(?P\$\{\{(|.*? ))` + trailing = `(?P( .*?|)\}\})` + + LiteralInExprRegExp = regexp.MustCompile(leading + `(true|false|null|-?\d+(\.\d+)?|0x[0-9A-Fa-f]+|-?\d+\.\d+e-?\d+|'[^']+')` + trailing) + SafeFunctionInExprRegExp = regexp.MustCompile(leading + `((always|cancelled|failure|success)\(\s*\)|(contains|endsWith|startsWith)\([^,]+,[^,)]+\)|(hashFiles)\(([^,]+,)*[^,)]+\))` + trailing) + + EmptyExprRegExp = regexp.MustCompile(`\$\{\{\s*\}\}`) +) + +func stripSafe(v []byte) []byte { + exps := []regexp.Regexp{ + *LiteralInExprRegExp, + *SafeFunctionInExprRegExp, + } + + r := v + for _, exp := range exps { + r = exp.ReplaceAll(r, []byte("$leading$trailing")) + } + + if !bytes.Equal(r, v) { + return stripSafe(r) + } + + return EmptyExprRegExp.ReplaceAll(v, nil) +} diff --git a/matchers_test.go b/matchers_test.go index bfb63b2..96ebab7 100644 --- a/matchers_test.go +++ b/matchers_test.go @@ -16,6 +16,7 @@ package ades import ( + "bytes" "fmt" "testing" ) @@ -27,70 +28,84 @@ func TestAllMatcher(t *testing.T) { } testCases := map[string]TestCase{ - "arbitrary expression ": { - value: "${{ foo.bar }}", + "not an expression": { + value: `'Hello world'`, + want: nil, + }, + "safe expression": { + value: `${{ true }}`, + want: nil, + }, + "unsafe expression ": { + value: `${{ foo.bar }}`, want: []string{ - "${{ foo.bar }}", + `${{ foo.bar }}`, }, }, "input expression": { - value: "${{ input.greeting }}", + value: `${{ input.greeting }}`, want: []string{ - "${{ input.greeting }}", + `${{ input.greeting }}`, }, }, "matrix expression": { - value: "${{ matrix.runtime }}", + value: `${{ matrix.runtime }}`, want: []string{ - "${{ matrix.runtime }}", + `${{ matrix.runtime }}`, }, }, "vars expression": { - value: "${{ vars.command }}", + value: `${{ vars.command }}`, want: []string{ - "${{ vars.command }}", + `${{ vars.command }}`, }, }, "secrets expression": { - value: "${{ secrets.value }}", + value: `${{ secrets.value }}`, want: []string{ - "${{ secrets.value }}", + `${{ secrets.value }}`, }, }, "github.event.issue.title": { - value: "${{ github.event.issue.title }}", + value: `${{ github.event.issue.title }}`, want: []string{ - "${{ github.event.issue.title }}", + `${{ github.event.issue.title }}`, }, }, "github.event.discussion.body": { - value: "${{ github.event.discussion.body }}", + value: `${{ github.event.discussion.body }}`, want: []string{ - "${{ github.event.discussion.body }}", + `${{ github.event.discussion.body }}`, }, }, "github.event.pages[*].page_name": { - value: "${{ github.event.pages[0].page_name }}", + value: `${{ github.event.pages[0].page_name }}`, want: []string{ - "${{ github.event.pages[0].page_name }}", + `${{ github.event.pages[0].page_name }}`, }, }, "github.event.commits[*].author.email": { - value: "${{ github.event.commits[1].author.email }}", + value: `${{ github.event.commits[1].author.email }}`, want: []string{ - "${{ github.event.commits[1].author.email }}", + `${{ github.event.commits[1].author.email }}`, }, }, "github.head_ref": { - value: "${{ github.head_ref }}", + value: `${{ github.head_ref }}`, want: []string{ - "${{ github.head_ref }}", + `${{ github.head_ref }}`, }, }, "github.event.workflow_run.pull_requests[*].head.ref": { - value: "${{ github.event.workflow_run.pull_requests[2].head.ref }}", + value: `${{ github.event.workflow_run.pull_requests[2].head.ref }}`, + want: []string{ + `${{ github.event.workflow_run.pull_requests[2].head.ref }}`, + }, + }, + "safe & unsafe combined": { + value: `${{ foo.bar || true }}`, want: []string{ - "${{ github.event.workflow_run.pull_requests[2].head.ref }}", + `${{ foo.bar || true }}`, }, }, } @@ -122,178 +137,186 @@ func TestConservativeMatcher(t *testing.T) { } testCases := map[string]TestCase{ + "not conservatively dangerous": { + value: `${{ input.greeting }}`, + want: nil, + }, + "not an expression": { + value: `'Hello world'`, + want: nil, + }, + "safe use of conservatively dangerous": { + value: `${{ contains(github.event.issue.title, 'foobar') }}`, + want: nil, + }, "github.event.issue.title": { - value: "${{ github.event.issue.title }}", + value: `${{ github.event.issue.title }}`, want: []string{ - "${{ github.event.issue.title }}", + `${{ github.event.issue.title }}`, }, }, "github.event.issue.body": { - value: "${{ github.event.issue.body }}", + value: `${{ github.event.issue.body }}`, want: []string{ - "${{ github.event.issue.body }}", + `${{ github.event.issue.body }}`, }, }, "github.event.discussion.title": { - value: "${{ github.event.discussion.title }}", + value: `${{ github.event.discussion.title }}`, want: []string{ - "${{ github.event.discussion.title }}", + `${{ github.event.discussion.title }}`, }, }, "github.event.discussion.body": { - value: "${{ github.event.discussion.body }}", + value: `${{ github.event.discussion.body }}`, want: []string{ - "${{ github.event.discussion.body }}", + `${{ github.event.discussion.body }}`, }, }, "github.event.comment.body": { - value: "${{ github.event.comment.body }}", + value: `${{ github.event.comment.body }}`, want: []string{ - "${{ github.event.comment.body }}", + `${{ github.event.comment.body }}`, }, }, "github.event.review.body": { - value: "${{ github.event.review.body }}", + value: `${{ github.event.review.body }}`, want: []string{ - "${{ github.event.review.body }}", + `${{ github.event.review.body }}`, }, }, "github.event.review_comment.body": { - value: "${{ github.event.review_comment.body }}", + value: `${{ github.event.review_comment.body }}`, want: []string{ - "${{ github.event.review_comment.body }}", + `${{ github.event.review_comment.body }}`, }, }, "github.event.pages[*].page_name": { - value: "${{ github.event.pages[0].page_name }}", + value: `${{ github.event.pages[0].page_name }}`, want: []string{ - "${{ github.event.pages[0].page_name }}", + `${{ github.event.pages[0].page_name }}`, }, }, "github.event.commits[*].message": { - value: "${{ github.event.commits[1].message }}", + value: `${{ github.event.commits[1].message }}`, want: []string{ - "${{ github.event.commits[1].message }}", + `${{ github.event.commits[1].message }}`, }, }, "github.event.commits[*].author.email": { - value: "${{ github.event.commits[2].author.email }}", + value: `${{ github.event.commits[2].author.email }}`, want: []string{ - "${{ github.event.commits[2].author.email }}", + `${{ github.event.commits[2].author.email }}`, }, }, "github.event.commits[*].author.name": { - value: "${{ github.event.commits[3].author.name }}", + value: `${{ github.event.commits[3].author.name }}`, want: []string{ - "${{ github.event.commits[3].author.name }}", + `${{ github.event.commits[3].author.name }}`, }, }, "github.event.head_commit.message": { - value: "${{ github.event.head_commit.message }}", + value: `${{ github.event.head_commit.message }}`, want: []string{ - "${{ github.event.head_commit.message }}", + `${{ github.event.head_commit.message }}`, }, }, "github.event.head_commit.author.email": { - value: "${{ github.event.head_commit.author.email }}", + value: `${{ github.event.head_commit.author.email }}`, want: []string{ - "${{ github.event.head_commit.author.email }}", + `${{ github.event.head_commit.author.email }}`, }, }, "github.event.head_commit.author.name": { - value: "${{ github.event.head_commit.author.name }}", + value: `${{ github.event.head_commit.author.name }}`, want: []string{ - "${{ github.event.head_commit.author.name }}", + `${{ github.event.head_commit.author.name }}`, }, }, "github.event.head_commit.committer.email": { - value: "${{ github.event.head_commit.committer.email }}", + value: `${{ github.event.head_commit.committer.email }}`, want: []string{ - "${{ github.event.head_commit.committer.email }}", + `${{ github.event.head_commit.committer.email }}`, }, }, "github.event.workflow_run.head_branch": { - value: "${{ github.event.workflow_run.head_branch }}", + value: `${{ github.event.workflow_run.head_branch }}`, want: []string{ - "${{ github.event.workflow_run.head_branch }}", + `${{ github.event.workflow_run.head_branch }}`, }, }, "github.event.workflow_run.head_commit.message": { - value: "${{ github.event.workflow_run.head_commit.message }}", + value: `${{ github.event.workflow_run.head_commit.message }}`, want: []string{ - "${{ github.event.workflow_run.head_commit.message }}", + `${{ github.event.workflow_run.head_commit.message }}`, }, }, "github.event.workflow_run.head_commit.author.email": { - value: "${{ github.event.workflow_run.head_commit.author.email }}", + value: `${{ github.event.workflow_run.head_commit.author.email }}`, want: []string{ - "${{ github.event.workflow_run.head_commit.author.email }}", + `${{ github.event.workflow_run.head_commit.author.email }}`, }, }, "github.event.workflow_run.head_commit.author.name": { - value: "${{ github.event.workflow_run.head_commit.author.name }}", + value: `${{ github.event.workflow_run.head_commit.author.name }}`, want: []string{ - "${{ github.event.workflow_run.head_commit.author.name }}", + `${{ github.event.workflow_run.head_commit.author.name }}`, }, }, "github.event.pull_request.title": { - value: "${{ github.event.pull_request.title }}", + value: `${{ github.event.pull_request.title }}`, want: []string{ - "${{ github.event.pull_request.title }}", + `${{ github.event.pull_request.title }}`, }, }, "github.event.pull_request.body": { - value: "${{ github.event.pull_request.body }}", + value: `${{ github.event.pull_request.body }}`, want: []string{ - "${{ github.event.pull_request.body }}", + `${{ github.event.pull_request.body }}`, }, }, "github.event.pull_request.head.label": { - value: "${{ github.event.pull_request.head.label }}", + value: `${{ github.event.pull_request.head.label }}`, want: []string{ - "${{ github.event.pull_request.head.label }}", + `${{ github.event.pull_request.head.label }}`, }, }, "github.event.pull_request.head.repo.default_branch": { - value: "${{ github.event.pull_request.head.repo.default_branch }}", + value: `${{ github.event.pull_request.head.repo.default_branch }}`, want: []string{ - "${{ github.event.pull_request.head.repo.default_branch }}", + `${{ github.event.pull_request.head.repo.default_branch }}`, }, }, "github.head_ref": { - value: "${{ github.head_ref }}", + value: `${{ github.head_ref }}`, want: []string{ - "${{ github.head_ref }}", + `${{ github.head_ref }}`, }, }, "github.event.pull_request.head.ref": { - value: "${{ github.event.pull_request.head.ref }}", + value: `${{ github.event.pull_request.head.ref }}`, want: []string{ - "${{ github.event.pull_request.head.ref }}", + `${{ github.event.pull_request.head.ref }}`, }, }, "github.event.workflow_run.pull_requests[*].head.ref": { - value: "${{ github.event.workflow_run.pull_requests[4].head.ref }}", + value: `${{ github.event.workflow_run.pull_requests[4].head.ref }}`, want: []string{ - "${{ github.event.workflow_run.pull_requests[4].head.ref }}", + `${{ github.event.workflow_run.pull_requests[4].head.ref }}`, }, }, "two, both are dangerous": { - value: "${{ github.event.pull_request.head.ref || github.head_ref }}", + value: `${{ github.event.pull_request.head.ref || github.head_ref }}`, want: []string{ - "${{ github.event.pull_request.head.ref || github.head_ref }}", + `${{ github.event.pull_request.head.ref || github.head_ref }}`, }, }, "two, only one is dangerous": { - value: "${{ github.event.pull_request.head.ref || inputs.backup }}", + value: `${{ github.event.pull_request.head.ref || inputs.backup }}`, want: []string{ - "${{ github.event.pull_request.head.ref || inputs.backup }}", + `${{ github.event.pull_request.head.ref || inputs.backup }}`, }, }, - "not conservatively dangerous": { - value: "${{ input.greeting }}", - want: []string{}, - }, } for name, tt := range testCases { @@ -315,3 +338,177 @@ func TestConservativeMatcher(t *testing.T) { }) } } + +func TestDefinitelySafe(t *testing.T) { + type TestCase struct { + value string + want string + } + + testCases := map[string]TestCase{ + "literals, boolean, true": { + value: `echo ${{ true }}`, + want: `echo `, + }, + "literals, boolean, false": { + value: `echo ${{ false }}`, + want: `echo `, + }, + "literals, null": { + value: `echo ${{ null }}`, + want: `echo `, + }, + "literals, number, integer": { + value: `echo ${{ 42 }}`, + want: `echo `, + }, + "literals, number, negative integer": { + value: `echo ${{ -36 }}`, + want: `echo `, + }, + "literals, number, float": { + value: `echo ${{ 3.14 }}`, + want: `echo `, + }, + "literals, number, hexadecimal (uppercase)": { + value: `echo ${{ 0x2A }}`, + want: `echo `, + }, + "literals, number, hexadecimal (lowercase)": { + value: `echo ${{ 0x2a }}`, + want: `echo `, + }, + "literals, number, hexadecimal (mixed case)": { + value: `echo ${{ 0xDeAdBeEf }}`, + want: `echo `, + }, + "literals, number, scientific notation, positive, small": { + value: `echo ${{ 2.99e-2 }}`, + want: `echo `, + }, + "literals, number, scientific notation, positive, big": { + value: `echo ${{ 2.99e2 }}`, + want: `echo `, + }, + "literals, number, scientific notation, negative, small": { + value: `echo ${{ -2.99e-2 }}`, + want: `echo `, + }, + "literals, number, scientific notation, negative, big": { + value: `echo ${{ -2.99e2 }}`, + want: `echo `, + }, + "literals, string": { + value: `echo ${{ 'Hello world' }}`, + want: `echo `, + }, + "literals, multiple": { + value: `echo ${{ true || 42 }}`, + want: `echo ${{ || }}`, + }, + "function, always": { + value: `echo ${{ always() }}`, + want: `echo `, + }, + "function, cancelled": { + value: `echo 'The job was cancelled: ${{ cancelled() }}'`, + want: `echo 'The job was cancelled: '`, + }, + "function, contains, no variables": { + value: `echo 'Does the string contain llo: ${{ contains('Hello world', 'llo') }}'`, + want: `echo 'Does the string contain llo: '`, + }, + "function, contains, with variable": { + value: `echo 'Does ${{ inputs.greeting }} contain llo: ${{ contains(inputs.greeting, 'llo') }}'`, + want: `echo 'Does ${{ inputs.greeting }} contain llo: '`, + }, + "function, endsWith, no variables": { + value: `echo 'Does the string end with llo: ${{ endsWith('Hello world', 'llo') }}'`, + want: `echo 'Does the string end with llo: '`, + }, + "function, endsWith, with variable": { + value: `echo 'Does ${{ inputs.greeting }} end with llo: ${{ endsWith(inputs.greeting, 'llo') }}'`, + want: `echo 'Does ${{ inputs.greeting }} end with llo: '`, + }, + "function, failure": { + value: `echo 'The job failed: ${{ failure() }}'`, + want: `echo 'The job failed: '`, + }, + "function, format, no variables": { + value: `echo ${{ format('Hello {0}', 'world') }}`, + want: `echo ${{ format('Hello {0}', 'world') }}`, + }, + "function, format, with variable": { + value: `echo ${{ format('Hello {0}', inputs.who) }}`, + want: `echo ${{ format('Hello {0}', inputs.who) }}`, + }, + "function, fromJSON, no variables": { + value: `obj=${{ fromJSON('["foo", "bar"]') }}`, + want: `obj=${{ fromJSON('["foo", "bar"]') }}`, + }, + "function, fromJSON, with variable": { + value: `obj=${{ fromJSON(inputs.json) }}`, + want: `obj=${{ fromJSON(inputs.json) }}`, + }, + "function, hashFiles, no variables": { + value: `echo 'hash: ${{ hashFiles('**/*.go') }}'`, + want: `echo 'hash: '`, + }, + "function, hashFiles, with variables": { + value: `echo 'hash: ${{ hashFiles(input.files) }}'`, + want: `echo 'hash: '`, + }, + "function, join, no variables": { + value: `echo 'The elements are: ${{ join(fromJSON('["foo", "bar"]), ', ') }}'`, + want: `echo 'The elements are: ${{ join(fromJSON('["foo", "bar"]), ', ') }}'`, + }, + "function, join, with variable": { + value: `echo 'The elements are: ${{ join(inputs.list, ', ') }}'`, + want: `echo 'The elements are: ${{ join(inputs.list, ', ') }}'`, + }, + "function, startsWith, no variables": { + value: `echo 'Does the string start with llo: ${{ startsWith('Hello world', 'llo') }}'`, + want: `echo 'Does the string start with llo: '`, + }, + "function, startsWith, with variable": { + value: `echo 'Does ${{ inputs.greeting }} start with llo: ${{ startsWith(inputs.greeting, 'llo') }}'`, + want: `echo 'Does ${{ inputs.greeting }} start with llo: '`, + }, + "function, success": { + value: `echo 'The job succeeded: ${{ success() }}'`, + want: `echo 'The job succeeded: '`, + }, + "function, toJSON, with variable": { + value: `json=${{ toJSON(inputs.value) }}`, + want: `json=${{ toJSON(inputs.value) }}`, + }, + + "edge case, literal with odd spacing": { + value: `echo ${{true}}`, + want: `echo `, + }, + "edge case, function with odd spacing": { + value: `echo ${{success( )}}`, + want: `echo `, + }, + "edge case, identifier like a literal": { + value: `echo ${{ trueish }}`, + want: `echo ${{ trueish }}`, + }, + "edge case, identifier like a function": { + value: `echo ${{ contained('foo', 'bar') }}`, + want: `echo ${{ contained('foo', 'bar') }}`, + }, + } + + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + v := []byte(tt.value) + if got, want := stripSafe(v), []byte(tt.want); !bytes.Equal(got, want) { + t.Errorf("Unexpected result (got %q, want %q)", got, want) + } + }) + } +}