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 findings section to detectorTool #21

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Add findings section to detectorTool #21

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9677ec6
Add findings section to detectorTool
drdavella Mar 8, 2024
2a1715a
Address code review feedback
drdavella Mar 8, 2024
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
62 changes: 55 additions & 7 deletions codetf.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +210,65 @@
"type": "string",
"description": "Name of the tool that detected the issue"
},
"ruleId": {
"type": "string",
"description": "ID of the tool vendor rule that detected the issue"
"rule": {
"$ref": "#/definitions/detector/rule",
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like the name detector. Consider chaining detectionTool to detector.

"description": "The rule that detected the issue"
},
"ruleDescription": {
"type": "string",
"description": "Short description of the tool vendor rule that detected the issue"
"findings": {
"type": "array",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the case where a codemod would have multiple findings for a single tool within a single change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The detectionTool belongs to the result and not to the change so I think we can expect potentially multiple findings here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I misread that.

I know it's not explicit in the data model, but to help with my understanding, what's the relationship that we expect between change and finding? Is it, every finding that has fixed: true has one or more changes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think so. Would it make sense to include the same finding ID as an optional field of cange or changeSet as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll vote YAGNI for now but we can consider enriching the data model with that link later.

"items": { "$ref": "#/definitions/detector/finding" },
"maxItems": 20
}
},
"additionalProperties": true,
"required": ["name", "ruleId"]
"required": ["name", "rule", "findings"]
},

"detector": {
"finding": {
"type": "object",
"properties": {
"id": {
"type": "string",
"description": "A unique identifier for the finding (e.g. 'guid' from SARIF)"
},
"fixed": {
"type": "boolean",
"description": "Whether the finding was fixed by the codemod"
},
"reason": {
"type": "string",
"description": "Reason the finding was not fixed"
}
},
"additionalProperties": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this section, but it could be potentially massive. Scanners commonly find pathological conditions where they end up reporting 50,000 things. Is there anything we can/should do in the spec to protect ourselves from massive, redundant info?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add a maxItems although it's incumbent upon the consumer to enforce that. Basically it would mean we'd ignore any results beyond maxItems.

"required": ["id", "fixed"],
"if": {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL you can have if statements in here

"properties": {
"fixed": { "boolean": false }
},
"required": ["reason"]
Comment on lines +246 to +250
Copy link
Contributor

Choose a reason for hiding this comment

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

fancy

}
},
"rule": {
"type": "object",
"properties": {
"id": {
"type": "string",
"description": "The ID of the rule"
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine also a web link to the rule definition.

I think the name and ID might be the same in some cases. For CodeQL, this is definitely the case. Is that okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think that's fine. This purpose is to enable us to be consistent with "Fixes rule from tool (id: )", but I don't think it's a problem if name == id.

"type": "string",
"description": "The name of the rule. Potentially the same as the ID, but more human-readable"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think description is too onerous to add for codemod authors. We should provide a link to their documentation, if they have one, and that's it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay that's fair. So optional ruleUrl or something similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think rule URL is needed and scratches this itch

"url": {
"type": "string",
"description": "Link to the rule documentation"
}
},
"additionalProperties": false,
"required": ["id", "name"]
}
}
},

Expand Down