Reject user texts matching a regex and provide the reason to the client-side

changelog.d/88.feature

@@ -0,0 +1 @@
+Reject user text (problem description) matching a regex and send the reason why to the client-side.

docs/blocked_rageshake.md

@@ -14,6 +14,8 @@ The rageshake server you attempted to upload a report to is not accepting ragesh
 Generally, the developers who run a rageshake server will only be able to handle reports for applications they are developing,
 and your application is not listed as one of those applications.
 
+The rageshake server could also be rejecting the text you wrote in your bug report because its content matches a rejection rule. This usually happens to prevent you from disclosing private information in the bug report itself.
+
 Please contact the distributor of your application or the administrator of the web site you visit to report this as a problem.
 
 ## For developers of matrix clients

main.go

@@ -27,6 +27,7 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"regexp"
 	"strings"
 	"text/template"
 	"time"
@@ -97,55 +98,73 @@ type config struct {
 	GenericWebhookURLs []string `yaml:"generic_webhook_urls"`
 }
 
-// RejectionCondition contains the fields that should match a bug report for it to be rejected.
+// RejectionCondition contains the fields that can match a bug report for it to be rejected.
+// All the (optional) fields must match for the rejection condition to apply
 type RejectionCondition struct {
-	// Required field: if a payload does not match this app name, the condition does not match.
+	// App name, checked only if not empty
 	App string `yaml:"app"`
-	// Optional: version that must also match in addition to the app and label. If empty, does not check version.
+	// Version, checked only if not empty
 	Version string `yaml:"version"`
-	// Optional: label that must also match in addition to the app and version. If empty, does not check label.
+	// Label, checked only if not empty
 	Label string `yaml:"label"`
+	// Message sent by the user, checked only if not empty
+	UserTextMatch string `yaml:"usertext"`
+	// Send this text to the client-side to inform the user why the server rejects the rageshake. Uses a default generic value if empty.
+	Reason string `yaml:"reason"`
 }
 
-// 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
+// shouldReject returns a rejection reason if the payload matches the condition, nil if the server should accept the rageshake
+func (c RejectionCondition) shouldReject(p *payload) (reason *string) {
+	defaultReason := "app or user text rejected"
+	if c.Reason != "" {
+		reason = &c.Reason
+	} else {
+		reason = &defaultReason
 	}
-	// version was a condition and it doesn't match => accept it
-	if version != c.Version && c.Version != "" {
-		return false
+	// Check all the non-empty attributes of the RejectionCondition
+	if c.App != "" && c.App != p.AppName {
+		reason = nil
+		return
+	}
+	version := ""
+	if p.Data != nil {
+		version = p.Data["Version"]
+	}
+	if c.Version != "" && c.Version != version {
+		reason = nil
+		return
 	}
-
-	// label was a condition and no label matches it => accept it
 	if c.Label != "" {
 		labelMatch := false
-		for _, l := range labels {
+		for _, l := range p.Labels {
 			if l == c.Label {
 				labelMatch = true
 				break
 			}
 		}
 		if !labelMatch {
-			return false
+			reason = nil
+			return
 		}
 	}
-
-	return true
+	if c.UserTextMatch != "" {
+		var userTextRegexp = regexp.MustCompile(c.UserTextMatch)
+		if !userTextRegexp.MatchString(p.UserText) {
+			reason = nil
+			return
+		}
+	}
+	return
 }
 
-func (c *config) matchesRejectionCondition(p *payload) bool {
+func (c *config) matchesRejectionCondition(p *payload) (reason *string) {
 	for _, rc := range c.RejectionConditions {
-		version := ""
-		if p.Data != nil {
-			version = p.Data["Version"]
-		}
-		if rc.shouldReject(p.AppName, version, p.Labels) {
-			return true
+		reject := rc.shouldReject(p)
+		if reject != nil {
+			return rc.shouldReject(p)
 		}
 	}
-	return false
+	return nil
 }
 
 func basicAuth(handler http.Handler, username, password, realm string) http.Handler {
@@ -319,14 +338,5 @@ 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

@@ -17,54 +17,86 @@ func TestConfigRejectionCondition(t *testing.T) {
 				App:     "my-app",
 				Version: "0.1.2",
 				Label:   "nightly",
+				Reason:  "no nightlies",
 			},
 			{
 				App: "block-my-app",
 			},
+			{
+				UserTextMatch: "(\\w{4}\\s){11}\\w{4}",
+				Reason:        "it matches a recovery key and recovery keys are private",
+			},
 		},
 	}
 	rejectPayloads := []payload{
 		{
 			AppName: "my-app",
 			Data: map[string]string{
 				"Version": "0.1.0",
+				// Hack add how we expect the rageshake to be rejected to the test
+				// The actual data in a rageshake has no ExpectedRejectReason field
+				"ExpectedRejectReason": "app or user text rejected",
 			},
 		},
 		{
 			AppName: "my-app",
-			Data:    map[string]string{},
-			Labels:  []string{"0.1.1"},
+			Data: map[string]string{
+				"ExpectedRejectReason": "app or user text rejected",
+			},
+			Labels: []string{"0.1.1"},
 		},
 		{
 			AppName: "my-app",
 			Labels:  []string{"foo", "nightly"},
 			Data: map[string]string{
-				"Version": "0.1.2",
+				"Version":              "0.1.2",
+				"ExpectedRejectReason": "no nightlies",
 			},
 		},
 		{
 			AppName: "block-my-app",
+			Data: map[string]string{
+				"ExpectedRejectReason": "app or user text rejected",
+			},
 		},
 		{
 			AppName: "block-my-app",
 			Labels:  []string{"foo"},
+			Data: map[string]string{
+				"ExpectedRejectReason": "app or user text rejected",
+			},
 		},
 		{
 			AppName: "block-my-app",
 			Data: map[string]string{
-				"Version": "42",
+				"Version":              "42",
+				"ExpectedRejectReason": "app or user text rejected",
 			},
 		},
 		{
 			AppName: "block-my-app",
 			Labels:  []string{"foo"},
 			Data: map[string]string{
-				"Version": "42",
+				"Version":              "42",
+				"ExpectedRejectReason": "app or user text rejected",
+			},
+		},
+		{
+			AppName:  "my-app",
+			UserText: "Looks like a recover key abcd abcd abcd abcd abcd abcd abcd abcd abcd abcd abcd abcd",
+			Data: map[string]string{
+				"ExpectedRejectReason": "it matches a recovery key and recovery keys are private",
 			},
 		},
 	}
 	for _, p := range rejectPayloads {
-		if !cfg.matchesRejectionCondition(&p) {
+		reject := cfg.matchesRejectionCondition(&p)
+		if reject != nil {
+			if *reject != p.Data["ExpectedRejectReason"] {
+				t.Errorf("payload was rejected with the wrong reason:\n payload=%+v\nconfig=%+v", p, cfg)
+			}
+		}
+		if reject == nil {
 			t.Errorf("payload was accepted when it should be rejected:\n payload=%+v\nconfig=%+v", p, cfg)
 		}
 	}
@@ -112,9 +144,14 @@ func TestConfigRejectionCondition(t *testing.T) {
 				"Version": "0.1.2",
 			},
 		},
+		{
+			AppName:  "my-app",
+			UserText: "Some description",
+		},
 	}
 	for _, p := range acceptPayloads {
-		if cfg.matchesRejectionCondition(&p) {
+		reject := cfg.matchesRejectionCondition(&p)
+		if reject != nil {
 			t.Errorf("payload was rejected when it should be accepted:\n payload=%+v\nconfig=%+v", p, cfg)
 		}
 	}

rageshake.sample.yaml

@@ -13,8 +13,8 @@ listings_auth_pass: secret
 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.
+# A condition is made by an union of optional fields: app, version, labels, user text. They all need to match for rejecting the rageshake
+# It can also contain an optional reason to explain why this server is rejecting a user's submission.
 rejection_conditions:
   - app: my-app
     version: "0.4.9" # if the submission has a Version which is exactly this value, reject the submission.
@@ -23,6 +23,9 @@ rejection_conditions:
   - app: my-app
     version: "0.4.9"
     label: "nightly" # both label and Version must match for this condition to be true
+    reason: "this server does not accept rageshakes from nightlies"
+  - usertext: "(\\w{4}\\s){11}\\w{4}" # reject text containing possible recovery keys
+    reason: "it matches a recovery key and recovery keys are private"
 
 # a GitHub personal access token (https://github.com/settings/tokens), which
 # will be used to create a GitHub issue for each report. It requires

submit.go

@@ -226,13 +226,15 @@ func (s *submitServer) handleSubmission(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)
+	rejection := s.cfg.matchesRejectionCondition(p)
+	if rejection != nil {
+		log.Printf("Blocking rageshake from app %s because it matches a rejection_condition: %s", p.AppName, *rejection)
 		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)
+		userErrorText := fmt.Sprintf("This server did not accept the rageshake because it matches a rejection condition: %s. See https://github.com/matrix-org/rageshake/blob/master/docs/blocked_rageshake.md", *rejection)
+		http.Error(w, userErrorText, 400)
 		return
 	}