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

Allow invocation.executableLocation to be relative #221

Closed
ghost opened this issue Aug 21, 2018 · 5 comments
Closed

Allow invocation.executableLocation to be relative #221

ghost opened this issue Aug 21, 2018 · 5 comments

Comments

@ghost
Copy link

ghost commented Aug 21, 2018

This is AFAIK the only fileLocation object in the spec whose uri property is required to be an absolute URI. Why didn't we just define invocation.executableLocation as a URI-formatted string?

Recall that we settled on an approach where we used fileLocation objects for things that there might be "many" of (to reduce file size) and URIs for things that there were "few" of (to reduce format complexity). I think we made the wrong choice here -- either that, or I failed to find this one when we settled on that approach.

I found this problem while implementing a SARIF validation rule that ensures that those URIs that are required to be absolute, actually are.

@michaelcfanning @kupsch

@kupsch
Copy link

kupsch commented Aug 21, 2018

Having a fileLocation object is useful here just like it for other uses of fileLocation objects: it makes normalization a bit simpler as it puts the installation directory in a variable, it can hide information like the user account if a tool is installed in the user's home directory, and it can reduce duplicate prefixes if running the assessment requires invoking multiple executables from the tool's installation directory. Having a file location would also allow an entry in the file object list where things like a checksum could be supplied. Can a URI-formatted string be in the files array?

As far as it being required to be absolute, I don't see whey the executableLocation should be any different from other file locations; Ideally it would be, but this should be the case with all file locations.

@michaelcfanning
Copy link
Contributor

The distinction here is whether something must exclusively refer to an absolute URI. These are expressed as URIs, the remainder are expressed as file locations.

I honestly can't recall why I was so convinced this property s/be expressed strictly as an absolute URL. A set of analysis tools might, after all, be checked in to version control. I am willing to convert this one to file location, sounds like Jim agrees.

@ghost
Copy link
Author

ghost commented Aug 21, 2018

@michaelcfanning @kupsch The reason we insisted that this one be an absolute URI was that we wanted the log to record the actual location of the tool that was executed (there might be more than one on the execution path). We can certainly change our minds, but that was the reasoning that led to this requirement.

Whether or not it needs to be absolute, the next question is whether to represent it as a URI-formatted string or as a fileLocation object. Either way, there can be a file-object-valued entry in the run.files dictionary that specifies (for example) its hash value.

Jim, I think that was a confusion on your part. You specify the location of a file with a fileLocation object (with its uri and uriBaseId properties). You specify metadata about a file with a file object.

For example, if executableLocation is required to be absolute, then either of these work:

Example 1: Specify absolute executableLocation with a string:

{ # A run object.
  "files": {
    "file:///C:/my-project/tools/scanner.exe": {
      "hashes": [
        "sha-256": "..."
      ]
    }
  },
  "invocations": [
    {
      "executableLocation": "file:///C:/my-project/tools/scanner.exe"
    }
  ]
}

Example 2: Specify absolute executableLocation with a fileLocation object:

{ # A run object.
  "files": {
    "file:///C:/my-project/tools/scanner.exe": {
      "hashes": [
        "sha-256": "..."
      ]
    }
  },
  "invocations": [
    {
      "executableLocation": {
        "uri": "file:///C:/my-project/tools/scanner.exe"
      }
    }
  ]
}

If executableLocation can be relative, both ways still work:

Example 3: Specify relative executableLocation with a string:

{ # A run object.
  "files": {
    "scanner.exe": {
      "hashes": [
        "sha-256": "..."
      ]
    }
  },
  "invocations": [
    {
      "executableLocation": "scanner.exe"
    }
  ]
}

Example 4: Specify relative executableLocation with a fileLocation object:

{ # A run object.
  "originalUriBaseIds": {
    "TOOLSROOT": "file:///C:/my-project/tools"
  },
  "files": {
    "scanner.exe": {
      "hashes": [
        "sha-256": "..."
      ]
    }
  },
  "invocations": [
    {
      "executableLocation": {
        "uri": "scanner.exe",
        "uriBaseId": "TOOLSROOT"
      }
    }
  ]
}

@ghost
Copy link
Author

ghost commented Aug 21, 2018

Having said all that: the scenario where tools are checked in to the tree, and so might appear anywhere on disk, argues for a fileLocation object (so we can specify uriBaseId), and for removing the requirement that its uri property be absolute.

@ghost ghost changed the title Why is invocation.executableLocation a fileLocation? Don't require invocation.executableLocation to be absolute. Aug 27, 2018
@ghost
Copy link
Author

ghost commented Aug 27, 2018

@michaelcfanning @kupsch

Having been convinced by Jim's argument, I changed the title of this issue to "Don't require invocation.executableLocation to be absolute."

@michaelcfanning This is a non-breaking change, but since I have it in front of me and it's a tiny change, I'll take care of it now (for the sake of a quick-and-easy reduction in our issue count).

@ghost ghost changed the title Don't require invocation.executableLocation to be absolute. Allow invocation.executableLocation to be relative Aug 27, 2018
@ghost ghost self-assigned this Aug 27, 2018
ghost pushed a commit that referenced this issue Aug 27, 2018
ghost pushed a commit that referenced this issue Sep 5, 2018
@ghost ghost closed this as completed Sep 5, 2018
ghost pushed a commit to microsoft/sarif-sdk that referenced this issue Sep 13, 2018
This change updates the schema with the following issues approved at TC <span>#</span>23 on 2018-09-12, and makes the required code and test changes:

- oasis-tcs/sarif-spec#174: Add result.occurrenceCount.
- oasis-tcs/sarif-spec#233: Make rule.id optional.
- oasis-tcs/sarif-spec#237: Make result.graphs and run.graphs dictionaries.

Also:
- Update code and tests for oasis-tcs/sarif-spec#221: Allow invocation.executableLocation to be relative. There was no schema change here, but running `Update-Expected.ps1` revealed that we hadn't made the necessary changes to the validator.
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants