diff --git a/changelog.d/88.feature b/changelog.d/88.feature new file mode 100644 index 0000000..a7cb866 --- /dev/null +++ b/changelog.d/88.feature @@ -0,0 +1 @@ +Reject user text (problem description) matching a regex and send the reason why to the client-side. diff --git a/docs/blocked_rageshake.md b/docs/blocked_rageshake.md index a39a876..c9b8ff8 100644 --- a/docs/blocked_rageshake.md +++ b/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 diff --git a/main.go b/main.go index ae39e07..f69b5a9 100644 --- a/main.go +++ b/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 } diff --git a/main_test.go b/main_test.go index bd5e84a..b3618fc 100644 --- a/main_test.go +++ b/main_test.go @@ -17,10 +17,15 @@ 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{ @@ -28,43 +33,70 @@ func TestConfigRejectionCondition(t *testing.T) { 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) } } diff --git a/rageshake.sample.yaml b/rageshake.sample.yaml index f567af6..c923595 100644 --- a/rageshake.sample.yaml +++ b/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 diff --git a/submit.go b/submit.go index 1ef5c6a..9a91017 100644 --- a/submit.go +++ b/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 }