Warn people to not use id containing '+' for AzExtTreeFileSystem items (

#1588) * Fix tests. Add most important info to interface * Fix lint * Fix test
microsoft · Sep 19, 2023 · 89cc483 · 89cc483
1 parent 288cc05
commit 89cc483
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 3 deletions.
diff --git a/utils/index.d.ts b/utils/index.d.ts
@@ -1450,6 +1450,9 @@ export type AzExtItemUriParts = {
 };
 
 export interface AzExtTreeFileSystemItem {
+    /**
+     * Warning: the identifier cannot contain plus sign '+'. No matter if it's exactly '+' or if it's URL encoded "%2B".
+     */
     id: string;
     refresh?(context: IActionContext): Promise<void>;
 }

diff --git a/utils/src/AzExtTreeFileSystem.ts b/utils/src/AzExtTreeFileSystem.ts
@@ -3,8 +3,8 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { parse as parseQuery, ParsedUrlQuery, stringify as stringifyQuery } from "querystring";
-import { Disposable, Event, EventEmitter, FileChangeEvent, FileStat, FileSystemError, FileSystemProvider, FileType, l10n, TextDocumentShowOptions, Uri, window } from "vscode";
+import { ParsedUrlQuery, parse as parseQuery, stringify as stringifyQuery } from "querystring";
+import { Disposable, Event, EventEmitter, FileChangeEvent, FileStat, FileSystemError, FileSystemProvider, FileType, TextDocumentShowOptions, Uri, l10n, window } from "vscode";
 import * as types from '../index';
 import { callWithTelemetryAndErrorHandling } from "./callWithTelemetryAndErrorHandling";
 import { nonNullProp } from "./utils/nonNull";
@@ -127,7 +127,7 @@ export abstract class AzExtTreeFileSystem<TItem extends types.AzExtTreeFileSyste
         return Uri.parse(`${this.scheme}:///${filePath}?${query}`);
     }
 
-    private async lookup(context: types.IActionContext, uri: Uri): Promise<TItem> {
+    protected async lookup(context: types.IActionContext, uri: Uri): Promise<TItem> {
         const item: TItem | undefined = this.findItem(this.getQueryFromUri(uri));
         if (!item) {
             context.telemetry.suppressAll = true;

diff --git a/utils/test/AzExtTreeFileSystem.test.ts b/utils/test/AzExtTreeFileSystem.test.ts
@@ -0,0 +1,92 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { TestUserInput } from "@microsoft/vscode-azext-dev";
+import * as vscode from 'vscode';
+import { IActionContext } from "..";
+import { AzExtTreeFileSystem } from "../src";
+import assert = require("assert");
+
+type MockItemType = { id: string };
+
+const mockFilePath = "mock_file_path";
+
+class MockAzExtTreeFileSystem extends AzExtTreeFileSystem<MockItemType> {
+    public override scheme = "mockAzExtTreeFileSystem";
+
+    public override async statImpl(_context: IActionContext, _item: MockItemType, _originalUri): Promise<vscode.FileStat> {
+        throw Error("not implemented");
+    }
+
+    public override async readFileImpl(_context: IActionContext, _item: MockItemType, _originalUri): Promise<Uint8Array> {
+        throw Error("not implemented");
+    }
+
+    public override async writeFileImpl(_context: IActionContext, _item: MockItemType) {
+        return;
+    }
+
+    public override getFilePath(_item: MockItemType): string {
+        return mockFilePath;
+    }
+
+    public async lookupPublic(context: IActionContext, uri: vscode.Uri) {
+        return this.lookup(context, uri);
+    }
+}
+
+suite("AzExtTreeFileSystem", function () {
+    const mockContext: IActionContext = { errorHandling: { issueProperties: {} }, telemetry: { measurements: {}, properties: {} }, ui: new TestUserInput(vscode), valuesToMask: [] };
+    test("lookup doesn't find item not shown", async function () {
+        const mockAzExtTreeFileSystem = new MockAzExtTreeFileSystem();
+        const id = "non_existent";
+        const uri: vscode.Uri = vscode.Uri.parse(`${mockAzExtTreeFileSystem.scheme}://mock_file_path?id=${id}`);
+        try {
+            await mockAzExtTreeFileSystem.lookupPublic(mockContext, uri);
+            assert(false, "should have thrown entry not found error");
+        } catch (error) {
+            assert.equal(error.name, "EntryNotFound (FileSystemError)");
+        }
+    });
+
+    test("lookup finds shown item whose id is a valid query parameter", async function () {
+        const mockAzExtTreeFileSystem = new MockAzExtTreeFileSystem();
+        const id = "mock_id";
+        try {
+            await mockAzExtTreeFileSystem.showTextDocument({ id: id });
+        } catch (error) {
+            // Ignore error. Only used showTextDocument to add the document to itemsCache.
+        }
+        const uri: vscode.Uri = vscode.Uri.parse(`${mockAzExtTreeFileSystem.scheme}://mock_file_path?id=${id}`);
+        const item = await mockAzExtTreeFileSystem.lookupPublic(mockContext, uri);
+        assert.equal(item?.id, id);
+    });
+
+    test("lookup finds shown item whose id contains ?", async function () {
+        const mockAzExtTreeFileSystem = new MockAzExtTreeFileSystem();
+        const id = "ZTA?AMGB????oaUBAAAAAAAAAA???";
+        try {
+            await mockAzExtTreeFileSystem.showTextDocument({ id: id });
+        } catch (error) {
+            // Ignore error. Only used showTextDocument to add the document to itemsCache.
+        }
+        const uri: vscode.Uri = vscode.Uri.parse(`${mockAzExtTreeFileSystem.scheme}://mock_file_path?id=${id}`);
+        const item = await mockAzExtTreeFileSystem.lookupPublic(mockContext, uri);
+        assert.equal(item?.id, id);
+    });
+
+    test("lookup finds shown item whose id contains =", async function () {
+        const mockAzExtTreeFileSystem = new MockAzExtTreeFileSystem();
+        const id = "ZTA=AMGBoaUB===AAAAAAAAAA==";
+        try {
+            await mockAzExtTreeFileSystem.showTextDocument({ id: id });
+        } catch (error) {
+            // Ignore error. Only used showTextDocument to add the document to itemsCache.
+        }
+        const uri: vscode.Uri = vscode.Uri.parse(`${mockAzExtTreeFileSystem.scheme}://mock_file_path?id=${id}`);
+        const item = await mockAzExtTreeFileSystem.lookupPublic(mockContext, uri);
+        assert.equal(item?.id, id);
+    });
+});