Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 49 additions & 26 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"net"
"net/http"
"os"
"regexp"
"strings"
"text/template"
"time"
Expand Down Expand Up @@ -98,54 +99,76 @@ type config struct {
}

// RejectionCondition contains the fields that should match a bug report for it to be rejected.
// The condition matches a specific app, with optionall version and label, or the user submitted text
davidegirardi marked this conversation as resolved.
Show resolved Hide resolved
type RejectionCondition struct {
// Required field: if a payload does not match this app name, the condition does not match.
// If a payload is not a UserTextMatch and does not match this app name, the condition does not match.
davidegirardi marked this conversation as resolved.
Show resolved Hide resolved
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"`
// Alternative to matching app names, match the content of the user text
UserTextMatch string `yaml:"usertext"`
UserTextMatchReason string `yaml:"reason"`
davidegirardi marked this conversation as resolved.
Show resolved Hide resolved
}

// shouldReject returns true if the app name AND version AND labels all match the rejection condition.
// shouldReject returns true in two situations:
// - if the app name AND version AND labels all match the rejection condition.
// - if the text provided by the user matches a regex specified in 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
func (c RejectionCondition) shouldReject(appName, version string, labels []string, userText string) (reject bool, reason string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I've been writing Rust too long, but I don't love the fact that (false, "blah") is a meaningless response.

Could maybe just return *string here, with nil meaning "do not reject" and anything else being the reason to reject.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in afd59fd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is fixed. shouldReject and matchesRejectionCondition both still return a bool and a *string, meaning that they can return nonsensical things like (false, "blah").

Recommend making them return just *string.

if c.App != "" {
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, ""
}
}
if !labelMatch {
return false
return true, "this application is not allowed to send rageshakes to this server"
}
if c.UserTextMatch != "" {
var userTextRegexp = regexp.MustCompile(c.UserTextMatch)
if userTextRegexp.MatchString(userText) {
fmt.Println("Match:", c.UserTextMatch)
fmt.Println("Reason:", c.UserTextMatchReason)
reason := c.UserTextMatchReason
if c.UserTextMatchReason == "" {
reason = "user text"
}
return true, reason
}
}

return true
// Nothing matches
return false, ""
}

func (c *config) matchesRejectionCondition(p *payload) bool {
func (c *config) matchesRejectionCondition(p *payload) (match bool, 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, reason := rc.shouldReject(p.AppName, version, p.Labels, p.UserText)
if reject {
return true, reason
}
}
return false
return false, ""
}

func basicAuth(handler http.Handler, username, password, realm string) http.Handler {
Expand Down
9 changes: 7 additions & 2 deletions rageshake.sample.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ 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.
# You can either match an 'app' name, with optional 'version' and 'label' or the 'usertext' submitted by the user when describing their issue.
#
# Adding an 'app' item will reject that app altogether, effectively acting as a blocklist for app in contrast to allowed_app_names. You can optionally add 'version' and 'label' to fine-tune the rejection condition
#
# 'usertext' is a regex to reject user descriptions with an optional 'reason' text to send to the client
rejection_conditions:
- app: my-app
version: "0.4.9" # if the submission has a Version which is exactly this value, reject the submission.
Expand All @@ -23,6 +26,8 @@ rejection_conditions:
- app: my-app
version: "0.4.9"
label: "nightly" # both label and Version must match for this condition to be true
- 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
Expand Down
8 changes: 5 additions & 3 deletions submit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
matchesRejection, reason := s.cfg.matchesRejectionCondition(p)
if matchesRejection {
log.Printf("Blocking rageshake from app %s because it matches a rejection_condition: %s", p.AppName, reason)
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", reason)
http.Error(w, userErrorText, 400)
return
}

Expand Down
Loading