bug: "0 */0 * * * *" is not detected as an invalid CRON expression

[jaller94]

Describe the bug

The endpoint POST https://app.windmill.dev/api/schedules/preview cannot deal with {"schedule":"0 */0 * * * *","offset":-120}.

To Reproduce

1. Create a schedule
2. While editing the CRON expression, you may temporarily enter 0 */0 * * * *
3. See an error
4. In the network inspector, you'll find an HTTP 502 Bad Gateway from Cloudflare.

Expected behavior

Windmill version Latest on app.windmill.dev as of 2022-09-16.

Additional notes

The following strings cause the same error:

• 0 /0 * * * *
• 0 0 /0
• 0/0 */1 * * *

[rubenfiszel]

Nice find, those expressions are making the server crash, will solve. But 0 */0 * * * is invalid I think. It is saying to do it on minute 0 of every 0 hours. Or am I mistaken ?

[rubenfiszel]

@sqwishy the string above 0 */0 * * * makes the server panick:

2022-09-17T12:59:54.312704Z ERROR windmill::error: error="Bad request: Invalid expression: Invalid cron expression."
thread 'tokio-runtime-worker' panicked at 'assertion failed: step != 0', /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/iter/adapters/step_by.rs:21:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:48:5
   3: core::iter::adapters::step_by::StepBy<I>::new
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/iter/adapters/step_by.rs:21:9
   4: core::iter::traits::iterator::Iterator::step_by
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/iter/traits/iterator.rs:384:9
   5: cron::time_unit::TimeUnitField::ordinals_from_root_specifier
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/cron-0.11.0/src/time_unit/mod.rs:310:17
   6: <T as cron::parsing::FromField>::from_field
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/cron-0.11.0/src/parsing.rs:60:50
   7: core::ops::function::FnMut::call_mut
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:164:5
   8: nom::combinator::map_res::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/combinator/mod.rs:115:11
   9: <F as nom::internal::Parser<I,O,E>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/internal.rs:325:5
  10: <(FnA,FnB,FnC,FnD,FnE,FnF,FnG) as nom::sequence::Tuple<Input,(A,B,C,D,E,F,G),Error>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/sequence/mod.rs:227:9
  11: nom::sequence::tuple::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/sequence/mod.rs:269:15
  12: <F as nom::internal::Parser<I,O,E>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/internal.rs:325:5
  13: nom::sequence::terminated::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/sequence/mod.rs:106:23
  14: <F as nom::internal::Parser<I,O,E>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/internal.rs:325:5
  15: nom::combinator::map::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/combinator/mod.rs:79:23
  16: cron::parsing::longhand
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/cron-0.11.0/src/parsing.rs:260:5
  17: core::ops::function::FnMut::call_mut
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:164:5
  18: <F as nom::internal::Parser<I,O,E>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/internal.rs:325:5
  19: <(A,B) as nom::branch::Alt<Input,Output,Error>>::choice
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/branch/mod.rs:160:33
  20: nom::branch::alt::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/branch/mod.rs:74:15
  21: <F as nom::internal::Parser<I,O,E>>::parse
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/internal.rs:325:5
  22: nom::combinator::all_consuming::{{closure}}
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-7.1.1/src/combinator/mod.rs:388:24
  23: cron::parsing::schedule
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/cron-0.11.0/src/parsing.rs:278:5
  24: cron::parsing::<impl core::str::traits::FromStr for cron::schedule::Schedule>::from_str
             at /home/rfiszel/.cargo/registry/src/github.com-1ecc6299db9ec823/cron-0.11.0/src/parsing.rs:21:15

which is not acceptable. I think we have the following alternatives:

• Fork cron: https://github.com/zslayton/cron to fix the bug then upstream our patch
• Use another library such https://github.com/nbari/cron-parser
• Catch "invalid cron expression" earlier before we pass them to the cron parser

In any case, we have to test this part with tests and make sure we have the same behavior as before. May I assign this to you?

[sqwishy]

Looks like this was already reported to the library back in May ...

I'm not really familiar with cron syntax so it might be tricky for me to evaluate this. I can follow up with more confidence once I'm a bit better informed.

A quick glance at the alternative library suggests it doesn't support some inputs supported by kubernetes? This makes me wonder if there is other syntax not supported. I don't really know much about cron to even evaluate the quality of the tests each project has.

My hunch is we will want to fix the library we're using now and hope they merge the fix.

I'm a little surprised these projects don't have fuzz testing?

[sqwishy]

Fixed in cron v0.12.0

[rubenfiszel]

Well done, can you do a PR to update the deps please

[sqwishy]

Yes I can, I thought there was bot that did that but I can later today.

[rubenfiszel]

There is but it only does it on monday and does not necessarily update all the deps.

[jaller94]

"0 */0 * * * *" is not valid. It means every 0th hour, so it's basically a "Division by 0" error.