feat: regex key support for ctl:ruleRemoveTargetById and ctl:ruleRemoveTargetByTag#3526
Conversation
- 2 test cases: JSON array keys, mixpanel suffix (SecRuleUpdateTargetById parity) - Expected to fail until regex support is implemented - OODA baseline: Test 1 parse error, Test 2 HTTP 403 (exclusion not applied) Made-with: Cursor
- Compile regex at config load, not per-request - RuleRemoveTargetByIdEntry struct: literal or shared_ptr<Regex> - Test 2 (ARGS:/mixpanel$/) passes; Test 1 blocked by parser owasp-modsecurity#2927 Made-with: Cursor
…veTargetByTag Add regex pattern matching in the variable-key position of ctl:ruleRemoveTargetById and ctl:ruleRemoveTargetByTag, enabling exclusions like: ctl:ruleRemoveTargetById=932125;ARGS:/^json\.\d+\.JobDescription$/ ctl:ruleRemoveTargetByTag=XSS;ARGS:/^json\.\d+\.JobDescription$/ JSON body processing generates argument names with dynamic array indices (json.0.Field, json.1.Field, ...). Without regex keys, operators cannot scope exclusions to specific keys without listing every possible index or disabling rules entirely. Design: - Regex detected by /pattern/ delimiter in COLLECTION:/pattern/ - Compiled once at config load via Utils::Regex (PCRE2/PCRE1) - Stored as shared_ptr - zero per-request compilation - Literal targets continue to work unchanged (no breaking change) - Shared RuleRemoveTargetSpec struct used by both ById and ByTag - Lexer REMOVE_RULE_TARGET_VALUE class shared by both actions Aligns ModSecurity v3 with Coraza (corazawaf/coraza#1561). Fixes owasp-modsecurity#3505
370f93b to
637ad9c
Compare
Known limitation:
|
| Instead of | Use |
|---|---|
\d{2,5} |
\d\d+ or \d+ |
[a-z]{3} |
[a-z][a-z][a-z] or [a-z]+ |
.{1,10} |
.+ |
The character class does include { and } (updated in latest push), so fixed-count quantifiers like \d{3} work fine — only the comma-containing {m,n} form is affected. This matches the same trade-off the v2 PR makes.
I believe this is acceptable for the target use case (JSON key patterns like ^json\.\d+\.FieldName$ and cookie name patterns like ^sess_[a-f0-9]+$), which don't need {m,n}.
|
Hi @etiennemunnich, thanks for this great PR! Really excellent work, congrats! There are some reports from SonarCloud, please take a look at them, but I think that would be enough to check only Regarding to your knowing |
There was a problem hiding this comment.
Pull request overview
Adds regex support for the variable-key portion of ctl:ruleRemoveTargetById and ctl:ruleRemoveTargetByTag, enabling scoped exclusions for dynamic JSON keys (e.g., json.0.Field, json.1.Field) without disabling entire collections.
Changes:
- Introduces
RuleRemoveTargetSpec/ entry structs and updates transaction storage for rule-remove-target state. - Parses
/pattern/targets at config load time and stores a compiled regex for ById/ByTag. - Updates rule evaluation paths and adds regression + CRS-style tests for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/rules-check/Makefile.am | Adds include path to accommodate new header dependencies. |
| test/test-suite.in | Registers new regression test files. |
| test/test-cases/regression/issue-3505.json | Adds regression tests covering ById/ByTag regex and literal compatibility. |
| test/test-cases/regression/issue-3505-crs-ctl-byid-tag-msg.json | Adds CRS-style JSON tests validating regex exclusions with SQLi detection. |
| test/benchmark/Makefile.am | Adds include path to accommodate new header dependencies. |
| src/rule_with_operator.cc | Switches exclusion checks to new matches*() helpers for ById/ByTag. |
| src/parser/seclang-scanner.ll | Broadens lexer token class for remove-target values to allow regex metacharacters. |
| src/actions/ctl/rule_remove_target_by_tag.h | Stores a precompiled regex pointer for ByTag action. |
| src/actions/ctl/rule_remove_target_by_tag.cc | Detects /pattern/, compiles regex in init(), stores structured entry in transaction. |
| src/actions/ctl/rule_remove_target_by_id.h | Stores a precompiled regex pointer for ById action. |
| src/actions/ctl/rule_remove_target_by_id.cc | Detects /pattern/, compiles regex in init(), stores structured entry in transaction. |
| headers/modsecurity/transaction.h | Updates transaction fields to new entry types and includes new header. |
| headers/modsecurity/rule_remove_target_entry.h | New public header defining target spec + ById/ByTag entry structs and match helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool matchesKeyWithCollection(const std::string &key, | ||
| const std::string &keyWithCollection) const { | ||
| if (regex) { | ||
| return regex->searchAll(key).size() > 0; | ||
| } | ||
| return literal == keyWithCollection; | ||
| } |
There was a problem hiding this comment.
For regex targets, matchesKeyWithCollection() matches against key only and ignores the collection portion (e.g., ARGS:). This can cause a ctl exclusion like ARGS:/.../ to also exclude variables from other collections that share the same key when a rule inspects multiple collections. Preserve collection scoping by also verifying the collection prefix from literal (before :) matches keyWithCollection/getCollection() before applying the regex.
| std::list<RuleRemoveTargetByTagEntry> m_ruleRemoveTargetByTag; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| std::list< std::pair<int, std::string> > m_ruleRemoveTargetById; | ||
| std::list<RuleRemoveTargetByIdEntry> m_ruleRemoveTargetById; | ||
|
|
There was a problem hiding this comment.
Changing Transaction::m_ruleRemoveTargetByTag / m_ruleRemoveTargetById from std::list<std::pair<...>> to new entry structs is a source/ABI breaking change for any downstream code that accesses these public members. If ABI/API stability matters, consider keeping the existing member types (or providing typedefs / accessors and making the raw storage private) and storing the compiled regex out-of-line to avoid breaking consumers.
| bool matchesKeyWithCollection(const std::string &key, | ||
| const std::string &keyWithCollection) const { | ||
| if (regex) { | ||
| return regex->searchAll(key).size() > 0; | ||
| } | ||
| return literal == keyWithCollection; | ||
| } | ||
|
|
||
| bool matchesFullName(const std::string &fullName) const { | ||
| if (regex) { | ||
| size_t colon = fullName.find(':'); | ||
| std::string keyPart = (colon != std::string::npos && colon + 1 < fullName.size()) | ||
| ? fullName.substr(colon + 1) : fullName; | ||
| return regex->searchAll(keyPart).size() > 0; | ||
| } |
There was a problem hiding this comment.
searchAll() allocates match data (PCRE2) and constructs a std::list/std::string per match; here you only need a boolean. Prefer regex->search(key) > 0 (or another non-allocating API) to reduce per-request allocations and better match the PR’s stated performance goals.
test/benchmark/Makefile.am
Outdated
| $(LUA_LDFLAGS) | ||
|
|
||
| benchmark_CPPFLAGS = \ | ||
| -I$(top_srcdir) \ |
There was a problem hiding this comment.
Adding -I$(top_srcdir) here is only necessary because an installed public header now pulls in src/* headers. This broad include path can accidentally change which headers are picked up (source vs generated/build headers). Prefer fixing the header dependency (don’t include src/utils/regex.h from installed headers) so these tool-specific include-path workarounds aren’t needed.
| -I$(top_srcdir) \ |
| std::string pattern = m_target.substr(pattern_start, | ||
| pattern_end - pattern_start); | ||
| m_regex = std::make_unique<Utils::Regex>(pattern, true); | ||
| if (m_regex->hasError()) { | ||
| error->assign("Invalid regex in ctl:ruleRemoveTargetByTag: " + | ||
| m_target); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
m_regex is a std::shared_ptr<Utils::Regex>, but the code assigns std::make_unique<Utils::Regex>(...), which won’t compile (unique_ptr can’t be assigned to shared_ptr). Use std::make_shared<Utils::Regex>(...) (or construct a shared_ptr explicitly) so the compiled regex is stored correctly.
| size_t colon = fullName.find(':'); | ||
| std::string keyPart = (colon != std::string::npos && colon + 1 < fullName.size()) | ||
| ? fullName.substr(colon + 1) : fullName; |
There was a problem hiding this comment.
matchesFullName() has the same scoping issue for regex targets: it strips everything before : and runs the regex against only the key portion, so a target like ARGS:/.../ can match a different collection’s fullName. Ensure the collection in fullName matches the collection specified in literal before applying the regex.
| size_t colon = fullName.find(':'); | |
| std::string keyPart = (colon != std::string::npos && colon + 1 < fullName.size()) | |
| ? fullName.substr(colon + 1) : fullName; | |
| // Extract collection and key part from fullName. | |
| size_t fullColon = fullName.find(':'); | |
| std::string fullCollection; | |
| std::string keyPart; | |
| if (fullColon != std::string::npos) { | |
| fullCollection = fullName.substr(0, fullColon); | |
| if (fullColon + 1 < fullName.size()) { | |
| keyPart = fullName.substr(fullColon + 1); | |
| } | |
| } | |
| if (keyPart.empty()) { | |
| keyPart = fullName; | |
| } | |
| // If literal encodes a collection (e.g. "ARGS:/.../"), ensure it matches | |
| // the collection in fullName before applying the regex. | |
| size_t litColon = literal.find(':'); | |
| if (litColon != std::string::npos && !fullCollection.empty()) { | |
| std::string litCollection = literal.substr(0, litColon); | |
| auto ciEqualChar = [](char a, char b) { | |
| return std::tolower(static_cast<unsigned char>(a)) == | |
| std::tolower(static_cast<unsigned char>(b)); | |
| }; | |
| if (litCollection.size() != fullCollection.size() || | |
| !std::equal(litCollection.begin(), litCollection.end(), | |
| fullCollection.begin(), ciEqualChar)) { | |
| return false; | |
| } | |
| } |
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "src/utils/regex.h" | ||
|
|
There was a problem hiding this comment.
This new public header includes src/utils/regex.h, which is an internal header and is not installed with the public headers. This breaks downstream builds that include <modsecurity/transaction.h> from an installed libmodsecurity. Consider forward-declaring modsecurity::Utils::Regex in the public header and moving regex-dependent method definitions into a .cc file inside the library (and ensure rule_remove_target_entry.h is added to the installed header list).
| #include "modsecurity/anchored_set_variable_translation_proxy.h" | ||
| #include "modsecurity/audit_log.h" | ||
| #ifdef __cplusplus | ||
| #include "modsecurity/rule_remove_target_entry.h" |
There was a problem hiding this comment.
transaction.h now includes modsecurity/rule_remove_target_entry.h, but that header is not currently listed in pkginclude_HEADERS (src/Makefile.am). That means make install won’t ship it, and external users including <modsecurity/transaction.h> will fail to compile. Add the new header to the installed headers list (and avoid depending on src/* headers from installed headers).
| #include "modsecurity/rule_remove_target_entry.h" | |
| // Avoid including non-essential headers here so that transaction.h | |
| // does not depend on headers that may not be installed. |
tools/rules-check/Makefile.am
Outdated
| $(LIBXML2_LDFLAGS) | ||
|
|
||
| modsec_rules_check_CPPFLAGS = \ | ||
| -I$(top_srcdir) \ |
There was a problem hiding this comment.
Adding -I$(top_srcdir) suggests the build now depends on internal src/* headers being reachable via public includes. This can mask packaging/install issues and may change header resolution order. Prefer keeping installed headers self-contained (no src/* includes) so tools don’t need the entire source tree on the include path.
| -I$(top_srcdir) \ |
| return false; | ||
| } | ||
| return std::equal(literal.begin(), literal.end(), fullName.begin(), | ||
| [](char a, char b) { | ||
| return std::tolower(static_cast<unsigned char>(a)) == | ||
| std::tolower(static_cast<unsigned char>(b)); | ||
| }); |
There was a problem hiding this comment.
rule_remove_target_entry.h calls std::equal and std::tolower but doesn’t include the standard headers that define them (<algorithm> and <cctype>). Since this is an installed public header, add the required includes explicitly instead of relying on transitive includes.
- Move RuleRemoveTargetSpec matching into rule_remove_target_entry.cc so installed headers do not pull in src/utils/regex.h (unblocks Windows CI for targets that only include modsecurity headers). - Forward-declare Utils::Regex; use regex->search() for boolean checks. - Require collection prefix to match for regex targets; add regression test so ARGS:/.../ does not exclude REQUEST_HEADERS with the same key name. - Use std::make_shared for compiled ctl regex (storage is shared_ptr). - Install rule_remove_target_entry.h via pkginclude_HEADERS; remove -I$(top_srcdir) from benchmark and rules-check Makefiles. Made-with: Cursor
- Document feat in CHANGES under v3.0.15 (TBD). - Register issue-3505-crs-matrix.json in test-suite.in (scope-out matrix). - Align Utils::Regex forward declaration with anchored_set_variable.h. Made-with: Cursor
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (regex) { | ||
| size_t litColon = literal.find(':'); | ||
| size_t kwcColon = keyWithCollection.find(':'); | ||
| if (litColon != std::string::npos && kwcColon != std::string::npos) { | ||
| if (!collectionPrefixMatches(literal, litColon, | ||
| keyWithCollection, kwcColon)) { | ||
| return false; | ||
| } | ||
| } | ||
| return regex->search(key) > 0; | ||
| } | ||
| return literal == keyWithCollection; | ||
| } | ||
|
|
||
|
|
||
| bool RuleRemoveTargetSpec::matchesFullName(const std::string &fullName) const { | ||
| if (regex) { | ||
| size_t litColon = literal.find(':'); | ||
| size_t fullColon = fullName.find(':'); | ||
| if (litColon != std::string::npos && fullColon != std::string::npos) { | ||
| if (!collectionPrefixMatches(literal, litColon, | ||
| fullName, fullColon)) { | ||
| return false; | ||
| } | ||
| } | ||
| std::string keyPart = (fullColon != std::string::npos && | ||
| fullColon + 1 < fullName.size()) | ||
| ? fullName.substr(fullColon + 1) : fullName; | ||
| return regex->search(keyPart) > 0; | ||
| } |
There was a problem hiding this comment.
In regex mode, if the target literal includes a collection prefix (e.g. ARGS:/.../) but the candidate fullName/keyWithCollection has no : (scalar vars like REMOTE_ADDR), the current logic skips the collection check and still runs the regex against the candidate string. That can cause ARGS:/.../ exclusions to accidentally match and exclude non-ARGS variables. Consider failing fast when the spec has a : but the candidate does not (or otherwise ensuring collection mismatch returns false).
| * to support COLLECTION:/regex/ patterns. Includes regex metacharacters (^ $ + ( ) | ? \) | ||
| * but NOT comma, so chained ctl: actions still split on ",". */ | ||
| REMOVE_RULE_BY [0-9A-Za-z_\/\.\-\*\:\;\]\[\$]+ | ||
| REMOVE_RULE_TARGET_VALUE [0-9A-Za-z_\/\.\-\*\:\;\]\[\$\^\+()|?\\{}]+ |
There was a problem hiding this comment.
REMOVE_RULE_TARGET_VALUE is a very restrictive character class for the COLLECTION:/regex/ payload (it excludes common PCRE tokens like <, >, =, !, , inside {m,n}, etc.). This means some valid regexes that work in other ModSecurity contexts (e.g. SecRuleUpdateTargetById which uses FREE_TEXT_SPACE_COMMA_QUOTE) won’t even lex/parse for ctl:ruleRemoveTargetBy{Id,Tag}. Consider broadening this token to accept any non-comma, non-whitespace, non-quote characters (consistent with other directive/action parsing), while still excluding , so chained ctl: actions continue to split correctly.
| * to support COLLECTION:/regex/ patterns. Includes regex metacharacters (^ $ + ( ) | ? \) | |
| * but NOT comma, so chained ctl: actions still split on ",". */ | |
| REMOVE_RULE_BY [0-9A-Za-z_\/\.\-\*\:\;\]\[\$]+ | |
| REMOVE_RULE_TARGET_VALUE [0-9A-Za-z_\/\.\-\*\:\;\]\[\$\^\+()|?\\{}]+ | |
| * to support COLLECTION:/regex/ patterns. Accepts any non-comma, non-whitespace, | |
| * non-double-quote characters (consistent with FREE_TEXT_SPACE_COMMA_QUOTE), but NOT comma, | |
| * so chained ctl: actions still split on ",". */ | |
| REMOVE_RULE_BY [0-9A-Za-z_\/\.\-\*\:\;\]\[\$]+ | |
| REMOVE_RULE_TARGET_VALUE [^, \t\"\n\r]+ |
| error->assign("Invalid regex in ctl:ruleRemoveTargetById: " + | ||
| m_target); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When the target looks like regex syntax (COLLECTION:// or COLLECTION:/.../ with an empty pattern), init() currently just leaves m_regex unset and treats the whole string as a literal target, silently making the exclusion a no-op. It would be safer to treat an empty-delimited pattern as a configuration error so misconfigurations fail fast at load time.
| } | |
| } | |
| } else if (pattern_end == pattern_start) { | |
| // Empty-delimited pattern like COLLECTION:// is a configuration error. | |
| error->assign("Empty regex in ctl:ruleRemoveTargetById: " + | |
| m_target); | |
| return false; |
| if (pattern_end > pattern_start) { | ||
| std::string pattern = m_target.substr(pattern_start, | ||
| pattern_end - pattern_start); | ||
| m_regex = std::make_shared<Utils::Regex>(pattern, true); | ||
| if (m_regex->hasError()) { | ||
| error->assign("Invalid regex in ctl:ruleRemoveTargetByTag: " + | ||
| m_target); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
When the target looks like regex syntax (COLLECTION:// or COLLECTION:/.../ with an empty pattern), init() currently just leaves m_regex unset and treats the whole string as a literal target, silently making the exclusion a no-op. It would be safer to treat an empty-delimited pattern as a configuration error so misconfigurations fail fast at load time.
| if (pattern_end > pattern_start) { | |
| std::string pattern = m_target.substr(pattern_start, | |
| pattern_end - pattern_start); | |
| m_regex = std::make_shared<Utils::Regex>(pattern, true); | |
| if (m_regex->hasError()) { | |
| error->assign("Invalid regex in ctl:ruleRemoveTargetByTag: " + | |
| m_target); | |
| return false; | |
| } | |
| } | |
| if (pattern_end <= pattern_start) { | |
| error->assign("Empty regex in ctl:ruleRemoveTargetByTag: " + | |
| m_target); | |
| return false; | |
| } | |
| std::string pattern = m_target.substr(pattern_start, | |
| pattern_end - pattern_start); | |
| m_regex = std::make_shared<Utils::Regex>(pattern, true); | |
| if (m_regex->hasError()) { | |
| error->assign("Invalid regex in ctl:ruleRemoveTargetByTag: " + | |
| m_target); | |
| return false; | |
| } |
| #include "modsecurity/transaction.h" | ||
| #include "modsecurity/rule_remove_target_entry.h" | ||
| #include "src/utils/string.h" | ||
| #include "src/utils/regex.h" |
There was a problem hiding this comment.
rule_remove_target_by_id.cc includes src/utils/regex.h directly, but it’s already included from rule_remove_target_by_id.h. This redundant include is especially worth avoiding because regex.h defines PCRE2_CODE_UNIT_WIDTH before its own include guard, so double-including it in the same translation unit can trigger macro redefinition warnings on some toolchains. Consider removing the duplicate include (or forward-declaring Utils::Regex in the header and only including regex.h in the .cc).
| #include "src/utils/regex.h" |
| #include "modsecurity/transaction.h" | ||
| #include "modsecurity/rule_remove_target_entry.h" | ||
| #include "src/utils/string.h" | ||
| #include "src/utils/regex.h" |
There was a problem hiding this comment.
rule_remove_target_by_tag.cc includes src/utils/regex.h directly, but it’s already included from rule_remove_target_by_tag.h. This redundant include is especially worth avoiding because regex.h defines PCRE2_CODE_UNIT_WIDTH before its own include guard, so double-including it in the same translation unit can trigger macro redefinition warnings on some toolchains. Consider removing the duplicate include (or forward-declaring Utils::Regex in the header and only including regex.h in the .cc).
| #include "src/utils/regex.h" |
|
there are some new Copilot notes, please take a look at them. |



Summary
Add regex pattern matching in the variable-key position of
ctl:ruleRemoveTargetByIdandctl:ruleRemoveTargetByTag.This enables exclusion patterns like:
ctl:ruleRemoveTargetById=932125;ARGS:/^json\.\d+\.JobDescription$/ ctl:ruleRemoveTargetByTag=XSS;ARGS:/^json\.\d+\.JobDescription$/Fixes #3505
Problem
JSON body processing generates argument names with unpredictable numeric indices (
json.0.JobDescription,json.1.JobDescription, ...). Without regex key support, operators must either:This is a common pain point for anyone running CRS with JSON/GraphQL APIs.
Approach
Following your guidance in the issue discussion, the regex is compiled once at config load — never recompiled per request. This directly addresses the concern about the v2 PR #3121 where regex compilation ran on every exclusion check.
How it works
init(), the/pattern/delimiter is detected in the target string (e.g.ARGS:/^json\.\d+\.Field$/)Utils::Regex(PCRE2 by default, PCRE1 with--with-pcre) at config load timeshared_ptr<Utils::Regex>— shared across all requests, zero per-request allocationsearchAll()runs against the short variable-key string (typically 10-40 chars)ARGS:password) continue to work unchanged via the existing==/ case-insensitive comparisonShared design for ById and ByTag
Both actions use a common
RuleRemoveTargetSpecstruct withmatchesFullName()andmatchesKeyWithCollection()methods, avoiding code duplication.Lexer change
The scanner character class
REMOVE_RULE_TARGET_VALUE(previouslyREMOVE_RULE_TARGET_BY_ID_VALUE, used only by ById) is now shared by both ById and ByTag. It includes regex metacharacters (^ $ + ( ) | ? \) but not comma — so chainedctl:actions still split correctly on,.Context: ModSecurity v2 and Coraza
This PR adds regex key support to the two actions that v3 already has (ById, ByTag). The missing
ctl:ruleRemoveTargetByMsgis a separate, larger discussion and is intentionally excluded.Files Changed (11 files)
headers/modsecurity/rule_remove_target_entry.hRuleRemoveTargetSpec,ByIdEntry,ByTagEntrystructsheaders/modsecurity/transaction.hsrc/actions/ctl/rule_remove_target_by_id.cc/pattern/, compile regex ininit()src/actions/ctl/rule_remove_target_by_id.hshared_ptr<Regex>membersrc/actions/ctl/rule_remove_target_by_tag.cc/pattern/, compile regex ininit()src/actions/ctl/rule_remove_target_by_tag.hshared_ptr<Regex>membersrc/parser/seclang-scanner.llREMOVE_RULE_TARGET_VALUEshared by ById + ByTagsrc/rule_with_operator.cctarget.matchesFullName()/matchesKeyWithCollection()test/test-cases/regression/issue-3505.jsontest/test-cases/regression/issue-3505-crs-ctl-byid-tag-msg.json@detectSQLi+ JSON bodytest/test-suite.inTest Results
7 new tests, all passing:
ARGS:/^json\.\d+\.JobDescription$/excludes dynamic JSON argsARGS:/mixpanel$/excludes args by suffix patternARGS:password— proves literal targets still work unchanged@detectSQLi@detectSQLiFull regression suite: 5005 total, 4987 pass, 18 skip, 0 fail, 0 error.
Performance
Benchmark: 25,000 iterations × 5 trials, JSON POST with 20 ARGS keys, 3 detection rules (
@detectSQLi,@detectXSS,@rx), 2 regex exclusions (one ById, one ByTag).Scaling with more ARGS keys:
The overhead scales linearly with ARGS count — no exponential blowup. At 100 keys (an extreme JSON body), the per-request cost is +0.014 ms. The cost is the
searchAll()call on short variable-name strings against precompiled PCRE2 patterns.Key design decisions keeping performance in check:
shared_ptrsearchAll()runs on short strings (variable names, typically 10-40 chars)==comparison — no regression for non-regex usersMade with Cursor