Skip to content

Commit 817e0f9

Browse files
committed
fix(data-mapper-v2): Address Copilot review comments for security and robustness
Security fixes: - Replace polynomial regex with string-based parsing in XsltMetadataSerializer - Add prototype pollution protection in XsltParser with safe property assignment Robustness improvements: - Add robust xslt3 path resolution checking multiple locations - Add XSLT size validation (5MB limit) before processing - Add XSLT parsing error handling with user-friendly messages - Handle empty map definition with null-coalescing fallback - Prevent stale test results with ref-based deduplication - Add detailed temp file cleanup logging Performance optimization: - Optimize position updates to avoid flooding undo history Testing: - Add comprehensive test suites for Redux slices, utilities, and query hooks - 217 new tests added (484 total tests now passing)
1 parent c24e650 commit 817e0f9

22 files changed

Lines changed: 2472 additions & 64 deletions

Localize/lang/strings.json

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,7 @@
400400
"7PtWvu": "(UTC-05:00) Eastern Time (US & Canada)",
401401
"7QymrD": "Required. The string from which the substring is taken.",
402402
"7ScdN6": "Name",
403+
"7V737/": "XSLT not yet generated. Save the map to generate XSLT output.",
403404
"7X4UA/": "Waiting",
404405
"7ZF1Hr": "Validation error",
405406
"7ZR1xr": "Add an action",
@@ -1964,6 +1965,7 @@
19641965
"_7PtWvu.comment": "Time zone value ",
19651966
"_7QymrD.comment": "Required string parameter required to obtain substring",
19661967
"_7ScdN6.comment": "Deployment model resource name label",
1968+
"_7V737/.comment": "Message to display when XSLT content hasn't been generated yet",
19671969
"_7X4UA/.comment": "The status message to show waiting in monitoring view.",
19681970
"_7ZF1Hr.comment": "The title of the validation error field in the static result parseJson action",
19691971
"_7ZR1xr.comment": "Text on example action node",
@@ -3166,6 +3168,7 @@
31663168
"_arjUBV.comment": "Error message when the workflow dark image is empty",
31673169
"_auUI93.comment": "label to inform to upload or select source schema to be used",
31683170
"_auci7r.comment": "Error validation message for CSVs",
3171+
"_aulGxd.comment": "Message when XSLT content is not available",
31693172
"_aurgrg.comment": "Authentication type",
31703173
"_b2aL+f.comment": "Text indicating a menu button to pin an action to the side panel",
31713174
"_b6G9bq.comment": "Label for description of custom encodeUriComponent Function",
@@ -3810,7 +3813,6 @@
38103813
"_qJpnIL.comment": "Label for description of custom endsWith Function",
38113814
"_qKVOwV.comment": "Placeholder text for the MCP server name field",
38123815
"_qMFpNH.comment": "Loading dynamic data",
3813-
"_qNy7WP.comment": "Message to display when XSLT content hasn't been generated yet",
38143816
"_qSejoi.comment": "Label for description of custom lessOrEquals Function",
38153817
"_qSt0Sb.comment": "Accessibility prefix for the input label",
38163818
"_qUWBUX.comment": "A duration of time shown in days",
@@ -4098,7 +4100,7 @@
40984100
"_wQcEXt.comment": "Required parameters for the custom Replace Function",
40994101
"_wQsEwc.comment": "Required length parameter to obtain substring",
41004102
"_wT/gMB.comment": "Description for featured connectors field",
4101-
"_wTaSTp.comment": "Message when XSLT content is not available",
4103+
"_wTaSTp.comment": "Tooltip for disabled test button",
41024104
"_wV3Lmd.comment": "Label to delete dictionary item",
41034105
"_wWVQuK.comment": "Label for description of custom if Function",
41044106
"_wWuzqz.comment": "Ok button text",
@@ -4276,6 +4278,7 @@
42764278
"arjUBV": "The dark image version of the workflow is required for publish.",
42774279
"auUI93": "Add or select a source schema to use for your map.",
42784280
"auci7r": "Enter a valid comma-separated string.",
4281+
"aulGxd": "Please save the map before testing",
42794282
"aurgrg": "Managed identity",
42804283
"b2aL+f": "Pin action",
42814284
"b6G9bq": "URL encodes the input string",
@@ -4920,7 +4923,6 @@
49204923
"qJpnIL": "Checks if the string ends with a value (case-insensitive, invariant culture)",
49214924
"qKVOwV": "Enter a name for the MCP server",
49224925
"qMFpNH": "Loading dynamic data",
4923-
"qNy7WP": "XSLT not yet generated. Save the map to generate XSLT output.",
49244926
"qSejoi": "Returns true if the first argument is less than or equal to the second",
49254927
"qSt0Sb": "Required",
49264928
"qUWBUX": "{days, plural, one {# day} other {# days}}",

apps/vs-code-designer/src/app/commands/dataMapper/DataMapperPanel.ts

Lines changed: 75 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,52 @@ import { copyOverImportedSchemas } from './DataMapperPanelUtils';
4646
import { switchToDataMapperV2 } from '../setDataMapperVersion';
4747
import SaxonJS from 'saxon-js';
4848

49+
/**
50+
* Finds the xslt3 CLI path by checking multiple possible locations.
51+
* The xslt3 package might be installed in the extension's node_modules,
52+
* or hoisted to a parent node_modules directory.
53+
*
54+
* @param extensionPath - The extension's root path
55+
* @returns The path to xslt3.js, or null if not found
56+
*/
57+
const findXslt3Path = (extensionPath: string): string | null => {
58+
const xslt3File = 'xslt3.js';
59+
60+
// Possible locations to check in order of preference
61+
const possiblePaths = [
62+
// Direct dependency in extension's node_modules
63+
path.join(extensionPath, 'node_modules', 'xslt3', xslt3File),
64+
// Hoisted to workspace root node_modules (for development)
65+
path.join(extensionPath, '..', '..', 'node_modules', 'xslt3', xslt3File),
66+
// Try using require.resolve as fallback (handles various node_modules structures)
67+
];
68+
69+
for (const possiblePath of possiblePaths) {
70+
if (fileExistsSync(possiblePath)) {
71+
return possiblePath;
72+
}
73+
}
74+
75+
// Try require.resolve as a final fallback
76+
try {
77+
// require.resolve finds the module regardless of hoisting
78+
const xslt3MainPath = require.resolve('xslt3');
79+
const xslt3Dir = path.dirname(xslt3MainPath);
80+
const xslt3JsPath = path.join(xslt3Dir, xslt3File);
81+
if (fileExistsSync(xslt3JsPath)) {
82+
return xslt3JsPath;
83+
}
84+
// Also check if the main IS xslt3.js
85+
if (xslt3MainPath.endsWith(xslt3File)) {
86+
return xslt3MainPath;
87+
}
88+
} catch {
89+
// require.resolve failed, xslt3 not found
90+
}
91+
92+
return null;
93+
};
94+
4995
export default class DataMapperPanel {
5096
public panel: WebviewPanel;
5197
public dataMapVersion: number;
@@ -242,6 +288,17 @@ export default class DataMapperPanel {
242288
* 4. Return result and clean up temp files
243289
*/
244290
public async handleTestXsltTransform(xsltContent: string, inputXml: string) {
291+
// Validate XSLT size before processing to prevent memory issues
292+
const MAX_XSLT_SIZE = 5 * 1024 * 1024; // 5MB limit
293+
const xsltSize = Buffer.byteLength(xsltContent, 'utf8');
294+
console.log('[DataMapper Test] XSLT size:', xsltSize, 'bytes');
295+
296+
if (xsltSize > MAX_XSLT_SIZE) {
297+
const sizeMB = (xsltSize / (1024 * 1024)).toFixed(2);
298+
const limitMB = (MAX_XSLT_SIZE / (1024 * 1024)).toFixed(0);
299+
throw new Error(`XSLT content is too large (${sizeMB}MB). Maximum size is ${limitMB}MB.`);
300+
}
301+
245302
const tempDir = os.tmpdir();
246303
const uniqueId = Date.now().toString();
247304
const xsltPath = path.join(tempDir, `datamap-${uniqueId}.xslt`);
@@ -256,12 +313,14 @@ export default class DataMapperPanel {
256313
writeFileSync(xsltPath, xsltContent, 'utf8');
257314

258315
// Step 2: Find xslt3 CLI path - use the .js file directly for cross-platform compatibility
259-
const xslt3Path = path.join(ext.context.extensionPath, 'node_modules', 'xslt3', 'xslt3.js');
316+
const xslt3Path = findXslt3Path(ext.context.extensionPath);
260317
console.log('[DataMapper Test] xslt3 path:', xslt3Path);
261-
console.log('[DataMapper Test] xslt3 exists:', fileExistsSync(xslt3Path));
262318

263-
if (!fileExistsSync(xslt3Path)) {
264-
throw new Error(`xslt3 CLI not found at: ${xslt3Path}`);
319+
if (!xslt3Path) {
320+
throw new Error(
321+
'xslt3 CLI not found. Please ensure the xslt3 package is installed. ' +
322+
'Checked locations: extension node_modules, workspace node_modules, and require.resolve paths.'
323+
);
265324
}
266325

267326
// Step 3: Compile XSLT to SEF using xslt3 CLI via node
@@ -341,15 +400,24 @@ export default class DataMapperPanel {
341400
} finally {
342401
// Clean up temp files
343402
try {
403+
let cleanedXslt = false;
404+
let cleanedSef = false;
405+
344406
if (fileExistsSync(xsltPath)) {
345407
removeFileSync(xsltPath);
408+
cleanedXslt = true;
346409
}
347410
if (fileExistsSync(sefPath)) {
348411
removeFileSync(sefPath);
412+
cleanedSef = true;
349413
}
350-
console.log('[DataMapper Test] Cleaned up temp files');
351-
} catch {
352-
// Ignore cleanup errors
414+
console.log('[DataMapper Test] Temp file cleanup:', {
415+
xsltPath: cleanedXslt ? 'deleted' : 'not found',
416+
sefPath: cleanedSef ? 'deleted' : 'not found',
417+
});
418+
} catch (cleanupError) {
419+
// Log cleanup errors for debugging but don't fail the operation
420+
console.warn('[DataMapper Test] Temp file cleanup warning:', cleanupError);
353421
}
354422
}
355423
}

apps/vs-code-designer/src/app/commands/dataMapper/dataMapper.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,16 @@ async function loadFromXsltFile(context: IActionContext, xsltPath: string): Prom
113113

114114
const fileContents = await fs.readFile(fileToLoad, 'utf-8');
115115

116-
// Try to extract embedded metadata
117-
const loadedData = DataMapperExt.loadMapFromXslt(fileContents, ext);
116+
// Try to extract embedded metadata with error handling
117+
let loadedData: ReturnType<typeof DataMapperExt.loadMapFromXslt> = null;
118+
try {
119+
loadedData = DataMapperExt.loadMapFromXslt(fileContents, ext);
120+
} catch (parseError) {
121+
const errorMessage = parseError instanceof Error ? parseError.message : 'Unknown error';
122+
ext.showError(localize('XsltParsingFailed', 'Failed to parse XSLT file: {0}. The file may be corrupted or malformed.', errorMessage));
123+
context.telemetry.properties.xsltParsingError = errorMessage;
124+
return;
125+
}
118126

119127
if (!loadedData) {
120128
// XSLT doesn't have embedded metadata - try to find and migrate from LML

apps/vs-code-react/src/webviewCommunication.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,9 @@ export const WebViewCommunication: React.FC<{ children: ReactNode }> = ({ childr
231231
dispatch(changeXsltContentV2(message.data.xsltContent));
232232
} else {
233233
// v2 format: use embedded mapDefinition directly
234-
dispatch(changeMapDefinitionV2(message.data.mapDefinition));
234+
// Fallback to empty object if mapDefinition is undefined or null
235+
const mapDefinition = message.data.mapDefinition ?? {};
236+
dispatch(changeMapDefinitionV2(mapDefinition));
235237
}
236238

237239
dispatch(changeDataMapFilename(message.data.mapDefinitionName));

libs/data-mapper-v2/src/core/DataMapDataProvider.tsx

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import type { AppDispatch } from './state/Store';
1111
import type { MapDefinitionEntry, DataMapSchema, IFileSysTreeItem, MapMetadataV2 } from '@microsoft/logic-apps-shared';
1212
import { Theme as ThemeType, SchemaType } from '@microsoft/logic-apps-shared';
1313
import type React from 'react';
14-
import { useContext, useEffect, useMemo } from 'react';
14+
import { useContext, useEffect, useMemo, useRef } from 'react';
1515
import { useDispatch } from 'react-redux';
1616
import { MapDefinitionDeserializer } from '../mapHandling/MapDefinitionDeserializer';
1717
import { updateDeserializationMessages } from './state/ErrorsSlice';
@@ -134,29 +134,50 @@ const DataProviderInner = ({
134134
dispatch(changeIsTestDisabledForOS(!!isTestDisabledForOS));
135135
}, [dispatch, isTestDisabledForOS]);
136136

137+
// Track the last processed result to avoid processing stale or duplicate results
138+
const lastProcessedResultRef = useRef<TestXsltTransformResult | null>(null);
139+
137140
// Handle XSLT transform test results from the extension host
138141
useEffect(() => {
139-
if (testXsltTransformResult) {
140-
if (testXsltTransformResult.success) {
141-
dispatch(
142-
updateTestOutput({
143-
response: {
144-
statusCode: testXsltTransformResult.statusCode,
145-
statusText: testXsltTransformResult.statusText,
146-
outputInstance: {
147-
$content: testXsltTransformResult.outputXml ?? '',
148-
'$content-type': 'application/xml',
149-
},
142+
// Skip if no result or if we've already processed this exact result
143+
if (!testXsltTransformResult) {
144+
return;
145+
}
146+
147+
// Check if this is the same result we already processed (prevents duplicate dispatches)
148+
const lastResult = lastProcessedResultRef.current;
149+
if (
150+
lastResult &&
151+
lastResult.success === testXsltTransformResult.success &&
152+
lastResult.outputXml === testXsltTransformResult.outputXml &&
153+
lastResult.error === testXsltTransformResult.error &&
154+
lastResult.statusCode === testXsltTransformResult.statusCode
155+
) {
156+
return; // Skip duplicate result
157+
}
158+
159+
// Store the current result as processed
160+
lastProcessedResultRef.current = testXsltTransformResult;
161+
162+
if (testXsltTransformResult.success) {
163+
dispatch(
164+
updateTestOutput({
165+
response: {
166+
statusCode: testXsltTransformResult.statusCode,
167+
statusText: testXsltTransformResult.statusText,
168+
outputInstance: {
169+
$content: testXsltTransformResult.outputXml ?? '',
170+
'$content-type': 'application/xml',
150171
},
151-
})
152-
);
153-
} else {
154-
dispatch(
155-
updateTestOutput({
156-
error: new Error(testXsltTransformResult.error ?? 'XSLT transformation failed'),
157-
})
158-
);
159-
}
172+
},
173+
})
174+
);
175+
} else {
176+
dispatch(
177+
updateTestOutput({
178+
error: new Error(testXsltTransformResult.error ?? 'XSLT transformation failed'),
179+
})
180+
);
160181
}
161182
}, [dispatch, testXsltTransformResult]);
162183

0 commit comments

Comments
 (0)