Add depth validation for ASTs loaded through ParsedExprToAst / Checke...#1334
Open
wilyan09007 wants to merge 4 commits into
Open
Add depth validation for ASTs loaded through ParsedExprToAst / Checke...#1334wilyan09007 wants to merge 4 commits into
wilyan09007 wants to merge 4 commits into
Conversation
ASTs loaded via ParsedExprToAst / CheckedExprToAst bypass the parser's recursion limit, so a deeply nested loaded AST could exhaust the Go stack during checking or planning and crash the process instead of returning a normal error. Add ast.ExceedsMaxDepth, a bounded traversal (default limit 250, the same as the parser's maxRecursionDepth) that Env.Check and Env.PlanProgram run before recursing, returning an ordinary error when the limit is exceeded. Includes a regression test that loads a synthetic over-deep AST. Closes google#1333
wilyan09007
added a commit
to wilyan09007/cel-go
that referenced
this pull request
Jun 5, 2026
The depth bound for ASTs ingested outside the parser (via ParsedExprToAst / CheckedExprToAst) was an exported package constant, common/ast.MaxNestingDepth. Replace it with a cel.ExpressionNestingDepthLimit functional option on NewEnv so the behavior is user-configurable with a sensible default rather than a public package-level knob. The option defaults to 250, matching the parser's maxRecursionDepth, and a negative value disables the check. Enforce it once during Program planning (PlanProgram) and drop the separate Env.Check guard, leaving a single configurable enforcement point.
wilyan09007
force-pushed
the
fix/issue-1333
branch
from
daf8cf6 to
662a019
Compare
Address review feedback on the loaded-AST depth guard: - common/ast: replace the bespoke depth.go traversal with ExceedsDepth in navigable.go, built on the existing NavigableExpr.Depth() visitor. The walk is bounded to maxDepth+1 levels so it stays safe on the adversarially deep inputs it guards against. - cel: move the depth check out of the PlanProgram hot path and into the ParsedExprToAst / CheckedExprToAst proto-conversion helpers, the actual entry points for ASTs that bypass the parser. The expensive traversal now runs once at load; Check/Program only read the recorded error. Embedders in full control of their AST inputs can skip the check by constructing the AST through the common/ast package directly. - cel: register limitMaxASTDepth in limitIDsToNames so the depth limit round-trips through env.Config export/import. Updates the regression test to cover the parsed and checked conversion paths, Check surfacing, the low-level bypass, and the env-config round-trip.
Collaborator
|
/gcbrun |
TristonianJones
previously approved these changes
Jun 15, 2026
Collaborator
|
/gcbrun |
TristonianJones
approved these changes
Jun 22, 2026
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
2 participants
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.
Summary
cel-goenforces a recursion/expression-depth limit while parsing CEL source, but ASTs ingested directly — throughParsedExprToAst/CheckedExprToAst— bypass that guard. A deeply nested loaded AST then flows into the recursive checker (Env.Check) and planner (Env.Program/PlanProgram), which can exhaust the Go stack and abort the process withfatal error: stack overflowinstead of returning a recoverable error. Embedders that load stored or serialized CEL ASTs as policy objects are the most exposed.This validates expression nesting depth before those recursive phases:
ast.ExceedsMaxDepth(expr, maxDepth)incommon/ast— a bounded traversal that never recurses pastmaxDepth+1levels, so it stays safe on the same adversarially deep inputs it guards against. The defaultast.MaxNestingDepth = 250mirrors the parser's defaultmaxRecursionDepth, keeping loaded ASTs consistent with parsed ones.Env.CheckandEnv.PlanProgramnow run that check up front and return an ordinary error/issue —input exceeds maximum expression nesting depth: 250— rather than recursing into a stack overflow.CheckandProgramreturn an error (not a crash), while normal expressions remain unaffected.Closes #1333