dashboard/app: allow some cross-reporting dups

Allow a special case of cross-reporting duping:
rom last but one reporting to the last one (which is stable, final destination)
provided that these two reportings have the same access level and type.
The rest of the combinations can lead to surprising states and
information hiding, so we still don't allow them.

Fixes #569

7 files changed, 187 insertions, 47 deletions

diff --git a/dashboard/app/access_test.go b/dashboard/app/access_test.go
index bc03ec9eb..45b673759 100644
--- a/dashboard/app/access_test.go
+++ b/dashboard/app/access_test.go
@@ -40,6 +40,9 @@ func TestAccessConfig(t *testing.T) {
 
 // TestAccess checks that all UIs respect access levels.
 func TestAccess(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
 	c := NewCtx(t)
 	defer c.Close()
 
diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go
index 6e9c21072..af4bacf48 100644
--- a/dashboard/app/app_test.go
+++ b/dashboard/app/app_test.go
@@ -90,6 +90,15 @@ var testConfig = &GlobalConfig{
 						MailMaintainers:    true,
 					},
 				},
+				{
+					Name:       "reporting3",
+					DailyLimit: 3,
+					Config: &EmailConfig{
+						Email:              "bugs@syzkaller.com",
+						DefaultMaintainers: []string{"default@maintainers.com"},
+						MailMaintainers:    true,
+					},
+				},
 			},
 		},
 		// Namespaces for access level testing.
diff --git a/dashboard/app/email_test.go b/dashboard/app/email_test.go
index f9c2c9236..40b2a9947 100644
--- a/dashboard/app/email_test.go
+++ b/dashboard/app/email_test.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/syzkaller/pkg/email"
 )
@@ -472,3 +473,61 @@ func TestEmailUndup(t *testing.T) {
 	c.expectOK(c.GET("/email_poll"))
 	c.expectEQ(len(c.emailSink), 0)
 }
+
+func TestEmailCrossReportingDup(t *testing.T) {
+	// TODO:
+	c := NewCtx(t)
+	defer c.Close()
+
+	build := testBuild(1)
+	c.client2.UploadBuild(build)
+
+	tests := []struct {
+		bug    int
+		dup    int
+		result bool
+	}{
+		{0, 0, true},
+		{0, 1, false},
+		{0, 2, false},
+		{1, 0, false},
+		{1, 1, true},
+		{1, 2, true},
+		{2, 0, false},
+		{2, 1, false},
+		{2, 2, true},
+	}
+	for i, test := range tests {
+		t.Logf("duping %v->%v, expect %v", test.bug, test.dup, test.result)
+		c.advanceTime(24 * time.Hour) // to not hit email limit per day
+		crash1 := testCrash(build, 1)
+		crash1.Title = fmt.Sprintf("bug_%v", i)
+		c.client2.ReportCrash(crash1)
+		bugSender := c.pollEmailBug()
+		for j := 0; j < test.bug; j++ {
+			c.incomingEmail(bugSender, "#syz upstream")
+			bugSender = c.pollEmailBug()
+		}
+
+		crash2 := testCrash(build, 2)
+		crash2.Title = fmt.Sprintf("dup_%v", i)
+		c.client2.ReportCrash(crash2)
+		dupSender := c.pollEmailBug()
+		for j := 0; j < test.dup; j++ {
+			c.incomingEmail(dupSender, "#syz upstream")
+			dupSender = c.pollEmailBug()
+		}
+
+		c.incomingEmail(bugSender, "#syz dup: "+crash2.Title)
+		if test.result {
+			c.expectEQ(len(c.emailSink), 0)
+		} else {
+			c.expectEQ(len(c.emailSink), 1)
+			msg := <-c.emailSink
+			if !strings.Contains(msg.Body, "> #syz dup:") ||
+				!strings.Contains(msg.Body, "Can't dup bug to a bug in different reporting") {
+				c.t.Fatalf("bad reply body:\n%v", msg.Body)
+			}
+		}
+	}
+}
diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go
index 2ca830141..1bb08ffc2 100644
--- a/dashboard/app/reporting.go
+++ b/dashboard/app/reporting.go
@@ -357,51 +357,11 @@ func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (bool, strin
 	now := timeNow(c)
 	dupHash := ""
 	if cmd.Status == dashapi.BugStatusDup {
-		bugReporting, _ := bugReportingByID(bug, cmd.ID)
-		dup, dupKey, err := findBugByReportingID(c, cmd.DupOf)
-		if err != nil {
-			// Email reporting passes bug title in cmd.DupOf, try to find bug by title.
-			dup, dupKey, err = findDupByTitle(c, bug.Namespace, cmd.DupOf)
-			if err != nil {
-				return false, "can't find the dup bug", err
-			}
-			dupReporting := bugReportingByName(dup, bugReporting.Name)
-			if dupReporting == nil {
-				return false, "can't find the dup bug",
-					fmt.Errorf("dup does not have reporting %q", bugReporting.Name)
-			}
-			cmd.DupOf = dupReporting.ID
-		}
-		dupReporting, _ := bugReportingByID(dup, cmd.DupOf)
-		if bugReporting == nil || dupReporting == nil {
-			return false, internalError, fmt.Errorf("can't find bug reporting")
-		}
-		if bugKey.StringID() == dupKey.StringID() {
-			if bugReporting.Name == dupReporting.Name {
-				return false, "Can't dup bug to itself.", nil
-			}
-			return false, fmt.Sprintf("Can't dup bug to itself in different reporting (%v->%v).\n"+
-				"Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.",
-				bugReporting.Name, dupReporting.Name), nil
-		}
-		if bug.Namespace != dup.Namespace {
-			return false, fmt.Sprintf("Duplicate bug corresponds to a different kernel (%v->%v).\n"+
-				"Please dup syzbot bugs only onto syzbot bugs for the same kernel.",
-				bug.Namespace, dup.Namespace), nil
-		}
-		if bugReporting.Name != dupReporting.Name {
-			return false, fmt.Sprintf("Can't dup bug to a bug in different reporting (%v->%v)."+
-				"Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.",
-				bugReporting.Name, dupReporting.Name), nil
-		}
-		dupCanon, err := canonicalBug(c, dup)
-		if err != nil {
-			return false, internalError, fmt.Errorf("failed to get canonical bug for dup: %v", err)
+		dupHash1, ok, reason, err := findDupBug(c, cmd, bug, bugKey)
+		if !ok || err != nil {
+			return ok, reason, err
 		}
-		if !dupReporting.Closed.IsZero() && dupCanon.Status == BugStatusOpen {
-			return false, "Dup bug is already upstreamed.", nil
-		}
-		dupHash = bugKeyHash(dup.Namespace, dup.Title, dup.Seq)
+		dupHash = dupHash1
 	}
 
 	ok, reply := false, ""
@@ -423,6 +383,91 @@ func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (bool, strin
 	return ok, reply, nil
 }
 
+func findDupBug(c context.Context, cmd *dashapi.BugUpdate, bug *Bug, bugKey *datastore.Key) (
+	string, bool, string, error) {
+	bugReporting, _ := bugReportingByID(bug, cmd.ID)
+	dup, dupKey, err := findBugByReportingID(c, cmd.DupOf)
+	if err != nil {
+		// Email reporting passes bug title in cmd.DupOf, try to find bug by title.
+		dup, dupKey, err = findDupByTitle(c, bug.Namespace, cmd.DupOf)
+		if err != nil {
+			return "", false, "can't find the dup bug", err
+		}
+		dupReporting := lastReportedReporting(dup)
+		if dupReporting == nil {
+			return "", false, "can't find the dup bug",
+				fmt.Errorf("dup does not have reporting %q", bugReporting.Name)
+		}
+		cmd.DupOf = dupReporting.ID
+	}
+	dupReporting, _ := bugReportingByID(dup, cmd.DupOf)
+	if bugReporting == nil || dupReporting == nil {
+		return "", false, internalError, fmt.Errorf("can't find bug reporting")
+	}
+	if bugKey.StringID() == dupKey.StringID() {
+		if bugReporting.Name == dupReporting.Name {
+			return "", false, "Can't dup bug to itself.", nil
+		}
+		return "", false, fmt.Sprintf("Can't dup bug to itself in different reporting (%v->%v).\n"+
+			"Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.",
+			bugReporting.Name, dupReporting.Name), nil
+	}
+	if bug.Namespace != dup.Namespace {
+		return "", false, fmt.Sprintf("Duplicate bug corresponds to a different kernel (%v->%v).\n"+
+			"Please dup syzbot bugs only onto syzbot bugs for the same kernel.",
+			bug.Namespace, dup.Namespace), nil
+	}
+	if !allowCrossReportingDup(c, bug, dup, bugReporting, dupReporting) {
+		return "", false, fmt.Sprintf("Can't dup bug to a bug in different reporting (%v->%v)."+
+			"Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.",
+			bugReporting.Name, dupReporting.Name), nil
+	}
+	dupCanon, err := canonicalBug(c, dup)
+	if err != nil {
+		return "", false, internalError, fmt.Errorf("failed to get canonical bug for dup: %v", err)
+	}
+	if !dupReporting.Closed.IsZero() && dupCanon.Status == BugStatusOpen {
+		return "", false, "Dup bug is already upstreamed.", nil
+	}
+	dupHash := bugKeyHash(dup.Namespace, dup.Title, dup.Seq)
+	return dupHash, true, "", nil
+}
+
+func allowCrossReportingDup(c context.Context, bug, dup *Bug,
+	bugReporting, dupReporting *BugReporting) bool {
+	bugIdx := getReportingIdx(c, bug, bugReporting)
+	dupIdx := getReportingIdx(c, dup, dupReporting)
+	if bugIdx < 0 || dupIdx < 0 {
+		return false
+	}
+	if bugIdx == dupIdx {
+		return true
+	}
+	// We generally allow duping only within the same reporting.
+	// But there is one exception: we also allow duping from last but one
+	// reporting to the last one (which is stable, final destination)
+	// provided that these two reportings have the same access level and type.
+	// The rest of the combinations can lead to surprising states and
+	// information hiding, so we don't allow them.
+	cfg := config.Namespaces[bug.Namespace]
+	bugConfig := &cfg.Reporting[bugIdx]
+	dupConfig := &cfg.Reporting[dupIdx]
+	lastIdx := len(cfg.Reporting) - 1
+	return bugIdx == lastIdx-1 && dupIdx == lastIdx &&
+		bugConfig.AccessLevel == dupConfig.AccessLevel &&
+		bugConfig.Config.Type() == dupConfig.Config.Type()
+}
+
+func getReportingIdx(c context.Context, bug *Bug, bugReporting *BugReporting) int {
+	for i := range bug.Reporting {
+		if bug.Reporting[i].Name == bugReporting.Name {
+			return i
+		}
+	}
+	log.Errorf(c, "failed to find bug reporting by name: %q/%q", bug.Title, bugReporting.Name)
+	return -1
+}
+
 func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate,
 	bugKey *datastore.Key, dupHash string) (bool, string, error) {
 	bug := new(Bug)
@@ -634,6 +679,15 @@ func bugReportingByName(bug *Bug, name string) *BugReporting {
 	return nil
 }
 
+func lastReportedReporting(bug *Bug) *BugReporting {
+	for i := len(bug.Reporting) - 1; i >= 0; i-- {
+		if !bug.Reporting[i].Reported.IsZero() {
+			return &bug.Reporting[i]
+		}
+	}
+	return nil
+}
+
 func queryCrashesForBug(c context.Context, bugKey *datastore.Key, limit int) (
 	[]*Crash, []*datastore.Key, error) {
 	var crashes []*Crash
diff --git a/dashboard/app/reporting_test.go b/dashboard/app/reporting_test.go
index be43899b5..ffbd4bce5 100644
--- a/dashboard/app/reporting_test.go
+++ b/dashboard/app/reporting_test.go
@@ -355,7 +355,6 @@ func TestReportingDupCrossReporting(t *testing.T) {
 	cmds := []*dashapi.BugUpdate{
 		{ID: rep1.ID, DupOf: rep1.ID},
 		{ID: rep1.ID, DupOf: rep2.ID},
-		{ID: rep1.ID, DupOf: rep3.ID},
 		{ID: rep2.ID, DupOf: rep1.ID},
 		{ID: rep2.ID, DupOf: rep2.ID},
 		{ID: rep2.ID, DupOf: rep3.ID},
@@ -369,6 +368,15 @@ func TestReportingDupCrossReporting(t *testing.T) {
 		reply, _ := c.client.ReportingUpdate(cmd)
 		c.expectEQ(reply.OK, false)
 	}
+	// Special case of cross-reporting duping:
+	cmd := &dashapi.BugUpdate{
+		Status: dashapi.BugStatusDup,
+		ID:     rep1.ID,
+		DupOf:  rep3.ID,
+	}
+	t.Logf("duping %v -> %v", cmd.ID, cmd.DupOf)
+	reply, _ := c.client.ReportingUpdate(cmd)
+	c.expectTrue(reply.OK)
 }
 
 func TestReportingFilter(t *testing.T) {
diff --git a/dashboard/app/util_test.go b/dashboard/app/util_test.go
index 00c7694a1..d3481b2f1 100644
--- a/dashboard/app/util_test.go
+++ b/dashboard/app/util_test.go
@@ -231,6 +231,13 @@ func (c *Ctx) checkURLContents(url string, want []byte) {
 	}
 }
 
+func (c *Ctx) pollEmailBug() string {
+	c.expectOK(c.GET("/email_poll"))
+	c.expectEQ(len(c.emailSink), 1)
+	msg := <-c.emailSink
+	return msg.Sender
+}
+
 type apiClient struct {
 	*Ctx
 	*dashapi.Dashboard
diff --git a/docs/syzbot.md b/docs/syzbot.md
index d6b78b6f4..38902bbe3 100644
--- a/docs/syzbot.md
+++ b/docs/syzbot.md
@@ -155,8 +155,8 @@ in [moderation](https://syzkaller.appspot.com/#upstream-moderation2) section
 and mailed to
 [syzkaller-upstream-moderation](https://groups.google.com/forum/#!forum/syzkaller-upstream-moderation)
 mailing list. Staged bugs accept all commands supported for reported bugs
-(`fix`, `dup`, `invalid`) with a restriction that reported and staged bugs
-can't be `dup`-ed onto each other in any direction. Additionally, staged bugs
+(`fix`, `dup`, `invalid`) with a restriction that bugs reported upstream
+can't be `dup`-ed onto bugs in moderation queue. Additionally, staged bugs
 accept upstream command:
 ```
 #syz upstream