mirror of
https://github.com/go-vikunja/vikunja.git
synced 2026-04-24 22:25:15 +00:00
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.
147 lines
4.2 KiB
Go
147 lines
4.2 KiB
Go
// Vikunja is a to-do list application to facilitate your life.
|
|
// Copyright 2018-present Vikunja and contributors. All rights reserved.
|
|
//
|
|
// This program is free software: you can redistribute it and/or modify
|
|
// it under the terms of the GNU Affero General Public License as published by
|
|
// the Free Software Foundation, either version 3 of the License, or
|
|
// (at your option) any later version.
|
|
//
|
|
// This program is distributed in the hope that it will be useful,
|
|
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
|
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
|
// GNU Affero General Public License for more details.
|
|
//
|
|
// You should have received a copy of the GNU Affero General Public License
|
|
// along with this program. If not, see <https://www.gnu.org/licenses/>.
|
|
|
|
package models
|
|
|
|
import (
|
|
"code.vikunja.io/api/pkg/web"
|
|
"xorm.io/builder"
|
|
"xorm.io/xorm"
|
|
)
|
|
|
|
// CanUpdate checks if a user can update a label
|
|
func (l *Label) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) {
|
|
return l.isLabelOwner(s, a) // Only owners should be allowed to update a label
|
|
}
|
|
|
|
// CanDelete checks if a user can delete a label
|
|
func (l *Label) CanDelete(s *xorm.Session, a web.Auth) (bool, error) {
|
|
return l.isLabelOwner(s, a) // Only owners should be allowed to delete a label
|
|
}
|
|
|
|
// CanRead checks if a user can read a label
|
|
func (l *Label) CanRead(s *xorm.Session, a web.Auth) (bool, int, error) {
|
|
return l.hasAccessToLabel(s, a)
|
|
}
|
|
|
|
// CanCreate checks if the user can create a label
|
|
// Currently a dummy.
|
|
func (l *Label) CanCreate(_ *xorm.Session, a web.Auth) (bool, error) {
|
|
if _, is := a.(*LinkSharing); is {
|
|
return false, nil
|
|
}
|
|
|
|
return true, nil
|
|
}
|
|
|
|
func (l *Label) isLabelOwner(s *xorm.Session, a web.Auth) (bool, error) {
|
|
|
|
if _, is := a.(*LinkSharing); is {
|
|
return false, nil
|
|
}
|
|
|
|
lorig, err := getLabelByIDSimple(s, l.ID)
|
|
if err != nil {
|
|
return false, err
|
|
}
|
|
return lorig.CreatedByID == a.GetID(), nil
|
|
}
|
|
|
|
// hasAccessToLabel reports whether the caller can read a label and, if so,
|
|
// the caller's maximum permission on it.
|
|
//
|
|
// 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)
|
|
|
|
var accessibleProjects builder.Cond
|
|
if isLinkShare {
|
|
accessibleProjects = builder.Eq{"project_id": linkShare.ProjectID}
|
|
} else {
|
|
accessibleProjects = builder.In(
|
|
"project_id",
|
|
getUserProjectsStatement(a.GetID(), "").Select("l.id"),
|
|
)
|
|
}
|
|
|
|
labelAttachedToAccessibleTask := builder.In(
|
|
"label_tasks.task_id",
|
|
builder.
|
|
Select("id").
|
|
From("tasks").
|
|
Where(accessibleProjects),
|
|
)
|
|
|
|
accessBranches := []builder.Cond{labelAttachedToAccessibleTask}
|
|
if !isLinkShare {
|
|
accessBranches = append(accessBranches, builder.Eq{"labels.created_by_id": a.GetID()})
|
|
}
|
|
|
|
cond := builder.And(
|
|
builder.Eq{"labels.id": l.ID},
|
|
builder.Or(accessBranches...),
|
|
)
|
|
|
|
has, err = s.Table("labels").
|
|
Join("LEFT", "label_tasks", "label_tasks.label_id = labels.id").
|
|
Where(cond).
|
|
Exist(&Label{})
|
|
if err != nil || !has {
|
|
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
|
|
}
|
|
|
|
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
|
|
}
|