Skip to content

Commit 913e1bb

Browse files
sam-robsonsam-robson
authored
Merge pull request #4283 from github/sam-robson/decode-diff-informed-sarif-uris
fix: decode SARIF location URIs in diff-informed alert filtering
2 parents c5320e3 + b05fe69 commit 913e1bb

4 files changed

Lines changed: 218 additions & 6 deletions

File tree

‎lib/entry-points.js‎

Lines changed: 10 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/upload-lib.test.ts‎

Lines changed: 192 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,11 @@ import * as diffUtils from "./diff-informed-analysis-utils";
1313
import { getEnv } from "./environment";
1414
import { Logger } from "./logging";
1515
import * as sarif from "./sarif";
16-
import { RecordingLogger, setupTests } from "./testing-utils";
16+
import {
17+
checkExpectedLogMessages,
18+
RecordingLogger,
19+
setupTests,
20+
} from "./testing-utils";
1721
import * as uploadLib from "./upload-lib";
1822
import { UploadPayload } from "./upload-lib/types";
1923
import { GitHubVariant, withTmpDir } from "./util";
@@ -1015,12 +1019,62 @@ for (const analysisKind of analyses.supportedAnalysisKinds) {
10151019
);
10161020
}
10171021

1022+
function createMockSarifWithResults(
1023+
results: sarif.Result[],
1024+
): Partial<sarif.Log> {
1025+
return {
1026+
version: "2.1.0",
1027+
runs: [
1028+
{
1029+
tool: {
1030+
driver: {
1031+
name: "test tool",
1032+
},
1033+
},
1034+
results,
1035+
},
1036+
],
1037+
};
1038+
}
1039+
1040+
function createMockResultWithUri(
1041+
uri: string,
1042+
startLine = 2,
1043+
relatedUri?: string,
1044+
): sarif.Result {
1045+
return {
1046+
ruleId: "test-rule",
1047+
message: { text: "test message" },
1048+
locations: [
1049+
{
1050+
physicalLocation: {
1051+
artifactLocation: { uri },
1052+
region: { startLine },
1053+
},
1054+
},
1055+
],
1056+
...(relatedUri
1057+
? {
1058+
relatedLocations: [
1059+
{
1060+
physicalLocation: {
1061+
artifactLocation: { uri: relatedUri },
1062+
region: { startLine },
1063+
},
1064+
},
1065+
],
1066+
}
1067+
: {}),
1068+
};
1069+
}
1070+
10181071
function runFilterAlertsByDiffRange(
10191072
input: Partial<sarif.Log>,
10201073
diffRanges: diffUtils.DiffThunkRange[],
1074+
logger: Logger = new RecordingLogger(),
10211075
): Partial<sarif.Log> {
10221076
sinon.stub(diffUtils, "readDiffRangesJsonFile").returns(diffRanges);
1023-
return uploadLib.filterAlertsByDiffRange(new RecordingLogger(), input);
1077+
return uploadLib.filterAlertsByDiffRange(logger, input);
10241078
}
10251079

10261080
test.serial(
@@ -1044,3 +1098,139 @@ test.serial(
10441098
t.deepEqual(actualOutput, expectedOutput);
10451099
},
10461100
);
1101+
1102+
for (const [filePath, uri] of [
1103+
["with space/board.js", "with%20space/board.js"],
1104+
["src/[id]/board.js", "src/%5Bid%5D/board.js"],
1105+
["src/file#1.js", "src/file%231.js"],
1106+
["src/100%.js", "src/100%25.js"],
1107+
["src/literal%20.js", "src/literal%2520.js"],
1108+
["src/foo:bar.js", "src/foo%3Abar.js"],
1109+
["src/caf\u00e9.js", "src/caf%C3%A9.js"],
1110+
["src/caf\u00e9.js", "src/caf%c3%a9.js"],
1111+
]) {
1112+
test.serial(
1113+
`filterAlertsByDiffRange matches percent-encoded URI ${uri}`,
1114+
(t) => {
1115+
const matchingResult = createMockResultWithUri(uri);
1116+
const nonMatchingResult = createMockResultWithUri("other.js");
1117+
const input = createMockSarifWithResults([
1118+
matchingResult,
1119+
nonMatchingResult,
1120+
]);
1121+
1122+
const actualOutput = runFilterAlertsByDiffRange(input, [
1123+
{ path: filePath, startLine: 1, endLine: 3 },
1124+
]);
1125+
1126+
t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]);
1127+
},
1128+
);
1129+
}
1130+
1131+
test.serial(
1132+
"filterAlertsByDiffRange filters out percent-encoded URI when outside diff line range",
1133+
(t) => {
1134+
const outOfRangeResult = createMockResultWithUri(
1135+
"with%20space/board.js",
1136+
2,
1137+
);
1138+
const inRangeResult = createMockResultWithUri("with%20space/board.js", 15);
1139+
const input = createMockSarifWithResults([outOfRangeResult, inRangeResult]);
1140+
const actualOutput = runFilterAlertsByDiffRange(input, [
1141+
{ path: "with space/board.js", startLine: 10, endLine: 20 },
1142+
]);
1143+
1144+
t.deepEqual(actualOutput.runs?.[0].results, [inRangeResult]);
1145+
},
1146+
);
1147+
1148+
test.serial(
1149+
"filterAlertsByDiffRange filters out percent-encoded URI when path does not match",
1150+
(t) => {
1151+
const input = createMockSarifWithResults([
1152+
createMockResultWithUri("with%20space/board.js"),
1153+
]);
1154+
const actualOutput = runFilterAlertsByDiffRange(input, [
1155+
{ path: "other/board.js", startLine: 1, endLine: 3 },
1156+
]);
1157+
1158+
t.deepEqual(actualOutput.runs?.[0].results, []);
1159+
},
1160+
);
1161+
1162+
test.serial(
1163+
"filterAlertsByDiffRange keeps alert when relatedLocations has a matching percent-encoded URI",
1164+
(t) => {
1165+
const matchingRelatedResult = createMockResultWithUri(
1166+
"src/promiseUtils.js",
1167+
2,
1168+
"with%20space/ParseObject.js",
1169+
);
1170+
const nonMatchingResult = createMockResultWithUri("other.js");
1171+
const input = createMockSarifWithResults([
1172+
matchingRelatedResult,
1173+
nonMatchingResult,
1174+
]);
1175+
1176+
const actualOutput = runFilterAlertsByDiffRange(input, [
1177+
{ path: "with space/ParseObject.js", startLine: 1, endLine: 3 },
1178+
]);
1179+
1180+
t.deepEqual(actualOutput.runs?.[0].results, [matchingRelatedResult]);
1181+
},
1182+
);
1183+
1184+
for (const uri of ["dir%ZZ/board.js", "src/bad%.js", "src/bad%C3%28.js"]) {
1185+
test.serial(
1186+
`filterAlertsByDiffRange falls back to raw path for malformed URI ${uri}`,
1187+
(t) => {
1188+
const logger = new RecordingLogger();
1189+
const matchingResult = createMockResultWithUri(uri);
1190+
const input = createMockSarifWithResults([matchingResult]);
1191+
const actualOutput = runFilterAlertsByDiffRange(
1192+
input,
1193+
[{ path: uri, startLine: 1, endLine: 3 }],
1194+
logger,
1195+
);
1196+
1197+
t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]);
1198+
checkExpectedLogMessages(t, logger.messages, [
1199+
`Failed to decode location URI "${uri}", comparing it as a raw path`,
1200+
]);
1201+
},
1202+
);
1203+
}
1204+
1205+
test.serial(
1206+
"filterAlertsByDiffRange filters out malformed URI when raw path does not match",
1207+
(t) => {
1208+
const logger = new RecordingLogger();
1209+
const input = createMockSarifWithResults([
1210+
createMockResultWithUri("dir%ZZ/board.js"),
1211+
]);
1212+
const actualOutput = runFilterAlertsByDiffRange(
1213+
input,
1214+
[{ path: "src/different.js", startLine: 1, endLine: 3 }],
1215+
logger,
1216+
);
1217+
1218+
t.deepEqual(actualOutput.runs?.[0].results, []);
1219+
checkExpectedLogMessages(t, logger.messages, [
1220+
'Failed to decode location URI "dir%ZZ/board.js", comparing it as a raw path',
1221+
]);
1222+
},
1223+
);
1224+
1225+
test.serial(
1226+
"filterAlertsByDiffRange matches raw unencoded path containing valid percent-encoding sequence",
1227+
(t) => {
1228+
const matchingResult = createMockResultWithUri("dir/100%20done.js");
1229+
const input = createMockSarifWithResults([matchingResult]);
1230+
const actualOutput = runFilterAlertsByDiffRange(input, [
1231+
{ path: "dir/100%20done.js", startLine: 1, endLine: 3 },
1232+
]);
1233+
1234+
t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]);
1235+
},
1236+
);

‎src/upload-lib.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,17 +1099,30 @@ export function filterAlertsByDiffRange(
10991099
return locations.some((physicalLocation) => {
11001100
const locationUri = physicalLocation?.artifactLocation?.uri;
11011101
const locationStartLine = physicalLocation?.region?.startLine;
1102-
if (!locationUri || locationStartLine === undefined) {
1102+
if (
1103+
typeof locationUri !== "string" ||
1104+
!locationUri ||
1105+
locationStartLine === undefined
1106+
) {
11031107
return false;
11041108
}
1109+
let decodedLocationUri = locationUri;
1110+
try {
1111+
decodedLocationUri = decodeURIComponent(locationUri);
1112+
} catch {
1113+
logger.debug(
1114+
`Failed to decode location URI "${locationUri}", comparing it as a raw path`,
1115+
);
1116+
}
11051117
// Alert filtering here replicates the same behavior as the restrictAlertsTo
11061118
// extensible predicate in CodeQL. See the restrictAlertsTo documentation
11071119
// https://codeql.github.com/codeql-standard-libraries/csharp/codeql/util/AlertFiltering.qll/predicate.AlertFiltering$restrictAlertsTo.3.html
11081120
// for more details, such as why the filtering applies only to the first line
11091121
// of an alert location.
11101122
return diffRanges.some(
11111123
(range) =>
1112-
range.path === locationUri &&
1124+
(range.path === decodedLocationUri ||
1125+
range.path === locationUri) &&
11131126
((range.startLine <= locationStartLine &&
11141127
range.endLine >= locationStartLine) ||
11151128
(range.startLine === 0 && range.endLine === 0)),

‎unreleased-change-notes/decode-diff-informed-sarif-uris.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Fixed diff-informed alert filtering incorrectly removing alerts in changed files whose paths contain spaces, special characters, or non-ASCII characters. [#4283](https://github.com/github/codeql-action/pull/4283)

0 commit comments

Comments
 (0)