Grunny/zap-cli

sys.exit(num_alerts) in cli.py

Closed this issue · 6 comments

haydenflinner commented

This doesn't match my understanding of Unix error code conventions. Usually an error code means something went wrong, not the tool functioned as usual. If anything, I've seen tools throw an error code when they didn't find anything (grep, pgrep, pkill).

Any other opinions?

Grunny commented

It depends on what you mean by "wrong." :) In your examples of grep and so forth, they still function as usual even if they don't find anything, they just consider "wrong" to be "not found." In the case of vulnerability scanning, we consider "wrong" to be that there's a vulnerability in the target application. This is important particularly when a tool like zap-cli is used as part of CI, since in general, a job fails if a part of a CI job returns a non-zero exit code, and you only want the job to fail if a vulnerability was found. If a vulnerability is not found, then it's OK and the build should succeed. This also matches the behaviour of ZAP's baseline scripts for scanning as well, i.e. https://github.com/zaproxy/zaproxy/blob/d98726bcab791a/build/docker/zap-baseline.py#L31-L35 and other similar previous tools, i.e. https://github.com/garethr/zapr/blob/8adf8a2457c4/lib/zapr/zap.rb#L52-L58

  • 👍1
  • ❤️1
haydenflinner commented

Makes sense!

excal04 commented

Having this behavior explicitly documented somewhere would be great.

haydenflinner commented

Opening this because I read something in a book today that worried me. The following one liners should demonstrate the problem with sys.exit(num_alerts).

┌─[hayden@devvm]─[~/code/zap-cli/zapcli]
└──╼ $sh -c "exit 255" && echo No error\! || echo $?
255
┌─[hayden@devvm]─[~/code/zap-cli/zapcli]
└──╼ $sh -c "exit 256" && echo No error\! || echo $?
No error!
┌─[hayden@devvm]─[~/code/zap-cli/zapcli]
└──╼ $sh -c "exit 257" && echo No error\! || echo $?
1
┌─[hayden@devvm]─[~/code/zap-cli/zapcli]
└──╼ $sh -c "exit 512" && echo No error\! || echo $?
No error!
┌─[hayden@devvm]─[~/code/zap-cli/zapcli]
└──╼ $
Grunny commented

That's a good point in the event you have that many alerts. We can just change it to more like the ZAP baseline script: https://github.com/zaproxy/zaproxy/blob/d98726bcab791a/build/docker/zap-baseline.py#L31-L35 rather than like zapr like it is now. So, we'll just exit with 1 if any number of alerts are found.