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

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

Open
vanderhoop opened this issue Apr 14, 2023 · 2 comments

Comments

@vanderhoop
Copy link

vanderhoop commented Apr 14, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants