From 7daaa06d53f0f496aa1a87656d16c81ebff37f73 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Thu, 22 Feb 2018 12:53:31 +0100 Subject: dashboard/app: restrict patch testing result CC list Currently dashboard sends patch testing result to full bug CC list (which includes kernel mailing lists). This is unnecessary and causes problems with patchwork. Reply only to people in the testing request CC list (adding our mailing list if it was missing). Fixes #526 --- dashboard/app/api.go | 9 +++++++++ dashboard/app/app_test.go | 5 +++-- dashboard/app/config_stub.go | 1 + dashboard/app/email_test.go | 6 ++++-- dashboard/app/entities.go | 1 + dashboard/app/jobs.go | 19 +++++++++--------- dashboard/app/jobs_test.go | 43 ++++++++++++++++++++++++++++------------ dashboard/app/reporting_email.go | 19 +++++++++--------- dashboard/app/util_test.go | 36 ++++++++++++++++++--------------- 9 files changed, 87 insertions(+), 52 deletions(-) (limited to 'dashboard') diff --git a/dashboard/app/api.go b/dashboard/app/api.go index 571812d89..335d7ad2f 100644 --- a/dashboard/app/api.go +++ b/dashboard/app/api.go @@ -412,6 +412,15 @@ func stringInList(list []string, str string) bool { return false } +func stringsInList(list, str []string) bool { + for _, s := range str { + if !stringInList(list, s) { + return false + } + } + return true +} + func apiReportBuildError(c context.Context, ns string, r *http.Request) (interface{}, error) { req := new(dashapi.BuildErrorReq) if err := json.NewDecoder(r.Body).Decode(req); err != nil { diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go index 88621cb34..0fe5c3f7b 100644 --- a/dashboard/app/app_test.go +++ b/dashboard/app/app_test.go @@ -74,8 +74,9 @@ var config = GlobalConfig{ Name: "reporting2", DailyLimit: 3, Config: &EmailConfig{ - Email: "bugs@syzkaller.com", - MailMaintainers: true, + Email: "bugs@syzkaller.com", + DefaultMaintainers: []string{"default@maintainers.com"}, + MailMaintainers: true, }, }, }, diff --git a/dashboard/app/config_stub.go b/dashboard/app/config_stub.go index 0c68453a8..932ebf324 100644 --- a/dashboard/app/config_stub.go +++ b/dashboard/app/config_stub.go @@ -2,6 +2,7 @@ // Use of this source code is governed by Apache 2 LICENSE that can be found in the LICENSE file. // +build !aetest +// +build package dash diff --git a/dashboard/app/email_test.go b/dashboard/app/email_test.go index 8ee1de5d6..1afac448e 100644 --- a/dashboard/app/email_test.go +++ b/dashboard/app/email_test.go @@ -195,7 +195,8 @@ report1 } extBugID1 = extBugID c.expectEQ(sender, fromAddr(c.ctx)) - c.expectEQ(msg.To, []string{"bar@foo.com", "bugs@syzkaller.com", "foo@bar.com"}) + c.expectEQ(msg.To, []string{"bar@foo.com", "bugs@syzkaller.com", + "default@maintainers.com", "foo@bar.com"}) c.expectEQ(msg.Subject, crash.Title) c.expectEQ(len(msg.Attachments), 3) c.expectEQ(msg.Attachments[0].Name, "raw.log.txt") @@ -280,7 +281,8 @@ Content-Type: text/plain t.Fatalf("failed to remove sender context: %v", err) } c.expectEQ(sender, fromAddr(c.ctx)) - c.expectEQ(msg.To, []string{"another@another.com", "bar@foo.com", "bugs@syzkaller.com", "foo@bar.com", "new@new.com", "qux@qux.com"}) + c.expectEQ(msg.To, []string{"another@another.com", "bar@foo.com", "bugs@syzkaller.com", + "default@maintainers.com", "foo@bar.com", "new@new.com", "qux@qux.com"}) c.expectEQ(msg.Subject, crash.Title) c.expectEQ(len(msg.Attachments), 4) c.expectEQ(msg.Attachments[0].Name, "raw.log.txt") diff --git a/dashboard/app/entities.go b/dashboard/app/entities.go index a444235df..0629809ce 100644 --- a/dashboard/app/entities.go +++ b/dashboard/app/entities.go @@ -135,6 +135,7 @@ type ReportingStateEntry struct { type Job struct { Created time.Time User string + CC []string Reporting string ExtID string // email Message-ID Link string // web link for the job (e.g. email in the group) diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go index 0be52e489..d53062d92 100644 --- a/dashboard/app/jobs.go +++ b/dashboard/app/jobs.go @@ -20,7 +20,8 @@ import ( // handleTestRequest added new job to datastore. // Returns empty string if job added successfully, or reason why it wasn't added. -func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, branch string) string { +func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, branch string, + jobCC []string) string { log.Infof(c, "test request: bug=%q user=%q extID=%q patch=%v, repo=%q branch=%q", bugID, user, extID, len(patch), repo, branch) for _, blacklisted := range config.EmailBlacklist { @@ -39,7 +40,7 @@ func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, return fmt.Sprintf("can't find the associated bug (do you have %v in To/CC?)", myEmail) } bugReporting, _ := bugReportingByID(bug, bugID) - reply, err := addTestJob(c, bug, bugKey, bugReporting, user, extID, link, patch, repo, branch) + reply, err := addTestJob(c, bug, bugKey, bugReporting, user, extID, link, patch, repo, branch, jobCC) if err != nil { log.Errorf(c, "test request failed: %v", err) if reply == "" { @@ -47,15 +48,15 @@ func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, } } // Update bug CC list in any case. - if !stringInList(strings.Split(bugReporting.CC, "|"), user) { + if !stringsInList(strings.Split(bugReporting.CC, "|"), jobCC) { tx := func(c context.Context) error { bug := new(Bug) if err := datastore.Get(c, bugKey, bug); err != nil { return err } bugReporting := bugReportingByName(bug, bugReporting.Name) - cc := strings.Split(bugReporting.CC, "|") - merged := email.MergeEmailLists(cc, []string{user}) + bugCC := strings.Split(bugReporting.CC, "|") + merged := email.MergeEmailLists(bugCC, jobCC) bugReporting.CC = strings.Join(merged, "|") if _, err := datastore.Put(c, bugKey, bug); err != nil { return err @@ -74,7 +75,7 @@ func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, } func addTestJob(c context.Context, bug *Bug, bugKey *datastore.Key, bugReporting *BugReporting, - user, extID, link, patch, repo, branch string) (string, error) { + user, extID, link, patch, repo, branch string, jobCC []string) (string, error) { crash, crashKey, err := findCrashForBug(c, bug) if err != nil { return "", err @@ -116,6 +117,7 @@ func addTestJob(c context.Context, bug *Bug, bugKey *datastore.Key, bugReporting job := &Job{ Created: timeNow(c), User: user, + CC: jobCC, Reporting: bugReporting.Name, ExtID: extID, Link: link, @@ -369,6 +371,7 @@ func createBugReportForJob(c context.Context, job *Job, jobKey *datastore.Key, c JobID: extJobID(jobKey), ExtID: job.ExtID, Title: bug.displayTitle(), + CC: job.CC, Log: crashLog, Report: report, OS: build.OS, @@ -386,10 +389,6 @@ func createBugReportForJob(c context.Context, job *Job, jobKey *datastore.Key, c Error: jobError, Patch: patch, } - if bugReporting.CC != "" { - rep.CC = strings.Split(bugReporting.CC, "|") - } - rep.CC = append(rep.CC, job.User) return rep, nil } diff --git a/dashboard/app/jobs_test.go b/dashboard/app/jobs_test.go index 9a0e98102..f5124a382 100644 --- a/dashboard/app/jobs_test.go +++ b/dashboard/app/jobs_test.go @@ -29,22 +29,30 @@ func TestJob(t *testing.T) { // Report crash without repro, check that test requests are not accepted. crash := testCrash(build, 1) + crash.Maintainers = []string{"maintainer@kernel.org"} c.expectOK(c.API(client2, key2, "report_crash", crash, nil)) c.expectOK(c.GET("/email_poll")) c.expectEQ(len(c.emailSink), 1) sender := (<-c.emailSink).Sender + c.incomingEmail(sender, "#syz upstream\n") + c.expectOK(c.GET("/email_poll")) + c.expectEQ(len(c.emailSink), 1) + sender = (<-c.emailSink).Sender _, extBugID, err := email.RemoveAddrContext(sender) if err != nil { t.Fatal(err) } + mailingList := config.Namespaces["test2"].Reporting[1].Config.(*EmailConfig).Email + c.incomingEmail(sender, "bla-bla-bla", EmailOptFrom("maintainer@kernel.org"), + EmailOptCC([]string{mailingList, "kernel@mailing.list"})) - c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, + EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList})) c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "This crash does not have a reproducer"), true) // Report crash with repro. - crash.Maintainers = []string{"foo@bar.com"} crash.ReproOpts = []byte("repro opts") crash.ReproSyz = []byte("repro syz") crash.ReproC = []byte("repro C") @@ -54,35 +62,44 @@ func TestJob(t *testing.T) { c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "syzbot has found reproducer"), true) - c.incomingEmail(sender, "#syz test: repo") + c.incomingEmail(sender, "#syz test: repo", + EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList})) c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "want 2 args"), true) - c.incomingEmail(sender, "#syz test: repo branch commit") + c.incomingEmail(sender, "#syz test: repo branch commit", + EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList})) c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "want 2 args"), true) - c.incomingEmail(sender, "#syz test: repo branch") + c.incomingEmail(sender, "#syz test: repo branch", + EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList})) c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "does not look like a valid git repo"), true) - c.incomingEmail(sender, "#syz test: git://git.git/git.git master") + c.incomingEmail(sender, "#syz test: git://git.git/git.git master", + EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList})) c.expectEQ(len(c.emailSink), 1) c.expectEQ(strings.Contains((<-c.emailSink).Body, "I don't see any patch attached to the request"), true) - c.incomingEmailFrom("\"foo\" ", sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, + EmailOptFrom("\"foo\" ")) c.expectOK(c.GET("/email_poll")) c.expectEQ(len(c.emailSink), 0) pollResp := new(dashapi.JobPollResp) c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp)) c.expectEQ(pollResp.ID, "") - c.incomingEmailID(1, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, + EmailOptMessageID(1), EmailOptFrom("test@requester.com"), + EmailOptCC([]string{"somebody@else.com"})) c.expectOK(c.GET("/email_poll")) c.expectEQ(len(c.emailSink), 0) // A dup of the same request with the same Message-ID. - c.incomingEmailID(1, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, + EmailOptMessageID(1), EmailOptFrom("test@requester.com"), + EmailOptCC([]string{"somebody@else.com"})) c.expectOK(c.GET("/email_poll")) c.expectEQ(len(c.emailSink), 0) @@ -117,8 +134,8 @@ func TestJob(t *testing.T) { c.expectEQ(len(c.emailSink), 1) { msg := <-c.emailSink - list := config.Namespaces["test2"].Reporting[0].Config.(*EmailConfig).Email - c.expectEQ(msg.To, []string{"default@sender.com", list}) + to := email.MergeEmailLists([]string{"test@requester.com", "somebody@else.com", mailingList}) + c.expectEQ(msg.To, to) c.expectEQ(msg.Subject, crash.Title) c.expectEQ(len(msg.Attachments), 3) c.expectEQ(msg.Attachments[0].Name, "patch.diff") @@ -148,7 +165,7 @@ Raw console output is attached. t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body) } } - c.incomingEmailID(2, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, EmailOptMessageID(2)) c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp)) jobDoneReq = &dashapi.JobDoneReq{ ID: pollResp.ID, @@ -187,7 +204,7 @@ Kernel config is attached. } } - c.incomingEmailID(3, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch) + c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch, EmailOptMessageID(3)) c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp)) jobDoneReq = &dashapi.JobDoneReq{ ID: pollResp.ID, diff --git a/dashboard/app/reporting_email.go b/dashboard/app/reporting_email.go index 38327b072..d41a503b8 100644 --- a/dashboard/app/reporting_email.go +++ b/dashboard/app/reporting_email.go @@ -92,6 +92,14 @@ func handleEmailPoll(w http.ResponseWriter, r *http.Request) { func emailPollBugs(c context.Context) error { reports := reportingPollBugs(c, emailType) for _, rep := range reports { + cfg := new(EmailConfig) + if err := json.Unmarshal(rep.Config, cfg); err != nil { + log.Errorf(c, "failed to unmarshal email config: %v", err) + continue + } + if cfg.MailMaintainers { + rep.CC = email.MergeEmailLists(rep.CC, rep.Maintainers, cfg.DefaultMaintainers) + } if err := emailReport(c, rep, "mail_bug.txt"); err != nil { log.Errorf(c, "failed to report bug: %v", err) continue @@ -138,11 +146,7 @@ func emailReport(c context.Context, rep *dashapi.BugReport, templ string) error if err := json.Unmarshal(rep.Config, cfg); err != nil { return fmt.Errorf("failed to unmarshal email config: %v", err) } - to := []string{cfg.Email} - if cfg.MailMaintainers { - to = email.MergeEmailLists(to, rep.Maintainers, cfg.DefaultMaintainers) - } - to = email.MergeEmailLists(to, rep.CC) + to := email.MergeEmailLists([]string{cfg.Email}, rep.CC) var attachments []aemail.Attachment // Note: order of attachments is important. Some email clients show them inline. if len(rep.Patch) != 0 { @@ -288,13 +292,10 @@ func incomingMail(c context.Context, r *http.Request) error { len(args)), nil) } reply := handleTestRequest(c, msg.BugID, email.CanonicalEmail(msg.From), - msg.MessageID, msg.Link, msg.Patch, args[0], args[1]) + msg.MessageID, msg.Link, msg.Patch, args[0], args[1], msg.Cc) if reply != "" { return replyTo(c, msg, reply, nil) } - if !mailingListInCC { - warnMailingListInCC(c, msg, mailingList) - } return nil } if fromMailingList && msg.Command != "" { diff --git a/dashboard/app/util_test.go b/dashboard/app/util_test.go index 8499b610f..17a8b460e 100644 --- a/dashboard/app/util_test.go +++ b/dashboard/app/util_test.go @@ -281,33 +281,37 @@ func (client *apiClient) updateBug(extID string, status dashapi.BugStatus, dup s return reply } -func (c *Ctx) incomingEmail(to, body string) { - c.incomingEmailImpl(0, "", to, body) -} - -func (c *Ctx) incomingEmailFrom(from, to, body string) { - c.incomingEmailImpl(0, from, to, body) -} - -func (c *Ctx) incomingEmailID(id int, to, body string) { - c.incomingEmailImpl(id, "", to, body) -} +type ( + EmailOptMessageID int + EmailOptFrom string + EmailOptCC []string +) -func (c *Ctx) incomingEmailImpl(id int, from, to, body string) { - if from == "" { - from = "default@sender.com" +func (c *Ctx) incomingEmail(to, body string, opts ...interface{}) { + id := 0 + from := "default@sender.com" + cc := []string{"test@syzkaller.com", "bugs@syzkaller.com"} + for _, o := range opts { + switch opt := o.(type) { + case EmailOptMessageID: + id = int(opt) + case EmailOptFrom: + from = string(opt) + case EmailOptCC: + cc = []string(opt) + } } email := fmt.Sprintf(`Sender: %v Date: Tue, 15 Aug 2017 14:59:00 -0700 Message-ID: <%v> Subject: crash1 From: %v -Cc: test@syzkaller.com, bugs@syzkaller.com +Cc: %v To: %v Content-Type: text/plain %v -`, from, id, from, to, body) +`, from, id, from, strings.Join(cc, ","), to, body) c.expectOK(c.POST("/_ah/mail/", email)) } -- cgit mrf-deployment