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.

Sign up for GitHub

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

Add OriginalUriBaseIds sample. #14

Merged
1 commit merged into from
Apr 24, 2020
Merged

Add OriginalUriBaseIds sample. #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions samples/OriginalUriBaseIds.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
"version": "2.1.0",
"properties": {
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

properties [](start = 3, length = 10)

btw - original url base ids shouldn't create 'loops'. since we have a dedicated 'don't screw up original uri base ids' rule, we could add this verification to that check. #Pending

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're about to have one of our partitioning discussions. Rule SARIF1018 is about the validity of the URIs in originalUriBaseIds: They have to end with a slash, and if there's no uriBaseId, they have to be absolute. The (good) rule you're proposing is about the structure of the uriBaseIds. I'll make it part of 1018 if you want. But do consider how you would (1) friendly-name, and (2) describe, the combined rule.

In reply to: 414814948 [](ancestors = 414814948)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed microsoft/sarif-sdk#1864, "Analysis rule: uriBaseIds should not form loops". In the description I explicitly say that we haven't decided whether this is a separate rule or an enhancement to an existing one.

In reply to: 414865952 [](ancestors = 414865952,414814948)

"comment": "This sample demonstrates the use of originalUriBaseIds."
},
"runs": [
{
"tool": {
"driver": {
"name": "SarifSamples"
}
},
"originalUriBaseIds": {
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originalUriBaseIds [](start = 7, length = 18)

you don't have an example in here that resolves to an absolute URL #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example (LOGSROOT resolving directly to [file:///C:/logs/](file:///C:/logs/`)).

In reply to: 414814508 [](ancestors = 414814508)

"PROJECTROOT": {
"description": {
"text": "The root directory for all project files."
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for all project files [](start = 40, length = 21)

Confusing example. most windows project files have their own directory, and therefore each project file might have its own root (it does in visual studio). You might change this to SOLUTIONROOT, of which we can more reasonably expect there to be a single definition for a scan run). #Resolved

Copy link
Author

@ghost ghost Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to REPOROOT, and re-described, per your comment below.

In reply to: 414790002 [](ancestors = 414790002)

},
"properties": {
"comment": "The SARIF producer has chosen not to specify a URI for PROJECTROOT. See §3.14.14, NOTE 1, for an explanation."
}
},
"SRCROOT": {
"uri": "src",
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src [](start = 18, length = 3)

Is this legal? shouldn't it end in a forward slash? do we have a hole in our validator (or maybe you haven't authored unit tests for these assets that validate?) btw - if this is a bug it's in our spec as well, which i'm looking at #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I've been running sarif validate on the sample files, but my installed copy doesn't yet have the "trailing slash" fix. I'll switch to using my private bits.

In reply to: 414813884 [](ancestors = 414813884)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... aaaaand I just found a bug in the trailing slash analysis. I'll file an SDK bug and push a PR.

In reply to: 414824105 [](ancestors = 414824105,414813884)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed and fixed the SDK bug, raised a PR on the SDK, added the slash here.

In reply to: 414829741 [](ancestors = 414829741,414824105,414813884)

"uriBaseId": "PROJECTROOT",
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PROJECTROOT [](start = 24, length = 11)

Ah, now I understand, by project root you mean, ENLISTMENTROOT. Suggest making that change. Or perhaps use REPOROOT . Project is just too overloaded a term in MS dev tooling to provide a clear example here. To an old MS workhorse like me, that is. :P #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to REPOROOT.

In reply to: 414791357 [](ancestors = 414791357)

"description": {
"text": "The root of the source tree."
},
"properties": {
"comment": "SRCROOT is expressed relative to PROJECTROOT."
}
}
},
"results": [
{
"ruleId": "SMP0001",
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SMP0001 [](start = 21, length = 7)

How is SMP pronounced? 'simp'? if so, unfortunate. Also 'too many zeros'. :)

How about a convention of SAMPXXX for the work we do here? I know this is a bit of a departure from conventions used by tools that reasonable allocation a range of 9999 potential issues. I think the adjustment helps with readability here. i.e., I like 'SAMP001' better than 'SMP0001'. Thoughts? This is nearly entirely in the realm of opinion... #Pending

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm SMPathetic to your concern about SMP, but I don't want to model, in our entire corpus of test assets, a pattern that we don't encourage tool authors to follow. How about TST0001, which I think you'll agree is obviously pronounced "test"?

Whatever we decide to do, I'll do in a separate PR that fixes all the files at once.

In reply to: 414792603 [](ancestors = 414792603)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no rigid standard here. Honestly, your string of zeros compromises readability and if you are adhering to some standard tooling convention, we don't typically start indexing from 1, these lower-level ids are almost always reserved for internal error codes. CS1002 is more typical.

How about TUT1001, TUT1002, etc. or TST1001, TST1002, etc.

In reply to: 414849147 [](ancestors = 414849147,414792603)

Copy link
Author

@ghost ghost Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sold! I filed #15, "Change all ruleIds to TUT1nnn, e.g., TUT1001".

In reply to: 414852413 [](ancestors = 414852413,414849147,414792603)

"message": {
"text": "A result outside the source tree."
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "README.md",
"uriBaseId": "PROJECTROOT",
"properties": {
"comment": "If PROJECTROOT is C:\\project, this file location resolves to C:\\project\\README.md"
}
}
}
}
]
},
{
"ruleId": "SMP0002",
"message": {
"text": "A result in a source file."
Copy link
Member

@michaelcfanning michaelcfanning Apr 24, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

source file [](start = 35, length = 11)

hm, shall we power up this sample and add a reference to a directory in addition to distinct files? is this a concern that belongs in a different sample? #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

In reply to: 414794276 [](ancestors = 414794276)

},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "io/kb.c",
"uriBaseId": "SRCROOT",
"properties": {
"comment": "If PROJECTROOT is C:\\project, this file location resolves to C:\\project\\src\\io\\kb.c"
}
}
}
}
]
}
],
"columnKind": "utf16CodeUnits"
}
]
}