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

Feedback from Harlene (MS) #379

Closed
ghost opened this issue Apr 16, 2019 · 0 comments
Closed

Feedback from Harlene (MS) #379

ghost opened this issue Apr 16, 2019 · 0 comments
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. bug merged Changes merged into provisional draft. resolved-fixed

Comments

@ghost
Copy link

ghost commented Apr 16, 2019

This is a set of issues found by Harleen Kaur Kohli at MS (preferred spelling: "Harlene" as in the issue title). She read the entire spec and provided a spreadsheet that included issues in both the spec and the SDK. I'll reproduce the spec issues here, for the most part. I'll file separate issues for anything sizable.

(Update: I've been through the whole list and nothing was large enough to warrant a separate issue.)


artifactLocation.artifactIndex in spec but artifactLocation.index in SDK.

Response: Previously fixed (#368)


§3.6 Object properties: run.artifacts in mentioned as an example. This should be changed since run.artifacts is not an object any more.

Response: Fixed. Using run.originalUriBaseIds instead.


§3.14.2.2: invocations property is missing in run.externalPropertyFileReferences table

Response: Previously fixed (#369)


logicalLocation.kind: value "declaration" is missing (for XML declaration).

Response: Fixed.


artifact.artifactLocation -> artifact.location

Response: Previously fixed (#368)


§3.30: address object example shows hex string values for baseAddress, offset, but these are integer values now.

Response: Fixed. Good catch!


logicalLocation.kind: "declaration" value is missing in spec.

Response: Spec is correct. That value no longer exists. See the e-ball-t-3 proposal comment in #202, and §3.32.8 in the spec.


graph.id is missing in spec (also this is a required property)

Response: Spec is correct. This property is no longer needed because graphs is an array, not a dictionary. So a graphTraversal refers to the graph it traverses by index, not by id.


edgeTraversal.stepOverEdgeCount: minimum should be 0.

Response: Fixed. ("... whose value is a non-negative integer...")


threadFlowLocation.nestingLevel: minimum should be 0

Response: Fixed. ("... whose value is a non-negative integer...")


reportingConfiguration.level is missing "none" value.

Response: Fixed.


§3.49 reportingDescriptorReference: section headers missing the word "property".

Response: Fixed.


reportingDescriptorRelationship.kinds: "willPrecede", "willFollow" values not present in spec.

Response: Fixed.


fix.artifactChanges in spec, fix.changes in SDK

Response: Spec is correct. This is one of two places I intentionally left "artifact" in a property name. The reason is that a fix in general consists of one or more changes to each of one or more files. So the object model has two levels of changes: a fix contains a set of artifactChange objects each of which describes the changes to a single artifact, and an artifactChange object contains a set of replacement objects, each of which describes the replacement of a set of bytes or characters in the artifact.


notification.descriptor is of type reportingDescriptor in spec, but reportingDescriptorReference in SDK.

Response: Fixed in spec. The value of this property is a reportingDescriptorReference.


notification.associatedRule is of type reportingDescriptor in spec, but reportingDescriptorReference in SDK.

Response: Fixed in spec. The value of this property is a reportingDescriptorReference.


notification.level is missing "none" value.

Response: Fixed. "none" in notification.level means "a trace message."


exception.message is of type string in SDK, but message in spec.

Response: Fixed. The right answer is string. We went back and forth on this. It originally was string. Then we changed it to message with the rationale that every other property named message was of type message. Then we changed it back because we decided that wasn't a compelling argument.


There's no reason threadFlowLocation.executionOrder needs to be positive. It just needs to increase with time.

Response: Fixed.


The minimum number of result.stacks should be 0, not 1, for consistency with most other arrays.

Response: Fixed.


@ghost ghost added the 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. label Apr 16, 2019
@ghost ghost self-assigned this Apr 16, 2019
ghost pushed a commit that referenced this issue Apr 16, 2019
ghost pushed a commit that referenced this issue Apr 16, 2019
ghost pushed a commit that referenced this issue Apr 16, 2019
@ghost ghost closed this as completed Apr 16, 2019
@ghost ghost added the merged Changes merged into provisional draft. label Apr 25, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. bug merged Changes merged into provisional draft. resolved-fixed
Projects
None yet
Development

No branches or pull requests

0 participants