refactor(labels): share one visibility condition between the list and single reads - #3673
Merged
Conversation
tink-bot
temporarily deployed
to
preview-trusted
August 30, 2026 12:13 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
tink-bot
changed the title
tink-bot
marked this pull request as ready for review
August 30, 2026 12:14
github-actions
Bot
added
area/labels
tink-bot
force-pushed
the
labels-shared-visibility-cond
branch
from
August 30, 2026 12:31
8f8946a to
f1be75a
Compare
tink-bot
temporarily deployed
to
preview-trusted
August 30, 2026 12:31 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Preview DeploymentPreview deployments for this PR are available at:
The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the Run locally with Dockerdocker pull ghcr.io/go-vikunja/vikunja:pr-3673
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3673Last updated for commit a42efd3 |
tink-bot
force-pushed
the
labels-shared-visibility-cond
branch
from
August 30, 2026 13:52
f1be75a to
1d4196b
Compare
tink-bot
temporarily deployed
to
preview-trusted
August 30, 2026 13:52 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
tink-bot
temporarily deployed
to
preview-trusted
August 30, 2026 14:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
kolaente
approved these changes
Aug 30, 2026
The rule deciding which labels a caller may see was written twice, in two different SQL shapes: labels.id IN (subquery) in GetLabelsForUser, a LEFT JOIN plus label_tasks.task_id IN (subquery) in hasAccessToLabel. Two hand-maintained shapes of one rule is a standing drift risk, and it already cost us once - on main before this branch, the list resolved its projects through getRawProjectsForUser, which drops archived projects, so a label reachable only through a task in an archived but owned project was returned by CanRead and hidden from the list. labelVisibleCond is now the single definition, and hasAccessToLabel loses its LEFT JOIN on label_tasks with it - the new cond references label_tasks only inside an uncorrelated subquery. The GHSA-hj5c-mhh2-g7jq warning moves onto the helper, restated as an instruction to callers: hand the cond to Where in one shot, never chain it onto the session. TestLabel_ReadAllMatchesCanRead pins the invariant this exists to protect, that ReadAll returns exactly what CanRead allows, for two users, a bot owner and a link share. It also pins the expected id sets outright: deriving both sides from one helper makes a pure parity assertion blind to that helper widening, which mutation testing confirmed.
builder.And silently drops an argument whose IsValid() is false, so an empty or invalid cond out of labelVisibleCond would leave hasAccessToLabel querying `WHERE labels.id = ?` alone - every label readable by everyone, the same class of bypass as GHSA-hj5c-mhh2-g7jq, and silent at compile time and at runtime. Not reachable today: the cond always carries the task branch, whose IsValid() is unconditionally true. Guarded inside the helper rather than at the two call sites so a future third consumer cannot inherit the contract without knowing it. This catches the bad state rather than making it unconstructable - the type-level version would need a cond wrapper that cannot be empty.
kolaente
force-pushed
the
labels-shared-visibility-cond
branch
from
August 30, 2026 15:04
64ddc47 to
a42efd3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #3660 — review that one first; this PR's diff is only its own commit.
Follow-up to a review question on #3660, split out at the maintainer's request.
The problem
Two functions decided which labels a caller may see, expressing the same rule in two different SQL shapes:
GetLabelsForUser(the list)labels.id IN (SELECT label_id FROM label_tasks INNER JOIN tasks …)hasAccessToLabel(CanRead, soGET /labels/{id}and every label attach)LEFT JOIN label_tasks+label_tasks.task_id IN (SELECT id FROM tasks …)Two hand-maintained shapes of one rule is a standing drift risk, and it already cost us once: before #3660, the list resolved its projects through
getRawProjectsForUser, which drops archived projects, so a label reachable only through a task in an archived-but-owned project was returned byCanReadand hidden from the list.The change
labelVisibleCond(s, a) (builder.Cond, error)is now the single definition, used by both.hasAccessToLabelloses itsLEFT JOINonlabel_tasks— the shared cond references that table only inside an uncorrelated subquery, so the join has nothing left to do.The GHSA-hj5c-mhh2-g7jq warning moves onto the helper, restated as an instruction to callers: hand the cond to
Wherein one shot, never chain it onto the session. That flattening (A OR B OR C AND D) is what leaked every label with anylabel_tasksrow to any authenticated user.Verification
This rewrites the condition tree of the function that advisory fixed, so it was checked rather than assumed:
ORgroup is parenthesised in every shape — single-label read, list with no search, list with a text search, list with an id search, link share (single branch), and the empty-accessible-project case, which renders0=1rather than a dropped match-everythingIN.label_tasksrow pointing at a task that does not exist, a link-share project's child, and duplicate attachments. Identical label sets for every auth.CanReadmatrix unchanged. 25 users × 5 link shares × 13 labels = 390 verdicts, comparing the boolean and the returned permission against the parent commit: byte-identical. GHSA regression label Importing from Trello does not work properly #6 stays reachable only by user 13; label Add details of using NGINX Proxy Manager to the Reverse Proxy docs #13 stays hidden from the owner of the project holding its soft-deleted task.pkg/models,pkg/webtests,pkg/caldavtestspass; lint clean.On the new test
TestLabel_ReadAllMatchesCanReadpins thatReadAllreturns exactly whatCanReadallows — the invariant whose absence let the original drift go unnoticed. Mutation testing showed a pure parity assertion is structurally blind to the shared cond widening (deletingtaskNotDeletedCondfrom the helper left it green), so it also pins the expected id sets outright. Both mutations now fail it.