error handler block in parse.ex throws on syntax error

[christian-schulze]

The following error handling block in lib/sobelow/parse.ex is incorrectly destructuring the line parameter: https://github.com/nccgroup/sobelow/blob/master/lib/sobelow/parse.ex#L45_L51

When scanning my code which contained a syntax error, the line variable was a list instead of a number, looking like this:

[line: 123, column: 456]

This in turn causes the IO.puts line to throw as it can't convert a list to a string.

I fixed this locally by changing the error handling block to the following:

{:error, {line, err, _}} ->
  if Application.get_env(:sobelow, :strict) do
    if is_list(line) do
      IO.puts(:stderr, "#{filepath}:#{line[:line]}:#{line[:column]}}: #{err}")
    else
      IO.puts(:stderr, "#{filepath}:#{line}: #{err}")
    end
    System.halt(2)
  else
    {}
  end

Happy to submit a PR with any feedback you may have.

[houllette]

Hey @christian-schulze! Would you be willing to share what code this errored off of (the one with the syntax error)? I'd be interested as to how it got into the state of using line as a list rather than the anticipated number; additionally trying to figure out how edge case-y this situation is.

[christian-schulze]

Hi @houllette, I've since moved on from the company I worked for when I found this issue, so unfortunately no longer have access to the code that caused this.

[houllette]

Shoot! No worries then, I will throw this on the backlog to implement something similar to what you outlined; whether that's with an if / case statement or break it out into guards for function clauses.

[volcov]

Hi @houllette , how are you? Is this still desired? If so, can I navigate these waters?

[houllette]

@volcov! Miss you dude! This is definitely still desired, so if you wanna take a crack at it you are more than welcome to!

[volcov]

@houllette Okay, I understood the situation, I would like to write a test for the ast/1 function of the Sobelow.Parse module, but it expects a file path as an argument, do you consider it a good idea to add Mox to simulate the file read, only to dev and test, or would it be better to resolve it natively?

[houllette]

Sorry for the delay, @volcov - I'm honestly a little torn on whether to use Mox or not. I mean...that is probably the best and most sustainable way going forward to create those sorts of tests, but since we aren't using it now I'm unsure how much work we're adding to the backlog to go in and add Mox to everything that needs it.

I'd say let's go for it with Mox and once you have a PR up using it we can take a look!