search

nccgroup / sobelow

Security-focused static analysis for the Phoenix Framework
Apache License 2.0
1.66k stars 92 forks source link

sobelow should not accept `# sobelow_skip` that are not needed #159

Open marcandre opened 4 months ago

marcandre commented 4 months ago

I notice in our code an instance of

  # sobelow_skip ["XSS.Raw"]
  def a_function(arg) do
    that_does_not_call_raw()
  end

I believe sobelow should raise an error on these. They do not reflect the code / current intention. Although unlikely, they could allow someone to add raw without it being super apparent in the diff of the resulting PR.

Thanks for sobelow

houllette commented 4 months ago

Oh interesting, thanks for the submission! I think this is an interesting feature add - I can't say I can prioritize adding it right now, but I'll add the proper labels to this request and see if anyone wants to take a crack at a PR until I have a chance to dive in!

marcandre commented 4 months ago

I tried to have a look at the code but couldn't find a single test of sobelow_skip; this makes it a bit difficult as a first issue.

While I'm here, I'll comment that --skip should be IMO the default. I'd even consider removing the option altogether, but rubocop has --ignore-disable-comments so some people might be using it.

houllette commented 4 months ago

The testing story in Sobelow could definitely stand to be improved 😅 I have a rough shape of how this change could be implemented, but I want to dig into the code first before outlining here what a potential fix would be for anyone to pick up. So standby for that!

RE: --skip as default - while I don't disagree with you in theory, the trouble is that I'm hesitant to make a change like that which has been in place for so long, especially around alerting (or the lack thereof). I know in custom solutions I've personally implementing in CI/CD or in 3rd-party solutions that build on top of Sobelow, they are expecting the behavior of --skip as it is now. It also feels like almost every minor version I've pushed since becoming the maintainer has been a breaking one, so trying to slow down on those haha.

I'll definitely take it under consideration and figure out what a clean transition could look like - this is especially relevant because there was some other discussion in #147 about what the future of sobelow-skips could look like and this conversation could line up with that.

marcandre commented 4 months ago

they are expecting the behavior of --skip as it is now.

I'm suggesting that --skip do nothing (i.e. it would still work and behave the same way, just not needed). The question is: are there any users calling sobelow without the --skip option that would be impacted.