cg-bid-net: refuse a discard of cards the player does not hold

Finding 3 of the 2026-07-29 security review: bids, passes and plays
were validated properly, but a kitty discard only checked that five
cards were named -- and cg-bid--discard removes cards with
cl-set-difference, which silently ignores cards not in the hand.  A
client naming five cards it did not hold kept all fifteen and played
on with more cards than the rules allow.

New cg-bid--net-holds-p checks every named card against the seat's
hand, respecting multiplicity (five copies of one held card do not
pass).  The check joins the phase/contractor/arity checks that already
live in the discard branch of cg-net-apply-move.

Test cgt-bid-net-discard-checked fails against the previous code: the
foreign discard was accepted (returned t) and the game advanced to
play with a 15-card hand.
This commit is contained in:
Claude 2026-08-03 20:32:45 -05:00 committed by Corwin Brust
parent f8873fb8fa
commit 3b55d780b0
2 changed files with 40 additions and 1 deletions

View file

@ -143,6 +143,17 @@ and a hand exposed by an open misère is revealed to everyone."
;;;; Apply a move on the host ;;;; Apply a move on the host
(defun cg-bid--net-holds-p (hand cards)
"Return non-nil when every card in CARDS is present in HAND.
Multiplicity counts: naming one held card five times is not holding
five cards. Cards are (SUIT . RANK) conses compared with `equal'."
(let ((left (copy-sequence hand)))
(catch 'missing
(dolist (c cards t)
(if (member c left)
(setq left (cl-remove c left :test #'equal :count 1))
(throw 'missing nil))))))
(cl-defmethod cg-net-apply-move ((game cg-bid-game) seat move) (cl-defmethod cg-net-apply-move ((game cg-bid-game) seat move)
"Apply MOVE made by absolute SEAT to the host's 500 GAME. "Apply MOVE made by absolute SEAT to the host's 500 GAME.
MOVE is (bid BID), (pass), (discard CARD...) or (play CARD). Return MOVE is (bid BID), (pass), (discard CARD...) or (play CARD). Return
@ -157,7 +168,8 @@ non-nil when the move was legal and applied, so the host broadcasts."
(cg-bid--auction-act game seat nil) (setq ok t))) (cg-bid--auction-act game seat nil) (setq ok t)))
(`(discard . ,cards) (`(discard . ,cards)
(when (and (eq phase 'kitty) (eql (cg-get game :contractor) seat) (when (and (eq phase 'kitty) (eql (cg-get game :contractor) seat)
(= (length cards) 5)) (= (length cards) 5)
(cg-bid--net-holds-p (cg-bid--hand game seat) cards))
(cg-bid--discard game seat cards) (setq ok t))) (cg-bid--discard game seat cards) (setq ok t)))
(`(play ,card) (`(play ,card)
(when (and (eq phase 'play) (eql (cg-get game :turn) seat) (when (and (eq phase 'play) (eql (cg-get game :turn) seat)

View file

@ -354,6 +354,33 @@ silent default."
(should-not (cg-net-apply-move g 0 '(play (0 . 0)))) ; wrong phase (should-not (cg-net-apply-move g 0 '(play (0 . 0)))) ; wrong phase
)) ))
(ert-deftest cgt-bid-net-discard-checked ()
"The host rejects a kitty discard of cards the seat does not hold.
Finding 3 of the 2026-07-29 review: the other move checks were real,
but a discard only counted its cards, and cl-set-difference silently
ignores cards that are not in the hand -- so five phantom discards
left a 15-card hand in play."
(let* ((cg-bid--human-seats '(0 1 2 3))
(g (cg-bid--deal (make-instance 'cg-bid-game) 3)))
;; put the game where a discard is legal: West won the auction
(cg-put g :phase 'kitty)
(cg-put g :contractor 1)
(cg-bid--set-hand g 1 (append (cg-get g :kitty) (cg-bid--hand g 1)))
(let* ((hand (copy-sequence (cg-bid--hand g 1)))
(foreign (cl-subseq (cg-bid--hand g 2) 0 5)))
;; five cards West does not hold: refused, nothing moves
(should-not (cg-net-apply-move g 1 (cons 'discard foreign)))
(should (equal hand (cg-bid--hand g 1)))
(should (eq 'kitty (cg-get g :phase)))
;; one held card named five times: also refused
(should-not (cg-net-apply-move g 1
(cons 'discard (make-list 5 (car hand)))))
(should (eq 'kitty (cg-get g :phase)))
;; an honest five from the hand is accepted and play begins
(should (cg-net-apply-move g 1 (cons 'discard (cl-subseq hand 0 5))))
(should (eq 'play (cg-get g :phase)))
(should (= 10 (length (cg-bid--hand g 1)))))))
(ert-deftest cgt-bid-net-loopback () (ert-deftest cgt-bid-net-loopback ()
"A 500 move travels client -> host -> filtered broadcast over TCP." "A 500 move travels client -> host -> filtered broadcast over TCP."
(condition-case _ (condition-case _