mirror of
https://github.com/open-policy-agent/opa.git
synced 2026-08-12 19:32:48 -06:00
topdown: don't leak internal vars in partial eval results (#8872)
Fixes: #6378 Calling a function with an unknown argument the function ignores produced a partial-eval result referencing an internal variable, e.g. __localcp0__ = input.project instead of the equivalent bare ref input.project To record that input.project must be defined, copy propagation keeps the ref but binds it to a generated variable that's used nowhere else — cluttering the result for no benefit. Track these generated variables as placeholders and emit the bare ref instead of the equality. --------- Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
This commit is contained in:
@@ -34,6 +34,9 @@ type CopyPropagator struct {
|
||||
ensureNonEmptyBody bool
|
||||
compiler *ast.Compiler
|
||||
localvargen *localVarGenerator
|
||||
// placeholders holds vars synthesized to keep a ref alive for its definedness.
|
||||
// They appear nowhere else, so their bindings can be emitted as the bare ref.
|
||||
placeholders ast.VarSet
|
||||
}
|
||||
|
||||
type localVarGenerator struct {
|
||||
@@ -47,10 +50,17 @@ func (l *localVarGenerator) Generate() ast.Var {
|
||||
|
||||
}
|
||||
|
||||
// generatePlaceholder returns a fresh local variable, recorded as a placeholder.
|
||||
func (p *CopyPropagator) generatePlaceholder() ast.Var {
|
||||
v := p.localvargen.Generate()
|
||||
p.placeholders.Add(v)
|
||||
return v
|
||||
}
|
||||
|
||||
// New returns a new CopyPropagator that optimizes queries while preserving vars
|
||||
// in the livevars set.
|
||||
func New(livevars ast.VarSet) *CopyPropagator {
|
||||
return &CopyPropagator{livevars: livevars, sorted: util.KeysSorted(livevars), localvargen: &localVarGenerator{}}
|
||||
return &CopyPropagator{livevars: livevars, sorted: util.KeysSorted(livevars), localvargen: &localVarGenerator{}, placeholders: ast.NewVarSet()}
|
||||
}
|
||||
|
||||
// WithEnsureNonEmptyBody configures p to ensure that results are always non-empty.
|
||||
@@ -188,7 +198,14 @@ func (p *CopyPropagator) Apply(query ast.Body) ast.Body {
|
||||
}
|
||||
|
||||
if providesSafety || (!safevarRef && !containedIn(b.v, result)) {
|
||||
result.Append(removedEq)
|
||||
// For a placeholder key, emit the bare ref rather than `__localcp0__ =
|
||||
// input.project`: both only require the ref to be defined, but the
|
||||
// equality leaks the internal var into results (#6378).
|
||||
if expr := p.placeholderRef(b); expr != nil {
|
||||
result.Append(expr)
|
||||
} else {
|
||||
result.Append(removedEq)
|
||||
}
|
||||
safe.Update(outputVars)
|
||||
}
|
||||
}
|
||||
@@ -311,7 +328,7 @@ func (p *CopyPropagator) updateBindings(pctx *plugContext, expr *ast.Expr) bool
|
||||
a, b := expr.Operand(0), expr.Operand(1)
|
||||
if a.Equal(b) {
|
||||
if p.livevarRef(a) {
|
||||
pctx.removedEqs.Put(p.localvargen.Generate(), a.Value)
|
||||
pctx.removedEqs.Put(p.generatePlaceholder(), a.Value)
|
||||
}
|
||||
return false
|
||||
}
|
||||
@@ -351,6 +368,20 @@ func (p *CopyPropagator) livevarRef(a *ast.Term) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// placeholderRef returns the ref a placeholder binding maps to, wrapped as a
|
||||
// bare expression, or nil if b is not a placeholder-to-ref binding.
|
||||
func (p *CopyPropagator) placeholderRef(b *binding) *ast.Expr {
|
||||
k, ok := b.k.(ast.Var)
|
||||
if !ok || !p.placeholders.Contains(k) {
|
||||
return nil
|
||||
}
|
||||
ref, ok := b.v.(ast.Ref)
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
return ast.NewExpr(ast.NewTerm(ref))
|
||||
}
|
||||
|
||||
func (p *CopyPropagator) updateBindingsEq(a, b *ast.Term) (ast.Var, ast.Value, bool) {
|
||||
k, v, keep := p.updateBindingsEqAsymmetric(a, b)
|
||||
if !keep {
|
||||
|
||||
@@ -2158,7 +2158,7 @@ func TestTopDownPartialEval(t *testing.T) {
|
||||
input.a == input.a
|
||||
}`,
|
||||
},
|
||||
wantQueries: []string{`__localcp0__ = input.a`},
|
||||
wantQueries: []string{`input.a`},
|
||||
},
|
||||
{
|
||||
note: "copy propagation: tautology, var ref, ref is input",
|
||||
@@ -2169,7 +2169,7 @@ func TestTopDownPartialEval(t *testing.T) {
|
||||
x.a == x.a
|
||||
}`,
|
||||
},
|
||||
wantQueries: []string{`__localcp0__ = input.a`},
|
||||
wantQueries: []string{`input.a`},
|
||||
},
|
||||
{
|
||||
note: "copy propagation: tautology, var ref, ref is unknown data",
|
||||
@@ -2180,7 +2180,7 @@ func TestTopDownPartialEval(t *testing.T) {
|
||||
data.bar.foo.a == data.bar.foo.a
|
||||
}`,
|
||||
},
|
||||
wantQueries: []string{`__localcp0__ = data.bar.foo.a`},
|
||||
wantQueries: []string{`data.bar.foo.a`},
|
||||
},
|
||||
{
|
||||
note: "copy propagation: tautology, var ref, ref is input, via unknown",
|
||||
@@ -2194,7 +2194,7 @@ func TestTopDownPartialEval(t *testing.T) {
|
||||
input.foo.a == input.foo.a
|
||||
}`,
|
||||
},
|
||||
wantQueries: []string{`__localcp0__ = input.foo.a`},
|
||||
wantQueries: []string{`input.foo.a`},
|
||||
},
|
||||
{
|
||||
note: "copy propagation: tautology, var ref, ref is head var",
|
||||
@@ -2204,7 +2204,19 @@ func TestTopDownPartialEval(t *testing.T) {
|
||||
x.a == x.a
|
||||
}`,
|
||||
},
|
||||
wantQueries: []string{`__localcp1__ = input.a`},
|
||||
wantQueries: []string{`input.a`},
|
||||
},
|
||||
{
|
||||
note: "copy propagation: unused function argument, unknown ref (#6378)",
|
||||
query: "data.test.allow == true",
|
||||
// Calling a function with an unknown arg it ignores must not leak the
|
||||
// internal placeholder var; the residual only requires the arg defined.
|
||||
unknowns: []string{"input.project"},
|
||||
modules: []string{`package test
|
||||
allowed(_) if { input.subject.on_duty }
|
||||
allow if { allowed(input.project) }`},
|
||||
input: `{"subject": {"on_duty": true}}`,
|
||||
wantQueries: []string{`input.project`},
|
||||
},
|
||||
{
|
||||
note: "save set vars are namespaced",
|
||||
|
||||
Reference in New Issue
Block a user