feat: use location.message if available as the file name; use shortDescription if available as the category #10
Conversation
|
Currently, here's what's displayed by this plugin: Compare that to what's displayed by Snyk's Fortify SSC plugin which implements a similar approach this PR: |
|
Thanks for your contribution; as stated in the other PR, I'll first need to check our company policies around 3rd-party PR's. I do see one potential issue with this PR; usually SSC would use the primary location to display the actual location in the source code snippet when you expand the issue, for example displaying the actual line in pom.xml on which this issue was identified. If we change the primary location to something that cannot be resolved to a source file, then SSC won't be able to show the source code snippet. So, maybe we should make your suggested behavior optional, for example based on a system property (as SSC doesn't provide any functionality for parser plugins to have configuration options). Maybe we should even provide make this configurable on a tool-by-tool basis, based on the SARIF Thoughts? |
I've only seen that behavior in Fortify SSC when view Fortify SCA / FPR findings. I didn't even know that was possible for anything else. Do you have any tips on things I can check to see why this viewing functionality isn't working for this plugin or for the Snyk's Fortify SSC plugin?
Is there something we can with I really want to make the UI look like what Snyk's plugin produces as shown in the issue description. Having a whole bunch of rows all with the location set to "pom.xml" is just not workable for users.
I don't like the idea of using system properties for configuration since it would require modifications to the deployment of Fortify SSC (k8s, script, or bespoke), and any change to them requires Fortify SSC to be restarted (meaning downtime has to be scheduled, testing has to be done, etc) - it would be a very hard sell to the operations group.
I think altering behavior based on the SARIF file's |
|
Never mind, I mixed up SSC views for standard Fortify vulnerabilities versus parser-provided vulnerabilities; the latter usually don't show source code snippets so probably the primary location doesn't need to match actual source file location after all. Once I get approval for merging 3rd-party PRs, I'll merge this one as well. Sorry for any confusion caused. |
Having "Primary Location" display the same value for every row (such as `/package.json` or `/pom.xml`) is not very user friendly. SARIF allows a "message" to be provided for a location and that can have more useful information. See: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317675 The result is that instead of showing many rows with "Primary Location" of "/pom.xml", now each row has a different value such as "Package: org.postgresql:postgresql@42.3.6" and "package: org.yaml:snakeyaml@1.30" Signed-off-by: Craig Andrews <candrews@integralblue.com>
From the SARIF specification regarding name: > A reportingDescriptor object MAY contain a property named name whose value is a localizable string (§3.5.1) containing an identifier that is understandable to an end user. If the name of a rule contains implementation details that change over time, a tool author might alter a rule's name (while leaving the stable id property (§3.49.3) unchanged). > NOTE: A rule name is suitable in contexts where a readable identifier is preferable and where the lack of stability is not a concern. > EXAMPLE: "name": "SpecifyMarshalingForPInvokeStringArguments" See: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317843 The name isn't very human readable. And the same name is often used repeatedly, making Fortify SSC's table view have many rows with the same values, reducing usability. Here's the definition of shortDescription: > A reportingDescriptor object MAY contain a property named shortDescription whose value is a localizable multiformatMessageString object (§3.12, §3.12.2) that provides a concise description of the reporting item. The shortDescription property SHOULD be a single sentence that is understandable when visible space is limited to a single line of text. > > EXAMPLE: > "Specify marshaling for P/Invoke string arguments." See: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317845 By using the shortDescription, if it is available, the Fortify SSC table view contains much more readable information that is less uselessly repetitive. Signed-off-by: Craig Andrews <candrews@integralblue.com>
|
I updated this PR to add a commit which will use Please let me know if you need to separate out these changes or if is any other way I can facilitate getting these improvements merged. Thanks again! |
candrews
changed the title
Having "Primary Location" display the same value for every row (such as
/package.jsonor/pom.xml) is not very user friendly.SARIF allows a "message" to be provided for a location and that can have more useful information. See:
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317675
The result is that instead of showing many rows with "Primary Location" of "/pom.xml", now each row has a different value such as "Package: org.postgresql:postgresql@42.3.6" and "package: org.yaml:snakeyaml@1.30"
Signed-off-by: Craig Andrews candrews@integralblue.com