liquid-rust icon indicating copy to clipboard operation
liquid-rust copied to clipboard

Ensure infinitely recursively includes error out instead of crash

Open epage opened this issue 6 years ago • 5 comments

Example test case:

#[test]
#[should_panic]
fn test_recursively_included_template_does_not_produce_endless_loop() {
    panic!("We don't check recursion depth");
    /*
    let parser = liquid::ParserBuilder::with_liquid().include_source(Box::new(InfiniteFileSystem)).build();
    parser.parse("{% include 'loop' %}").unwrap();
    */
}

We probably want Parser to have an Option<number> that gets passed to the context. The context would then track the depth and error appropriately.

Resources

epage avatar Dec 11 '18 04:12 epage

Note: to get the max recursion from the end-client to the linked Context, it'll need to pass through the following layers

  • ParserBuilder
  • Parser
  • Template
  • ContextBuilder
  • Context

epage avatar Apr 12 '19 02:04 epage

Max recursion:

  • Let's say the default could be 1000
  • We can use Option<usize> so someone can disable the recursion limit test

epage avatar Apr 12 '19 02:04 epage

The bug marks "fn run_in_named_scope()" as being the location to fix. Wouldn't the recursion in "fn run_in_scope()" also be at risk for infinite recursion? Also looking at the function passed in to run_in_named_scope , " | mut scope | " , I'm not sure I see where that closure calls run_in_named_scope again. Any suggestions for understanding how scope and new_scope work? I will just proceed and test based on our conversation, perhaps I will gain an understanding of the rest as I fix various bugs.

veatnik avatar Apr 15 '19 03:04 veatnik

The bug marks "fn run_in_named_scope()" as being the location to fix. Wouldn't the recursion in "fn run_in_scope()" also be at risk for infinite recursion?

Good question. This requires some background I didn't think to include in the issue.

The difference between the two is use case.

  • run_in_scope is used for things like for-loops. A user has to hand-write these blocks and so therefore it can't recurse infinitely.
  • run_in_named_scope is used for things like include. You can have a file include itself but you need to be sure to have a base case or else infinite recursion will occur. That is why the check is needed here.

? Also looking at the function passed in to run_in_named_scope , " | mut scope | " , I'm not sure I see where that closure calls run_in_named_scope again. Any suggestions for understanding how scope and new_scope work?

It isn't as obvious because it is hidden behind trait-functions.

If you look at Include::render_to, you'll see that inside of render_to we call run_in_named_scope which calls an included-file's render_to.

epage avatar Apr 15 '19 13:04 epage

I'd like to try and tackle this one too, if @veatnik isn't working on it anymore.

ryanyz10 avatar Sep 11 '19 16:09 ryanyz10