Skip to content

Commit a9baf34

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #20324 from michaelnebel/actions/ql4ql
Actions: Fix some Ql4Ql violations.
2 parents f2b8ac1 + 64f9758 commit a9baf34

File tree

13 files changed

+55
-78
lines changed

13 files changed

+55
-78
lines changed

‎actions/ql/lib/codeql/Locations.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ class Location extends TLocation, TBaseLocation {
7070

7171
/**
7272
* Holds if this element is at the specified location.
73-
* The location spans column `startcolumn` of line `startline` to
74-
* column `endcolumn` of line `endline` in file `filepath`.
73+
* The location spans column `sc` of line `sl` to
74+
* column `ec` of line `el` in file `p`.
7575
* For more information, see
7676
* [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
7777
*/

‎actions/ql/lib/codeql/actions/Ast.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ class If extends AstNode instanceof IfImpl {
261261
}
262262

263263
/**
264-
* An Environemnt node representing a deployment environment.
264+
* An Environment node representing a deployment environment.
265265
*/
266266
class Environment extends AstNode instanceof EnvironmentImpl {
267267
string getName() { result = super.getName() }

‎actions/ql/lib/codeql/actions/ast/internal/Ast.qll‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -125,12 +125,11 @@ abstract class AstNodeImpl extends TAstNode {
125125
* Gets the enclosing Step.
126126
*/
127127
StepImpl getEnclosingStep() {
128-
if this instanceof StepImpl
129-
then result = this
130-
else
131-
if this instanceof ScalarValueImpl
132-
then result.getAChildNode*() = this.getParentNode()
133-
else none()
128+
this instanceof StepImpl and
129+
result = this
130+
or
131+
this instanceof ScalarValueImpl and
132+
result.getAChildNode*() = this.getParentNode()
134133
}
135134

136135
/**
@@ -1416,9 +1415,8 @@ class ExternalJobImpl extends JobImpl, UsesImpl {
14161415
override string getVersion() {
14171416
exists(YamlString name |
14181417
n.lookup("uses") = name and
1419-
if not name.getValue().matches("\\.%")
1420-
then result = name.getValue().regexpCapture(repoUsesParser(), 4)
1421-
else none()
1418+
not name.getValue().matches("\\.%") and
1419+
result = name.getValue().regexpCapture(repoUsesParser(), 4)
14221420
)
14231421
}
14241422
}

‎actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ private module Cached {
286286
/**
287287
* Holds if `cfn` is the `i`th node in basic block `bb`.
288288
*
289-
* In other words, `i` is the shortest distance from a node `bb`
289+
* In other words, `i` is the shortest distance from a node `bbStart`
290290
* that starts a basic block to `cfn` along the `intraBBSucc` relation.
291291
*/
292292
cached

‎actions/ql/lib/codeql/actions/dataflow/ExternalFlow.qll‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,10 @@ predicate madSource(DataFlow::Node source, string kind, string fieldName) {
6363
(
6464
if fieldName.trim().matches("env.%")
6565
then source.asExpr() = uses.getInScopeEnvVarExpr(fieldName.trim().replaceAll("env.", ""))
66-
else
67-
if fieldName.trim().matches("output.%")
68-
then source.asExpr() = uses
69-
else none()
66+
else (
67+
fieldName.trim().matches("output.%") and
68+
source.asExpr() = uses
69+
)
7070
)
7171
)
7272
}

‎actions/ql/lib/codeql/actions/dataflow/FlowSources.qll‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,14 @@ abstract class RemoteFlowSource extends SourceNode {
3131
class GitHubCtxSource extends RemoteFlowSource {
3232
string flag;
3333
string event;
34-
GitHubExpression e;
3534

3635
GitHubCtxSource() {
37-
this.asExpr() = e and
38-
// github.head_ref
39-
e.getFieldName() = "head_ref" and
40-
flag = "branch" and
41-
(
36+
exists(GitHubExpression e |
37+
this.asExpr() = e and
38+
// github.head_ref
39+
e.getFieldName() = "head_ref" and
40+
flag = "branch"
41+
|
4242
event = e.getATriggerEvent().getName() and
4343
event = "pull_request_target"
4444
or
@@ -148,7 +148,6 @@ class GhCLICommandSource extends RemoteFlowSource, CommandSource {
148148
class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
149149
string cmd;
150150
string flag;
151-
string access_path;
152151
Run run;
153152

154153
// Examples
@@ -163,7 +162,7 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
163162
run.getScript().getACommand() = cmd and
164163
cmd.matches("jq%") and
165164
cmd.matches("%GITHUB_EVENT_PATH%") and
166-
exists(string regexp |
165+
exists(string regexp, string access_path |
167166
untrustedEventPropertiesDataModel(regexp, flag) and
168167
not flag = "json" and
169168
access_path = "github.event" + cmd.regexpCapture(".*\\s+([^\\s]+)\\s+.*", 1) and

‎actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ abstract class ArgumentInjectionSink extends DataFlow::Node {
1919
*/
2020
class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
2121
string command;
22-
string argument;
2322

2423
ArgumentInjectionFromEnvVarSink() {
2524
exists(Run run, string var |
@@ -28,7 +27,7 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
2827
exists(run.getInScopeEnvVarExpr(var)) or
2928
var = "GITHUB_HEAD_REF"
3029
) and
31-
run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, argument)
30+
run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, _)
3231
)
3332
}
3433

@@ -44,13 +43,12 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
4443
*/
4544
class ArgumentInjectionFromCommandSink extends ArgumentInjectionSink {
4645
string command;
47-
string argument;
4846

4947
ArgumentInjectionFromCommandSink() {
5048
exists(CommandSource source, Run run |
5149
run = source.getEnclosingRun() and
5250
this.asExpr() = run.getScript() and
53-
run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, argument)
51+
run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, _)
5452
)
5553
}
5654

‎actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll‎

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,6 @@ class LegitLabsDownloadArtifactActionStep extends UntrustedArtifactDownloadStep,
125125
}
126126

127127
class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, UsesStep {
128-
string script;
129-
130128
ActionsGitHubScriptDownloadStep() {
131129
// eg:
132130
// - uses: actions/github-script@v6
@@ -149,12 +147,14 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
149147
// var fs = require('fs');
150148
// fs.writeFileSync('${{github.workspace}}/test-results.zip', Buffer.from(download.data));
151149
this.getCallee() = "actions/github-script" and
152-
this.getArgument("script") = script and
153-
script.matches("%listWorkflowRunArtifacts(%") and
154-
script.matches("%downloadArtifact(%") and
155-
script.matches("%writeFileSync(%") and
156-
// Filter out artifacts that were created by pull-request.
157-
not script.matches("%exclude_pull_requests: true%")
150+
exists(string script |
151+
this.getArgument("script") = script and
152+
script.matches("%listWorkflowRunArtifacts(%") and
153+
script.matches("%downloadArtifact(%") and
154+
script.matches("%writeFileSync(%") and
155+
// Filter out artifacts that were created by pull-request.
156+
not script.matches("%exclude_pull_requests: true%")
157+
)
158158
}
159159

160160
override string getPath() {
@@ -171,10 +171,10 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
171171
.getScript()
172172
.getACommand()
173173
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
174-
else
175-
if this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp())
176-
then result = "GITHUB_WORKSPACE/"
177-
else none()
174+
else (
175+
this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) and
176+
result = "GITHUB_WORKSPACE/"
177+
)
178178
}
179179
}
180180

@@ -207,12 +207,13 @@ class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
207207
.getScript()
208208
.getACommand()
209209
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
210-
else
211-
if
210+
else (
211+
(
212212
this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) or
213213
this.getScript().getACommand().regexpMatch(unzipRegexp())
214-
then result = "GITHUB_WORKSPACE/"
215-
else none()
214+
) and
215+
result = "GITHUB_WORKSPACE/"
216+
)
216217
}
217218
}
218219

@@ -259,15 +260,15 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
259260

260261
class ArtifactPoisoningSink extends DataFlow::Node {
261262
UntrustedArtifactDownloadStep download;
262-
PoisonableStep poisonable;
263263

264264
ArtifactPoisoningSink() {
265-
download.getAFollowingStep() = poisonable and
266-
// excluding artifacts downloaded to the temporary directory
267-
not download.getPath().regexpMatch("^/tmp.*") and
268-
not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
269-
not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*") and
270-
(
265+
exists(PoisonableStep poisonable |
266+
download.getAFollowingStep() = poisonable and
267+
// excluding artifacts downloaded to the temporary directory
268+
not download.getPath().regexpMatch("^/tmp.*") and
269+
not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
270+
not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*")
271+
|
271272
poisonable.(Run).getScript() = this.asExpr() and
272273
(
273274
// Check if the poisonable step is a local script execution step

‎actions/ql/lib/codeql/actions/security/ControlChecks.qll‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,11 +159,8 @@ abstract class CommentVsHeadDateCheck extends ControlCheck {
159159

160160
/* Specific implementations of control checks */
161161
class LabelIfCheck extends LabelCheck instanceof If {
162-
string condition;
163-
164162
LabelIfCheck() {
165-
condition = normalizeExpr(this.getCondition()) and
166-
(
163+
exists(string condition | condition = normalizeExpr(this.getCondition()) |
167164
// eg: contains(github.event.pull_request.labels.*.name, 'safe to test')
168165
condition.regexpMatch(".*(^|[^!])contains\\(\\s*github\\.event\\.pull_request\\.labels\\b.*")
169166
or

‎actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,8 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
5555
* echo "COMMIT_MESSAGE=${COMMIT_MESSAGE}" >> $GITHUB_ENV
5656
*/
5757
class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
58-
CommandSource inCommand;
59-
string injectedVar;
60-
string command;
61-
6258
EnvVarInjectionFromCommandSink() {
63-
exists(Run run |
59+
exists(Run run, CommandSource inCommand, string injectedVar, string command |
6460
this.asExpr() = inCommand.getEnclosingRun().getScript() and
6561
run = inCommand.getEnclosingRun() and
6662
run.getScript().getACmdReachingGitHubEnvWrite(inCommand.getCommand(), injectedVar) and
@@ -86,12 +82,8 @@ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
8682
* echo "FOO=$BODY" >> $GITHUB_ENV
8783
*/
8884
class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink {
89-
string inVar;
90-
string injectedVar;
91-
string command;
92-
9385
EnvVarInjectionFromEnvVarSink() {
94-
exists(Run run |
86+
exists(Run run, string inVar, string injectedVar, string command |
9587
run.getScript() = this.asExpr() and
9688
exists(run.getInScopeEnvVarExpr(inVar)) and
9789
run.getScript().getAnEnvReachingGitHubEnvWrite(inVar, injectedVar) and

0 commit comments

Comments
 (0)