Merge pull request #85 from matrix-org/kegan/reject-app-versions

Add support for blocking specific app/version/label combinations

changelog.d/85.feature

@@ -0,0 +1 @@
+Add support for blocking specific app/version/label combinations.

main.go

@@ -36,8 +36,9 @@ import (
 	"golang.org/x/oauth2"
 
 	"gopkg.in/yaml.v2"
+
+	_ "embed"
 )
-import _ "embed"
 
 // DefaultIssueBodyTemplate is the default template used for `issue_body_template_file` in the config.
 //
@@ -63,6 +64,9 @@ type config struct {
 	// Allowed rageshake app names
 	AllowedAppNames []string `yaml:"allowed_app_names"`
 
+	// List of rejection conditions
+	RejectionConditions []RejectionCondition `yaml:"rejection_conditions"`
+
 	// A GitHub personal access token, to create a GitHub issue for each report.
 	GithubToken string `yaml:"github_token"`
 
@@ -93,6 +97,57 @@ type config struct {
 	GenericWebhookURLs []string `yaml:"generic_webhook_urls"`
 }
 
+// RejectionCondition contains the fields that should match a bug report for it to be rejected.
+type RejectionCondition struct {
+	// Required field: if a payload does not match this app name, the condition does not match.
+	App string `yaml:"app"`
+	// Optional: version that must also match in addition to the app and label. If empty, does not check version.
+	Version string `yaml:"version"`
+	// Optional: label that must also match in addition to the app and version. If empty, does not check label.
+	Label string `yaml:"label"`
+}
+
+// shouldReject returns true if the app name AND version AND labels all match the rejection condition.
+// If any one of these do not match the condition, it is not rejected.
+func (c RejectionCondition) shouldReject(appName, version string, labels []string) bool {
+	if appName != c.App {
+		return false
+	}
+	// version was a condition and it doesn't match => accept it
+	if version != c.Version && c.Version != "" {
+		return false
+	}
+
+	// label was a condition and no label matches it => accept it
+	if c.Label != "" {
+		labelMatch := false
+		for _, l := range labels {
+			if l == c.Label {
+				labelMatch = true
+				break
+			}
+		}
+		if !labelMatch {
+			return false
+		}
+	}
+
+	return true
+}
+
+func (c *config) matchesRejectionCondition(p *payload) bool {
+	for _, rc := range c.RejectionConditions {
+		version := ""
+		if p.Data != nil {
+			version = p.Data["Version"]
+		}
+		if rc.shouldReject(p.AppName, version, p.Labels) {
+			return true
+		}
+	}
+	return false
+}
+
 func basicAuth(handler http.Handler, username, password, realm string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		user, pass, ok := r.BasicAuth() // pull creds from the request
@@ -264,5 +319,14 @@ func loadConfig(configPath string) (*config, error) {
 	if err = yaml.Unmarshal(contents, &cfg); err != nil {
 		return nil, err
 	}
+	// sanity check rejection conditions
+	for _, rc := range cfg.RejectionConditions {
+		if rc.App == "" {
+			fmt.Println("rejection_condition missing an app field so will never match anything.")
+		}
+		if rc.Label == "" && rc.Version == "" {
+			fmt.Println("rejection_condition missing both label and version so will always match, specify label and/or version")
+		}
+	}
 	return &cfg, nil
 }

main_test.go

@@ -0,0 +1,121 @@
+package main
+
+import "testing"
+
+func TestConfigRejectionCondition(t *testing.T) {
+	cfg := config{
+		RejectionConditions: []RejectionCondition{
+			{
+				App:     "my-app",
+				Version: "0.1.0",
+			},
+			{
+				App:   "my-app",
+				Label: "0.1.1",
+			},
+			{
+				App:     "my-app",
+				Version: "0.1.2",
+				Label:   "nightly",
+			},
+			{
+				App: "block-my-app",
+			},
+		},
+	}
+	rejectPayloads := []payload{
+		{
+			AppName: "my-app",
+			Data: map[string]string{
+				"Version": "0.1.0",
+			},
+		},
+		{
+			AppName: "my-app",
+			Data:    map[string]string{},
+			Labels:  []string{"0.1.1"},
+		},
+		{
+			AppName: "my-app",
+			Labels:  []string{"foo", "nightly"},
+			Data: map[string]string{
+				"Version": "0.1.2",
+			},
+		},
+		{
+			AppName: "block-my-app",
+		},
+		{
+			AppName: "block-my-app",
+			Labels:  []string{"foo"},
+		},
+		{
+			AppName: "block-my-app",
+			Data: map[string]string{
+				"Version": "42",
+			},
+		},
+		{
+			AppName: "block-my-app",
+			Labels:  []string{"foo"},
+			Data: map[string]string{
+				"Version": "42",
+			},
+		},
+	}
+	for _, p := range rejectPayloads {
+		if !cfg.matchesRejectionCondition(&p) {
+			t.Errorf("payload was accepted when it should be rejected:\n payload=%+v\nconfig=%+v", p, cfg)
+		}
+	}
+	acceptPayloads := []payload{
+		{
+			AppName: "different-app",
+			Data: map[string]string{
+				"Version": "0.1.0",
+			},
+		},
+		{
+			AppName: "different-app",
+			Data:    map[string]string{},
+			Labels:  []string{"0.1.1"},
+		},
+		{
+			AppName: "different-app",
+			Labels:  []string{"foo", "nightly"},
+			Data: map[string]string{
+				"Version": "0.1.2",
+			},
+		},
+		{
+			AppName: "my-app",
+			Data: map[string]string{
+				"Version": "0.1.0-suffix",
+			},
+		},
+		{
+			AppName: "my-app",
+			Data:    map[string]string{},
+			Labels:  []string{"0.1.1-suffix"},
+		},
+		{
+			AppName: "my-app",
+			Labels:  []string{"foo", "nightly-suffix"},
+			Data: map[string]string{
+				"Version": "0.1.2",
+			},
+		},
+		{ // version matches but label does not (it's Label AND Version not OR)
+			AppName: "my-app",
+			Labels:  []string{"foo"},
+			Data: map[string]string{
+				"Version": "0.1.2",
+			},
+		},
+	}
+	for _, p := range acceptPayloads {
+		if cfg.matchesRejectionCondition(&p) {
+			t.Errorf("payload was rejected when it should be accepted:\n payload=%+v\nconfig=%+v", p, cfg)
+		}
+	}
+}

rageshake.sample.yaml

@@ -12,6 +12,18 @@ listings_auth_pass: secret
 # An empty or missing list will retain legacy behaviour and permit reports from any application name.
 allowed_app_names: []
 
+# If any submission matches one of these rejection conditions, the submission is rejected.
+# The 'app' field is required, but 'version' and 'label' are both optional. A condition with just
+# an 'app' will reject that app altogether, effectively acting as a blocklist for app in contrast to allowed_app_names.
+rejection_conditions:
+  - app: my-app
+    version: "0.4.9" # if the submission has a Version which is exactly this value, reject the submission.
+  - app: my-app
+    label: "0.4.9" # if any label matches this value, the submission is rejected.
+  - app: my-app
+    version: "0.4.9"
+    label: "nightly" # both label and Version must match for this condition to be true
+
 # a GitHub personal access token (https://github.com/settings/tokens), which
 # will be used to create a GitHub issue for each report. It requires
 # `public_repo` scope. If omitted, no issues will be created.

submit.go

@@ -99,7 +99,7 @@ type issueBodyTemplatePayload struct {
 type genericWebhookPayload struct {
 	payload
 	// If a github/gitlab report is generated, this is set.
-	ReportURL  string `json:"report_url"`
+	ReportURL string `json:"report_url"`
 	// Complete link to the listing URL that contains all uploaded logs
 	ListingURL string `json:"listing_url"`
 }
@@ -109,24 +109,24 @@ type genericWebhookPayload struct {
 // !!! Since this is inherited by `issueBodyTemplatePayload`, remember to keep it in step
 // with the documentation in `templates/README.md` !!!
 type payload struct {
-	// A unique ID for this payload, generated within this server 
-	ID         string            `json:"id"`
-	// A multi-line string containing the user description of the fault. 
-	UserText   string            `json:"user_text"`
+	// A unique ID for this payload, generated within this server
+	ID string `json:"id"`
+	// A multi-line string containing the user description of the fault.
+	UserText string `json:"user_text"`
 	// A short slug to identify the app making the report
-	AppName    string            `json:"app"`
+	AppName string `json:"app"`
 	// Arbitrary data to annotate the report
-	Data       map[string]string `json:"data"`
+	Data map[string]string `json:"data"`
 	// Short labels to group reports
-	Labels     []string          `json:"labels"`
+	Labels []string `json:"labels"`
 	// A list of names of logs recognised by the server
-	Logs       []string          `json:"logs"`
+	Logs []string `json:"logs"`
 	// Set if there are log parsing errors
-	LogErrors  []string          `json:"logErrors"`
+	LogErrors []string `json:"logErrors"`
 	// A list of other files (not logs) uploaded as part of the rageshake
-	Files      []string          `json:"files"`
+	Files []string `json:"files"`
 	// Set if there are file parsing errors
-	FileErrors []string          `json:"fileErrors"`
+	FileErrors []string `json:"fileErrors"`
 }
 
 func (p payload) WriteTo(out io.Writer) {
@@ -183,7 +183,10 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		respond(200, w)
 		return
 	}
+	s.handleSubmission(w, req)
+}
 
+func (s *submitServer) handleSubmission(w http.ResponseWriter, req *http.Request) {
 	// create the report dir before parsing the request, so that we can dump
 	// files straight in
 	t := time.Now().UTC()
@@ -222,6 +225,15 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		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)
 		return
 	}
+	if s.cfg.matchesRejectionCondition(p) {
+		log.Printf("Blocking rageshake from app %s because it matches a rejection_condition", p.AppName)
+		if err := os.RemoveAll(reportDir); err != nil {
+			log.Printf("Unable to remove report dir %s after rejected upload: %v\n",
+				reportDir, err)
+		}
+		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)
+		return
+	}
 
 	// We use this prefix (eg, 2022-05-01/125223-abcde) as a unique identifier for this rageshake.
 	// 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) {
 
 // we use a quite restrictive regexp for the filenames; in particular:
 //
-// * a limited set of extensions. We are careful to limit the content-types
-//   we will serve the files with, but somebody might accidentally point an
-//   Apache or nginx at the upload directory, which would serve js files as
-//   application/javascript and open XSS vulnerabilities. We also allow gzipped
-//   text and json on the same basis (there's really no sense allowing gzipped images).
+//   - a limited set of extensions. We are careful to limit the content-types
+//     we will serve the files with, but somebody might accidentally point an
+//     Apache or nginx at the upload directory, which would serve js files as
+//     application/javascript and open XSS vulnerabilities. We also allow gzipped
+//     text and json on the same basis (there's really no sense allowing gzipped images).
 //
-// * no silly characters (/, ctrl chars, etc)
+//   - no silly characters (/, ctrl chars, etc)
 //
-// * nothing starting with '.'
+//   - nothing starting with '.'
 var filenameRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]+\.(jpg|png|txt|json|txt\.gz|json\.gz)$`)
 
 // saveFormPart saves a file upload to the report directory.
@@ -551,9 +563,9 @@ func (s *submitServer) submitGenericWebhook(p payload, listingURL string, report
 		return nil
 	}
 	genericHookPayload := genericWebhookPayload{
-		payload: p,
-		ReportURL:     reportURL,
-		ListingURL:    listingURL,
+		payload:    p,
+		ReportURL:  reportURL,
+		ListingURL: listingURL,
 	}
 	for _, url := range s.cfg.GenericWebhookURLs {
 		// Enrich the payload with a reportURL and listingURL, to convert a single struct