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

Jump to bottom

@W-11111352@: SARIF and Table format now properly format output in the event of SFGE errors. #679

Merged
merged 2 commits into from
May 16, 2022
Merged

@W-11111352@: SARIF and Table format now properly format output in the event of SFGE errors. #679

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ee5940
@W-11111352@: SARIF and Table format now properly format output in th…
jfeingold35 May 16, 2022
1ee5092
@W-11111352@: Integrated feedback from code review.
jfeingold35 May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 7 additions & 4 deletions sfge/src/main/java/com/salesforce/rules/Violation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
import java.util.Objects;

public abstract class Violation implements Comparable<Violation>, RuleThrowable {
private static final String INTERNAL_ERROR_RULENAME = "InternalExecutionError";
private static final String INTERNAL_ERROR_CATEGORY = "InternalExecutionError";

/** The simple violation message that will be conveyed to users of the plugin. */
protected final String message;

Expand Down Expand Up @@ -323,8 +326,8 @@ public InternalErrorViolation(final String details, final SFVertex vertex) {
// Internal error violations don't have a rule associated with them, so we should set
// the rule-related properties
// to non-null values now, to avoid NPEs during sorting.
this.ruleName = "";
this.category = "";
this.ruleName = INTERNAL_ERROR_RULENAME;
this.category = INTERNAL_ERROR_CATEGORY;
this.description = "";
this.severity = AbstractRule.SEVERITY.LOW.code;
}
Expand Down Expand Up @@ -404,8 +407,8 @@ public TimeoutViolation(final String message, final SFVertex vertex) {
// Timeout violations don't have an actual rule associated with them, so we should set
// the rule-related properties
// to non-null values now, to avoid NPEs during sorting.
this.ruleName = "";
this.category = "";
this.ruleName = INTERNAL_ERROR_RULENAME;
this.category = INTERNAL_ERROR_CATEGORY;
this.description = "";
this.severity = AbstractRule.SEVERITY.LOW.code;
}
Expand Down
19 changes: 16 additions & 3 deletions src/lib/formatter/RuleResultRecombinator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ type DfaTableRow = BaseTableRow & {
"Source Location": string;
"Source Line": number;
"Source Column": number;
"Sink Location": string;
"Sink Line": number;
"Sink Column": number;
"Sink Location": string|null;
"Sink Line": number|null;
"Sink Column": number|null;
};

export class RuleResultRecombinator {
Expand Down Expand Up @@ -307,13 +307,26 @@ URL: ${url}`;
Severity: violation.severity,
Engine: result.engine
};
// A pathless violation can be trivially converted into a row.
if (isPathlessViolation(violation)) {
rows.push({
...baseRow,
Location: `${relativeFile}:${violation.line}`,
Line: violation.line,
Column: violation.column
});
} else if (violation.sinkFileName == null) {
// If the violation is path-based but has no sink file, then the violation indicates an error of some
// kind. So use the source information we were given, but use null for everything else.
rows.push({
...baseRow,
"Source Location": `${relativeFile}:${violation.sourceLine}`,
"Source Line": violation.sourceLine,
"Source Column": violation.sourceColumn,
"Sink Location": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with these nulls as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we want the line and column to still be null, since those are supposed to be numbers. But the location can be an empty string, I believe.

"Sink Line": null,
"Sink Column": null
});
} else {
const relativeSinkFile = path.relative(process.cwd(), violation.sinkFileName);
rows.push({
Expand Down
2 changes: 1 addition & 1 deletion src/lib/formatter/SarifFormatter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ abstract class SarifFormatter {
result.locations = [location];

// If the violation is DFA, we'll want to create a secondary location for the sink vertex.
if (!isPathless) {
if (!isPathless && v.sinkFileName != null) {
const secondaryLocation = SarifFormatter.getLocation(v.sinkFileName);
secondaryLocation.message = {
text: SINK_VERTEX_MESSAGE
Expand Down
6 changes: 3 additions & 3 deletions src/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ export type DfaRuleViolation = BaseViolation & {
sourceColumn: number;
sourceType: string;
sourceMethodName: string;
sinkLine: number;
sinkColumn: number;
sinkFileName: string;
sinkLine: number|null;
sinkColumn: number|null;
sinkFileName: string|null;
};

export type RuleViolation = PathlessRuleViolation | DfaRuleViolation;
Expand Down
61 changes: 56 additions & 5 deletions test/lib/formatter/RuleResultRecombinator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,28 @@ const allFakeDfaRuleResults: RuleResult[] = [
}
];

const fakeDfaWithError: RuleResult[] = [
{
engine: 'sfge',
fileName: sampleFile5,
violations: [{
"sourceLine": 15,
"sourceColumn": 8,
"sourceType": "BearController",
"sourceMethodName": "getAllBears",
"sinkFileName": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would empty strings be safer than nulls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. Stand by for that.

"sinkLine": null,
"sinkColumn": null,
"ruleName": "InternalExecutionError",
"severity": 3,
"message": "Path evaluation timed out after 40 ms",
"url": "https://thisurldoesnotmatter.com",
"category": "InternalExecutionError"
}]
}
]


const allFakeDfaRuleResultsNormalized: RuleResult[] = [
{
engine: 'sfge',
Expand Down Expand Up @@ -525,7 +547,7 @@ describe('RuleResultRecombinator', () => {
expect(invocation.toolExecutionNotifications).to.have.lengthOf(0, 'Tool Execution');
}

function validateSfgeSarif(run: unknown, normalizeSeverity: boolean): void {
function validateSfgeSarif(run: unknown, normalizeSeverity: boolean, expectingErrors: boolean): void {
const driver = run['tool']['driver'];
expect(driver.name).to.equal(ENGINE.SFGE);
expect(driver.version).to.equal(SFGE_VERSION);
Expand All @@ -534,7 +556,11 @@ describe('RuleResultRecombinator', () => {

// tool.driver.rules
expect(driver['rules']).to.have.lengthOf(1, 'Incorrect rule count');
expect(driver['rules'][0].id).to.equal('ApexFlsViolationRule');
if (expectingErrors) {
expect(driver['rules'][0].id).to.equal('InternalExecutionError');
} else {
expect(driver['rules'][0].id).to.equal('ApexFlsViolationRule');
}
if (normalizeSeverity) {
expect(driver['rules'][0].properties.normalizedSeverity).to.equal(1);
} else {
Expand All @@ -546,7 +572,11 @@ describe('RuleResultRecombinator', () => {
expect(run['results']).to.have.lengthOf(1, 'Incorrect result count');
const results = run['results'];
expect(results[0].ruleIndex).to.equal(0);
expect(results[0].relatedLocations).to.exist;
if (expectingErrors) {
expect(results[0].relatedLocations).to.not.exist;
} else {
expect(results[0].relatedLocations).to.exist;
}

// Invocations
expect(run['invocations']).to.have.lengthOf(1, 'Invocations');
Expand Down Expand Up @@ -613,7 +643,7 @@ describe('RuleResultRecombinator', () => {
const run = runs[i];
const engine = run['tool']['driver']['name'];
if (engine === ENGINE.SFGE) {
validateSfgeSarif(run, false);
validateSfgeSarif(run, false, false);
} else {
fail(`Unexpected engine: ${engine}`);
}
Expand All @@ -634,7 +664,7 @@ describe('RuleResultRecombinator', () => {
const run = runs[i];
const engine = run['tool']['driver']['name'];
if (engine === ENGINE.SFGE) {
validateSfgeSarif(run, true);
validateSfgeSarif(run, true, false);
} else {
fail(`Unexpected engine: ${engine}`);
}
Expand All @@ -645,6 +675,27 @@ describe('RuleResultRecombinator', () => {
expect(summaryMap.get('sfge')).to.deep.equal({fileCount: 1, violationCount: 1}, 'Sfge summary should be correct');
});

it ('DFA with timeout error', async () => {
const {minSev, results, summaryMap} = await RuleResultRecombinator.recombineAndReformatResults(fakeDfaWithError, OUTPUT_FORMAT.SARIF, new Set(['sfge']));
const sarifResults: unknown[] = JSON.parse(results as string);
expect(sarifResults['runs']).to.have.lengthOf(1, 'Runs');
const runs = sarifResults['runs'];

for (let i = 0; i < runs.length; i++) {
const run = runs[i];
const engine = run['tool']['driver']['name'];
if (engine === ENGINE.SFGE) {
validateSfgeSarif(run, false, true);
} else {
fail(`Unexpected engine: ${engine}`);
}
}

expect(minSev).to.equal(3, 'Most severe problem');
expect(summaryMap.size).to.equal(1, 'Each supposedly executed engine needs a summary');
expect(summaryMap.get('sfge')).to.deep.equal({fileCount: 1, violationCount: 1}, 'Sfge summary should be correct');
});

it('Handles all pathless engines', async () => {
const allEngines = PathlessEngineFilters.map(engine => engine.valueOf());
for (const engine of allEngines) {
Expand Down