Skip to content

Validate every DB clause in COPY against ACL db= permissions#3801

Open
enjoy-binbin wants to merge 1 commit into
valkey-io:unstablefrom
enjoy-binbin:fix_copy
Open

Validate every DB clause in COPY against ACL db= permissions#3801
enjoy-binbin wants to merge 1 commit into
valkey-io:unstablefrom
enjoy-binbin:fix_copy

Conversation

@enjoy-binbin
Copy link
Copy Markdown
Member

copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in #2309.

copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin requested a review from dvkashapov May 21, 2026 12:56
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label May 21, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ae560a81-e210-4f94-a9f1-decba3f176f5

📥 Commits

Reviewing files that changed from the base of the PR and between d778b37 and 60c80da.

📒 Files selected for processing (2)
  • src/db.c
  • tests/unit/acl-v2.tcl

📝 Walkthrough

Walkthrough

The copyDbIdArgs() function is rewritten to parse COPY command destination database clauses in an order-independent manner, supporting multiple DB tokens and varied argument positions. A two-pass parser validates syntax, counts DB occurrences, allocates an array, and collects all destination DB ids. Test coverage is expanded to validate the new parsing behavior across database permissions, REPLACE semantics, and DRYRUN assertions.

Changes

COPY Command Order-Independent Database Argument Parsing

Layer / File(s) Summary
COPY argument parser rewrite
src/db.c
copyDbIdArgs() changed from single fixed-position DB parsing to a two-pass, order-independent parser that validates all arguments after COPY for DB/REPLACE tokens, counts every DB occurrence, allocates an array, and returns all collected destination DB ids.
Database permission and DRYRUN tests
tests/unit/acl-v2.tcl
Test coverage extended to validate COPY operations targeting DB 1 with successful COPY calls covering REPLACE and argument-order variations, expanded permission-denied assertions for DB 2 with varied DB/REPLACE combinations, and DRYRUN variants checking both allowed and disallowed database access.

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: validating every DB clause in COPY commands against ACL db= permissions, which is the primary focus of the changeset.
Description check ✅ Passed The description clearly explains the bug being fixed (copyDbIdArgs assumed DB token was at argv[3]), provides concrete examples of bypassed restrictions, and describes the solution approach.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.69%. Comparing base (aa7488f) to head (60c80da).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #3801   +/-   ##
=========================================
  Coverage     76.69%   76.69%           
=========================================
  Files           162      162           
  Lines         80710    80721   +11     
=========================================
+ Hits          61901    61911   +10     
- Misses        18809    18810    +1     
Files with missing lines Coverage Δ
src/db.c 94.85% <100.00%> (+0.04%) ⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes This issue should get a line item in the release notes

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant