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

Add support for blocking specific app/version/label combinations #85

Merged
merged 6 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions changelog.d/85.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for blocking specific app/version/label combinations.
66 changes: 65 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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"`
kegsay marked this conversation as resolved.
Show resolved Hide resolved

// A GitHub personal access token, to create a GitHub issue for each report.
GithubToken string `yaml:"github_token"`

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
121 changes: 121 additions & 0 deletions main_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
12 changes: 12 additions & 0 deletions rageshake.sample.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
kegsay marked this conversation as resolved.
Show resolved Hide resolved
# The 'app' field is required, but 'version' and 'label' are both optional. A condition with just
# an 'app' will reject those apps, effectively acting as a blocklist for app in contrast to allowed_app_names.
kegsay marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
56 changes: 34 additions & 22 deletions submit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking comment: any idea why this is happening? Has gofmt changed the rules about struct formatting?I'm not convinced that the updated style is an improvement, but I don't care that much as long as we're not just going to flip-flop between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure why this is happening tbh. Empirically, this is what go fmt . does, so it's not an IDE thing.

go version
go version go1.22.1 darwin/arm64

Copy link
Member

@richvdh richvdh May 10, 2024

Choose a reason for hiding this comment

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

Empirically, this is what go fmt . does,

same for me. weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://tip.golang.org/doc/go1.19#go-doc are you on an old version?

Copy link
Member

Choose a reason for hiding this comment

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

No, I'm saying that I'm on go1.22, and it does the same as you. It's entirely possible the current formatting came from an earlier version of go. Thanks for hunting down the changes.

// Complete link to the listing URL that contains all uploaded logs
ListingURL string `json:"listing_url"`
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
kegsay marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Expand Down Expand Up @@ -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
Expand Down
Loading