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] }