Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Fix problems with early errors #1398

Closed
wants to merge 1 commit into from

Conversation

dcooper16
Copy link
Contributor

This PR addresses the issues raised in #1397. Rather than just deal with the Bash error messages that occur if there is a problem with the command line and either $do_json or $do_pretty_json is set to true, this PR addresses the larger problem.

html_header() sets the name for the HTML file and ensures that it is okay to write to that file. So, nothing should be written to $HTMLFILE until html_header() has been called and has checked that $HTMLFILE may be written to. So, this PR defines $HTMLFILENAME_SET, which is false until html_header() successfully completes. Any place that wants to write to $HTMLFILE then checks that $HTMLFILENAME_SET is true before performing the write.

The same issues apply for the $CSVFILE, $JSONFILE, and $LOGFILE. So, similar changes are made for each of these files.

I believe that this PR fixes the issues raised in #1397. However, given the potential impact of the changes this PR makes, it needs to be tested very thoroughly before it is merged. So, I marked it as "WIP" until I've had a chance to perform more testing.

This PR addresses the issues raised in testssl#1397. Rather than just deal with the Bash error messages that occur if there is a problem with the command line and either $do_json or $do_pretty_json is set to true, this PR addresses the larger problem.

html_header() sets the name for the HTML file and ensures that it is okay to write to that file. So, nothing should be written to $HTMLFILE until html_header() has been called and has checked that $HTMLFILE may be written to. So, this PR defines, $HTMLFILENAME_SET which is false until html_header() successfully completes. Any place that wants to write to $HTMLFILE then checks that $HTMLFILENAME_SET is true before performing the write.

The same issues apply for the $CSVFILE, $JSONFILE, and $LOGFILE. So, similar changes are made for each of these files.

I believe that this PR fixes the issues raised in testssl#1397. However, given the potential impact of the changes this PR makes, it needs to be tested very thoroughly before it is merged. So, I marked it as "WIP" until I've had a chance to perform more testing.
@drwetter
Copy link
Collaborator

drwetter commented Nov 25, 2019

Hi David,

thanks!

Before you spend to much time on this, please keep in mind that this is partly a user error and for the other part testssl.sh doesn't give the user correct feedback.

Best approach would be to detect if a e.g. --json switch is being followed by a file but I didn't see a perfect way how to do that. Maybe if the URI testssl.sh has detected has a pattern like [a-zA-Z0-9].json and there's maybe another arg we didn''t parse we should just throw an error. That would be preferred.

Also if that wouldn't be perfect it would be better than repairing wrong user input.

I tried a while back to redesign the json*, html* and csv* command so that we can get rid of the *file-counterpart but that proved to be kind of difficult.

Cheers, Dirk

@dcooper16
Copy link
Contributor Author

Hi Dirk,

We can drop this PR if you'd like. The problems this PR addresses are almost always the result of user error. In the case of output to an HTML, CSV, or JSON file, the problem can only occur if there is an error on the command line. Either:

  • parse_cmd_line() itself calls fatal(), or
  • html_header(), csv_header(), or json_header() finds a problem with the provided file name and calls fatal().

The call to prepare_logging() comes later, so there are more opportunities for problems to be found before prepare_logging() is called. But, prepare_logging() is called before actual testing begins.

So, the problem this PR fixes is a minor problem, but it still seems as if it should be fixed.

As you noted, the error message printed when there is a problem with the command line is sometimes misleading, but that is a separate problem, which would be more difficult to fix.

@drwetter
Copy link
Collaborator

I am more tending to close this but I'd like to look at it closer first, i.e. end of this week. Sorry

@drwetter
Copy link
Collaborator

drwetter commented Dec 2, 2019

Got the idea now, Thanks.

Let me try to see whether I can address the issue better!

@drwetter
Copy link
Collaborator

drwetter commented Dec 2, 2019

PR (not perfect as indicated) follows. Thus I am closing this. Despite that: thanks, David

@drwetter drwetter closed this Dec 2, 2019
drwetter added a commit that referenced this pull request Dec 2, 2019
testssl.sh hiccups when a user supplied after --json*/--html/-csv
a filename instead of using the corresponding  --json*file/--htmlfile/-csvfile
arguments, see #1397.

This PR adresses that in a sense that it tries to detect to following
argument of --json*/--html/-csv. If that matches a suspected filename
it bails out using fatal().

This is not intended to be perfect (when the pattern doesn't match)
but catches the user error in an early stage. See also #1398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants