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

feat: add option to skip def_ws prefix in sarif reports #2383

Conversation

janderssonse
Copy link
Contributor

@janderssonse janderssonse commented Feb 21, 2023

Fixes #2006

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)
(see clear_default_workspace_prefix(...

Implementation is done by line parsing and replacing, as a node traversal solution quickly would 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 json dumps() and resulting json string be used in
    a reliable way to stream 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).

Proposed Changes

  1. Move the SARIF Logic to an earlier phase
  2. Add optional removal of DEFAULT_WORKSPACE prefix
  3. Activated by SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@nvuillam
Copy link
Member

That seems great :)

I think SARIF_REPORTER_NORMALIZE_LINTERS_OUTPUT should be true by default :)

@janderssonse janderssonse force-pushed the feature/option-to-skip-def-ws-prefix-sarif branch from a9cc332 to 897c62e Compare February 21, 2023 21:11
@janderssonse
Copy link
Contributor Author

janderssonse commented Feb 21, 2023

Thanks, fixed so it is default true now. Btw,How can I make the checks failing tests succed, if i interprets the checks logs correct they would need permission conf (a PAT token) for the repo etc. Learning for this and future fixes.:)

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 24, 2023
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 24, 2023
@Kurt-von-Laven
Copy link
Collaborator

I believe the errors are the linter violations, not the bit about the PAT, which you can ignore.

@janderssonse janderssonse force-pushed the feature/option-to-skip-def-ws-prefix-sarif branch from 897c62e to e5eb3de Compare April 7, 2023 22:24
@janderssonse
Copy link
Contributor Author

janderssonse commented Apr 9, 2023

I believe the errors are the linter violations, not the bit about the PAT, which you can ignore.

Uh, oh, I see now, preparing a fix for these.
Edit: Green!:)

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>
@janderssonse janderssonse force-pushed the feature/option-to-skip-def-ws-prefix-sarif branch from e5eb3de to 94130ea Compare April 9, 2023 08:06
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed green, well done , thanks :):

@nvuillam nvuillam merged commit 79f6abd into oxsecurity:main Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEFAULT_WORKSPACE is prefixed to SARIF-output field uri but should not be.
3 participants