diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 2bfedd623f5..3123518d369 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -121,7 +121,6 @@ class Outputs extends AstNode instanceof OutputsImpl { Expression getOutputExpr(string outputName) { result = super.getOutputExpr(outputName) } - // TODO: REMOVE override string toString() { result = "Job outputs node" } } diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index 63b25229a58..028f2280680 100644 --- a/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -252,11 +252,6 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - // override AstNodeImpl getAChildNode() { - // result = this.getInputs() or - // result = this.getOutputs() or - // result = this.getRuns() - // } override AstNodeImpl getParentNode() { none() } override string getAPrimaryQlClass() { result = "CompositeActionImpl" } @@ -292,12 +287,6 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - // override AstNodeImpl getAChildNode() { - // result = this.getAJob() or - // result = this.getStrategy() or - // result = this.getEnv() or - // result = this.getPermissions() - // } override AstNodeImpl getParentNode() { none() } override string getAPrimaryQlClass() { result = "WorkflowImpl" } @@ -306,8 +295,6 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode { override YamlMapping getNode() { result = n } - // /** Gets the `jobs` mapping from job IDs to job definitions in this workflow. */ - // YamlMapping getJobs() { result = this.asYamlMapping().lookup("jobs") } /** Gets the 'global' `env` mapping in this workflow. */ EnvImpl getEnv() { result.getNode() = n.lookup("env") } @@ -346,11 +333,6 @@ class ReusableWorkflowImpl extends AstNodeImpl, WorkflowImpl { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - // override AstNodeImpl getAChildNode() { - // result = super.getAChildNode() or - // result = this.getInputs() or - // result = this.getOutputs() - // } OutputsImpl getOutputs() { result.getNode() = workflow_call.(YamlMapping).lookup("outputs") } ExpressionImpl getAnOutputExpr() { result = this.getOutputs().getAnOutputExpr() } @@ -378,7 +360,6 @@ class RunsImpl extends AstNodeImpl, TRunsNode { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - //override AstNodeImpl getAChildNode() { result = this.getAStep() } override CompositeActionImpl getParentNode() { result.getAChildNode() = this } override string getAPrimaryQlClass() { result = "RunsImpl" } @@ -450,7 +431,6 @@ class OutputsImpl extends AstNodeImpl, TOutputsNode { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - //override AstNodeImpl getAChildNode() { result = this.getAnOutputExpr() } override AstNodeImpl getParentNode() { result.getAChildNode() = this } override string getAPrimaryQlClass() { result = "OutputsImpl" } @@ -503,7 +483,6 @@ class StrategyImpl extends AstNodeImpl, TStrategyNode { override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } - //override ExpressionImpl getAChildNode() { result = this.getAMatrixVarExpr() } override AstNodeImpl getParentNode() { result.getAChildNode() = this } override string getAPrimaryQlClass() { result = "StrategyImpl" } @@ -557,10 +536,8 @@ class JobImpl extends AstNodeImpl, TJobNode { workflow.getNode().lookup("jobs").(YamlMapping).lookup(jobId) = n } - // TODO: REMOVE override string toString() { result = "Job: " + jobId } - //override string toString() { result = n.toString() } override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } override WorkflowImpl getParentNode() { result.getAChildNode() = this } @@ -739,7 +716,6 @@ class UsesStepImpl extends StepImpl, UsesImpl { result.getParentNode().getNode() = n.lookup("with").(YamlMapping).lookup(key) } - // TODO: REMOVE override string toString() { if exists(this.getId()) then result = "Uses Step: " + this.getId() else result = "Uses Step" } @@ -760,7 +736,6 @@ class ExternalJobImpl extends JobImpl, UsesImpl { ExternalJobImpl() { n.lookup("uses") = u } - //override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() } override string getCallee() { if u.getValue().matches("./%") then result = u.getValue().regexpCapture(pathUsesParser(), 1) @@ -796,7 +771,6 @@ class RunImpl extends StepImpl { ExpressionImpl getAnScriptExpr() { result.getParentNode().getNode() = script } - // TODO: REMOVE override string toString() { if exists(this.getId()) then result = "Run Step: " + this.getId() else result = "Run Step" } @@ -807,14 +781,6 @@ class RunImpl extends StepImpl { * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability */ abstract class ContextExpressionImpl extends ExpressionImpl { - // TODO: REMOVE - // ContextExpressionImpl() { - // expression - // .regexpMatch([ - // stepsCtxRegex(), needsCtxRegex(), jobsCtxRegex(), envCtxRegex(), inputsCtxRegex(), - // matrixCtxRegex() - // ]) - // } abstract string getFieldName(); abstract AstNodeImpl getTarget(); diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index 32d37efdaae..23ae225e07e 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -96,6 +96,7 @@ private predicate isExternalUserControlledWorkflowRun(string context) { exists(string reg | reg = [ + "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow\\s*\\.\\s*path\\b", "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_branch\\b", "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*display_title\\b", "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_repository\\b\\s*\\.\\s*description\\b", diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll index 52c2ae6a483..bda55da5c82 100644 --- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll +++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll @@ -72,7 +72,6 @@ class DataFlowCall instanceof Cfg::Node { /** Gets a textual representation of this element. */ string toString() { result = super.toString() } - //Location getLocation() { result = super.getLocation() } string getName() { result = super.getAstNode().(Uses).getCallee() } DataFlowCallable getEnclosingCallable() { result = super.getScope() } @@ -84,7 +83,6 @@ class DataFlowCall instanceof Cfg::Node { class DataFlowCallable instanceof Cfg::CfgScope { string toString() { result = super.toString() } - //Location getLocation() { result = super.getLocation() } string getName() { if this instanceof ReusableWorkflow then result = this.(ReusableWorkflow).getLocation().getFile().getRelativePath() diff --git a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql index fd4f03e1edd..66a055634c7 100644 --- a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql +++ b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql @@ -34,14 +34,14 @@ module MyFlow = TaintTracking::Global; import MyFlow::PathGraph -from MyFlow::PathNode source, MyFlow::PathNode sink +from MyFlow::PathNode source, MyFlow::PathNode sink, Workflow w where MyFlow::flowPath(source, sink) and - source - .getNode() - .asExpr() - .getEnclosingWorkflow() - .hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent()) + w = source.getNode().asExpr().getEnclosingWorkflow() and + ( + w instanceof ReusableWorkflow or + w.hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent()) + ) select sink.getNode(), source, sink, "Potential expression injection in $@, which may be controlled by an external user.", sink, sink.getNode().asExpr().(Expression).getExpression() diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog.yml new file mode 100644 index 00000000000..0ee850f183d --- /dev/null +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog.yml @@ -0,0 +1,100 @@ +name: changelog + +on: + workflow_call: + inputs: + create: + description: Add a log to the changelog + type: boolean + required: false + default: false + update: + description: Update the existing changelog + type: boolean + required: false + default: false + +jobs: + changelog: + runs-on: ubuntu-latest + env: + file: CHANGELOG.md + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + - name: Check ${{ env.file }} + run: | + if [[ $(git diff --name-only origin/master HEAD -- ${{ env.file }} | grep '^${{ env.file }}$' -c) -eq 0 ]]; then + echo "Expected '${{ env.file }}' to be modified" + exit 1 + fi + update: + runs-on: ubuntu-latest + needs: changelog + if: (inputs.create && failure()) || (inputs.update && success()) + continue-on-error: true + env: + file: CHANGELOG.md + next_version: next + link: '[#${{ github.event.number }}](https://github.com/fabricjs/fabric.js/pull/${{ github.event.number }})' + steps: + - uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.ref }} + - name: Update ${{ env.file }} from PR title + id: update + uses: actions/github-script@v6 + env: + log: '- ${{ github.event.pull_request.title }} ${{ env.link }}\n' + prev_log: '- ${{ github.event.changes.title.from }} ${{ env.link }}\n' + with: + result-encoding: string + script: | + const fs = require('fs'); + const file = './${{ env.file }}'; + let content = fs.readFileSync(file).toString(); + const title = '[${{ env.next_version }}]'; + const log = '${{ env.log }}'; + let exists = ${{ needs.changelog.result == 'success' }}; + + if (!content.includes(title)) { + const insertAt = content.indexOf('\n') + 1; + content = + content.slice(0, insertAt) + + `\n## ${title}\n\n\n` + + content.slice(insertAt); + } + + const insertAt = content.indexOf('\n', content.indexOf(title) + title.length + 1) + 1; + if (exists && ${{ github.event.action == 'edited' }}) { + const prevLog = '${{ env.prev_log }}'; + const index = content.indexOf(prevLog, insertAt); + if (index > -1) { + content = content.slice(0, index) + content.slice(index + prevLog.length); + exists = false; + } + } + + if (!exists) { + content = content.slice(0, insertAt) + log + content.slice(insertAt); + fs.writeFileSync(file, content); + return true; + } + + return false; + - name: Setup node + if: fromJson(steps.update.outputs.result) + uses: actions/setup-node@v3 + with: + node-version: 18.x + - name: Commit & Push + if: fromJson(steps.update.outputs.result) + run: | + npm ci + npx prettier --write ${{ env.file }} + git config user.name github-actions[bot] + git config user.email github-actions[bot]@users.noreply.github.com + git add ${{ env.file }} + git commit -m "update ${{ env.file }}" + git push diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog_required.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog_required.yml new file mode 100644 index 00000000000..b0a1ea5ed68 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog_required.yml @@ -0,0 +1,9 @@ +name: '📋' + +on: + pull_request: + branches: [master] + +jobs: + changelog: + uses: ./.github/workflows/changelog.yml