A optional depth-limit when parsing policies #2383
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds depth-limited parsing entry points to the cedar-policy public API to reject policies/expressions that exceed a caller-provided maximum expression depth, reducing the risk of stack overflow from deeply nested inputs. It also introduces iterative CST depth computation and an iterative Drop implementation for CST expressions to avoid stack overflow during cleanup after parse failures.
Changes:
- Add
*_with_depth_limitparsing functions forPolicySet,Policy,Template, andExpressionincedar-policy. - Add CST-based “effective depth” computation + depth-limit enforcement in
cedar-policy-coreparser. - Add iterative
Dropfor CSTExprand add new tests for depth limit + “no stack overflow” scenarios; update changelog.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cedar-policy/src/test/test.rs |
Adds public-API tests covering depth-limit behavior and deep-input stack overflow regression tests |
cedar-policy/src/api.rs |
Exposes new depth-limited parse APIs and refactors PolicySet construction to reuse a helper |
cedar-policy/CHANGELOG.md |
Documents the new depth-limited parsing APIs |
cedar-policy-core/src/parser/err.rs |
Adds a new error kind for “expression too deep” |
cedar-policy-core/src/parser/depth.rs |
Implements iterative CST effective-depth computation + checks (new module) |
cedar-policy-core/src/parser/cst.rs |
Adds Default for CST Node and iterative Drop for CST Expr |
cedar-policy-core/src/parser.rs |
Wires depth-limit checks into new parser entry points and refactors policy-text extraction |
cedar-policy-core/Cargo.toml |
Adds rstest dev-dependency for new parser depth tests |
Cargo.lock |
Updates lockfile for the added dev-dependency version(s) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.74% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.90% Status: PASSED ✅ Details
|
| pub fn from_str_with_depth_limit( | ||
| policies: &str, | ||
| depth_limit: usize, | ||
| ) -> Result<Self, ParseErrors> { |
There was a problem hiding this comment.
Would users also want to know the depth of the policies they parsed? I guess it's not really useful and we expect them to test parsing with enough depth that it accepts their policies, but doesn't stack overflow.
In terms of API, what does backwards compatibility mean, does it imply if parsing X with depth D suceeds in one version, then it should succeed in a non-breaking version?
john-h-kastner-aws
force-pushed
the
cst-depth-guard
branch
from
29381cd to
2323620
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.74% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 88.02% Status: PASSED ✅ Details
|
Iteratively compute the depth of the CST prior to converting to AST. Depth measure accounts for CST nodes Which yield deeper ASTs. Add depth-limited variants of the policy/template parsing functions that reject expressions exceeding a configured limit. Signed-off-by: John Kastner <jkastner@amazon.com>
john-h-kastner-aws
force-pushed
the
cst-depth-guard
branch
from
2323620 to
87d5d44
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.74% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 88.03% Status: PASSED ✅ Details
|
Description of changes
Adds new functions to the public API to configure an optional depth limit for policies when parsing. These function will return an error result for any policy that exceeds the limit, avoiding any possible stack overflow.
As an additional complication, I also had to add a
Dropimplementation for the CST so that we would not stackoverflow when dropping that structure after returning an error. The drop impl is from Claude. IIUC, Rust will always full free the structure regardless of what we do in our Drop, so there's no risk of memory leaks.Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy(e.g., changes to the signature of an existing API).cedar-policy(e.g., addition of a new API).cedar-policy.cedar-policy-core,cedar-validator, etc.)I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):cedar-spec, and how you have tested that your updates are correct.)cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):cedar-docs. PRs should be targeted at astaging-X.Ybranch, notmain.)