`EscapeAssertion` rewrites a nested attribute dot when the request/policy field name contains an underscore

Closed Beginner friendly
  • Is something wrong?
#277 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
92/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
cpp
Domain
security

Research direction

Start with EscapeAssertion in casbin/util/escape_assertion.cpp and its declaration in include/casbin/util/util.h. Run the existing TestEscapeAssertion in tests/util_test.cpp, then add a case covering r.sub_data.attr.id; the expected result must preserve the nested attribute dots while escaping only the r. and p. separators. Confirm existing utility and enforcement tests still pass.

Written by the indexing model from the issue text.

Description

Location (apache/casbin-cpp @ default branch 0ad64a7):

std::string EscapeAssertion(std::string s) {
    std::regex regex_s("[a-zA-Z0-9. ]+");

    std::sregex_iterator words_begin = std::sregex_iterator(s.begin(), s.end(), regex_s);
    auto words_end = std::sregex_iterator();

    for (std::sregex_iterator k = words_begin; k != words_end; ++k) {
        std::smatch match = *k;
        std::string match_str = match.str();
        int pos = static_cast<int>(match_str.find("."));
        if (pos != -1) {
            std::string new_str = match_str.replace(pos, 1, "_");
            s = s.replace(match.position(), match.str().length(), new_str);
        }
    }

    return s;
}

Problem

casbin::EscapeAssertion("r.sub_data.attr.id == p.sub && r.act == p.act");
// actual:   "r_sub_data_attr.id == p_sub && r_act == p_act"
// expected: "r_sub_data.attr.id == p_sub && r_act == p_act"

The function tokenizes with [a-zA-Z0-9. ]+, which doesn't include _. In r.sub_data.attr.id the underscore ends the first match (r.sub), and data.attr.id becomes a second match whose first dot is replaced. Only the r. / p. separator should become _; the attribute path .attr.id should be left alone, which is what already happens for names without underscores: tests/util_test.cpp expects r.attr.value == p.attr → r_attr.value == p_attr.

Scope: EscapeAssertion is declared in the public header include/casbin/util/util.h (included by casbin/casbin.h), documented as taking "the value of the matcher and effect assertions". In the current Enforce path it is only applied to the argument names of eval(...) (casbin/enforcer.cpp:120-124), so this is not an enforcement failure: Enforce results are not affected. This is about the result of the public utility for code that calls it directly (model tooling, converters).

Reproduction
Verbatim gtest output for the call above:

Expected equality of these values:
  my_res
    Which is: "r_sub_data_attr.id == p_sub && r_act == p_act"
  res
    Which is: "r_sub_data.attr.id == p_sub && r_act == p_act"

Environment:

  • Commit: 0ad64a7 (master)
  • Toolchain: GCC 11.5.0

Already fixed in apache/casbin-node-casbin
PR: https://github.com/apache/casbin-node-casbin/pull/394 — "fix: escape assersion bug" (merged 2022-10-04T14:04:04Z, merge commit 5a339d858a9). node-casbin's escapeAssertion also rewrote dots inside the nested attribute path of r.sub_data.attr.id (its old regex replaced every r. / p., including the one at the end of attr.). The fix only rewrites r. / p. when preceded by a space or (, i.e. at the start of an identifier, and the added example model uses the same matcher as above. In node-casbin the helper is on the enforcement path, so the bug there also changed Enforce results; in casbin-cpp only the public helper's output is affected.

How node-casbin fixed it:

--- a/src/util/util.ts
+++ b/src/util/util.ts
@@ -17,8 +17,10 @@ import * as fs from 'fs';
 // escapeAssertion escapes the dots in the assertion,
 // because the expression evaluation doesn't support such variable names.
 function escapeAssertion(s: string): string {
-  s = s.replace(/r\./g, 'r_');
-  s = s.replace(/p\./g, 'p_');
+  s = ' ' + s;
+  s = s.replace(/(?<=[\(| ])r\./g, 'r_');
+  s = s.replace(/(?<=[\(| ])p\./g, 'p_');
+  s = s.trim();
   return s;
 }
 
--- a/examples/abac_attr_model.conf
+++ b/examples/abac_attr_model.conf
@@ -0,0 +1,11 @@
+[request_definition]
+r = sub_data, act
+
+[policy_definition]
+p = sub, act
+
+[policy_effect]
+e = some(where (p.eft == allow))
+
+[matchers]
+m = r.sub_data.attr.id == p.sub && r.act == p.act
\ No newline at end of file

Suggested fix
Rewrite only the dot right after an r/p token (optionally numbered) that starts at an identifier boundary, as node-casbin anchors its replacement, so the rest of the attribute path is kept:

diff --git a/casbin/util/escape_assertion.cpp b/casbin/util/escape_assertion.cpp
--- a/casbin/util/escape_assertion.cpp
+++ b/casbin/util/escape_assertion.cpp
@@ -32,22 +32,9 @@ namespace casbin {
  * @return the escaped value.
  */
 std::string EscapeAssertion(std::string s) {
-    std::regex regex_s("[a-zA-Z0-9. ]+");
-
-    std::sregex_iterator words_begin = std::sregex_iterator(s.begin(), s.end(), regex_s);
-    auto words_end = std::sregex_iterator();
-
-    for (std::sregex_iterator k = words_begin; k != words_end; ++k) {
-        std::smatch match = *k;
-        std::string match_str = match.str();
-        int pos = static_cast<int>(match_str.find("."));
-        if (pos != -1) {
-            std::string new_str = match_str.replace(pos, 1, "_");
-            s = s.replace(match.position(), match.str().length(), new_str);
-        }
-    }
-
-    return s;
+    // Only the dot right after an r/p token (e.g. r., p., r2.) is escaped; the token must start at an identifier boundary.
+    static const std::regex regex_s("(^|[^A-Za-z0-9_])([rp][0-9]*)\\.");
+    return std::regex_replace(s, regex_s, "$1$2_");
 }
 
 } // namespace casbin

Verified on 0ad64a7 (GCC 11.4.0, GoogleTest 1.14.0): with the example above added to TestEscapeAssertion in tests/util_test.cpp, the test fails without this patch and passes with it; no other test results change, including the existing TestEscapeAssertion cases.

I can submit a PR for this.

Related: searched issues/PRs (all states) for underscore, ABAC attribute dot, escape assertion: no hits. Dots inside quoted string literals are a separate problem with a different cause and are reported separately.

Dominant language
C++
Stars
257
Forks
67
Avg merge
10h 23m
Merged PRs (30d)
1

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from apache/casbin-cpp

All issues in apache/casbin-cpp

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.