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

Provide mechanism for inlining externalized properties data into the root log #321

Closed
michaelcfanning opened this issue Jan 29, 2019 · 8 comments
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. design-approved The TC approved the design and I can write the change draft e-ballot e-ballot-2 enhancement impact-non-breaking-change merged Changes merged into provisional draft. resolved-fixed sdk-code-complete tc-32

Comments

@michaelcfanning
Copy link
Contributor

This required in order to accommodate sarifLog.taxonomies metadata, which will be a log level construct, as the data itself is entirely external to any tool execution (e.g., the CWE definitions).

The property names would be a JSON path to the construct which has been externalized.

@lgolding per discussion

@michaelcfanning michaelcfanning added the 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. label Jan 29, 2019
@michaelcfanning
Copy link
Contributor Author

Disposition: add a taxonomies property to the run object externalPropertyFiles object. Taxonomies is externalizable. Any externalized property reference can utilize the SARIF protocol to refer to data within the log file.

@kupsch
Copy link

kupsch commented Feb 22, 2019

Below is an example SARIF file, for my proposal to inline external data that can be shared between runs. Add a new top level property whose value is an array of externalPropertyFile objects. The sarif URI protocol is used to reference the location in the array. Each run is unchanged except for the fileLocation of the external property, and the contents of the external file is unchanged. There is no coupling of runs using this scheme, and file location in an external property file is always an externalPropertyFile object.

A further step would be to not require the externalPropertyFiles fileLocation in a run to change its fileLocation, but to inform consumers that if an inlineExternalFiles property exists with a matching instanceGuid then this is the externalPropertyFile object; this makes inline and externalizing simple.

{
  ...
  
  "inlineExternalFiles":  [
    {
      "$schema":
        "http:///json.schemastore.org/sarif-external-property-file-2.0.0",

      "version": "2.0.0",

      "instanceGuid": "22222222-2222-2222-2222-222222222222",

      "run.files": {
        "apple.png": {
          "mimeType": "image/png"
        },
        "banana.png": {
          "mimeType": "image/png"
        }
      }
    }
  ],

  "runs": [
    { 
      "originalUriBaseIds": {
        "INLINE": {
          "uri": "sarif:///inlineExternalPropertyFiles/"
        }
      },

      "externalPropertyFiles": {
        "files": [
          { 
            "fileLocation": {
              "baseUriId": "INLINE",
              "uri": "0",
              "fileIndex": 1
            },
            "instanceGuid": "22222222-2222-2222-2222-222222222222",
            "itemCount": 2
          }
      ]
    }
  ]
}

@ghost
Copy link

ghost commented Feb 22, 2019 via email

@michaelcfanning michaelcfanning changed the title Consider moving externalFiles to log object Provide ability to inline externalized properties into the root SARIF log Feb 22, 2019
@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Feb 24, 2019

EBALLOT PROPOSAL

Provide a mechanism for inlining externalized property data at the log level. This facility allows for shared data between runs that is itself persisted to the log (e.g., common taxonomy metadata will be useful to store in this way).

API IMPACT

SDK PR: microsoft/sarif-sdk#1319
Add sarifLog.inlineExternalPropertyFiles, an array of externalPropertyFile objects (already defined in the schema)

NOTES

The run-level property run.externalPropertyFiles can specify the location of an inline external property file in two ways.

  1. A uri property under run.externalPropertyFiles (for example, externalPropertyFiles/conversion/fileLocation/uri) can specify the sarif scheme and a JSON pointer (e.g., sarif://inlineExternalPropertyFiles/6)
  2. An instanceGuid property under run.externalPropertyFiles (for example, externalPropertyFiles/conversion/instanceGuid) can match the instanceGuid property of one of the externalPropertyFile objects in sarifLog.inlineExternalPropertyFiles.

POST-BALLOT NOTES:

  • Erratum: The name of the new property should be sarifLog.inlineExternalProperties, not inlineExternalPropertyFiles, and it is an array of externalProperties objects (as defined in §4.3 of the spec), not externalPropertyFile objects.

    If you look at all the examples above, you'll see that they really are arrays of externalProperties objects. @kupsch's first example proposed to name the new property inlineExternalFiles, which would be ok. Later in the thread, I proposed inlineExternalProperties, as suggested here, which I prefer because it matches the name of the objects in the array.

    "inlineExternalPropertyFiles", as we proposed in the ballot, is just wrong. There is an object in the spec called externalPropertyFile, but it is just a "locator" for an external file. Our new sarifLog property contains the contents of "external" files, not locators for them.

@michaelcfanning michaelcfanning changed the title Provide ability to inline externalized properties into the root SARIF log Provide mechanism for inlining externalized properties data into the root log Feb 27, 2019
@ghost ghost mentioned this issue Mar 1, 2019
michaelcfanning pushed a commit to microsoft/sarif-sdk that referenced this issue Mar 4, 2019
* adding inline externalpropertyfiles at root level

* Update release notes

* Tweak release history
ghost pushed a commit that referenced this issue Mar 8, 2019
ghost pushed a commit that referenced this issue Mar 8, 2019
This change draft will ultimately contain the changes for issues #321
(inlining externalized properties) and #335 (external property-file
renames), including all post-ballot changes.

Currently, it contains only the changes for #335.
ghost pushed a commit that referenced this issue Mar 9, 2019
@ghost ghost added the change-draft-superfluous The change is a simple rename; no change draft needed. label Mar 9, 2019
ghost pushed a commit that referenced this issue Mar 12, 2019
The change draft said that if an externalProperties object occurs as an
element of sarifLog.inlineExternalProperties, then if its $schema
property is omitted, it inherits the value from the containing
sarifLog.$schema. But this is wrong, because sarifLog.$schema is the
schema for a SARIF log file, while externalProperties.$schema is the
schema for an external properties file.

The change draft correctly said that externalProperties.version can
inherit from sarifLog.version. This is right because both properties
refer to a version of the SARIF specification.
@ghost ghost added change-draft-available and removed change-draft-superfluous The change is a simple rename; no change draft needed. labels Mar 14, 2019
@ghost ghost self-assigned this Mar 14, 2019
@ghost ghost added enhancement impact-non-breaking-change design-approved The TC approved the design and I can write the change draft labels Mar 14, 2019
ghost pushed a commit that referenced this issue Mar 14, 2019
@ghost ghost added the merged Changes merged into provisional draft. label Mar 14, 2019
@kupsch
Copy link

kupsch commented Mar 18, 2019

In 3.10.3, the sentence describing the sarif sheme ("Such a URI..."), would be more specific as:

Such a URI uses the sarif scheme. The sarif URI scheme consists of only a scheme (with the value sarif) and a path component. The path component is interpreted as a JSON pointer [RFC6901] into the SARIF document containing the URI. The authority, query and fragment URI components are forbidden.

@kupsch
Copy link

kupsch commented Mar 18, 2019

The sarif scheme should be registered with IANA. If not on someone list of task already.

@ghost
Copy link

ghost commented Mar 18, 2019

@kupsch @michaelcfanning

Jim, I added your proposed text change to the punch list in #342. I opened #343, "Register the sarif scheme with IANA", labled tracking-only (although if we get it registered before we become a standard, I'll mention it in the spec).

ghost pushed a commit that referenced this issue Mar 19, 2019
This draft contains all the changes through "e-ballot #2," which
opened on Friday March 15 and will close on Friday March 22. It contains
changes for ballot issues #168, #291, #309, #320, #321, #326, #335,
and #341, as well as for previously approved issue #340.

It does _not_ contain changes for any issues from "e-ballot #3,"
which will open on Friday March 22 and close on Friday March 29.
@ghost
Copy link

ghost commented Mar 29, 2019

Approved in e-ballot #2.

@ghost ghost closed this as completed Mar 29, 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. design-approved The TC approved the design and I can write the change draft e-ballot e-ballot-2 enhancement impact-non-breaking-change merged Changes merged into provisional draft. resolved-fixed sdk-code-complete tc-32
Projects
None yet
Development

No branches or pull requests

2 participants