From 3a44689875ca0c40c54c0ab0482024d9f8f6cf50 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Fri, 11 Sep 2026 14:51:28 -0700 Subject: [PATCH] Check the SORT destination that is actually written sortGetKeys() scans the SORT arguments for the STORE option to report the destination key to the ACL layer, to COMMAND GETKEYS and to the cluster slot check. When it found a STORE it recorded the destination position but left the scan cursor on the STORE token, so the next iteration examined the destination name as if it were an option keyword. A destination named "by" or "get" therefore consumed the argument after it and "limit" consumed two, hiding a later STORE clause. A destination named "store" was taken for another STORE clause, reporting whatever followed it instead. Either way the reported key and the key SORT writes are different, because SORT keeps the last STORE. Permissions were checked against the reported key while the write landed on the real one, so a user could write outside their allowed key patterns. COMMAND GETKEYS reported the wrong key, and in cluster mode the destination was added to the reported key's slot, leaving a key that KEYS lists but EXISTS cannot find. Advance the cursor past the destination so a key name can no longer be reparsed as an option. Signed-off-by: Madelyn Olson --- src/db.c | 4 ++++ tests/unit/acl-v2.tcl | 31 +++++++++++++++++++++++++++++++ tests/unit/sort.tcl | 16 ++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/src/db.c b/src/db.c index ee295e1ce..8860340f4 100644 --- a/src/db.c +++ b/src/db.c @@ -2888,6 +2888,10 @@ int sortGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult found_store = 1; keys[num].pos = i + 1; /* */ keys[num].flags = CMD_KEY_OW | CMD_KEY_UPDATE; + /* Skip the destination. It is a key name, so it must never be + * examined as an option: a key that spells one would hide the + * later STORE clause that SORT actually writes to. */ + i++; break; } } diff --git a/tests/unit/acl-v2.tcl b/tests/unit/acl-v2.tcl index 767818aa6..8aa52211c 100644 --- a/tests/unit/acl-v2.tcl +++ b/tests/unit/acl-v2.tcl @@ -628,6 +628,37 @@ start_server {tags {"acl external:skip"}} { r del v1 mylist } + test {Test SORT STORE destination that is named like an option} { + # Every decoy destination below is permitted, so the only reason to + # reject a command is the real destination the server writes to. + r ACL setuser test-sort-store on nopass ~allowed:* ~by ~get ~limit ~alpha +@all + r rpush allowed:src c b a + + # A dedicated client, because deleting the user below kills it. + set r3 [valkey_client] + $r3 auth test-sort-store nopass + + # A destination spelling an option that takes arguments hides the later + # STORE clause that SORT actually uses. + foreach keyword {by get limit} { + assert_equal "User test-sort-store has no permissions to access the 'forbidden:dst' key" \ + [r ACL DRYRUN test-sort-store SORT allowed:src ALPHA STORE $keyword STORE forbidden:dst] + assert_error "*NOPERM*key*" {$r3 sort allowed:src ALPHA STORE $keyword STORE forbidden:dst} + assert_equal 0 [r exists forbidden:dst] + } + + # A destination spelling STORE reports whatever follows it instead. + assert_equal "User test-sort-store has no permissions to access the 'store' key" \ + [r ACL DRYRUN test-sort-store SORT allowed:src STORE store alpha] + assert_error "*NOPERM*key*" {$r3 sort allowed:src STORE store alpha} + assert_equal 0 [r exists store] + + # cleanup + $r3 close + r ACL deluser test-sort-store + r del allowed:src + } + test {Test DRYRUN with wrong number of arguments} { r ACL setuser test-dry-run +@all ~v* diff --git a/tests/unit/sort.tcl b/tests/unit/sort.tcl index 0626aaa02..9241fcb7c 100644 --- a/tests/unit/sort.tcl +++ b/tests/unit/sort.tcl @@ -120,6 +120,22 @@ foreach command {SORT SORT_RO} { r command getkeys sort abc store invalid store stillbad store def } {abc def} + test "SORT extracts STORE correctly when the destination is named like an option" { + # A destination is a key name, never an option keyword, even when it + # spells one. Parsed as an option it would consume the arguments that + # follow it and hide the later STORE that SORT really writes to. + foreach keyword {by get limit} { + assert_equal {abc def} [r command getkeys sort abc store $keyword store def] + assert_equal [list abc $keyword] [r command getkeys sort abc store $keyword] + } + + # A destination spelling STORE would instead be taken for another STORE + # clause, reporting whatever follows it. + assert_equal {abc store} [r command getkeys sort abc store store alpha] + assert_equal {abc STORE} [r command getkeys sort abc store STORE alpha] + assert_equal {abc store} [r command getkeys sort abc store store by w_*] + } + test "SORT DESC" { assert_equal [lsort -decreasing -integer $result] [r sort tosort DESC] }