From 0246eec4ed8e63a2203b83137feb80fa7ba3711e Mon Sep 17 00:00:00 2001 From: Kim Santiago <31145923+kisantia@users.noreply.github.com> Date: Mon, 22 Jul 2019 12:36:46 -0700 Subject: [PATCH] Fix #4461 Dacpac: Extract lets me save to invalid file path like 'sd' and never adds .dacpac / .bacpac (#6424) * check for valid path * add unit tests * bump package version * addressing comments --- extensions/dacpac/package.json | 2 +- extensions/dacpac/src/test/dacpac.test.ts | 72 ++++++++++++++++++- .../dacpac/src/wizard/api/dacFxConfigPage.ts | 19 +++-- extensions/dacpac/src/wizard/api/utils.ts | 62 ++++++++++++++-- .../src/wizard/dataTierApplicationWizard.ts | 23 +++--- .../src/wizard/pages/exportConfigPage.ts | 6 ++ .../src/wizard/pages/extractConfigPage.ts | 5 ++ 7 files changed, 160 insertions(+), 29 deletions(-) diff --git a/extensions/dacpac/package.json b/extensions/dacpac/package.json index 4f6374ddd6..6248dd245a 100644 --- a/extensions/dacpac/package.json +++ b/extensions/dacpac/package.json @@ -2,7 +2,7 @@ "name": "dacpac", "displayName": "SQL Server Dacpac", "description": "SQL Server Dacpac for Azure Data Studio.", - "version": "0.4.0", + "version": "0.5.0", "publisher": "Microsoft", "preview": true, "engines": { diff --git a/extensions/dacpac/src/test/dacpac.test.ts b/extensions/dacpac/src/test/dacpac.test.ts index d6d25eadfe..ef4a43dd08 100644 --- a/extensions/dacpac/src/test/dacpac.test.ts +++ b/extensions/dacpac/src/test/dacpac.test.ts @@ -7,8 +7,10 @@ import 'mocha'; import * as should from 'should'; import * as os from 'os'; -import { isValidFilenameCharacter, sanitizeStringForFilename } from '../wizard/api/utils'; +import * as path from 'path'; +import { isValidFilenameCharacter, sanitizeStringForFilename, isValidBasename } from '../wizard/api/utils'; +const isWindows = os.platform() === 'win32'; describe('Sanitize database name for filename tests', function (): void { it('Should only validate if one character is passed', async () => { @@ -25,8 +27,6 @@ describe('Sanitize database name for filename tests', function (): void { }); it('Should determine invalid Windows file name characters', async () => { - let isWindows = os.platform() === 'win32'; - // invalid only for Windows should(isValidFilenameCharacter('?')).equal(isWindows ? false : true); should(isValidFilenameCharacter(':')).equal(isWindows ? false : true); @@ -45,3 +45,69 @@ describe('Sanitize database name for filename tests', function (): void { should(sanitizeStringForFilename(invalidDbName)).equal(isWindows ? expectedWindows : expectedNonWindows); }); }); + +describe('Check for invalid filename tests', function (): void { + it('Should determine invalid filenames', async () => { + // valid filename + should(isValidBasename(formatFileName('ValidName.dacpac'))).equal(true); + + // invalid for both Windows and non-Windows + should(isValidBasename(formatFileName(' .dacpac'))).equal(false); + should(isValidBasename(formatFileName(' .dacpac'))).equal(false); + should(isValidBasename(formatFileName(' .dacpac'))).equal(false); + should(isValidBasename(formatFileName('..dacpac'))).equal(false); + should(isValidBasename(formatFileName('...dacpac'))).equal(false); + should(isValidBasename(null)).equal(false); + should(isValidBasename(undefined)).equal(false); + should(isValidBasename('\\')).equal(false); + should(isValidBasename('/')).equal(false); + + // most file systems do not allow files > 255 length + should(isValidBasename(formatFileName('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.dacpac'))).equal(false); + }); + + it('Should determine invalid Windows filenames', async () => { + // invalid characters only for Windows + should(isValidBasename(formatFileName('?.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName(':.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('*.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('<.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('>.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('|.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('".dacpac'))).equal(isWindows ? false : true); + + // Windows filenames cannot end with a whitespace + should(isValidBasename(formatFileName('test .dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('test .dacpac'))).equal(isWindows ? false : true); + }); + + it('Should determine Windows forbidden filenames', async () => { + // invalid only for Windows + should(isValidBasename(formatFileName('CON.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('PRN.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('AUX.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('NUL.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM1.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM2.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM3.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM4.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM5.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM6.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM7.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM8.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('COM9.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT1.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT2.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT3.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT4.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT5.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT6.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT7.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT8.dacpac'))).equal(isWindows ? false : true); + should(isValidBasename(formatFileName('LPT9.dacpac'))).equal(isWindows ? false : true); + }); +}); + +function formatFileName(filename: string): string { + return path.join(os.tmpdir(), filename); +} \ No newline at end of file diff --git a/extensions/dacpac/src/wizard/api/dacFxConfigPage.ts b/extensions/dacpac/src/wizard/api/dacFxConfigPage.ts index cfc9bb7df5..962e680a83 100644 --- a/extensions/dacpac/src/wizard/api/dacFxConfigPage.ts +++ b/extensions/dacpac/src/wizard/api/dacFxConfigPage.ts @@ -12,7 +12,7 @@ import * as path from 'path'; import { DataTierApplicationWizard } from '../dataTierApplicationWizard'; import { DacFxDataModel } from './models'; import { BasePage } from './basePage'; -import { sanitizeStringForFilename } from './utils'; +import { sanitizeStringForFilename, isValidBasename } from './utils'; const localize = nls.loadMessageBundle(); @@ -142,9 +142,12 @@ export abstract class DacFxConfigPage extends BasePage { } protected async createFileBrowserParts() { - this.fileTextBox = this.view.modelBuilder.inputBox().withProperties({ - required: true - }).component(); + this.fileTextBox = this.view.modelBuilder.inputBox().withValidation( + component => isValidBasename(component.value) + ) + .withProperties({ + required: true + }).component(); this.fileButton = this.view.modelBuilder.button().withProperties({ label: '•••', @@ -164,6 +167,14 @@ export abstract class DacFxConfigPage extends BasePage { // return rootpath of opened folder in file explorer if one is open, otherwise default to user home directory return vscode.workspace.rootPath ? vscode.workspace.rootPath : os.homedir(); } + + protected appendFileExtensionIfNeeded() { + // make sure filepath ends in proper file extension if it's a valid name + if (!this.model.filePath.endsWith(this.fileExtension) && isValidBasename(this.model.filePath)) { + this.model.filePath += this.fileExtension; + this.fileTextBox.value = this.model.filePath; + } + } } interface ConnectionDropdownValue extends azdata.CategoryValue { diff --git a/extensions/dacpac/src/wizard/api/utils.ts b/extensions/dacpac/src/wizard/api/utils.ts index 93d8f63ba5..525bf174d9 100644 --- a/extensions/dacpac/src/wizard/api/utils.ts +++ b/extensions/dacpac/src/wizard/api/utils.ts @@ -3,8 +3,12 @@ * Licensed under the Source EULA. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import * as os from 'os'; -const INVALID_FILE_CHARS_Windows = /[\\/:\*\?"<>\|]/g; -const INVALID_FILE_CHARS = /[\\/]/g; +import * as path from 'path'; + +const WINDOWS_INVALID_FILE_CHARS = /[\\/:\*\?"<>\|]/g; +const UNIX_INVALID_FILE_CHARS = /[\\/]/g; +const isWindows = os.platform() === 'win32'; +const WINDOWS_FORBIDDEN_NAMES = /^(con|prn|aux|clock\$|nul|lpt[0-9]|com[0-9])$/i; /** * Determines if a given character is a valid filename character @@ -16,18 +20,17 @@ export function isValidFilenameCharacter(c: string): boolean { return false; } let isWindows = os.platform() === 'win32'; - INVALID_FILE_CHARS_Windows.lastIndex = 0; - INVALID_FILE_CHARS.lastIndex = 0; - if (isWindows && INVALID_FILE_CHARS_Windows.test(c)) { + WINDOWS_INVALID_FILE_CHARS.lastIndex = 0; + UNIX_INVALID_FILE_CHARS.lastIndex = 0; + if (isWindows && WINDOWS_INVALID_FILE_CHARS.test(c)) { return false; - } else if (!isWindows && INVALID_FILE_CHARS.test(c)) { + } else if (!isWindows && UNIX_INVALID_FILE_CHARS.test(c)) { return false; } return true; } - /** * Replaces invalid filename characters in a string with underscores * @param s The string to be sanitized for a filename @@ -40,4 +43,49 @@ export function sanitizeStringForFilename(s: string): string { } return result; +} + +/** + * Returns true if the string is a valid filename + * Logic is copied from src\vs\base\common\extpath.ts + * @param name filename to check + */ +export function isValidBasename(name: string | null | undefined): boolean { + const invalidFileChars = isWindows ? WINDOWS_INVALID_FILE_CHARS : UNIX_INVALID_FILE_CHARS; + + if (!name) { + return false; + } + + if (isWindows && name[name.length - 1] === '.') { + return false; // Windows: file cannot end with a "." + } + + let basename = path.parse(name).name; + if (!basename || basename.length === 0 || /^\s+$/.test(basename)) { + return false; // require a name that is not just whitespace + } + + invalidFileChars.lastIndex = 0; + if (invalidFileChars.test(basename)) { + return false; // check for certain invalid file characters + } + + if (isWindows && WINDOWS_FORBIDDEN_NAMES.test(basename)) { + return false; // check for certain invalid file names + } + + if (basename === '.' || basename === '..') { + return false; // check for reserved values + } + + if (isWindows && basename.length !== basename.trim().length) { + return false; // Windows: file cannot end with a whitespace + } + + if (basename.length > 255) { + return false; // most file systems do not allow files > 255 length + } + + return true; } \ No newline at end of file diff --git a/extensions/dacpac/src/wizard/dataTierApplicationWizard.ts b/extensions/dacpac/src/wizard/dataTierApplicationWizard.ts index 1a31fdbbbc..cd888f8fae 100644 --- a/extensions/dacpac/src/wizard/dataTierApplicationWizard.ts +++ b/extensions/dacpac/src/wizard/dataTierApplicationWizard.ts @@ -161,22 +161,17 @@ export class DataTierApplicationWizard { }); this.wizard.onPageChanged(async (event) => { - let idx = event.newPage; - let page = this.getPage(idx); - - if (page !== undefined) { - page.dacFxPage.setupNavigationValidator(); - page.dacFxPage.onPageEnter(); + let idxLast = event.lastPage; + let lastPage = this.getPage(idxLast); + if (lastPage) { + lastPage.dacFxPage.onPageLeave(); } - //do onPageLeave for summary page so that GenerateScript button only shows up if upgrading database - let idxLast = event.lastPage; - - if (this.isSummaryPage(idxLast)) { - let lastPage = this.pages.get('summary'); - if (lastPage) { - lastPage.dacFxPage.onPageLeave(); - } + let idx = event.newPage; + let page = this.getPage(idx); + if (page) { + page.dacFxPage.setupNavigationValidator(); + page.dacFxPage.onPageEnter(); } }); diff --git a/extensions/dacpac/src/wizard/pages/exportConfigPage.ts b/extensions/dacpac/src/wizard/pages/exportConfigPage.ts index 5f7b574652..c2b18dd039 100644 --- a/extensions/dacpac/src/wizard/pages/exportConfigPage.ts +++ b/extensions/dacpac/src/wizard/pages/exportConfigPage.ts @@ -52,6 +52,12 @@ export class ExportConfigPage extends DacFxConfigPage { return r1 && r2; } + + async onPageLeave(): Promise { + this.appendFileExtensionIfNeeded(); + return true; + } + public setupNavigationValidator() { this.instance.registerNavigationValidator(() => { if (this.databaseLoader.loading) { diff --git a/extensions/dacpac/src/wizard/pages/extractConfigPage.ts b/extensions/dacpac/src/wizard/pages/extractConfigPage.ts index 790fbb6995..f1b392a05b 100644 --- a/extensions/dacpac/src/wizard/pages/extractConfigPage.ts +++ b/extensions/dacpac/src/wizard/pages/extractConfigPage.ts @@ -55,6 +55,11 @@ export class ExtractConfigPage extends DacFxConfigPage { return r1 && r2; } + async onPageLeave(): Promise { + this.appendFileExtensionIfNeeded(); + return true; + } + public setupNavigationValidator() { this.instance.registerNavigationValidator(() => { if (this.databaseLoader.loading) {