Skip to content

Commit 8bd116e

Browse files
jqnatividadclaude
andcommitted
fix(mcp): address Copilot review — security, extension alignment, fallback order
- Validate browse_directory paths via resolvePath() to enforce allowed-directory security (prevents arbitrary filesystem enumeration) - Align TABULAR_EXTS with FilesystemResourceProvider (.pqt→.pq, +.xlsm/.xlsb/.ods) - Reorder set_working_dir fallback: App → native picker → elicitation → text - Gate _meta.ui.resourceUri on config.enableMcpApps so clients don't see App hints when the feature is disabled - Fix incorrect comment about path.join() always returning POSIX paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent bc4dcc0 commit 8bd116e

File tree

5 files changed

+35
-23
lines changed

5 files changed

+35
-23
lines changed

.claude/skills/src/browse-directory.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@ import { readdir, stat as fsStat } from "node:fs/promises";
1010

1111
/** Tabular file extensions recognised by the directory scanner. */
1212
export const TABULAR_EXTS = new Set([
13-
".csv", ".tsv", ".tab", ".ssv", ".parquet", ".pqt",
13+
".csv", ".tsv", ".tab", ".ssv", ".parquet", ".pq",
1414
".jsonl", ".ndjson", ".json", ".xlsx", ".xls",
15+
".xlsm", ".xlsb", ".ods",
1516
]);
1617

1718
export interface SubdirectoryInfo {

.claude/skills/src/mcp-server.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -712,19 +712,19 @@ class QsvMcpServer {
712712
} as { content: Array<{ type: "text"; text: string }> };
713713
}
714714

715-
// 2. Try interactive elicitation form
716-
const elicitResult = await this.elicitWorkingDirectory();
717-
if (elicitResult.directory) {
718-
const newDir = this.updateWorkingDirectory(elicitResult.directory);
715+
// 2. Try native OS folder picker (macOS Finder dialog)
716+
const nativePick = await this.showNativeFolderPicker();
717+
if (nativePick) {
718+
const newDir = this.updateWorkingDirectory(nativePick);
719719
this.manuallySetWorkingDir = true;
720720
this.workingDirConfirmed = true;
721721
return successResult(`Working directory set to: ${newDir}\n\nAll relative file paths will now be resolved from this directory. Pass "auto" to re-enable automatic root-based sync.`);
722722
}
723723

724-
// 3. Try native OS folder picker (macOS Finder dialog)
725-
const nativePick = await this.showNativeFolderPicker();
726-
if (nativePick) {
727-
const newDir = this.updateWorkingDirectory(nativePick);
724+
// 3. Try interactive elicitation form
725+
const elicitResult = await this.elicitWorkingDirectory();
726+
if (elicitResult.directory) {
727+
const newDir = this.updateWorkingDirectory(elicitResult.directory);
728728
this.manuallySetWorkingDir = true;
729729
this.workingDirConfirmed = true;
730730
return successResult(`Working directory set to: ${newDir}\n\nAll relative file paths will now be resolved from this directory. Pass "auto" to re-enable automatic root-based sync.`);
@@ -950,12 +950,14 @@ class QsvMcpServer {
950950
private async handleBrowseDirectory(
951951
args: Record<string, unknown>,
952952
): Promise<{ content: Array<{ type: "text"; text: string }>; isError?: boolean }> {
953-
const targetDir =
953+
const rawDir =
954954
typeof args.directory === "string" && args.directory.trim().length > 0
955955
? args.directory.trim()
956956
: this.filesystemProvider.getWorkingDirectory();
957957

958958
try {
959+
// Validate the directory is within allowed directories before scanning
960+
const targetDir = await this.filesystemProvider.resolvePath(rawDir);
959961
const result = await scanDirectory(targetDir);
960962
return successResult(JSON.stringify(result));
961963
} catch (err) {

.claude/skills/src/mcp-tools.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3207,12 +3207,16 @@ Call without arguments to show an interactive directory picker (when supported b
32073207
},
32083208
required: [],
32093209
},
3210-
_meta: {
3211-
"ui/resourceUri": "ui://qsv/directory-picker",
3212-
ui: {
3213-
resourceUri: "ui://qsv/directory-picker",
3214-
},
3215-
},
3210+
...(config.enableMcpApps
3211+
? {
3212+
_meta: {
3213+
"ui/resourceUri": "ui://qsv/directory-picker",
3214+
ui: {
3215+
resourceUri: "ui://qsv/directory-picker",
3216+
},
3217+
},
3218+
}
3219+
: {}),
32163220
};
32173221
}
32183222

.claude/skills/src/ui/directory-picker-html.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ function updateSelectedState(path) {
356356
357357
function renderBreadcrumbs(path) {
358358
breadcrumbsEl.innerHTML = "";
359-
// Server uses Node.js path.join() which always returns POSIX paths
359+
// Server returns POSIX-style paths (forward slashes) on macOS/Linux
360360
const parts = path.split("/").filter(Boolean);
361361
362362
// Root

.claude/skills/tests/mcp-app.test.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,19 @@ import { join } from "node:path";
1515
import { createTestDir, cleanupTestDir } from "./test-helpers.js";
1616

1717
describe("MCP App tool definitions", () => {
18-
test("qsv_set_working_dir has _meta.ui.resourceUri", async () => {
18+
test("qsv_set_working_dir _meta.ui.resourceUri depends on enableMcpApps config", async () => {
1919
const { createSetWorkingDirTool } = await import("../src/mcp-tools.js");
20+
const { config } = await import("../src/config.js");
2021
const tool = createSetWorkingDirTool();
2122

22-
assert.ok(tool._meta, "tool should have _meta");
23-
const ui = tool._meta!.ui as Record<string, unknown>;
24-
assert.ok(ui, "tool._meta should have ui");
25-
assert.strictEqual(ui.resourceUri, "ui://qsv/directory-picker");
23+
if (config.enableMcpApps) {
24+
assert.ok(tool._meta, "tool should have _meta when apps enabled");
25+
const ui = tool._meta!.ui as Record<string, unknown>;
26+
assert.ok(ui, "tool._meta should have ui");
27+
assert.strictEqual(ui.resourceUri, "ui://qsv/directory-picker");
28+
} else {
29+
assert.strictEqual(tool._meta, undefined, "tool should not have _meta when apps disabled");
30+
}
2631
});
2732

2833
test("qsv_browse_directory has _meta.ui.visibility: ['app']", async () => {
@@ -239,7 +244,7 @@ describe("scanDirectory (extracted from qsv_browse_directory handler)", () => {
239244

240245
test("TABULAR_EXTS covers expected extensions", async () => {
241246
const { TABULAR_EXTS } = await import("../src/browse-directory.js");
242-
for (const ext of [".csv", ".tsv", ".tab", ".ssv", ".parquet", ".pqt", ".jsonl", ".ndjson", ".json", ".xlsx", ".xls"]) {
247+
for (const ext of [".csv", ".tsv", ".tab", ".ssv", ".parquet", ".pq", ".jsonl", ".ndjson", ".json", ".xlsx", ".xls", ".xlsm", ".xlsb", ".ods"]) {
243248
assert.ok(TABULAR_EXTS.has(ext), `should include ${ext}`);
244249
}
245250
assert.ok(!TABULAR_EXTS.has(".txt"), "should not include .txt");

0 commit comments

Comments
 (0)