Reject user texts matching a regex (#88)

Sometimes users send something they really should not in their rageshake descriptions. I propose here an approach to reject rageshakes whose description matches a regex. The rejection reason is reflected client-side so the user's client can show them want went wrong.

The sample config file provides a regex to reject user texts that could contain a recovery key.

Co-authored-by: Richard van der Hoff <[email protected]>

Author: davidegirardi

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,76 @@ 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, applies 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, applies 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, applies only if not empty
 	Label string `yaml:"label"`
+	// Message sent by the user, applies 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
-	}
-	// version was a condition and it doesn't match => accept it
-	if version != c.Version && c.Version != "" {
-		return false
+func (c RejectionCondition) matchesApp(p *payload) bool {
+	// Empty `RejectionCondition.App` is a wildcard which matches anything
+	return c.App == "" || c.App == p.AppName
+}
+
+func (c RejectionCondition) matchesVersion(p *payload) bool {
+	version := ""
+	if p.Data != nil {
+		version = p.Data["Version"]
 	}
+	// Empty `RejectionCondition.Version` is a wildcard which matches anything
+	return c.Version == "" || c.Version == version
+}
 
-	// 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
+func (c RejectionCondition) matchesLabel(p *payload) bool {
+	// Empty `RejectionCondition.Label` is a wildcard which matches anything
+	if c.Label == "" {
+		return true
+	}
+	// Otherwise return true only if there is a label that matches
+	labelMatch := false
+	for _, l := range p.Labels {
+		if l == c.Label {
+			labelMatch = true
+			break
 		}
 	}
+	return labelMatch
+}
 
-	return true
+func (c RejectionCondition) matchesUserText(p *payload) bool {
+	// Empty `RejectionCondition.UserTextMatch` is a wildcard which matches anything
+	return c.UserTextMatch == "" || regexp.MustCompile(c.UserTextMatch).MatchString(p.UserText)
 }
 
-func (c *config) matchesRejectionCondition(p *payload) bool {
-	for _, rc := range c.RejectionConditions {
-		version := ""
-		if p.Data != nil {
-			version = p.Data["Version"]
+func (c RejectionCondition) shouldReject(p *payload) *string {
+	if c.matchesApp(p) && c.matchesVersion(p) && c.matchesLabel(p) && c.matchesUserText(p) {
+		// RejectionCondition matches all of the conditions: we should reject this submission/
+		defaultReason := "app or user text rejected"
+		if c.Reason != "" {
+			return &c.Reason
 		}
-		if rc.shouldReject(p.AppName, version, p.Labels) {
-			return true
+		return &defaultReason
+	}
+	return nil
+}
+
+func (c *config) matchesRejectionCondition(p *payload) *string {
+	for _, rc := range c.RejectionConditions {
+		reject := rc.shouldReject(p)
+		if reject != nil {
+			return reject
 		}
 	}
-	return false
+	return nil
 }
 
 func basicAuth(handler http.Handler, username, password, realm string) http.Handler {
@@ -319,14 +341,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
 	}