keyMatch2 and keyMatch3 can loop indefinitely

[valkum]

The loop in keyMatch2 and keyMatch3 can spin forever when encountering a /{} or a /:. The go implementation does not seem to have this problem.

Instead of loop forever, either this should panic or not match anything.

We can provide a MR when someone decided how this should be fixed.

[casbin-bot]

@smrpn @hackerchai @PsiACE @GopherJ

[hsluoyz]

@valkum can we use the similar implementation as Go? Will this issue be gone?

[valkum]

Using a replace_all approach similar to go ends up in a panic, as you end up with regex's like: ^/foo/{}$. The go regex engine is fine with such a regex string and interprets { and } as characters instead of an invalid quantifier.

I would imagine this could be abused in a deny rule when /{} is wrongly used as an unnamed wildcard and thus does not match in the go implementation. E.g. deny access to all properties of resource x /resource/{}/property.

The question is: Should this panic or should {} also be replaced by [^/]+? I have a PR ready with switching to a replace_all approach, without any performance drawbacks, but the last question remains. I can provide a final PR after it is decided what should happen with /{}.

[hsluoyz]

@PsiACE @hackerchai

[PsiACE]

The question is: Should this panic or should {} also be replaced by [^/]+?

If using [^/]+ can be consistent with go, then it should be done.

Otherwise, it is best to provide a suitable panic message to tell others how to avoid the problem.

[valkum]

If I am right, the go implementation will result in the following:

act	regex	matches
/foo/{bar}	/foo/[^/]+	/foo/{} :heavy_check_mark: /foo/baz :heavy_check_mark:
/foo/{}	/foo/{}	/foo/{} :heavy_check_mark: /foo/baz :negative_squared_cross_mark:

---

I would assume, as the docs do not say otherwise, to get the following behavior:	act	regex	matches
/foo/{bar}	/foo/[^/]+	/foo/{} :heavy_check_mark: /foo/baz :heavy_check_mark:
/foo/{}	/foo/{}	/foo/[^/]+ :heavy_check_mark: /foo/baz :heavy_check_mark:

---

Regarding the rust version: Having the code panic at this point might be reasonable but also makes it hard to allow for frontend editing for e.g. customers if you provide some way of PaaS, because there is no way to catch that except making sure you do not insert a policy with such an action.

[PsiACE]

ok, i think we can use [^/]+