-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
DEFAULT_WORKSPACE is prefixed to SARIF-output field uri but should not be. #2006
Comments
There is already a replacement in generated SARIF for some values like /tmp/lint or /github/workspace The case with custom value is indeed not taken in account, we can arrange that ,:) Meanwhile if you use /tmp/lint it will be ok :) As some linters directly generate a SARIF file, content of SARIF folder may not be post-processed, you may directly use the unique SARIF file generated at report root :) |
This issue has been automatically marked as stale because it has not had recent activity. If you think this issue should stay open, please remove the |
I will give this one a shot when I have the time. I had a look at the code, and I think this one could be implemented in the method fix_sarif_physical_location by just looking if the uri field starts with the same as DEFAULT_WORKSPACE, (if set) and if so, remove that prefix. |
A small PR is now here #2119. That will fix the problem for the aggregated sarifreport. However, to fully fix the problem, I suggest moving the whole run of the sarif-fixing logic in SarifReporter-class, to be run on the individiual sarif outputs. Ie to move the fix phase so it is done just somewhere around Linter.py, row 899, with a call to a "normallize_sarif" which is basically the same logit that currently is happing in SarifReporter.py for cleaning up. The benefits would be that both the indiividual sarif output files for each linteer and the end aggregate report would be useful! If that sounds like a good idea, i could start creating an pr for adjusting an moving the sarif report cleanup method to that earlier phase instead. |
@nvuillam Working on small pr for this, where the sarif-fix/normalization are done earlier in the phase (as above described, in practice just moves the logic call.) We'll see if you like it and if I get to finish it, and so on, but a small practical question here: currently the files are saved under the sarif-files folder under report folder. Would prefer that each of those is overwritten with the fixed/normalized version OR that the fixed version is also added with, for example a .normalized.sarif prefix? I.e Would you prefer as now: where these two are "normalized" files OR BASH_SHELLCHECK.sarif ie doubles the number of files, but makes any bug-hunting easier in the future (if something is normalized to much :) |
@janderssonse I'd prefer option 3 :) |
This issue has been automatically marked as stale because it has not had recent activity. If you think this issue should stay open, please remove the |
Hi don't stale me yet. I did an working feature take a few weeks ago, I only need to get the time free to add as configurable, and wrap it up. |
This PR is a suggestion for solving the use case of needing to remove the DEFAULT_WORKSPACE from the out put in the generated SARIF output. (oxsecurity#2006). It moves the SARIF logic to an earlier phase, to be handled before the aggregate SARIF generation. It replaces the prefix if the flag SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT: true is set Implementation is done by line parsing and replacing, as a node traversal solution quickly grew due to the many places in the sarif out put the uri can be found (metris, relatedLocations, and so on), and the code is much simpler this way to maintain. Improvements and suggestions: Could dumps and resulting json string be used in a reliable way to line parse an json file? I didn't find a good way. Should the option be renamed to SARIF_REPORTER_DISABLE_DEFAULT_WORKSPACE_IN_OUTPUT or alike. As the pre existing normalization still happens? (We don't change that pre existing behaviour in this PR, only the DEFAULT_WORKSPACE prefix part). Signed-off-by: Josef Andersson <josef.andersson@gmail.com>
This PR is a suggestion for solving the use case of needing to remove the DEFAULT_WORKSPACE from the out put in the generated SARIF output. (oxsecurity#2006). It moves the SARIF logic to an earlier phase, to be handled before the aggregate SARIF generation. It replaces the prefix if the flag SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT: true is set (default: true). Implementation is done by line parsing and replacing, as a node traversal solution quickly grew due to the many places in the sarif out put the uri can be found (metris, relatedLocations, and so on), and the code is much simpler this way to maintain. Improvements and suggestions: Could dumps and resulting json string be used in a reliable way to line parse an json file? I didn't find a good way. Should the option be renamed to SARIF_REPORTER_DISABLE_DEFAULT_WORKSPACE_IN_OUTPUT or alike. As the pre existing normalization still happens? (We don't change that pre existing behaviour in this PR, only the DEFAULT_WORKSPACE prefix part). Signed-off-by: Josef Andersson <josef.andersson@gmail.com>
This PR is a suggestion for solving the use case of needing to remove the DEFAULT_WORKSPACE from the out put in the generated SARIF output. (oxsecurity#2006). It moves the SARIF logic to an earlier phase, to be handled before the aggregate SARIF generation. It replaces the prefix if the flag SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT: true is set (default: true). Implementation is done by line parsing and replacing, as a node traversal solution quickly grew due to the many places in the sarif out put the uri can be found (metris, relatedLocations, and so on), and the code is much simpler this way to maintain. Improvements and suggestions: Could dumps and resulting json string be used in a reliable way to line parse an json file? I didn't find a good way. Should the option be renamed to SARIF_REPORTER_DISABLE_DEFAULT_WORKSPACE_IN_OUTPUT or alike. As the pre existing normalization still happens? (We don't change that pre existing behaviour in this PR, only the DEFAULT_WORKSPACE prefix part). Signed-off-by: Josef Andersson <josef.andersson@gmail.com>
This PR is a suggestion for solving the use case of needing to remove the DEFAULT_WORKSPACE from the out put in the generated SARIF output. (oxsecurity#2006). It moves the SARIF logic to an earlier phase, to be handled before the aggregate SARIF generation. It replaces the prefix if the flag SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT: true is set (default: true). Implementation is done by line parsing and replacing, as a node traversal solution quickly grew due to the many places in the sarif out put the uri can be found (metris, relatedLocations, and so on), and the code is much simpler this way to maintain. Improvements and suggestions: Could dumps and resulting json string be used in a reliable way to line parse an json file? I didn't find a good way. Should the option be renamed to SARIF_REPORTER_DISABLE_DEFAULT_WORKSPACE_IN_OUTPUT or alike. As the pre existing normalization still happens? (We don't change that pre existing behaviour in this PR, only the DEFAULT_WORKSPACE prefix part). Signed-off-by: Josef Andersson <josef.andersson@gmail.com>
This PR is a suggestion for solving the use case of needing to remove the DEFAULT_WORKSPACE from the out put in the generated SARIF output. (#2006). It moves the SARIF logic to an earlier phase, to be handled before the aggregate SARIF generation. It replaces the prefix if the flag SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT: true is set (default: true). Implementation is done by line parsing and replacing, as a node traversal solution quickly grew due to the many places in the sarif out put the uri can be found (metris, relatedLocations, and so on), and the code is much simpler this way to maintain. Improvements and suggestions: Could dumps and resulting json string be used in a reliable way to line parse an json file? I didn't find a good way. Should the option be renamed to SARIF_REPORTER_DISABLE_DEFAULT_WORKSPACE_IN_OUTPUT or alike. As the pre existing normalization still happens? (We don't change that pre existing behaviour in this PR, only the DEFAULT_WORKSPACE prefix part). Signed-off-by: Josef Andersson <josef.andersson@gmail.com>
Describe the bug
DEFAULT_WORKSPACE is prefixed to uri in the SARIF-output, in my case, breaks SARIF-Viewer/VSCode, but would be the same for any alike setup.
To Reproduce
Example:
Run the megalinter container, here standing in the root of the repo you want to check:
Expected behavior
DEFAULT_WORKSPACE should not be prefixed to SARIF-output. This variable is something that most likely only exists during runtime generation, and makes the SARIF-output closely tied to that run.
The text was updated successfully, but these errors were encountered: