diff --git a/changelog.d/85.feature b/changelog.d/85.feature new file mode 100644 index 0000000..0e597c1 --- /dev/null +++ b/changelog.d/85.feature @@ -0,0 +1 @@ +Add support for blocking specific app/version/label combinations. diff --git a/main.go b/main.go index ffe3636..ae39e07 100644 --- a/main.go +++ b/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 } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..bd5e84a --- /dev/null +++ b/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) + } + } +} diff --git a/rageshake.sample.yaml b/rageshake.sample.yaml index 204a919..f567af6 100644 --- a/rageshake.sample.yaml +++ b/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. diff --git a/submit.go b/submit.go index ae68edb..746b7f7 100644 --- a/submit.go +++ b/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