Skip to content

Commit

Permalink
Fix bad shorthand parse
Browse files Browse the repository at this point in the history
In crontab.go, we rely on https://github.com/gorhill/cronexpr to parse
cron expressions, and to tell us when they're invalid.

To choose what to feed into cronexpr, we pick out a number of tokens
from the start of the cron line that might yield a valid cron
expression, starting with the most tokens (we try 7, then 6, then 5,
then 1). When we hit a valid cron expression, we stop there.

Unfortunately, I overlooked the fact that cronexpr isn't exactly
designed for this use case, and will not return an error when
passed e.g. `@hourly foo` (it just ignores `foo` in this case).

This is a problem if the user is using a shorthand cron expression and a
command with 4 or more tokens: in this case, we'll attempt to parse the
shorthand and the 4 first tokens in cronexpr, which will work (it'll
ignore the tokens altogether), and we'll be left with a truncated
command (missing the 4 first tokens).

The ideal fix would be to have cronexpr reject this invalid expression
for us, so I'm opening a PR there. In the meantime, I'm also opening
this PR in Supercronic with a fix my fork of cronexpr.
  • Loading branch information
krallin committed Aug 1, 2018
1 parent 2cf5742 commit 4d5c9b9
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 9 deletions.
43 changes: 36 additions & 7 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@

[[constraint]]
name = "github.com/gorhill/cronexpr"
version = "~1.0.0"
branch = "strict"
source = "github.com/krallin/cronexpr"

[[constraint]]
name = "github.com/sirupsen/logrus"
Expand Down
2 changes: 1 addition & 1 deletion crontab/crontab.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func parseJobLine(line string) (*CrontabLine, error) {
// TODO: Should receive a logger?
logrus.Debugf("try parse(%d): %s[0:%d] = %s", count, line, scheduleEnds, line[0:scheduleEnds])

expr, err := cronexpr.Parse(line[:scheduleEnds])
expr, err := cronexpr.ParseStrict(line[:scheduleEnds])

if err != nil {
continue
Expand Down
18 changes: 18 additions & 0 deletions crontab/crontab_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,24 @@ var parseCrontabTestCases = []struct {
},
},

{
"@hourly foo1 foo2 foo3 foo4 foo5 foo6",
&Crontab{
Context: &Context{
Shell: "/bin/sh",
Environ: map[string]string{},
},
Jobs: []*Job{
{
CrontabLine: CrontabLine{
Schedule: "@hourly",
Command: "foo1 foo2 foo3 foo4 foo5 foo6",
},
},
},
},
},

// Failure cases
{"* foo \n", nil},
{"* some * * * more\n", nil},
Expand Down

0 comments on commit 4d5c9b9

Please sign in to comment.