djc / askama

Type-safe, compiled Jinja-like templates for Rust
Apache License 2.0
3.35k stars 215 forks source link

Limit nested filters to avoid stack overflow #979

Open djc opened 6 months ago

GuillaumeGomez commented 6 months ago

Can you add a ui test to see the error output too please?

GuillaumeGomez commented 6 months ago

I think you forgot the ui stderr file. ;)

djc commented 6 months ago

I haven't actually used the ui tests before... I finally figured out how to work the system, but not sure how to get a better error message.

GuillaumeGomez commented 6 months ago

Like I did in https://github.com/djc/askama/pull/954: by providing your own. :)

djc commented 6 months ago

That's providing a separate error, but not inspecting the nom error to refine the error. Made an attempt which doesn't seem to work. Not sure how much more time I want to spend on this.

Kijewski commented 6 months ago

Can you try

    fn filtered(i: &'a str, mut level: Level) -> ParseResult<'a, Self> {
        #[allow(clippy::type_complexity)]
        fn filter<'a>(
            i: &'a str,
            level: &mut Level,
        ) -> ParseResult<'a, (&'a str, Option<Vec<Expr<'a>>>)> {
            let (i, _) = char('|')(i)?;
            *level = level.nest(i)?.1;
            pair(ws(identifier), opt(|i| Expr::arguments(i, *level, false)))(i)
        }

        let (mut i, mut res) = Self::prefix(i, level)?;
        while let (j, Some((fname, args))) = opt(|i| filter(i, &mut level))(i)? {
            i = j;
            res = Self::Filter(fname, {
                let mut args = match args {
                    Some(inner) => inner,
                    None => Vec::new(),
                };
                args.insert(0, res);
                args
            })
        }
        Ok((i, res))
    }
GuillaumeGomez commented 6 months ago

If it becomes too much of a hassle for you, I can take over this PR. ;)

djc commented 5 months ago

@Kijewski that worked well, thanks!

I don't trust that the new ui test is actually working well? Feels like it shouldn't be triggering here (since the Level-based nesting should allow greater depth than the previous approach).