fix(cards): keep card labels within the card's own board - #569
Open
nickmeinhold wants to merge 4 commits into
Open
fix(cards): keep card labels within the card's own board#569nickmeinhold wants to merge 4 commits into
nickmeinhold wants to merge 4 commits into
Conversation
Labels are board-scoped (labels.boardId), but _card_labels stores only (cardId, labelId), so the schema cannot reject a card on one board holding a label from another. Every permission check on the label paths is workspace-scoped, and a workspace holds many boards, so nothing enforced it. Three writers could produce the invalid link, not one: - card.create accepts any labelPublicIds and inserts them unchecked - card.duplicate copies labels to a target list that only has to be in the same workspace, so duplicating to another board carried the source board's labels - card.addOrRemoveLabel resolves a label by publicId and links it with no board comparison Only the board importer was correct, because it creates fresh labels on the board it is building. Enforced in the repository writers rather than in the three routers, so the invariant holds for any future caller by construction. The check fails closed: an unknown card or label is refused, since without both boards there is no evidence the link is legitimate. card.duplicate now selects the labels that belong to the destination board before writing, so duplicating a card to another board still works and carries the labels that legitimately transfer, rather than failing outright. Reported indirectly: the Discord #help thread "Moving cards between board" asks for cross-board card moves. The UI cannot do it today, but card.duplicate already accepts a list on another board, which is what makes this reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PBJ4E8uw7c2Ctdyb5Dcnen
…oard copies Cage-match round 1 on this branch (Kelvin, Carnot, Tesla), 4/4 seats, all REQUEST_CHANGES. Six real findings, four addressed here. The writer census was incomplete. I grepped naming conventions, but the thing every writer must touch is the Drizzle table symbol, and `insert(cardsToLabels)` finds five, not three. labelRepo.create is the fourth: it creates a label on a caller-supplied board and links it to a caller-supplied card without comparing them. It now takes the same check, inside a transaction, so a rejected link no longer strands the label it just created. The check was not atomic. A card's board is a property of whatever list its listId points at, and card.update can repoint that, so two SELECTs followed by an INSERT left the board free to change in between. Check and insert now share one transaction with the card rows locked (FOR UPDATE OF card), so a concurrent move blocks rather than racing. card.create created the card before validating its labels, so a wrong-board label turned a corrupt success into an orphaned card plus a 500. Labels are now resolved and checked before the card exists, and the answer is a BAD_REQUEST. card.duplicate filtered the source card's labels against the destination board. Those labels all carry the SOURCE board, so the filter kept everything on a same-board copy and nothing on a cross-board one: copyLabels became a silent no-op that still reported success. It now refuses explicitly and says why. The fixture check compared two list ids, which differ even when both lists sit on one board. It compares the boards. Still open, deliberately: moving a card to another board via card.update orphans the labels it already holds. No join row is written, so a writer guard cannot see it, and what should happen to those labels is the same question kanbn#568 puts to the maintainer. Filed rather than answered here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PBJ4E8uw7c2Ctdyb5Dcnen
…lookups Cage-match round 2. Carnot and Tesla independently found that the previous commit fixed orphan-then-error in card.create, wrote the reason into a comment as a principle, and then created the same bug in card.duplicate 1200 lines later: cardRepo.create ran before the cross-board copyLabels refusal, so a rejected duplicate left a card behind. Worse, the error message tells the caller to retry with copyLabels: false, and following it produced a second card. cardRepo.create opens its own transaction, so the router cannot roll it back; the check has to precede it. It now does, and the labels are fetched once. card.create treated a partial label lookup as success: if one public ID of several did not resolve, the card was created holding fewer labels than asked for. It now requires every requested id to resolve. assertCardLabelBoardsMatch took Pick<dbClient, "select">, which erased the requirement that the caller be inside the transaction doing the insert. A future caller could have passed a plain db handle, got a passing check, and inserted outside any lock. It now takes the transaction type, so that is a compile error (verified: TS2345). The lock comment claimed more than the lock does. FOR UPDATE OF card covers card.listId changing; it does not lock lists, so a writer that retargeted a list to another board would not be serialised against it. Nothing mutates lists.boardId today. The comment now says which of those it covers. Not addressed, and named in the PR body rather than half-solved: the bare Error from the repository surfaces as 500 on the writers that do not pre-map it, and whole-batch refusal means rows already corrupted by the open move axis will fail unrelated batches. Both follow from enforcing a cross-table invariant in application code, which is the design question in the filed task. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PBJ4E8uw7c2Ctdyb5Dcnen
Cage-match round 3, and the last: the round cap is three, and two of its stop
signals have fired (my round-2 fix generated round-3 findings, and the error
typing has now been raised in all three rounds).
Tesla: three failure modes shared one string. Unknown card, unknown label and a
genuine board mismatch all threw the same bare Error, so a missing row was
reported as a board lie and every writer that did not pre-map it surfaced a
client-correctable input as a 500. There is now a CardLabelBoardError carrying a
closed violation type, and the public add-label path maps it. A middleware could
do that once for every transport, but it changes the error path for every
procedure, so it is named rather than smuggled in here.
Tesla: the multi-card SELECT FOR UPDATE had no deterministic order, so two
overlapping bulk writes could lock the same cards in opposite order and
deadlock. It orders by card id.
Carnot: the exported assertion built inArray(_, []) on empty input. It returns
early.
Tesla: every refusal test asserted .rejects.toThrow(), which a SQL error or a
lock timeout satisfies just as well as the invariant. They now name the error
class.
Verified and REJECTED, with proof:
- Carnot read getWithListAndMembersByPublicId's nested `with: { label: ... }`
and concluded duplicate passes undefined publicIds. The function flattens on
the way out (labels: card.labels.map((label) => label.label)), so the shape is
correct.
- Tesla predicted the importer could pass a pool handle inside an outer
transaction, making the inner transaction a blind second connection. import.ts
uses ctx.db throughout with no outer transaction, so that path does not exist.
Confirmed and NOT fixed: the FOR UPDATE lock is untested. Deleting the line
leaves all 12 tests green, which Tesla called correctly. A check whose outcome is
independent of the thing it checks is not a check, and that applies to my own
lock. Proving it needs a contention test, and closing the class properly needs
the schema-level decision already filed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBJ4E8uw7c2Ctdyb5Dcnen
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
1 participant
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.
Description
Labels are board-scoped (
labels.boardId), but_card_labelsstores only(cardId, labelId), so the schema cannot reject a card on one board holding a label from another. Every permission check on these paths is workspace-scoped, and a workspace holds many boards.Five writers touch the join table (
insert(cardsToLabels)). Four could produce an invalid link:card.createlabelPublicIdsuncheckedcard.duplicatecard.addOrRemoveLabellabelRepo.create(with acardId)The importer is already correct, because it creates fresh labels on the board it builds.
Reachable today through the public API and MCP, not through the UI, because the pickers are board-scoped.
The fix is enforced in the repository writers rather than the routers, so it holds for future callers. The check and the insert share one transaction with the card rows locked (
FOR UPDATE OF card, ordered by id), because a card's board is a property of whatever list itslistIdpoints at.Also fixed along the way:
card.createvalidated labels after creating the card, so a bad label meant an orphaned card and a 500. It validates first and returnsBAD_REQUEST.card.duplicatehad the same ordering problem, and retrying as its own error message suggested produced a second card.CardLabelBoardErrorcarries a closed violation type, so "unknown card", "unknown label" and "board mismatch" stop sharing one string.Known limits, stated rather than implied:
card.updatecan still move a card to another board, leaving the labels it already holds invalid. No join row is written, so a writer guard cannot see it. What should happen to those labels (drop, match by name, recreate, block the move) is a product decision and the same question as Move cards between boards #568, so I have not answered it here.FOR UPDATElock is not covered by a test. Removing it leaves all 12 green. Proving it needs a contention test.Happy to take this in a different direction if you would rather the invariant live in the schema.
Reviewing
Most of the diff is the new test file.
label.repo.tsreads as ~60 lines but is 15 insertions and 2 deletions ignoring whitespace, the rest being re-indentation from wrapping the body in a transaction.https://github.com/kanbn/kan/pull/569/files?w=1
Type of change
Checklist
Linked issue
Relates to #568 (deliberately not
Closes: that issue asks for cross-board card moves, which this PR does not implement. It fixes the label scoping that feature would otherwise expose.)12 integration tests against real Postgres. Every cross-board assertion was confirmed red before its fix, with same-board positive controls green throughout.