Skip to content

Commit

Permalink
ast: Enforce inlined schemas without --schemas flag
Browse files Browse the repository at this point in the history
Always parse annotations.
Always use `schemas` annotations with inlined schemas for type checking.
Ignore `schemas` annotations with schema refs if no `--schema` flag was provided.

Enabled for commands:
* eval
* check
* test

Fixes: open-policy-agent#5506

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
  • Loading branch information
johanfylling committed Feb 23, 2023
1 parent 8f5500d commit 5d82e37
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 15 deletions.
6 changes: 6 additions & 0 deletions ast/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) {
tc.err([]*Error{err})
continue
}
if ref == nil && refType == nil {
continue
}
prefixRef, t := getPrefix(env, ref)
if t == nil || len(prefixRef) == len(ref) {
env.tree.Put(ref, refType)
Expand Down Expand Up @@ -1213,6 +1216,9 @@ func processAnnotation(ss *SchemaSet, annot *SchemaAnnotation, rule *Rule, allow
var schema interface{}

if annot.Schema != nil {
if ss == nil {
return nil, nil, nil
}
schema = ss.Get(annot.Schema)
if schema == nil {
return nil, nil, NewError(TypeErr, rule.Location, "undefined schema: %v", annot.Schema)
Expand Down
6 changes: 3 additions & 3 deletions cmd/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func checkModules(params checkParams, args []string) error {
for _, path := range args {
b, err := loader.NewFileLoader().
WithSkipBundleVerification(true).
WithProcessAnnotation(ss != nil).
WithProcessAnnotation(true).
WithCapabilities(capabilities).
AsBundle(path)
if err != nil {
Expand All @@ -81,7 +81,7 @@ func checkModules(params checkParams, args []string) error {
}

result, err := loader.NewFileLoader().
WithProcessAnnotation(ss != nil).
WithProcessAnnotation(true).
WithCapabilities(capabilities).
Filtered(args, f.Apply)
if err != nil {
Expand All @@ -99,7 +99,7 @@ func checkModules(params checkParams, args []string) error {
WithSchemas(ss).
WithEnablePrintStatements(true).
WithStrict(params.strict).
WithUseTypeCheckAnnotations(ss != nil)
WithUseTypeCheckAnnotations(true)

compiler.Compile(modules)
if compiler.Failed() {
Expand Down
49 changes: 49 additions & 0 deletions cmd/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,52 @@ p if "opa" in input.tools`,
})
}
}

func testCheckWithSchemasAnnotationButNoSchemaFlag(policy string) error {
files := map[string]string{
"test.rego": policy,
}

var err error
test.WithTempFS(files, func(path string) {
params := newCheckParams()

err = checkModules(params, []string{path})
})

return err
}

// Assert that 'schemas' annotations with schema refs are only informing the type checker when the --schema flag is used
func TestCheckWithSchemasAnnotationButNoSchemaFlag(t *testing.T) {
policyWithSchemaRef := `
package test
# METADATA
# schemas:
# - input: schema["input"]
p {
rego.metadata.rule() # presence of rego.metadata.* calls must not trigger unwanted schema evaluation
input.foo == 42 # type mismatch with schema that should be ignored
}`

err := testCheckWithSchemasAnnotationButNoSchemaFlag(policyWithSchemaRef)
if err != nil {
t.Fatalf("unexpected error from eval with schema ref: %v", err)
}

policyWithInlinedSchema := `
package test
# METADATA
# schemas:
# - input.foo: {"type": "boolean"}
p {
rego.metadata.rule() # presence of rego.metadata.* calls must not trigger unwanted schema evaluation
input.foo == 42 # type mismatch with schema that should be ignored
}`

err = testCheckWithSchemasAnnotationButNoSchemaFlag(policyWithInlinedSchema)
// We expect an error here, as inlined schemas are always used for type checking
if !strings.Contains(err.Error(), "rego_type_error: match error") {
t.Fatalf("unexpected error from eval with inlined schema, got: %v", err)
}
}
10 changes: 6 additions & 4 deletions cmd/eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"os"
"path/filepath"
"reflect"
"strings"
"testing"

"github.com/open-policy-agent/opa/ast"
Expand Down Expand Up @@ -470,14 +471,14 @@ func testEvalWithSchemasAnnotationButNoSchemaFlag(policy string) error {
var defined bool
defined, err = eval([]string{query}, params, &buf)
if !defined || err != nil {
err = fmt.Errorf("Unexpected error or undefined from evaluation: %v", err)
err = fmt.Errorf(buf.String())
}
})

return err
}

// Assert that 'schemas' annotations are only informing the type checker when the --schema flag is used
// Assert that 'schemas' annotations with schema refs are only informing the type checker when the --schema flag is used
func TestEvalWithSchemasAnnotationButNoSchemaFlag(t *testing.T) {
policyWithSchemaRef := `
package test
Expand Down Expand Up @@ -505,8 +506,9 @@ p {
}`

err = testEvalWithSchemasAnnotationButNoSchemaFlag(policyWithInlinedSchema)
if err != nil {
t.Fatalf("unexpected error from eval with inlined schema: %v", err)
// We expect an error here, as inlined schemas are always used for type checking
if !strings.Contains(err.Error(), `"code": "rego_type_error"`) {
t.Fatalf("unexpected error from eval with inlined schema, got: %v", err)
}
}

Expand Down
10 changes: 7 additions & 3 deletions cmd/test_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package cmd
import (
"bytes"
"context"
"github.com/open-policy-agent/opa/ast"
"strings"
"testing"

"github.com/open-policy-agent/opa/rego"
Expand Down Expand Up @@ -228,7 +230,7 @@ func testSchemasAnnotation(mod string) error {
return err
}

// Assert that 'schemas' annotations are ignored
// Assert that 'schemas' annotations with schema ref are ignored, but not inlined schemas
func TestSchemasAnnotation(t *testing.T) {
policyWithSchemaRef := `
package test
Expand Down Expand Up @@ -264,7 +266,9 @@ test_p {
}`

err = testSchemasAnnotation(policyWithInlinedSchema)
if err != nil {
t.Fatalf("unexpected error when inlined schema is present: %v", err)
if err == nil {
t.Fatalf("didn't get expected error when inlined schema is present")
} else if !strings.Contains(err.Error(), "rego_type_error: match error") {
t.Fatalf("didn't get expected %s error when inlined schema is present; got: %v", ast.TypeErr, err)
}
}
20 changes: 18 additions & 2 deletions docs/content/annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,12 @@ allow {
The `schemas` annotation is a list of key value pairs, associating schemas to data values.
In-depth information on this topic can be found [here](../schemas#schema-annotations).

#### Example
#### Schema Reference Format

Schema files can be referenced by path, where each path starts with the `schema` namespace, and trailing components specify
the path of the schema file (sans file-ending) relative to the root directory specified by the `--schema` flag on applicable commands.

```live:rego/metadata/schemas:module:read_only
```live:rego/metadata/schemas_ref:module:read_only
# METADATA
# schemas:
# - input: schema.input
Expand All @@ -238,6 +241,19 @@ allow {
}
```

#### Inlined Schema Format

Schema definitions can be inlined by specifying the schema structure as a yaml map.

```live:rego/metadata/schemas_inline:module:read_only
# METADATA
# schemas:
# - input.x: {type: number}
allow {
input.x == 42
}
```

### Entrypoint

The `entrypoint` annotation is a boolean used to mark rules and packages that should be used as entrypoints for a policy.
Expand Down
6 changes: 3 additions & 3 deletions rego/rego.go
Original file line number Diff line number Diff line change
Expand Up @@ -1156,7 +1156,7 @@ func New(options ...func(r *Rego)) *Rego {
WithCapabilities(r.capabilities).
WithEnablePrintStatements(r.enablePrintStatements).
WithStrict(r.strict).
WithUseTypeCheckAnnotations(r.schemaSet != nil)
WithUseTypeCheckAnnotations(true)
}

if r.store == nil {
Expand Down Expand Up @@ -1753,7 +1753,7 @@ func (r *Rego) loadFiles(ctx context.Context, txn storage.Transaction, m metrics

result, err := loader.NewFileLoader().
WithMetrics(m).
WithProcessAnnotation(r.schemaSet != nil).
WithProcessAnnotation(true).
Filtered(r.loadPaths.paths, r.loadPaths.filter)
if err != nil {
return err
Expand Down Expand Up @@ -1782,7 +1782,7 @@ func (r *Rego) loadBundles(ctx context.Context, txn storage.Transaction, m metri
for _, path := range r.bundlePaths {
bndl, err := loader.NewFileLoader().
WithMetrics(m).
WithProcessAnnotation(r.schemaSet != nil).
WithProcessAnnotation(true).
WithSkipBundleVerification(r.skipBundleVerification).
AsBundle(path)
if err != nil {
Expand Down

0 comments on commit 5d82e37

Please sign in to comment.