Skip to content

Commit 39ff3c7

Browse files
JarLobJarLob
committed
Remove label sanitizer because it is prone to race conditions
1 parent 8ea4182 commit 39ff3c7

File tree

2 files changed

+10
-42
lines changed

2 files changed

+10
-42
lines changed

‎javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql‎

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import javascript
1717
import semmle.javascript.Actions
1818

1919
/**
20-
* An action step that doesn't contain `actor` or `label` check in `if:` or
20+
* An action step that doesn't contain `actor` check in `if:` or
2121
* the check requires manual analysis.
2222
*/
2323
class ProbableStep extends Actions::Step {
@@ -29,25 +29,13 @@ class ProbableStep extends Actions::Step {
2929
// needs manual analysis if there is OR
3030
this.getIf().getValue().matches("%||%")
3131
or
32-
// labels can be assigned by owners only
33-
not exists(
34-
this.getIf()
35-
.getValue()
36-
.regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
37-
_, _)
38-
) and
39-
not exists(
40-
this.getIf()
41-
.getValue()
42-
.regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
43-
) and
4432
// actor check means only the user is able to run it
4533
not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
4634
}
4735
}
4836

4937
/**
50-
* An action job that doesn't contain `actor` or `label` check in `if:` or
38+
* An action job that doesn't contain `actor` check in `if:` or
5139
* the check requires manual analysis.
5240
*/
5341
class ProbableJob extends Actions::Job {
@@ -59,45 +47,19 @@ class ProbableJob extends Actions::Job {
5947
// needs manual analysis if there is OR
6048
this.getIf().getValue().matches("%||%")
6149
or
62-
// labels can be assigned by owners only
63-
not exists(
64-
this.getIf()
65-
.getValue()
66-
.regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
67-
_, _)
68-
) and
69-
not exists(
70-
this.getIf()
71-
.getValue()
72-
.regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
73-
) and
7450
// actor check means only the user is able to run it
7551
not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
7652
}
7753
}
7854

7955
/**
80-
* An action step that doesn't contain `actor` or `label` check in `if:` or
56+
* on: pull_request_target
8157
*/
8258
class ProbablePullRequestTarget extends Actions::On, YamlMappingLikeNode {
8359
ProbablePullRequestTarget() {
8460
exists(YamlNode prtNode |
8561
// The `on:` is triggered on `pull_request_target`
86-
this.getNode("pull_request_target") = prtNode and
87-
(
88-
// and either doesn't contain `types` filter
89-
not exists(prtNode.getAChild())
90-
or
91-
// or has the filter, that is something else than just [labeled]
92-
exists(YamlMappingLikeNode prt, YamlMappingLikeNode types |
93-
types = prt.getNode("types") and
94-
prtNode = prt and
95-
(
96-
not types.getElementCount() = 1 or
97-
not exists(types.getNode("labeled"))
98-
)
99-
)
100-
)
62+
this.getNode("pull_request_target") = prtNode
10163
)
10264
}
10365
}

‎javascript/ql/test/experimental/Security/CWE-094/UntrustedCheckout.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1+
| .github/workflows/pull_request_target_if_job.yml:9:7:12:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
2+
| .github/workflows/pull_request_target_if_job.yml:16:7:19:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
13
| .github/workflows/pull_request_target_if_job.yml:30:7:33:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
24
| .github/workflows/pull_request_target_if_job.yml:36:7:38:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
5+
| .github/workflows/pull_request_target_if_step.yml:9:7:14:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
6+
| .github/workflows/pull_request_target_if_step.yml:14:7:19:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
37
| .github/workflows/pull_request_target_if_step.yml:24:7:29:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
48
| .github/workflows/pull_request_target_if_step.yml:29:7:31:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
9+
| .github/workflows/pull_request_target_label_only.yml:10:7:12:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
10+
| .github/workflows/pull_request_target_label_only_mapping.yml:11:7:13:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
511
| .github/workflows/pull_request_target_labels_mapping.yml:13:7:15:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
612
| .github/workflows/pull_request_target_labels_sequence.yml:10:7:12:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
713
| .github/workflows/pull_request_target_mapping.yml:8:7:10:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |

0 commit comments

Comments
 (0)