nvuillam/node-sarif-builder

SarifResultBuilder Constructor Causes Loss of Valid SARIF Result Properties

AlexWilson-GIS opened this issue · 2 comments

AlexWilson-GIS commented

The Problem

I am trying to set properties in a SarifResultOptions object. Here is the type definition of this object:

node-sarif-builder/src/types/node-sarif-builder.ts

Lines 300 to 455 in 2682b61

export interface SarifResultOptions {
/**
* Identifies the artifact that the analysis tool was instructed to scan. This need not be the same as the artifact
* where the result actually occurred.
*/
analysisTarget?: ArtifactLocation | undefined;
/**
* A set of artifacts relevant to the result.
*/
attachments?: Attachment[] | undefined;
/**
* The state of a result relative to a baseline of a previous run.
*/
baselineState?: Result.baselineState | undefined;
/**
* An array of 'codeFlow' objects relevant to the result.
*/
codeFlows?: CodeFlow[] | undefined;
/**
* A stable, unique identifier for the equivalence class of logically identical results to which this result
* belongs, in the form of a GUID.
*/
correlationGuid?: string | undefined;
/**
* A set of strings each of which individually defines a stable, unique identity for the result.
*/
fingerprints?: { [key: string]: string } | undefined;
/**
* An array of 'fix' objects, each of which represents a proposed fix to the problem indicated by the result.
*/
fixes?: Fix[] | undefined;
/**
* An array of zero or more unique graph objects associated with the result.
*/
graphs?: Graph[] | undefined;
/**
* An array of one or more unique 'graphTraversal' objects.
*/
graphTraversals?: GraphTraversal[] | undefined;
/**
* A stable, unique identifer for the result in the form of a GUID.
*/
guid?: string | undefined;
/**
* An absolute URI at which the result can be viewed.
*/
hostedViewerUri?: string | undefined;
/**
* A value that categorizes results by evaluation state.
*/
kind?: Result.kind | undefined;
/**
* A value specifying the severity level of the result.
*/
level?: Result.level | undefined;
/**
* The set of locations where the result was detected. Specify only one location unless the problem indicated by
* the result can only be corrected by making a change at every specified location.
*/
locations?: Location[] | undefined;
/**
* A message that describes the result. The first sentence of the message only will be displayed when visible space
* is limited.
*/
message?: Message;
/**
* A positive integer specifying the number of times this logically unique result was observed in this run.
*/
occurrenceCount?: number | undefined;
/**
* A set of strings that contribute to the stable, unique identity of the result.
*/
partialFingerprints?: { [key: string]: string } | undefined;
/**
* Information about how and when the result was detected.
*/
provenance?: ResultProvenance | undefined;
/**
* A number representing the priority or importance of the result.
*/
rank?: number | undefined;
/**
* A set of locations relevant to this result.
*/
relatedLocations?: Location[] | undefined;
/**
* A reference used to locate the rule descriptor relevant to this result.
*/
rule?: ReportingDescriptorReference | undefined;
/**
* The stable, unique identifier of the rule, if any, to which this notification is relevant. This member can be
* used to retrieve rule metadata from the rules dictionary, if it exists.
*/
ruleId?: string | undefined;
/**
* The index within the tool component rules array of the rule object associated with this result.
*/
ruleIndex?: number | undefined;
/**
* An array of 'stack' objects relevant to the result.
*/
stacks?: Stack[] | undefined;
/**
* A set of suppressions relevant to this result.
*/
suppressions?: Suppression[] | undefined;
/**
* An array of references to taxonomy reporting descriptors that are applicable to the result.
*/
taxa?: ReportingDescriptorReference[] | undefined;
/**
* A web request associated with this result.
*/
webRequest?: WebRequest | undefined;
/**
* A web response associated with this result.
*/
webResponse?: WebResponse | undefined;
/**
* The URIs of the work items associated with this result.
*/
workItemUris?: string[] | undefined;
/**
* Key/value pairs that provide additional information about the result.
*/
properties?: PropertyBag | undefined;
}

After trying to figure out why my test runs are creating SARIF files with lost data, I traced down through the code to discover the source of the issue in the constructor:

node-sarif-builder/src/lib/sarif-result-builder.ts

Lines 7 to 19 in 2682b61

export class SarifResultBuilder {
// Default result value
result: Result = {
level: 'error',
message: {},
ruleId:
'SARIF_BUILDER_INVALID: Please send the rule Id ruleId property, or call setRuleId(ruleId)',
};
// Initialize SARIF Result builder
constructor(options: SarifResultOptions = {}) {
setOptionValues(options, this.result);
}

node-sarif-builder/src/lib/utils.ts

Lines 1 to 8 in 2682b61

export function setOptionValues(options, object: any) {
for (const key of Object.keys(object)) {
if (options[key] !== undefined) {
object[key] = options[key];
}
}
return object;
}

This places a large filter over the SarifResultOption that only allows three properties through: level, message, and ruleId.

Proposed Solutions

  1. Ensure the properties in SarifResultOptions objects align with properties in the Result object, then:
    a. change the setOptionsValues function to merge the default properties object and the user provided one using the spread operator, like this:
export function setOptionValues(options, object: any) { 
  return {
    ...object,
    ...options
  }
 }

This will merge the two objects together. Any fields found in both objects will resolve to the value in options. This will ensure the default values for each of the three fields found in the result object persist if the user-provided options don't supply them, while allowing the other properties of a SarifResultOptions object to pass through, and making it so they override the default values where applicable. You would then need to change the constructor to re-set the internal result property.

b. Rewrite the constructor using the same logic I specified in option 1.

c. Tweak setOptionValues to iterate over the user-supplied keys, rather than the default object keys.

  1. Rewrite the constructor to map each field from SarifResultOptions objects to Result objects.
  2. Reduce the type definition of SarifResultOption to only have the properties that can actually make it through the existing constructor, and document that any additional properties need to be added in to the underlying Result manually after construction.
nvuillam commented

@AlexWilson-GIS thanks for reporting the issue :)

Please can you share the code you use to call node-sarif-builder ?

The main goal of this package is to provide an easy way to build SARIF which is a quite complex format with (too many ?) sub levels, that's what there is not a lof of options with initSimple() methods

There are 2 ways to handle additional options:

  • Update node-sarif- builder to be able to handle more "simple" options, and make sure we handle default values if they are not set
  • Directly update SARIF JS object from your code, after calling node-sarif-builder methods, to add the extra properties that you need (as node-sarif-builder is packaged with official SARIF types, it shouldn't be so hard from your code)
AlexWilson-GIS commented

Unfortunately, I cannot share the code because my company's policy does not allow it. At a high level, what I'm doing is iterating over the output of analysis tools in another format and using this library to convert it to SARIF, with the end-goal of uploading the results into GitHub's Security tab. At this point, I'm getting around the issue by doing your second bullet point, and I think it will work. I'm still doing testing, but the properties look like they are carried through.

  • 👍1