ast: forbid dynamic recursion with hidden document #3876
Conversation
srenatus
force-pushed
the
sr/ast/dynamic-recursion-with-hidden-document
branch
from
e1efa89
to
6eb5fb9
Compare
ast/compile.go
Outdated
| @@ -591,9 +591,6 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { | |||
| // This part of the ref is a dynamic term. We can't know what it refers | |||
| // to and will just need to try all of the children. | |||
| for _, child := range node.Children { | |||
| if child.Hide { | |||
| continue | |||
| } | |||
| insertRules(set, child.Values) | |||
| walk(child, i+1) | |||
| } | |||
There was a problem hiding this comment.
I was a bit hesitant changing GetRulesDynamic, but I think the graph that's built with it isn't used in other places. Tests seem to agree.
There was a problem hiding this comment.
Hrmmm, I agree that this is probably safe and unlikely to break anyone... on the other hand, we could extend the compiler with another function that accepts an option to skip (or include) hidden nodes in the result; then the compiler can be modified to use that new function while building the graph and the GetRulesDynamic function doesn't have to change--there's also the GetRulesWithPrefix function (which appears unused) that also checks the 'hide' bit on the nodes.
GetRulesDynamic could be updated to call the new function and deprecated. Just my 2c.
ast/compile.go
Outdated
| @@ -591,9 +591,6 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { | |||
| // This part of the ref is a dynamic term. We can't know what it refers | |||
| // to and will just need to try all of the children. | |||
| for _, child := range node.Children { | |||
| if child.Hide { | |||
| continue | |||
| } | |||
| insertRules(set, child.Values) | |||
| walk(child, i+1) | |||
| } | |||
There was a problem hiding this comment.
Hrmmm, I agree that this is probably safe and unlikely to break anyone... on the other hand, we could extend the compiler with another function that accepts an option to skip (or include) hidden nodes in the result; then the compiler can be modified to use that new function while building the graph and the GetRulesDynamic function doesn't have to change--there's also the GetRulesWithPrefix function (which appears unused) that also checks the 'hide' bit on the nodes.
GetRulesDynamic could be updated to call the new function and deprecated. Just my 2c.
Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/ast/dynamic-recursion-with-hidden-document
branch
from
6eb5fb9
to
f855040
Compare
Before, this package would slip through:
package system.main
allow {
data[input]
}
and wreck havoc either in evaluation (topdown) or the wasm compilation.
Now, it'll be caught:
open-policy-agent/opa % opa eval -fpretty -d r.rego 'data.system.main'
1 error occurred: r.rego:3: rego_recursion_error: rule allow is recursive: allow -> allow
Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/ast/dynamic-recursion-with-hidden-document
branch
from
f855040
to
9c7de2f
Compare
Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/ast/dynamic-recursion-with-hidden-document
branch
from
9c7de2f
to
bfbb7e5
Compare
| @@ -347,7 +347,7 @@ func saveRequired(c *ast.Compiler, ic *inliningControl, icIgnoreInternal bool, s | |||
| } else if ic.Disabled(v.ConstantPrefix(), icIgnoreInternal) { | |||
| found = true | |||
| } else { | |||
| for _, rule := range c.GetRulesDynamic(v) { | |||
| for _, rule := range c.GetRulesDynamicWithOpts(v, ast.RulesOptions{IncludeHiddenModules: false}) { | |||
There was a problem hiding this comment.
👀 This is for backwards-compatibility with how this worked before. I wonder if we really don't want system modules here, though; if they would ever come in play with this....
There was a problem hiding this comment.
That's a good question--we probably do.
There was a problem hiding this comment.
I wouldn't want to touch that without a test case, but I also seriously want this merged 😅
I'll check this in a follow-up!
…#3876) * rego/PrepareForEval: don't compile wasm if there's no engine * ast/compile: include hidden nodes in recursion check Before, this package would slip through: package system.main allow { data[input] } and wreck havoc either in evaluation (topdown) or the wasm compilation. Now, it'll be caught: open-policy-agent/opa % opa eval -fpretty -d r.rego 'data.system.main' 1 error occurred: r.rego:3: rego_recursion_error: rule allow is recursive: allow -> allow Signed-off-by: Stephan Renatus <[email protected]> Signed-off-by: Dolev Farhi <[email protected]>
Before, this package would slip through:
and wreck havoc either in evaluation (topdown) or the wasm compilation.
Now, it'll be caught:
Also includes