Skip to content

Commit 4e8622d

Browse files
kegsaykegsay
authored
Merge pull request #85 from matrix-org/kegan/reject-app-versions
Add support for blocking specific app/version/label combinations
2 parents ae1ce0c + 44cbed9 commit 4e8622d

File tree

5 files changed

+233
-23
lines changed

5 files changed

+233
-23
lines changed

Diff for: ‎changelog.d/85.feature

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Add support for blocking specific app/version/label combinations.

Diff for: ‎main.go

+65-1
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,9 @@ import (
3636
"golang.org/x/oauth2"
3737

3838
"gopkg.in/yaml.v2"
39+
40+
_ "embed"
3941
)
40-
import _ "embed"
4142

4243
// DefaultIssueBodyTemplate is the default template used for `issue_body_template_file` in the config.
4344
//
@@ -63,6 +64,9 @@ type config struct {
6364
// Allowed rageshake app names
6465
AllowedAppNames []string `yaml:"allowed_app_names"`
6566

67+
// List of rejection conditions
68+
RejectionConditions []RejectionCondition `yaml:"rejection_conditions"`
69+
6670
// A GitHub personal access token, to create a GitHub issue for each report.
6771
GithubToken string `yaml:"github_token"`
6872

@@ -93,6 +97,57 @@ type config struct {
9397
GenericWebhookURLs []string `yaml:"generic_webhook_urls"`
9498
}
9599

100+
// RejectionCondition contains the fields that should match a bug report for it to be rejected.
101+
type RejectionCondition struct {
102+
// Required field: if a payload does not match this app name, the condition does not match.
103+
App string `yaml:"app"`
104+
// Optional: version that must also match in addition to the app and label. If empty, does not check version.
105+
Version string `yaml:"version"`
106+
// Optional: label that must also match in addition to the app and version. If empty, does not check label.
107+
Label string `yaml:"label"`
108+
}
109+
110+
// shouldReject returns true if the app name AND version AND labels all match the rejection condition.
111+
// If any one of these do not match the condition, it is not rejected.
112+
func (c RejectionCondition) shouldReject(appName, version string, labels []string) bool {
113+
if appName != c.App {
114+
return false
115+
}
116+
// version was a condition and it doesn't match => accept it
117+
if version != c.Version && c.Version != "" {
118+
return false
119+
}
120+
121+
// label was a condition and no label matches it => accept it
122+
if c.Label != "" {
123+
labelMatch := false
124+
for _, l := range labels {
125+
if l == c.Label {
126+
labelMatch = true
127+
break
128+
}
129+
}
130+
if !labelMatch {
131+
return false
132+
}
133+
}
134+
135+
return true
136+
}
137+
138+
func (c *config) matchesRejectionCondition(p *payload) bool {
139+
for _, rc := range c.RejectionConditions {
140+
version := ""
141+
if p.Data != nil {
142+
version = p.Data["Version"]
143+
}
144+
if rc.shouldReject(p.AppName, version, p.Labels) {
145+
return true
146+
}
147+
}
148+
return false
149+
}
150+
96151
func basicAuth(handler http.Handler, username, password, realm string) http.Handler {
97152
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
98153
user, pass, ok := r.BasicAuth() // pull creds from the request
@@ -264,5 +319,14 @@ func loadConfig(configPath string) (*config, error) {
264319
if err = yaml.Unmarshal(contents, &cfg); err != nil {
265320
return nil, err
266321
}
322+
// sanity check rejection conditions
323+
for _, rc := range cfg.RejectionConditions {
324+
if rc.App == "" {
325+
fmt.Println("rejection_condition missing an app field so will never match anything.")
326+
}
327+
if rc.Label == "" && rc.Version == "" {
328+
fmt.Println("rejection_condition missing both label and version so will always match, specify label and/or version")
329+
}
330+
}
267331
return &cfg, nil
268332
}

Diff for: ‎main_test.go

+121
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package main
2+
3+
import "testing"
4+
5+
func TestConfigRejectionCondition(t *testing.T) {
6+
cfg := config{
7+
RejectionConditions: []RejectionCondition{
8+
{
9+
App: "my-app",
10+
Version: "0.1.0",
11+
},
12+
{
13+
App: "my-app",
14+
Label: "0.1.1",
15+
},
16+
{
17+
App: "my-app",
18+
Version: "0.1.2",
19+
Label: "nightly",
20+
},
21+
{
22+
App: "block-my-app",
23+
},
24+
},
25+
}
26+
rejectPayloads := []payload{
27+
{
28+
AppName: "my-app",
29+
Data: map[string]string{
30+
"Version": "0.1.0",
31+
},
32+
},
33+
{
34+
AppName: "my-app",
35+
Data: map[string]string{},
36+
Labels: []string{"0.1.1"},
37+
},
38+
{
39+
AppName: "my-app",
40+
Labels: []string{"foo", "nightly"},
41+
Data: map[string]string{
42+
"Version": "0.1.2",
43+
},
44+
},
45+
{
46+
AppName: "block-my-app",
47+
},
48+
{
49+
AppName: "block-my-app",
50+
Labels: []string{"foo"},
51+
},
52+
{
53+
AppName: "block-my-app",
54+
Data: map[string]string{
55+
"Version": "42",
56+
},
57+
},
58+
{
59+
AppName: "block-my-app",
60+
Labels: []string{"foo"},
61+
Data: map[string]string{
62+
"Version": "42",
63+
},
64+
},
65+
}
66+
for _, p := range rejectPayloads {
67+
if !cfg.matchesRejectionCondition(&p) {
68+
t.Errorf("payload was accepted when it should be rejected:\n payload=%+v\nconfig=%+v", p, cfg)
69+
}
70+
}
71+
acceptPayloads := []payload{
72+
{
73+
AppName: "different-app",
74+
Data: map[string]string{
75+
"Version": "0.1.0",
76+
},
77+
},
78+
{
79+
AppName: "different-app",
80+
Data: map[string]string{},
81+
Labels: []string{"0.1.1"},
82+
},
83+
{
84+
AppName: "different-app",
85+
Labels: []string{"foo", "nightly"},
86+
Data: map[string]string{
87+
"Version": "0.1.2",
88+
},
89+
},
90+
{
91+
AppName: "my-app",
92+
Data: map[string]string{
93+
"Version": "0.1.0-suffix",
94+
},
95+
},
96+
{
97+
AppName: "my-app",
98+
Data: map[string]string{},
99+
Labels: []string{"0.1.1-suffix"},
100+
},
101+
{
102+
AppName: "my-app",
103+
Labels: []string{"foo", "nightly-suffix"},
104+
Data: map[string]string{
105+
"Version": "0.1.2",
106+
},
107+
},
108+
{ // version matches but label does not (it's Label AND Version not OR)
109+
AppName: "my-app",
110+
Labels: []string{"foo"},
111+
Data: map[string]string{
112+
"Version": "0.1.2",
113+
},
114+
},
115+
}
116+
for _, p := range acceptPayloads {
117+
if cfg.matchesRejectionCondition(&p) {
118+
t.Errorf("payload was rejected when it should be accepted:\n payload=%+v\nconfig=%+v", p, cfg)
119+
}
120+
}
121+
}

Diff for: ‎rageshake.sample.yaml

+12
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ listings_auth_pass: secret
1212
# An empty or missing list will retain legacy behaviour and permit reports from any application name.
1313
allowed_app_names: []
1414

15+
# If any submission matches one of these rejection conditions, the submission is rejected.
16+
# The 'app' field is required, but 'version' and 'label' are both optional. A condition with just
17+
# an 'app' will reject that app altogether, effectively acting as a blocklist for app in contrast to allowed_app_names.
18+
rejection_conditions:
19+
- app: my-app
20+
version: "0.4.9" # if the submission has a Version which is exactly this value, reject the submission.
21+
- app: my-app
22+
label: "0.4.9" # if any label matches this value, the submission is rejected.
23+
- app: my-app
24+
version: "0.4.9"
25+
label: "nightly" # both label and Version must match for this condition to be true
26+
1527
# a GitHub personal access token (https://github.com/settings/tokens), which
1628
# will be used to create a GitHub issue for each report. It requires
1729
# `public_repo` scope. If omitted, no issues will be created.

Diff for: ‎submit.go

+34-22
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ type issueBodyTemplatePayload struct {
9999
type genericWebhookPayload struct {
100100
payload
101101
// If a github/gitlab report is generated, this is set.
102-
ReportURL string `json:"report_url"`
102+
ReportURL string `json:"report_url"`
103103
// Complete link to the listing URL that contains all uploaded logs
104104
ListingURL string `json:"listing_url"`
105105
}
@@ -109,24 +109,24 @@ type genericWebhookPayload struct {
109109
// !!! Since this is inherited by `issueBodyTemplatePayload`, remember to keep it in step
110110
// with the documentation in `templates/README.md` !!!
111111
type payload struct {
112-
// A unique ID for this payload, generated within this server
113-
ID string `json:"id"`
114-
// A multi-line string containing the user description of the fault.
115-
UserText string `json:"user_text"`
112+
// A unique ID for this payload, generated within this server
113+
ID string `json:"id"`
114+
// A multi-line string containing the user description of the fault.
115+
UserText string `json:"user_text"`
116116
// A short slug to identify the app making the report
117-
AppName string `json:"app"`
117+
AppName string `json:"app"`
118118
// Arbitrary data to annotate the report
119-
Data map[string]string `json:"data"`
119+
Data map[string]string `json:"data"`
120120
// Short labels to group reports
121-
Labels []string `json:"labels"`
121+
Labels []string `json:"labels"`
122122
// A list of names of logs recognised by the server
123-
Logs []string `json:"logs"`
123+
Logs []string `json:"logs"`
124124
// Set if there are log parsing errors
125-
LogErrors []string `json:"logErrors"`
125+
LogErrors []string `json:"logErrors"`
126126
// A list of other files (not logs) uploaded as part of the rageshake
127-
Files []string `json:"files"`
127+
Files []string `json:"files"`
128128
// Set if there are file parsing errors
129-
FileErrors []string `json:"fileErrors"`
129+
FileErrors []string `json:"fileErrors"`
130130
}
131131

132132
func (p payload) WriteTo(out io.Writer) {
@@ -183,7 +183,10 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
183183
respond(200, w)
184184
return
185185
}
186+
s.handleSubmission(w, req)
187+
}
186188

189+
func (s *submitServer) handleSubmission(w http.ResponseWriter, req *http.Request) {
187190
// create the report dir before parsing the request, so that we can dump
188191
// files straight in
189192
t := time.Now().UTC()
@@ -222,6 +225,15 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
222225
http.Error(w, "This server does not accept rageshakes from your application. See https://github.com/matrix-org/rageshake/blob/master/docs/blocked_rageshake.md", 400)
223226
return
224227
}
228+
if s.cfg.matchesRejectionCondition(p) {
229+
log.Printf("Blocking rageshake from app %s because it matches a rejection_condition", p.AppName)
230+
if err := os.RemoveAll(reportDir); err != nil {
231+
log.Printf("Unable to remove report dir %s after rejected upload: %v\n",
232+
reportDir, err)
233+
}
234+
http.Error(w, "This server does not accept rageshakes from your application + version. See https://github.com/matrix-org/rageshake/blob/master/docs/blocked_rageshake.md", 400)
235+
return
236+
}
225237

226238
// We use this prefix (eg, 2022-05-01/125223-abcde) as a unique identifier for this rageshake.
227239
// This is going to be used to uniquely identify rageshakes, even if they are not submitted to
@@ -422,15 +434,15 @@ func formPartToPayload(field, data string, p *payload) {
422434

423435
// we use a quite restrictive regexp for the filenames; in particular:
424436
//
425-
// * a limited set of extensions. We are careful to limit the content-types
426-
// we will serve the files with, but somebody might accidentally point an
427-
// Apache or nginx at the upload directory, which would serve js files as
428-
// application/javascript and open XSS vulnerabilities. We also allow gzipped
429-
// text and json on the same basis (there's really no sense allowing gzipped images).
437+
// - a limited set of extensions. We are careful to limit the content-types
438+
// we will serve the files with, but somebody might accidentally point an
439+
// Apache or nginx at the upload directory, which would serve js files as
440+
// application/javascript and open XSS vulnerabilities. We also allow gzipped
441+
// text and json on the same basis (there's really no sense allowing gzipped images).
430442
//
431-
// * no silly characters (/, ctrl chars, etc)
443+
// - no silly characters (/, ctrl chars, etc)
432444
//
433-
// * nothing starting with '.'
445+
// - nothing starting with '.'
434446
var filenameRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]+\.(jpg|png|txt|json|txt\.gz|json\.gz)$`)
435447

436448
// saveFormPart saves a file upload to the report directory.
@@ -551,9 +563,9 @@ func (s *submitServer) submitGenericWebhook(p payload, listingURL string, report
551563
return nil
552564
}
553565
genericHookPayload := genericWebhookPayload{
554-
payload: p,
555-
ReportURL: reportURL,
556-
ListingURL: listingURL,
566+
payload: p,
567+
ReportURL: reportURL,
568+
ListingURL: listingURL,
557569
}
558570
for _, url := range s.cfg.GenericWebhookURLs {
559571
// Enrich the payload with a reportURL and listingURL, to convert a single struct

0 commit comments

Comments
 (0)