{{ message }}
Repository navigation
fix: look up pattern tokens by group name in KeyMatch4 and KeyGet2/3 - #1767
Open
arshilkxwork wants to merge 1 commit into
Open
arshilkxwork wants to merge 1 commit into
arshilkxwork wants to merge 1 commit into
Conversation
KeyMatch4, KeyGet2 and KeyGet3 assumed the n-th pattern variable was
capture group n+1. A policy pattern with a capture group of its own
breaks that: "/(a|b)/{id}" made KeyMatch4 panic with "number of tokens
is not equal to number of values", and KeyGet2("/a/x/42", "/(a|b)/x/:id",
"id") returned "a" instead of "42". With KeyMatch4 registered as a
role-manager matching function, role lookups such as
GetNamedImplicitRolesForUser hit the panic outside Enforce's recover.
Each variable now compiles to a named group (?P<casbinTokenN>...), and
compilePattern records its submatch index with SubexpIndex. The count
check that panicked is removed because the two can no longer disagree.
This branch has not been deployed
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
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.

KeyMatch4,KeyGet2andKeyGet3read the value of the n-th pattern variable from capture group n+1. That only holds when the pattern has no capture groups of its own. With one, the indices shift:KeyMatch4("/a/42", "/(a|b)/{id}")panics withKeyMatch4: number of tokens is not equal to number of values.Enforcerecovers it and returns apanic:error with a stack trace. Whenutil.KeyMatch4is registered withAddNamedMatchingFunc, role lookups run outside that recover: after addingg2, /(book|pen)/{id}/copy/{id}, book_group,GetNamedImplicitRolesForUser("g2", "/book/1/copy/1")panics the caller. This is the same role-manager path 0a73027 covered for invalid patterns.KeyGet2("/a/x/42", "/(a|b)/x/:id", "id")returns"a"instead of"42", andKeyGet3does the same with{id}. No error is raised. A group that comes after the variable (/:id/(a|b)) happens to work today.Each pattern variable now compiles to a named group,
(?P<casbinToken0>[^/]+)and so on, andcompilePatternstores the submatch index of each one usingSubexpIndex.keyGetandkeyMatch4read values through those indices. The token/value count check inkeyMatch4is removed, since the two can no longer disagree.Parse errors from these three functions quote the rewritten regexp, so they now show the group names.
TestKeyGet2Funcpins that text and is updated to`^/(?P<casbinToken0>[^/]+)/(using$`.KeyGet2/KeyGet3also switch fromFindAllStringSubmatchtoFindStringSubmatch. The pattern is anchored, so only the first match was ever used. On my machineBenchmarkKeyGet2goes from 171 to 134 ns/op (3 allocs to 1),BenchmarkKeyGet3from 217 to 200 ns/op, andBenchmarkKeyMatch4is unchanged.Testing:
TestKeyGet2,TestKeyGet3andTestKeyMatch4with groups before, between and after the variables. On master the KeyGet cases return the wrong segment andTestKeyMatch4panics.TestPatternWithCaptureGroupInPolicyregistersutil.KeyMatch4as the g2 matching function with/(book|pen)/{id}/copy/{id}and checksEnforceandGetNamedImplicitRolesForUser. On masterEnforcereturns the panic error andGetNamedImplicitRolesForUserpanics.go test -race ./...passes, and golangci-lint v2.15.0 reports no issues.