diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 24da62364..d7e0a9870 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -5,6 +5,7 @@ ## 1.3.5 - 27 October 2020 - Fix a bug where archived source folders for databases were not showing any contents. +- Fix URI encoding for databases that were created with special characters in their paths. ## 1.3.4 - 22 October 2020 diff --git a/extensions/ql-vscode/src/archive-filesystem-provider.ts b/extensions/ql-vscode/src/archive-filesystem-provider.ts index e1318101b..260ae5f50 100644 --- a/extensions/ql-vscode/src/archive-filesystem-provider.ts +++ b/extensions/ql-vscode/src/archive-filesystem-provider.ts @@ -84,7 +84,7 @@ export function encodeSourceArchiveUri(ref: ZipFileReference): vscode.Uri { // This lets us separate the paths, ignoring the leading slash if we added one. const sourceArchiveZipPathEndIndex = sourceArchiveZipPathStartIndex + sourceArchiveZipPath.length; const authority = `${sourceArchiveZipPathStartIndex}-${sourceArchiveZipPathEndIndex}`; - return vscode.Uri.parse(zipArchiveScheme + ':/').with({ + return vscode.Uri.parse(zipArchiveScheme + ':/', true).with({ path: encodedPath, authority, }); diff --git a/extensions/ql-vscode/src/contextual/fileRangeFromURI.ts b/extensions/ql-vscode/src/contextual/fileRangeFromURI.ts index 797f85e4b..5807e4364 100644 --- a/extensions/ql-vscode/src/contextual/fileRangeFromURI.ts +++ b/extensions/ql-vscode/src/contextual/fileRangeFromURI.ts @@ -20,9 +20,8 @@ export default function fileRangeFromURI(uri: UrlValue | undefined, db: Database Math.max(0, (loc.endLine || 0) - 1), Math.max(0, (loc.endColumn || 0))); try { - const parsed = vscode.Uri.parse(uri.uri, true); - if (parsed.scheme === 'file') { - return new vscode.Location(db.resolveSourceFile(parsed.fsPath), range); + if (uri.uri.startsWith('file:')) { + return new vscode.Location(db.resolveSourceFile(uri.uri), range); } return undefined; } catch (e) { diff --git a/extensions/ql-vscode/src/contextual/locationFinder.ts b/extensions/ql-vscode/src/contextual/locationFinder.ts index 1ebe20f71..a497c51a5 100644 --- a/extensions/ql-vscode/src/contextual/locationFinder.ts +++ b/extensions/ql-vscode/src/contextual/locationFinder.ts @@ -43,7 +43,7 @@ export async function getLocationsForUriString( token: vscode.CancellationToken, filter: (src: string, dest: string) => boolean ): Promise { - const uri = decodeSourceArchiveUri(vscode.Uri.parse(uriString)); + const uri = decodeSourceArchiveUri(vscode.Uri.parse(uriString, true)); const sourceArchiveUri = encodeArchiveBasePath(uri.sourceArchiveZipPath); const db = dbm.findDatabaseItemBySourceArchive(sourceArchiveUri); diff --git a/extensions/ql-vscode/src/contextual/templateProvider.ts b/extensions/ql-vscode/src/contextual/templateProvider.ts index 9e4548ebb..729ecb13c 100644 --- a/extensions/ql-vscode/src/contextual/templateProvider.ts +++ b/extensions/ql-vscode/src/contextual/templateProvider.ts @@ -136,7 +136,7 @@ export class TemplatePrintAstProvider { return new AstBuilder( queryResults, this.cli, - this.dbm.findDatabaseItem(vscode.Uri.parse(queryResults.database.databaseUri!))!, + this.dbm.findDatabaseItem(vscode.Uri.parse(queryResults.database.databaseUri!, true))!, document.fileName ); } diff --git a/extensions/ql-vscode/src/databases.ts b/extensions/ql-vscode/src/databases.ts index 35b285fec..d9e6fbc2a 100644 --- a/extensions/ql-vscode/src/databases.ts +++ b/extensions/ql-vscode/src/databases.ts @@ -341,28 +341,29 @@ export class DatabaseItemImpl implements DatabaseItem { } } - public resolveSourceFile(uri: string | undefined): vscode.Uri { + public resolveSourceFile(uriStr: string | undefined): vscode.Uri { const sourceArchive = this.sourceArchive; - // Sometimes, we are passed a path, sometimes a file URI. - // We need to convert this to a file path that is probably inside of a zip file. - const file = uri?.replace(/file:/, ''); + const uri = uriStr ? vscode.Uri.parse(uriStr, true) : undefined; + if (uri && uri.scheme !== 'file') { + throw new Error(`Invalid uri scheme in ${uriStr}. Only 'file' is allowed.`); + } if (!sourceArchive) { - if (file) { - // Treat it as an absolute path. - return vscode.Uri.file(file); + if (uri) { + return uri; } else { return this.databaseUri; } } - if (file) { - const absoluteFilePath = file.replace(':', '_'); - // Strip any leading slashes from the file path, and replace `:` with `_`. - const relativeFilePath = absoluteFilePath.replace(/^\/*/, '').replace(':', '_'); + if (uri) { + const relativeFilePath = decodeURI(uri.path).replace(':', '_').replace(/^\/*/, ''); if (sourceArchive.scheme === zipArchiveScheme) { const zipRef = decodeSourceArchiveUri(sourceArchive); + const pathWithinSourceArchive = zipRef.pathWithinSourceArchive === '/' + ? relativeFilePath + : zipRef.pathWithinSourceArchive + '/' + relativeFilePath; return encodeSourceArchiveUri({ - pathWithinSourceArchive: zipRef.pathWithinSourceArchive + '/' + absoluteFilePath, + pathWithinSourceArchive, sourceArchiveZipPath: zipRef.sourceArchiveZipPath, }); @@ -579,7 +580,7 @@ export class DatabaseManager extends DisposableObject { displayName, dateAdded }; - const item = new DatabaseItemImpl(vscode.Uri.parse(state.uri), undefined, fullOptions, + const item = new DatabaseItemImpl(vscode.Uri.parse(state.uri, true), undefined, fullOptions, (event) => { this._onDidChangeDatabaseItem.fire(event); }); diff --git a/extensions/ql-vscode/src/pure/bqrs-cli-types.ts b/extensions/ql-vscode/src/pure/bqrs-cli-types.ts index 024a9c53d..333392f48 100644 --- a/extensions/ql-vscode/src/pure/bqrs-cli-types.ts +++ b/extensions/ql-vscode/src/pure/bqrs-cli-types.ts @@ -77,10 +77,10 @@ export interface WholeFileLocation { endColumn: never; } -export type UrlValue = LineColumnLocation | WholeFileLocation | string; - export type ResolvableLocationValue = WholeFileLocation | LineColumnLocation; +export type UrlValue = ResolvableLocationValue | string; + export type ColumnValue = EntityValue | number | string | boolean; export type ResultRow = ColumnValue[]; diff --git a/extensions/ql-vscode/src/pure/sarif-utils.ts b/extensions/ql-vscode/src/pure/sarif-utils.ts index 81ec1dce8..e4b651ea2 100644 --- a/extensions/ql-vscode/src/pure/sarif-utils.ts +++ b/extensions/ql-vscode/src/pure/sarif-utils.ts @@ -72,10 +72,28 @@ export function getPathRelativeToSourceLocationPrefix( sourceLocationPrefix: string, sarifRelativeUri: string ) { - const normalizedSourceLocationPrefix = sourceLocationPrefix.replace(/\\/g, '/'); - return `file:${normalizedSourceLocationPrefix}/${sarifRelativeUri}`; + // convert a platform specific path into encoded path uri segments + // need to be careful about drive letters and ensure that there + // is a starting '/' + let prefix = ''; + if (sourceLocationPrefix[1] === ':') { + // assume this is a windows drive separator + prefix = sourceLocationPrefix.substring(0, 2); + sourceLocationPrefix = sourceLocationPrefix.substring(2); + } + const normalizedSourceLocationPrefix = prefix + sourceLocationPrefix.replace(/\\/g, '/') + .split('/') + .map(encodeURIComponent) + .join('/'); + const slashPrefix = normalizedSourceLocationPrefix.startsWith('/') ? '' : '/'; + return `file:${slashPrefix + normalizedSourceLocationPrefix}/${sarifRelativeUri}`; } +/** + * + * @param loc specifies the database-relative location of the source location + * @param sourceLocationPrefix a file path (usually a full path) to the database containing the source location. + */ export function parseSarifLocation( loc: Sarif.Location, sourceLocationPrefix: string diff --git a/extensions/ql-vscode/src/query-history.ts b/extensions/ql-vscode/src/query-history.ts index 256a14007..803a637de 100644 --- a/extensions/ql-vscode/src/query-history.ts +++ b/extensions/ql-vscode/src/query-history.ts @@ -453,7 +453,7 @@ export class QueryHistoryManager extends DisposableObject { queryText: encodeURIComponent(await this.getQueryText(singleItem)), }); const uri = vscode.Uri.parse( - `codeql:${singleItem.query.queryID}-${queryName}?${params.toString()}` + `codeql:${singleItem.query.queryID}-${queryName}?${params.toString()}`, true ); const doc = await vscode.workspace.openTextDocument(uri); await vscode.window.showTextDocument(doc, { preview: false }); diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/fileRangeFromURI.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/fileRangeFromURI.test.ts index 129b215b0..7cdfaf9fd 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/fileRangeFromURI.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/fileRangeFromURI.test.ts @@ -8,10 +8,30 @@ import { DatabaseItem } from '../../../databases'; import { WholeFileLocation, LineColumnLocation } from '../../../pure/bqrs-cli-types'; describe('fileRangeFromURI', () => { - it('should return undefined when value is a string', () => { + it('should return undefined when value is not a file URI', () => { expect(fileRangeFromURI('hucairz', createMockDatabaseItem())).to.be.undefined; }); + it('should fail to find a location when not a file URI and a full 5 part location', () => { + expect(fileRangeFromURI({ + uri: 'https://yahoo.com', + startLine: 1, + startColumn: 2, + endLine: 3, + endColumn: 4, + } as LineColumnLocation, createMockDatabaseItem())).to.be.undefined; + }); + + it('should fail to find a location when there is a silly protocol', () => { + expect(fileRangeFromURI({ + uri: 'filesilly://yahoo.com', + startLine: 1, + startColumn: 2, + endLine: 3, + endColumn: 4, + } as LineColumnLocation, createMockDatabaseItem())).to.be.undefined; + }); + it('should return undefined when value is an empty uri', () => { expect(fileRangeFromURI({ uri: 'file:/', @@ -46,7 +66,7 @@ describe('fileRangeFromURI', () => { function createMockDatabaseItem(): DatabaseItem { return { - resolveSourceFile: (file: string) => Uri.file(file) + resolveSourceFile: (file: string) => Uri.parse(file) } as DatabaseItem; } }); diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts index 913ccadfa..127d80674 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts @@ -87,39 +87,31 @@ describe('databases', () => { }); describe('resolveSourceFile', () => { - describe('unzipped source archive', () => { - it('should resolve a source file in an unzipped database', () => { - const db = createMockDB(); - const resolved = db.resolveSourceFile('abc'); - expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc'); - }); + it('should fail to resolve when not a uri', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + expect(() => db.resolveSourceFile('abc')).to.throw('Scheme is missing'); + }); - it('should resolve a source file in an unzipped database with trailing slash', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - const resolved = db.resolveSourceFile('abc'); - expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc'); - }); - - it('should resolve a source uri in an unzipped database with trailing slash', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - const resolved = db.resolveSourceFile('file:/abc'); - expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc'); - }); + it('should fail to resolve when not a file uri', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + expect(() => db.resolveSourceFile('http://abc')).to.throw('Invalid uri scheme'); }); describe('no source archive', () => { - it('should resolve a file', () => { + it('should resolve undefined', () => { const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); (db as any)._contents.sourceArchiveUri = undefined; - const resolved = db.resolveSourceFile('abc'); - expect(resolved.toString()).to.eq('file:///abc'); + const resolved = db.resolveSourceFile(undefined); + expect(resolved.toString()).to.eq('file:///database-uri'); }); it('should resolve an empty file', () => { const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); (db as any)._contents.sourceArchiveUri = undefined; const resolved = db.resolveSourceFile('file:'); - expect(resolved.toString()).to.eq('file:///database-uri'); + expect(resolved.toString()).to.eq('file:///'); }); }); @@ -160,7 +152,7 @@ describe('databases', () => { pathWithinSourceArchive: 'def' })); const resolved = db.resolveSourceFile('file:'); - expect(resolved.toString()).to.eq('codeql-zip-archive://1-18/sourceArchive-uri/def'); + expect(resolved.toString()).to.eq('codeql-zip-archive://1-18/sourceArchive-uri/def/'); }); }); diff --git a/extensions/ql-vscode/test/pure-tests/sarif-utils.test.ts b/extensions/ql-vscode/test/pure-tests/sarif-utils.test.ts index dc71c8384..6976217a7 100644 --- a/extensions/ql-vscode/test/pure-tests/sarif-utils.test.ts +++ b/extensions/ql-vscode/test/pure-tests/sarif-utils.test.ts @@ -51,20 +51,24 @@ describe('parsing sarif', () => { it('should normalize source locations', () => { expect(getPathRelativeToSourceLocationPrefix('C:\\a\\b', '?x=test')) - .to.eq('file:C:/a/b/?x=test'); + .to.eq('file:/C:/a/b/?x=test'); expect(getPathRelativeToSourceLocationPrefix('C:\\a\\b', '%3Fx%3Dtest')) - .to.eq('file:C:/a/b/%3Fx%3Dtest'); + .to.eq('file:/C:/a/b/%3Fx%3Dtest'); + expect(getPathRelativeToSourceLocationPrefix('C:\\a =\\b c?', '?x=test')) + .to.eq('file:/C:/a%20%3D/b%20c%3F/?x=test'); + expect(getPathRelativeToSourceLocationPrefix('/a/b/c', '?x=test')) + .to.eq('file:/a/b/c/?x=test'); }); describe('parseSarifLocation', () => { it('should parse a sarif location with "no location"', () => { - expect(parseSarifLocation({ }, '')).to.deep.equal({ + expect(parseSarifLocation({}, '')).to.deep.equal({ hint: 'no physical location' }); expect(parseSarifLocation({ physicalLocation: {} }, '')).to.deep.equal({ hint: 'no artifact location' }); - expect(parseSarifLocation({ physicalLocation: { artifactLocation: { } } }, '')).to.deep.equal({ + expect(parseSarifLocation({ physicalLocation: { artifactLocation: {} } }, '')).to.deep.equal({ hint: 'artifact location has no uri' }); }); @@ -78,7 +82,7 @@ describe('parsing sarif', () => { } }; expect(parseSarifLocation(location, 'prefix')).to.deep.equal({ - uri: 'file:prefix/abc?x=test', + uri: 'file:/prefix/abc?x=test', userVisibleFile: 'abc?x=test' }); }); @@ -87,13 +91,13 @@ describe('parsing sarif', () => { const location: Sarif.Location = { physicalLocation: { artifactLocation: { - uri: 'file:abc%3Fx%3Dtest' + uri: 'file:/abc%3Fx%3Dtest' } } }; expect(parseSarifLocation(location, 'prefix')).to.deep.equal({ - uri: 'file:abc%3Fx%3Dtest', - userVisibleFile: 'abc?x=test' + uri: 'file:/abc%3Fx%3Dtest', + userVisibleFile: '/abc?x=test' }); });