fix(labels): derive label max permission from accessible tasks only

The previous hasAccessToLabel implementation ran `Get(ll)` against a
label_tasks LEFT JOIN with no ORDER BY, which meant the database was
free to pick any matching row. When a label had multiple attachments,
or when access was granted via the creator branch while the label also
had label_tasks rows pointing at inaccessible tasks, the picked row
could belong to a task the caller could not actually read.

That led to two concrete problems reported on the follow-up review of
GHSA-hj5c-mhh2-g7jq:

  1. maxPermission (exposed as the x-max-permission response header)
     could be derived from a task the caller has no access to, ending
     up as 0 or lower than the caller's real best permission on the
     label.
  2. Task.CanRead on a dangling/inaccessible task could return an
     error and surface as a 500, even though the label itself was
     perfectly readable via the creator branch.

Split the logic instead:

  * Use `Exist` for the boolean access check, using the same carefully
    grouped `And(Eq{labels.id}, Or(accessibleTask, creator))` cond.
  * Compute maxPermission by selecting the label_tasks rows whose
    task lives in a project the caller can access, then iterating
    those tasks with `Task.CanRead` and taking the maximum.
  * Fall back to PermissionRead when the access was granted via the
    creator branch and no accessible task attachment exists.

pkg/db/fixtures/label_tasks.yml

@@ -22,10 +22,14 @@
   task_id: 35
   label_id: 5
   created: 2018-12-01 15:13:12
-# Attaches label #6 to task #34 (project 20, owned by user 13 and not shared
-# with user 1). Regression fixture for GHSA-hj5c-mhh2-g7jq — without this
-# row the label cannot reproduce the precedence leak.
+# GHSA-hj5c-mhh2-g7jq: without this row label #6 cannot reproduce the leak.
 - id: 7
   task_id: 34
   label_id: 6
   created: 2018-12-01 15:13:12
+# Label #8 attached to an inaccessible task, for the creator-branch
+# maxPermission fallback.
+- id: 8
+  task_id: 34
+  label_id: 8
+  created: 2018-12-01 15:13:12

pkg/db/fixtures/labels.yml

@@ -23,18 +23,23 @@
   created_by_id: 2
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
-# Label #6 is owned by user 13 and attached only to task #34, which lives in
-# project 20 (owned by user 13 and not shared with anyone). User 1 must NOT
-# be able to read this label. Regression fixture for GHSA-hj5c-mhh2-g7jq.
+# Regression for GHSA-hj5c-mhh2-g7jq: only attached to task #34
+# (project 20, user 13 only), must be unreachable for everyone else.
 - id: 6
   title: 'Label #6 - private, attached only to private task'
   created_by_id: 13
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
-# Label #7 is owned by user 1 but has no label_tasks row. Protects the
-# "creator can still see their own unattached labels" path.
+# Covers the creator-can-read-own-unattached-label path.
 - id: 7
   title: 'Label #7 - created by user 1, no task attachment'
   created_by_id: 1
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
+# Covers the creator-branch maxPermission fallback: user 1's label whose
+# only label_tasks row points at task #34 (inaccessible to user 1).
+- id: 8
+  title: 'Label #8 - user 1 creator, only attached to inaccessible task'
+  created_by_id: 1
+  updated: 2018-12-02 15:13:12
+  created: 2018-12-01 15:13:12

pkg/models/label_permissions.go

@@ -60,23 +60,17 @@ func (l *Label) isLabelOwner(s *xorm.Session, a web.Auth) (bool, error) {
 	return lorig.CreatedByID == a.GetID(), nil
 }
 
-// Helper method to check if a user can see a specific label.
+// hasAccessToLabel reports whether the caller can read a label and, if so,
+// the caller's maximum permission on it.
 //
-// A user can read a label when at least one of the following is true:
-//
-//  1. The auth is a real user and the user created the label, OR
-//  2. The label is attached to a task in a project the auth can access.
-//
-// The implementation uses explicit builder.And / builder.Or grouping so
-// the boolean precedence is unambiguous. A previous implementation chained
-// xorm session .Where / .Or / .And calls which SQL flattened to
-// `WHERE A OR B OR C AND D`, leaking any label with any label_tasks row
+// The access cond is assembled with explicit builder.And / builder.Or.
+// Chaining xorm's session .Where/.Or/.And instead flattens the SQL to
+// `A OR B OR C AND D`, which leaked any label with any label_tasks row
 // to any authenticated user (GHSA-hj5c-mhh2-g7jq).
 func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPermission int, err error) {
 
 	linkShare, isLinkShare := a.(*LinkSharing)
 
-	// Build the "task is in a project the caller can access" subquery.
 	var accessibleProjects builder.Cond
 	if isLinkShare {
 		accessibleProjects = builder.Eq{"project_id": linkShare.ProjectID}
@@ -95,11 +89,6 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm
 			Where(accessibleProjects),
 	)
 
-	// A user can see a label if:
-	//   - they created it (only when the auth is an actual user), OR
-	//   - it is attached to a task in a project they have access to.
-	//
-	// The outer AND enforces that the result is scoped to the requested label ID.
 	accessBranches := []builder.Cond{labelAttachedToAccessibleTask}
 	if !isLinkShare {
 		accessBranches = append(accessBranches, builder.Eq{"labels.created_by_id": a.GetID()})
@@ -110,28 +99,48 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm
 		builder.Or(accessBranches...),
 	)
 
-	ll := &LabelTask{}
 	has, err = s.Table("labels").
-		Select("label_tasks.*").
 		Join("LEFT", "label_tasks", "label_tasks.label_id = labels.id").
 		Where(cond).
-		Get(ll)
+		Exist(&Label{})
 	if err != nil || !has {
 		return
 	}
 
-	// If the label was matched via an attached task, compute the caller's
-	// permission level from that task. Otherwise (creator-only branch with
-	// no attachment) default to read permission.
-	if ll.TaskID > 0 {
-		t := &Task{ID: ll.TaskID}
-		_, maxPermission, err = t.CanRead(s, a)
-		if err != nil {
-			return
-		}
+	// maxPermission is derived only from label_tasks rows whose task is
+	// actually accessible. The pre-fix code used Get(ll) against the
+	// unrestricted LEFT JOIN, so it could return an inaccessible row and
+	// yield a wrong (or errored) permission.
+	accessibleTaskIDs := []int64{}
+	err = s.Table("label_tasks").
+		Join("INNER", "tasks", "tasks.id = label_tasks.task_id").
+		Where(builder.And(
+			builder.Eq{"label_tasks.label_id": l.ID},
+			accessibleProjects,
+		)).
+		Cols("label_tasks.task_id").
+		Find(&accessibleTaskIDs)
+	if err != nil {
 		return
 	}
 
-	maxPermission = int(PermissionRead)
+	for _, taskID := range accessibleTaskIDs {
+		t := &Task{ID: taskID}
+		_, taskPermission, tErr := t.CanRead(s, a)
+		if tErr != nil {
+			err = tErr
+			return
+		}
+		if taskPermission > maxPermission {
+			maxPermission = taskPermission
+		}
+	}
+
+	// Creator-branch fallback: access came from created_by_id with no
+	// accessible task to derive a permission from.
+	if len(accessibleTaskIDs) == 0 {
+		maxPermission = int(PermissionRead)
+	}
+
 	return
 }

pkg/models/label_test.go

@@ -125,6 +125,16 @@ func TestLabel_ReadAll(t *testing.T) {
 						Updated:     testUpdatedTime,
 					},
 				},
+				{
+					Label: Label{
+						ID:          8,
+						Title:       "Label #8 - user 1 creator, only attached to inaccessible task",
+						CreatedByID: 1,
+						CreatedBy:   user1,
+						Created:     testCreatedTime,
+						Updated:     testUpdatedTime,
+					},
+				},
 			},
 		},
 		{
@@ -193,13 +203,15 @@ func TestLabel_ReadOne(t *testing.T) {
 		ExportFileID:                 1,
 	}
 	tests := []struct {
-		name          string
-		fields        fields
-		want          *Label
-		wantErr       bool
-		errType       func(error) bool
-		auth          web.Auth
-		wantForbidden bool
+		name                string
+		fields              fields
+		want                *Label
+		wantErr             bool
+		errType             func(error) bool
+		auth                web.Auth
+		wantForbidden       bool
+		assertMaxPermission bool
+		wantMaxPermission   int
 	}{
 		{
 			name: "Get label #1",
@@ -214,7 +226,9 @@ func TestLabel_ReadOne(t *testing.T) {
 				Created:     testCreatedTime,
 				Updated:     testUpdatedTime,
 			},
-			auth: &user.User{ID: 1},
+			auth:                &user.User{ID: 1},
+			assertMaxPermission: true,
+			wantMaxPermission:   int(PermissionRead),
 		},
 		{
 			name: "Get nonexistant label",
@@ -235,6 +249,8 @@ func TestLabel_ReadOne(t *testing.T) {
 			auth:          &user.User{ID: 1},
 		},
 		{
+			// Label 4 is attached to tasks in project 1 (user 1 is admin),
+			// so the accessible-tasks iteration must yield PermissionAdmin.
 			name: "Get label #4 - other user",
 			fields: fields{
 				ID: 4,
@@ -258,13 +274,13 @@ func TestLabel_ReadOne(t *testing.T) {
 				Created: testCreatedTime,
 				Updated: testUpdatedTime,
 			},
-			auth: &user.User{ID: 1},
+			auth:                &user.User{ID: 1},
+			assertMaxPermission: true,
+			wantMaxPermission:   int(PermissionAdmin),
 		},
 		{
-			// Regression for GHSA-hj5c-mhh2-g7jq: label 6 is owned by user 13
-			// and attached only to task 34 (project 20, not shared with user 1).
-			// The old hasAccessToLabel query leaked any label with any
-			// label_tasks row to any authenticated user.
+			// PoC for GHSA-hj5c-mhh2-g7jq: label 6 is reachable only via task
+			// 34 in the private project 20, user 1 must not see it.
 			name: "PoC GHSA-hj5c-mhh2-g7jq: label 6 attached only to unreachable task must be forbidden",
 			fields: fields{
 				ID: 6,
@@ -286,7 +302,29 @@ func TestLabel_ReadOne(t *testing.T) {
 				Created:     testCreatedTime,
 				Updated:     testUpdatedTime,
 			},
-			auth: &user.User{ID: 1},
+			auth:                &user.User{ID: 1},
+			assertMaxPermission: true,
+			wantMaxPermission:   int(PermissionRead),
+		},
+		{
+			// Label 8's only label_tasks row points at inaccessible task 34,
+			// so access must come from the creator branch and the
+			// maxPermission fallback to PermissionRead must kick in.
+			name: "creator can read own label only attached to inaccessible task",
+			fields: fields{
+				ID: 8,
+			},
+			want: &Label{
+				ID:          8,
+				Title:       "Label #8 - user 1 creator, only attached to inaccessible task",
+				CreatedByID: 1,
+				CreatedBy:   user1,
+				Created:     testCreatedTime,
+				Updated:     testUpdatedTime,
+			},
+			auth:                &user.User{ID: 1},
+			assertMaxPermission: true,
+			wantMaxPermission:   int(PermissionRead),
 		},
 		{
 			// Non-creator must not be able to read an unattached label owned
@@ -318,13 +356,16 @@ func TestLabel_ReadOne(t *testing.T) {
 			s := db.NewSession()
 			defer s.Close()
 
-			allowed, _, _ := l.CanRead(s, tt.auth)
+			allowed, maxPermission, _ := l.CanRead(s, tt.auth)
 			if !allowed && !tt.wantForbidden {
 				t.Errorf("Label.CanRead() forbidden, want %v", tt.wantForbidden)
 			}
 			if allowed && tt.wantForbidden {
 				t.Errorf("Label.CanRead() allowed, want forbidden")
 			}
+			if tt.assertMaxPermission && maxPermission != tt.wantMaxPermission {
+				t.Errorf("Label.CanRead() maxPermission = %d, want %d", maxPermission, tt.wantMaxPermission)
+			}
 			err := l.ReadOne(s, tt.auth)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("Label.ReadOne() error = %v, wantErr %v", err, tt.wantErr)