Report exact dbid for COPY in ACL LOG when db= access is denied#3888
Merged
Conversation
Extend commandDbIdArgs with an out-parameter that returns the argv index of each reported dbid, and use it in ACLSelectorCheckCmd instead of the cmd->proc-based offset table, so we don't need to maintain the whole block, just need to maintain get_dbid_args. The fix is for COPY: the old chain fell through to 0, so the "object" field in ACL LOG was always showed "copy", it now reports the first denied dbid. Also see valkey-io#3801 for more details, DB ACL was added in valkey-io#2309. Signed-off-by: Binbin <binloveplay1314@qq.com>
📝 WalkthroughWalkthroughHelpers now return argv-index positions for DB-id tokens; callers (ACL checks and MULTI) read dbids via those positions and set key/index reporting from the offending argument position. Tests updated to assert the reported db argument value is included in DRYRUN errors and ACL LOG entries. ChangesACL Database ID Position Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server.h (1)
2554-2554: ⚡ Quick winConsider documenting the typedef parameters, especially the new
positionsout-parameter.The
commandDbIdArgstypedef signature has been extended with a new out-parameter, but there's no documentation explaining:
- The purpose of the
positionsparameter- Whether
positionscan be NULL (appears optional based on PR context)- Memory ownership (who allocates, who frees the returned array)
- The relationship between
countand thepositionsarray size📝 Suggested documentation
+/* Helper function to get database ID arguments from a command. + * Returns an array of database IDs, with count set to the number of IDs. + * If positions is non-NULL, it will be set to a newly allocated array + * containing the argv index of each database ID (caller must free). + * Returns NULL if the command has no database ID arguments. */ typedef int *commandDbIdArgs(robj **argv, int argc, int *count, int **positions);As per coding guidelines: "Document why code exists, not just what it does; document all functions in C code"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.h` at line 2554, Add a header comment above the typedef commandDbIdArgs that documents each parameter: explain that argv/argc are the command arguments, count is an out-parameter returning the number of DB-IDs found, and positions is an optional out-parameter that, when non-NULL, will be set to point to an array of integer positions (size equal to *count) indicating where DB-ID arguments were found; state whether positions may be NULL, who allocates the returned positions array (caller or callee), and who is responsible for freeing it, and clarify the relationship between count and the positions array length. Make the wording explicit and follow the file's existing C-doc style so callers know allocation/ownership and nullability semantics for commandDbIdArgs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/server.h`:
- Line 2554: Add a header comment above the typedef commandDbIdArgs that
documents each parameter: explain that argv/argc are the command arguments,
count is an out-parameter returning the number of DB-IDs found, and positions is
an optional out-parameter that, when non-NULL, will be set to point to an array
of integer positions (size equal to *count) indicating where DB-ID arguments
were found; state whether positions may be NULL, who allocates the returned
positions array (caller or callee), and who is responsible for freeing it, and
clarify the relationship between count and the positions array length. Make the
wording explicit and follow the file's existing C-doc style so callers know
allocation/ownership and nullability semantics for commandDbIdArgs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b7cb4114-eb5f-42f1-a283-10ac3852df05
📒 Files selected for processing (5)
src/acl.csrc/db.csrc/multi.csrc/server.htests/unit/acl-v2.tcl
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #3888 +/- ##
============================================
+ Coverage 76.66% 76.70% +0.03%
============================================
Files 162 162
Lines 80728 80731 +3
============================================
+ Hits 61893 61922 +29
+ Misses 18835 18809 -26
🚀 New features to boost your workflow:
|
dvkashapov
approved these changes
Jun 2, 2026
dvkashapov
left a comment
Member
There was a problem hiding this comment.
LGTM! Very good, thank you!
zuiderkwast
reviewed
Jun 4, 2026
zuiderkwast
reviewed
Jun 5, 2026
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/acl.c (1)
1856-1884:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCurrent-db ACL enforcement still gets skipped for MOVE/COPY.
This branch still short-circuits the fallback
ACLSelectorCanAccessDb(selector, dbid)check for any command withcmd->get_dbid_args. That is fine forSELECT/SWAPDB, butmoveDbIdArgs()only reports the destination DB andcopyDbIdArgs()only reports explicitDB <dbid>clauses, whilemoveCommand()andcopyCommand()insrc/db.cstill operate onc->dbas the source/default DB. A user denied on the current DB can therefore still access it viaCOPY key key2orMOVE key <allowed-db>as long as the explicit DB args pass.🔒 Suggested fix
- if (cmd->get_dbid_args) { + if (cmd->get_dbid_args) { int count = 0; int *positions = cmd->get_dbid_args(argv, argc, &count); if (positions) { for (int i = 0; i < count; i++) { long long dbid; /* The helper has already validated argv[positions[i]] as a * valid in-range dbid, so this should never fail. */ serverAssert(getLongLongFromObject(argv[positions[i]], &dbid) == C_OK); if (!ACLSelectorCanAccessDb(selector, (int)dbid)) { if (keyidxptr) *keyidxptr = positions[i]; zfree(positions); return ACL_DENIED_DB; } } zfree(positions); } - } else if ((cmd->flags & CMD_ALL_DBS) && !(selector->flags & SELECTOR_FLAG_ALLDBS)) { + } + + if ((cmd->flags & CMD_ALL_DBS) && !(selector->flags & SELECTOR_FLAG_ALLDBS)) { for (int i = 0; i < server.dbnum; i++) { if (!ACLSelectorCanAccessDb(selector, i)) { if (keyidxptr) *keyidxptr = 0; return ACL_DENIED_DB; } } } else if (shouldRestrictCmd(cmd) && !ACLSelectorCanAccessDb(selector, dbid)) { if (keyidxptr) *keyidxptr = 0; return ACL_DENIED_DB; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acl.c` around lines 1856 - 1884, The get_dbid_args branch currently only checks DBs returned by cmd->get_dbid_args, which misses the command's source/default DB for commands like moveCommand/copyCommand whose helpers (moveDbIdArgs/copyDbIdArgs) only report destination/explicit DBs; add an extra check after the positions loop: if cmd is moveCommand or copyCommand (or if the get_dbid_args used is moveDbIdArgs/copyDbIdArgs) then also call ACLSelectorCanAccessDb(selector, dbid) for the source/default dbid and return ACL_DENIED_DB (setting keyidxptr appropriately) when denied so users blocked on the current DB cannot bypass via MOVE/COPY.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/multi.c`:
- Around line 117-129: Fix the formatting in the multi.c hunk by removing the
stray space so the assertion call uses serverAssert(...) instead of serverAssert
(...); locate the block guarded by mc->cmd->get_dbid_args && mc->cmd->proc ==
selectCommand (references: mc->cmd->get_dbid_args, getLongLongFromObject,
serverAssert, c->mstate->transaction_db_id) and run clang-format-18 on the file
to apply the project’s C formatting rules before committing.
---
Outside diff comments:
In `@src/acl.c`:
- Around line 1856-1884: The get_dbid_args branch currently only checks DBs
returned by cmd->get_dbid_args, which misses the command's source/default DB for
commands like moveCommand/copyCommand whose helpers (moveDbIdArgs/copyDbIdArgs)
only report destination/explicit DBs; add an extra check after the positions
loop: if cmd is moveCommand or copyCommand (or if the get_dbid_args used is
moveDbIdArgs/copyDbIdArgs) then also call ACLSelectorCanAccessDb(selector, dbid)
for the source/default dbid and return ACL_DENIED_DB (setting keyidxptr
appropriately) when denied so users blocked on the current DB cannot bypass via
MOVE/COPY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3366561d-1ba7-45ec-bb49-949e9afed4a4
📒 Files selected for processing (4)
src/acl.csrc/db.csrc/multi.csrc/server.h
zuiderkwast
approved these changes
Jun 8, 2026
zuiderkwast
left a comment
Contributor
There was a problem hiding this comment.
Yes, I think this is better. Thanks!
Member
Author
|
Thanks for the review, i am merging it. |
valkeyrie-ops Bot
pushed a commit
that referenced
this pull request
Jun 9, 2026
Rework commandDbIdArgs so each helper returns the argv index of every dbid argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr directly, replacing the cmd->proc-based offset table. The caller now reads the dbid value via getLongLongFromObject(argv[positions[i]], ...). New db=-checked commands only need to implement their own *DbIdArgs helper. The fix is for COPY: the old chain fell through to 0, so the "object" field in ACL LOG was always showed "copy", it now reports the first denied dbid. Based on #3801, also see it for more details, DB ACL was added in #2309. Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot
pushed a commit
that referenced
this pull request
Jun 10, 2026
Rework commandDbIdArgs so each helper returns the argv index of every dbid argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr directly, replacing the cmd->proc-based offset table. The caller now reads the dbid value via getLongLongFromObject(argv[positions[i]], ...). New db=-checked commands only need to implement their own *DbIdArgs helper. The fix is for COPY: the old chain fell through to 0, so the "object" field in ACL LOG was always showed "copy", it now reports the first denied dbid. Based on #3801, also see it for more details, DB ACL was added in #2309. Signed-off-by: Binbin <binloveplay1314@qq.com>
ranshid
added a commit
that referenced
this pull request
Jun 11, 2026
# Backport sweep for 9.1 Automated cherry-picks from PRs marked "To be backported". ## Applied | Source PR | Title | Detail | |---|---|---| | #3743 | Fix buffered_reply assert in HFE commands with module keyspace notifications | cherry-picked in a prior sweep | | #3766 | Fix flaky block_keyspace_notification test for HGETDEL notify race | cherry-picked in a prior sweep | | #3800 | Fix heap-use-after-free in ACL LOAD when client free is deferred | cherry-picked in a prior sweep | | #3723 | Fix double-finish and RESP reply violation in cluster slot migration | cherry-picked in a prior sweep | | #3872 | Redacting customer information when hide_user_data_from_log is true in rdb.c, networking.c, debug.c and t_hash | cherry-picked in a prior sweep | | #3846 | Fix use-after-free in VM_RegisterClusterMessageReceiver | cherry-picked in a prior sweep | | #3806 | Add ALL_DBS flag to CLUSTER FLUSHSLOT for database-level ACL | cherry-picked in a prior sweep | | #3847 | Harden SENTINEL commands and config rewrite against control-character injection | | | #3801 | Validate every DB clause in COPY against ACL db= permissions | | | #3851 | Replace AUTOMATION_PAT with Valkeyrie Bot GitHub App token | | | #3848 | Fix cluster AUX-field control-character and delimiter injection | | | #3544 | Revert "IO-Threads redesign cleanup work (#3544)" | cherry-picked in a prior sweep | | #3888 | Report exact dbid for COPY in ACL LOG when db= access is denied | conflicts resolved by Claude Code | | #3942 | Fix shard_id format specifier in UPDATE message log | | | #3941 | Avoid random() % 0 undefined behaviour when cluster-node-timeout < 30 | | --- *Generated by valkey-ci-agent using Claude Code.* --------- Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: chx9 <lovelypiska@outlook.com> Signed-off-by: zackcam <zackcam@amazon.com> Signed-off-by: Eran Ifrah <eifrah@amazon.com> Signed-off-by: Jules Lasarte <lasartej@amazon.com> Signed-off-by: jjuleslasarte <jules.lasarte@gmail.com> Signed-off-by: akash kumar <akumdev@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: lovelypiska <lovelypiska@outlook.com> Co-authored-by: zackcam <zackcam@amazon.com> Co-authored-by: eifrah-aws <eifrah@amazon.com> Co-authored-by: jjuleslasarte <jules.lasarte@gmail.com> Co-authored-by: Jules Lasarte <lasartej@amazon.com> Co-authored-by: Akash Kumar <45854686+akashkgit@users.noreply.github.com>
sarthakaggarwal97
added a commit
to sarthakaggarwal97/valkey
that referenced
this pull request
Jun 24, 2026
…ey-io#3888) (#345) Rework commandDbIdArgs so each helper returns the argv index of every dbid argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr directly, replacing the cmd->proc-based offset table. The caller now reads the dbid value via getLongLongFromObject(argv[positions[i]], ...). New db=-checked commands only need to implement their own *DbIdArgs helper. The fix is for COPY: the old chain fell through to 0, so the "object" field in ACL LOG was always showed "copy", it now reports the first denied dbid. Based on valkey-io#3801, also see it for more details, DB ACL was added in valkey-io#2309. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
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
4 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.
Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.
The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.
Based on #3801, also see it for more details, DB ACL was added in #2309.