Skip to content

Commit

Permalink
Fix the caveat expr limit to be processed by our code
Browse files Browse the repository at this point in the history
Right now, as a hack to ensure caveat error messages have the correct line and col information, we replace the preceding schema with newlines. However, the CEL parser has a default limit on the size of the expression it can be given, which was causing the error. With this change, we turn off the CEL check and instead check the *trimmed* expression ourselves for a limit.

Long term, we'll want to issue a change in CEL to allow updating of the positioning information instead of this hack

Adds tests and fixes #1637
  • Loading branch information
josephschorr committed Nov 3, 2023
1 parent 31a1001 commit 49bfa31
Show file tree
Hide file tree
Showing 6 changed files with 21,507 additions and 0 deletions.
7 changes: 7 additions & 0 deletions pkg/caveats/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package caveats

import (
"fmt"
"strings"

"github.com/authzed/cel-go/cel"
"github.com/authzed/cel-go/common"
Expand All @@ -13,6 +14,8 @@ import (

const anonymousCaveat = ""

const maxCaveatExpressionSize = 100_000 // characters

// CompiledCaveat is a compiled form of a caveat.
type CompiledCaveat struct {
// env is the environment under which the CEL program was compiled.
Expand Down Expand Up @@ -80,6 +83,10 @@ func CompileCaveatWithSource(env *Environment, name string, source common.Source
return nil, err
}

if len(strings.TrimSpace(source.Content())) > maxCaveatExpressionSize {
return nil, fmt.Errorf("caveat expression provided exceeds maximum allowed size of %d characters", maxCaveatExpressionSize)
}

ast, issues := celEnv.CompileSource(source)
if issues != nil && issues.Err() != nil {
return nil, CompilationErrors{issues.Err(), issues}
Expand Down
6 changes: 6 additions & 0 deletions pkg/caveats/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ func (e *Environment) asCelEnvironment() (*cel.Env, error) {
// See: https://github.com/google/cel-go/issues/474
opts = append(opts, cel.EnableMacroCallTracking())

// ParserExpressionSizeLimit: disable the size limit for codepoints in expressions.
// This has to be disabled due to us padding out the whitespace in expression parsing based on
// schema size. We instead do our own expression size check in the Compile method.
// TODO(jschorr): Remove this once the whitespace hack is removed.
opts = append(opts, cel.ParserExpressionSizeLimit(-1))

for name, varType := range e.variables {
opts = append(opts, cel.Variable(name, varType.CelType()))
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/schemadsl/compiler/compiler_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package compiler

import (
"os"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -1000,3 +1001,17 @@ func filterSourcePositions(m protoreflect.Message) {
return true
})
}

func TestSuperLargeCaveatCompile(t *testing.T) {
b, err := os.ReadFile("../parser/tests/superlarge.zed")
if err != nil {
panic(err)
}

compiled, err := Compile(InputSchema{
input.Source("superlarge"), string(b),
}, new(string))
require.NoError(t, err)
require.Equal(t, 29, len(compiled.ObjectDefinitions))
require.Equal(t, 1, len(compiled.CaveatDefinitions))
}
1 change: 1 addition & 0 deletions pkg/schemadsl/parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func TestParser(t *testing.T) {
{"unclosed caveat test", "unclosedcaveat"},
{"invalid caveat expr test", "invalidcaveatexpr"},
{"associativity test", "associativity"},
{"super large test", "superlarge"},
}

for _, test := range parserTests {
Expand Down
Loading

0 comments on commit 49bfa31

Please sign in to comment.