nccgroup / sobelow

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

DevEx: consider defaulting to [FILE_PATH]:[LINE_NUMBER] format for default vulnerability output #135

Open vanderhoop opened 1 year ago

vanderhoop commented 1 year ago

Hello, and thanks for a great library! I'm here because sobelow flagged something that was indeed vulnerable, so 🎩 and 🙏 to you.

One common part of my workflow is command clicking a particular [FILE_PATH]:[LINE_NUMBER] in terminal output such that the file will be opened in my editor at the exact line number. This format is already somewhat conventional in Elixir, as mix test provides this format in failure output, and it's also technically already available in sobelow via the --compact flag, however sobelow's compact output doesn't surface useful details like the variable name or sobelow's level of confidence in the vulnerability, both of which are helpful when debugging.

Proposal (current versus future state):

Current output for an item flagged as a potential vulnerability:

XSS.Raw: XSS - Low Confidence
File: lib/redacted_web/controllers/redacted_html/redacted.html.heex
Line: 8
Variable: Redacted.redacted()

Suggested output for an item flagged as a potential vulnerability:

XSS.Raw: XSS - Low Confidence
Path: lib/redacted_web/controllers/redacted_html/redacted.html.heex:8
Variable: Redacted.redacted()

The latter would enable command clicks to the exact source location of a potential vulnerability, and add in a worst case 6 characters to the Path attribute (if the source file somehow ballooned to a length greater 10000 lines, which I think would be a security vulnerability unto itself 😅).

I imagine the structure of vulnerabilities expressed in the json format could remain unchanged, and this change would only be for the txt format, but that assumes downstream consumers aren't parsing the txt format for lines, which I hope is a "safe" assumption.

Feel free to close if you're opposed, but if you're amenable to the change, I'm happy to take it on to give back to a great tool. Thanks!

houllette commented 1 year ago

I imagine the structure of vulnerabilities expressed in the json format could remain unchanged, and this change would only be for the txt format, but that assumes downstream consumers aren't parsing the txt format for lines, which I hope is a "safe" assumption.

This is exactly my only concern with this change - I think some amount of cursory research may be needed to confirm this assumption in a general way, but I don't think it should block development here.

I think this would be an awesome QoL change, so if you're willing to take a crack at implementing this change, I would be more than willing to review and assist with research! Otherwise I will add this to my backlog as a lower hanging fruit item and try to scope it into the next minor release version 🙂

houllette commented 1 year ago

I started looking into this and it's a bit more convoluted than I thought - the reason it works for the --compact flag is cause by invoking that argument, Sobelow essentially disregards the normal output flow in favor of the more condensed version that it just manually crafts with string interpolation.

Cause ideally the change to address this issue would be a proper fix in that it changes the underlying Sobelow.Finding structure, thusly allowing us to actually remove the code in the --compact output since it will be there already - but I'm unsure how badly that will mess stuff up.