Skip to content

Commit 50e3b99

Browse files
committed
Improve CDS extractor diagnostics and tests
Improves the diagnostic messages created by the CDS extractor reports for the edge case where some file path is associated with a diagnostic warning or error but is not a path within the source root directory that the CDS extractor was configured to scan. This change attempts to continue to prevent path injection and path traversal attacks for any diagnostics generated by the CDS extractor while ensuring the unlinkability of this edge case is explained to any user viewing such diagnostics. We don't expect to encounter situations where a diagnostic error or warning is reported for any file outside of the scanned source root directory, but we want to handle such situations well and we do so here by improving the text of our diagnostic message to the user without giving the user a link to a non-repo file.
1 parent 7d1e1bc commit 50e3b99

File tree

4 files changed

+63
-4
lines changed

4 files changed

+63
-4
lines changed

extractors/cds/tools/dist/cds-extractor.bundle.js

Lines changed: 11 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

extractors/cds/tools/dist/cds-extractor.bundle.js.map

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

extractors/cds/tools/src/diagnostics.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,21 @@ function addDiagnostic(
7979
? convertToRelativePath(filePath, sourceRoot)
8080
: resolve(filePath);
8181

82+
// If the file was remapped to source root due to being outside the repository,
83+
// append an explanatory note to the message
84+
let finalMessage = message;
85+
if (sourceRoot && finalFilePath === '.' && filePath !== sourceRoot) {
86+
const resolvedSourceRoot = resolve(sourceRoot);
87+
const resolvedFilePath = filePath.startsWith('/')
88+
? resolve(filePath)
89+
: resolve(resolvedSourceRoot, filePath);
90+
91+
// Only add the note if the file was actually outside the source root
92+
if (resolvedFilePath !== resolvedSourceRoot) {
93+
finalMessage = `${message}\n\n**Note**: The file \`${filePath}\` is located outside the scanned source directory and cannot be linked directly in this diagnostic. This diagnostic is associated with the repository root instead.`;
94+
}
95+
}
96+
8297
try {
8398
execFileSync(codeqlExePath, [
8499
'database',
@@ -88,7 +103,7 @@ function addDiagnostic(
88103
`--source-id=${sourceId}`,
89104
`--source-name=${sourceName}`,
90105
`--severity=${severity}`,
91-
`--markdown-message=${message}`,
106+
`--markdown-message=${finalMessage}`,
92107
`--file-path=${finalFilePath}`,
93108
'--',
94109
`${process.env.CODEQL_EXTRACTOR_CDS_WIP_DATABASE ?? ''}`,

extractors/cds/tools/test/src/diagnostics.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,13 @@ describe('diagnostics', () => {
7777

7878
expect(result).toBe(true);
7979
expectRelativePathInCall(`app${sep}models${sep}schema.cds`);
80+
// Should NOT include explanatory note for files inside source root
81+
expect(childProcess.execFileSync).toHaveBeenCalledWith(
82+
expect.any(String),
83+
expect.arrayContaining([
84+
expect.stringMatching(/--markdown-message=Syntax error in CDS file$/),
85+
]),
86+
);
8087
restoreMockEnvironment(originalEnv);
8188
});
8289

@@ -116,6 +123,15 @@ describe('diagnostics', () => {
116123
expect(result).toBe(true);
117124
// Should point to source root '.' when file is outside source root
118125
expectRelativePathInCall('.');
126+
// Should include explanatory note in the message
127+
expect(childProcess.execFileSync).toHaveBeenCalledWith(
128+
expect.any(String),
129+
expect.arrayContaining([
130+
expect.stringMatching(
131+
/--markdown-message=.*\*\*Note\*\*.*located outside the scanned source directory/s,
132+
),
133+
]),
134+
);
119135
restoreMockEnvironment(originalEnv);
120136
});
121137

@@ -155,6 +171,15 @@ describe('diagnostics', () => {
155171
expect(result).toBe(true);
156172
// Should point to source root '.' when path tries to escape source root
157173
expectRelativePathInCall('.');
174+
// Should include explanatory note in the message
175+
expect(childProcess.execFileSync).toHaveBeenCalledWith(
176+
expect.any(String),
177+
expect.arrayContaining([
178+
expect.stringMatching(
179+
/--markdown-message=.*\*\*Note\*\*.*located outside the scanned source directory/s,
180+
),
181+
]),
182+
);
158183
restoreMockEnvironment(originalEnv);
159184
});
160185

@@ -176,6 +201,15 @@ describe('diagnostics', () => {
176201
expect(result).toBe(true);
177202
// Should point to source root '.' when resolved path is outside source root
178203
expectRelativePathInCall('.');
204+
// Should include explanatory note in the message
205+
expect(childProcess.execFileSync).toHaveBeenCalledWith(
206+
expect.any(String),
207+
expect.arrayContaining([
208+
expect.stringMatching(
209+
/--markdown-message=.*\*\*Note\*\*.*located outside the scanned source directory/s,
210+
),
211+
]),
212+
);
179213
restoreMockEnvironment(originalEnv);
180214
});
181215
});

0 commit comments

Comments
 (0)