Ensure uris are using encoded strings (#653)

This fixes a bug where if there are special characters in a database
path, it is not possible to navigate to that file from the results view.

Note that the results from our BQRS returned properly encoded URIs, but
our paths coming from sarif were unencoded. Our path parsing handled
the latter correctly (even though these are not correct URIs) and the
former incorrectly.

The fix here is to first ensure all uris are properly encoded. We do
this by running `encodeURI` in sarif-utils (can't run encodeURIComponent
or else the path separators `/` will also be encoded).

Then, we ensure that when we resolve locations, we decode all file
paths.

This works in all cases I have tried. I still have an issue with running
View AST on some of these databases, but that I believe is a separate
issue.

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
 

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,
   });

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 (uri.uri.startsWith('file:')) {
-      if (parsed.scheme === 'file') {
+        return new vscode.Location(db.resolveSourceFile(uri.uri), range);
-        return new vscode.Location(db.resolveSourceFile(parsed.fsPath), range);
       }
       return undefined;
     } catch (e) {

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<FullLocationLink[]> {
-  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);

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
     );
   }

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.
+    const uri = uriStr ? vscode.Uri.parse(uriStr, true) : undefined;
-    // We need to convert this to a file path that is probably inside of a zip file.
+    if (uri && uri.scheme !== 'file') {
-    const file = uri?.replace(/file:/, '');
+      throw new Error(`Invalid uri scheme in ${uriStr}. Only 'file' is allowed.`);
+    }
     if (!sourceArchive) {
-      if (file) {
+      if (uri) {
-        // Treat it as an absolute path.
+        return uri;
-        return vscode.Uri.file(file);
       } else {
         return this.databaseUri;
       }
     }
 
-    if (file) {
+    if (uri) {
-      const absoluteFilePath = file.replace(':', '_');
+      const relativeFilePath = decodeURI(uri.path).replace(':', '_').replace(/^\/*/, '');
-      // Strip any leading slashes from the file path, and replace `:` with `_`.
-      const relativeFilePath = absoluteFilePath.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);
       });

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[];

extensions/ql-vscode/src/pure/sarif-utils.ts

@@ -72,10 +72,28 @@ export function getPathRelativeToSourceLocationPrefix(
   sourceLocationPrefix: string,
   sarifRelativeUri: string
 ) {
-  const normalizedSourceLocationPrefix = sourceLocationPrefix.replace(/\\/g, '/');
+  // convert a platform specific path into encoded path uri segments
-  return `file:${normalizedSourceLocationPrefix}/${sarifRelativeUri}`;
+  // 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

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 });

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;
   }
 });

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 fail to resolve when not a uri', () => {
-      it('should resolve a source file in an unzipped database', () => {
+      const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
-        const db = createMockDB();
+      (db as any)._contents.sourceArchiveUri = undefined;
-        const resolved = db.resolveSourceFile('abc');
+      expect(() => db.resolveSourceFile('abc')).to.throw('Scheme is missing');
-        expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc');
+    });
-      });
 
-      it('should resolve a source file in an unzipped database with trailing slash', () => {
+    it('should fail to resolve when not a file uri', () => {
-        const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+      const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
-        const resolved = db.resolveSourceFile('abc');
+      (db as any)._contents.sourceArchiveUri = undefined;
-        expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc');
+      expect(() => db.resolveSourceFile('http://abc')).to.throw('Invalid uri scheme');
-      });
-
-      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');
-      });
     });
 
     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');
+        const resolved = db.resolveSourceFile(undefined);
-        expect(resolved.toString()).to.eq('file:///abc');
+        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/');
       });
     });
 

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',
+        uri: 'file:/abc%3Fx%3Dtest',
-        userVisibleFile: 'abc?x=test'
+        userVisibleFile: '/abc?x=test'
       });
     });