Escape path names during import/export. Skip redundant chdirs (#5770)

* Fix security issues. Change to using less instead of sass (as it has a tar dependency)

* Fix import/export paths and remove redundant chdirs.

Author: rchiodo

news/2 Fixes/5386.md

@@ -0,0 +1 @@
+Fix import/export paths to be escaped on windows.

src/client/datascience/constants.ts

@@ -122,7 +122,8 @@ export namespace Settings {
 }
 
 export namespace CodeSnippits {
-    export const ChangeDirectory = ['{0}', 'import os', 'try:', '\tos.chdir(os.path.join(os.getcwd(), \'{1}\'))', '\tprint(os.getcwd())', 'except:', '\tpass', ''];
+    export const ChangeDirectory = ['{0}', '{1}', 'import os', 'try:', '\tos.chdir(os.path.join(os.getcwd(), \'{2}\'))', '\tprint(os.getcwd())', 'except:', '\tpass', ''];
+    export const ChangeDirectoryCommentIdentifier = '# ms-python.python added'; // Not translated so can compare.
 }
 
 export namespace Identifiers {

src/client/datascience/jupyter/jupyterExporter.ts

@@ -8,11 +8,12 @@ import * as path from 'path';
 import * as uuid from 'uuid/v4';
 
 import { IWorkspaceService } from '../../common/application/types';
-import { IFileSystem } from '../../common/platform/types';
+import { IFileSystem, IPlatformService } from '../../common/platform/types';
 import { IConfigurationService, ILogger } from '../../common/types';
 import * as localize from '../../common/utils/localize';
 import { noop } from '../../common/utils/misc';
 import { CellMatcher } from '../cellMatcher';
+import { concatMultilineString } from '../common';
 import { CodeSnippits, Identifiers } from '../constants';
 import { CellState, ICell, IJupyterExecution, INotebookExporter, ISysInfo } from '../types';
 
@@ -24,7 +25,9 @@ export class JupyterExporter implements INotebookExporter {
         @inject(ILogger) private logger: ILogger,
         @inject(IWorkspaceService) private workspaceService: IWorkspaceService,
         @inject(IConfigurationService) private configService: IConfigurationService,
-        @inject(IFileSystem) private fileSystem: IFileSystem) {
+        @inject(IFileSystem) private fileSystem: IFileSystem,
+        @inject(IPlatformService) private readonly platform: IPlatformService
+        ) {
     }
 
     public dispose() {
@@ -75,7 +78,7 @@ export class JupyterExporter implements INotebookExporter {
         const changeDirectory = await this.calculateDirectoryChange(file, cells);
 
         if (changeDirectory) {
-            const exportChangeDirectory = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.exportChangeDirectoryComment(), changeDirectory);
+            const exportChangeDirectory = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.exportChangeDirectoryComment(), CodeSnippits.ChangeDirectoryCommentIdentifier, changeDirectory);
 
             const cell: ICell = {
                 data: {
@@ -117,21 +120,30 @@ export class JupyterExporter implements INotebookExporter {
     }
 
     private calculateDirectoryChange = async (notebookFile: string, cells: ICell[]): Promise<string | undefined> => {
+        // Make sure we don't already have a cell with a ChangeDirectory comment in it.
         let directoryChange: string | undefined;
-        const notebookFilePath = path.dirname(notebookFile);
-        // First see if we have a workspace open, this only works if we have a workspace root to be relative to
-        if (this.workspaceService.hasWorkspaceFolders) {
-            const workspacePath = await this.firstWorkspaceFolder(cells);
-
-            // Make sure that we have everything that we need here
-            if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
-                directoryChange = path.relative(notebookFilePath, workspacePath);
+        const haveChangeAlready = cells.find(c => concatMultilineString(c.data.source).includes(CodeSnippits.ChangeDirectoryCommentIdentifier));
+        if (!haveChangeAlready) {
+            const notebookFilePath = path.dirname(notebookFile);
+            // First see if we have a workspace open, this only works if we have a workspace root to be relative to
+            if (this.workspaceService.hasWorkspaceFolders) {
+                const workspacePath = await this.firstWorkspaceFolder(cells);
+
+                // Make sure that we have everything that we need here
+                if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
+                    directoryChange = path.relative(notebookFilePath, workspacePath);
+                }
             }
         }
 
         // If path.relative can't calculate a relative path, then it just returns the full second path
         // so check here, we only want this if we were able to calculate a relative path, no network shares or drives
         if (directoryChange && !path.isAbsolute(directoryChange)) {
+            // Escape windows path chars so they end up in the source escaped
+            if (this.platform.isWindows) {
+                directoryChange = directoryChange.replace('\\', '\\\\');
+            }
+
             return directoryChange;
         } else {
             return undefined;

src/client/datascience/jupyter/jupyterImporter.ts

@@ -7,7 +7,7 @@ import * as os from 'os';
 import * as path from 'path';
 
 import { IWorkspaceService } from '../../common/application/types';
-import { IFileSystem } from '../../common/platform/types';
+import { IFileSystem, IPlatformService } from '../../common/platform/types';
 import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import * as localize from '../../common/utils/localize';
 import { noop } from '../../common/utils/misc';
@@ -38,7 +38,9 @@ export class JupyterImporter implements INotebookImporter {
         @inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry,
         @inject(IConfigurationService) private configuration: IConfigurationService,
         @inject(IJupyterExecution) private jupyterExecution: IJupyterExecution,
-        @inject(IWorkspaceService) private workspaceService: IWorkspaceService) {
+        @inject(IWorkspaceService) private workspaceService: IWorkspaceService,
+        @inject(IPlatformService) private readonly platform: IPlatformService
+        ) {
         this.templatePromise = this.createTemplateFile();
     }
 
@@ -49,7 +51,7 @@ export class JupyterImporter implements INotebookImporter {
         const settings = this.configuration.getSettings();
         let directoryChange: string | undefined;
         if (settings.datascience.changeDirOnImportExport) {
-            directoryChange = this.calculateDirectoryChange(file);
+            directoryChange = await this.calculateDirectoryChange(file);
         }
 
         // Use the jupyter nbconvert functionality to turn the notebook into a python file
@@ -70,27 +72,39 @@ export class JupyterImporter implements INotebookImporter {
     }
 
     private addDirectoryChange = (pythonOutput: string, directoryChange: string): string => {
-        const newCode = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.importChangeDirectoryComment(), directoryChange);
+        const newCode = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.importChangeDirectoryComment(), CodeSnippits.ChangeDirectoryCommentIdentifier, directoryChange);
         return newCode.concat(pythonOutput);
     }
 
     // When importing a file, calculate if we can create a %cd so that the relative paths work
-    private calculateDirectoryChange = (notebookFile: string): string | undefined => {
+    private async calculateDirectoryChange(notebookFile: string): Promise<string | undefined> {
         let directoryChange: string | undefined;
-        const notebookFilePath = path.dirname(notebookFile);
-        // First see if we have a workspace open, this only works if we have a workspace root to be relative to
-        if (this.workspaceService.hasWorkspaceFolders) {
-            const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath;
-
-            // Make sure that we have everything that we need here
-            if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
-                directoryChange = path.relative(workspacePath, notebookFilePath);
+        // Make sure we don't already have an import/export comment in the file
+        const contents = await this.fileSystem.readFile(notebookFile);
+        const haveChangeAlready = contents.includes(CodeSnippits.ChangeDirectoryCommentIdentifier);
+
+        if (!haveChangeAlready) {
+            const notebookFilePath = path.dirname(notebookFile);
+            // First see if we have a workspace open, this only works if we have a workspace root to be relative to
+            if (this.workspaceService.hasWorkspaceFolders) {
+                const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath;
+
+                // Make sure that we have everything that we need here
+                if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
+                    directoryChange = path.relative(workspacePath, notebookFilePath);
+                }
             }
         }
 
         // If path.relative can't calculate a relative path, then it just returns the full second path
         // so check here, we only want this if we were able to calculate a relative path, no network shares or drives
         if (directoryChange && !path.isAbsolute(directoryChange)) {
+
+            // Escape windows path chars so they end up in the source escaped
+            if (this.platform.isWindows) {
+                directoryChange = directoryChange.replace('\\', '\\\\');
+            }
+
             return directoryChange;
         } else {
             return undefined;

src/test/datascience/notebook.functional.test.ts

@@ -354,8 +354,11 @@ suite('DataScience notebook tests', () => {
             // Try importing this. This should verify export works and that importing is possible
             const results = await importer.importFromFile(temp.filePath);
 
-            // Make sure we added a chdir into our results
-            assert.ok(results.includes('os.chdir'));
+            // Make sure we have a single chdir in our results
+            const first = results.indexOf('os.chdir');
+            assert.ok(first >= 0, 'No os.chdir in import');
+            const second = results.indexOf('os.chdir', first + 1);
+            assert.equal(second, -1, 'More than one chdir in the import. It should be skipped');
 
             // Make sure we have a cell in our results
             assert.ok(/#\s*%%/.test(results), 'No cells in returned import');