From 28ceca2cbdb8c6d0c4e6afa2520cc416155aed65 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 24 Jun 2026 15:43:12 -0700 Subject: [PATCH] Address PR review feedback (#1747) - core/store: include workspaceWorktrees in the slim and activity-log-limited SELECT lists (rowToTask reads it, but the explicit column lists omitted it, so slim/limited reads dropped the field and could misclassify workspace tasks); add regression tests for both read surfaces - dashboard/register-git-github: validate caller-supplied repoPath in resolveGitDir via isPathWithin containment check (path-traversal hardening for all git endpoints); make loadWorkspaceConfig a static @fusion/core import per AGENTS.md - dashboard/legacy: preserve repoPath in the string-form pullBranch overload - dashboard/GitManagerModal: revalidate selectedRepo against the fetched repo list so a stale selection can't persist across project switches Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/store-persistence.test.ts | 19 ++++++++++++++ packages/core/src/store.ts | 4 +-- packages/dashboard/app/api/legacy.ts | 12 ++++++--- .../app/components/GitManagerModal.tsx | 26 ++++++++++++++----- .../src/routes/register-git-github.ts | 21 ++++++++++++--- 5 files changed, 67 insertions(+), 15 deletions(-) diff --git a/packages/core/src/__tests__/store-persistence.test.ts b/packages/core/src/__tests__/store-persistence.test.ts index a39f43ac25..c54be7ee39 100644 --- a/packages/core/src/__tests__/store-persistence.test.ts +++ b/packages/core/src/__tests__/store-persistence.test.ts @@ -102,6 +102,25 @@ describe("TaskStore", () => { expect(detail.workspaceWorktrees).toEqual(sampleMap); }); + // Surface enumeration (PR #1747 review): rowToTask reads row.workspaceWorktrees, but the + // explicit slim and activity-log-limited SELECT lists are separate from `*` — if the column is + // omitted there, slim/limited reads silently drop the field even though getTask("*") works. + it("survives the activity-log-limited read (explicit limited SELECT clause)", async () => { + const task = await harness.store().createTask({ description: "Workspace task limited read" }); + await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap }); + + const detail = await harness.store().getTask(task.id, { activityLogLimit: 1 }); + expect(detail.workspaceWorktrees).toEqual(sampleMap); + }); + + it("survives the slim search read (explicit slim SELECT clause)", async () => { + const task = await harness.store().createTask({ description: "Workspace slimsearchmarker task" }); + await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap }); + + const found = (await harness.store().searchTasks("slimsearchmarker", { slim: true })).find((t) => t.id === task.id); + expect(found?.workspaceWorktrees).toEqual(sampleMap); + }); + it("normalizes an empty map to undefined so isWorkspaceTask stays false", async () => { const task = await harness.store().createTask({ description: "Empty workspace map" }); const updated = await harness.store().updateTask(task.id, { workspaceWorktrees: {} }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 6db34a6944..64d26fca25 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -2607,7 +2607,7 @@ export class TaskStore extends EventEmitter { "tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenUsageModelProvider", "tokenUsageModelId", "tokenUsagePerModel", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride", "createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt", "dependencies", "steps", "customFields", "comments", "review", "reviewState", "workflowStepResults", "steeringComments", - "attachments", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", + "attachments", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", "workspaceWorktrees", "breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles", "missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource", "sourceType", "sourceAgentId", "sourceRunId", "sourceSessionId", "sourceMessageId", "sourceParentTaskId", "sourceMetadata", @@ -2656,7 +2656,7 @@ export class TaskStore extends EventEmitter { "tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenUsageModelProvider", "tokenUsageModelId", "tokenUsagePerModel", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride", "createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt", "dependencies", "steps", "customFields", "attachments", "steeringComments", - "comments", "review", "reviewState", "workflowStepResults", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", + "comments", "review", "reviewState", "workflowStepResults", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", "workspaceWorktrees", "breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles", "missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource", "sourceType", "sourceAgentId", "sourceRunId", "sourceSessionId", "sourceMessageId", "sourceParentTaskId", "sourceMetadata", diff --git a/packages/dashboard/app/api/legacy.ts b/packages/dashboard/app/api/legacy.ts index 28b33deb29..08d78d6d76 100644 --- a/packages/dashboard/app/api/legacy.ts +++ b/packages/dashboard/app/api/legacy.ts @@ -3128,10 +3128,16 @@ export function pullBranch( projectId?: string, repoPath?: string, ): Promise { - const options = typeof optionsOrProjectId === "string" ? undefined : optionsOrProjectId; - const resolvedProjectId = typeof optionsOrProjectId === "string" ? optionsOrProjectId : projectId; + // FNXC:DashboardGitApi 2026-06-24-00:00: + // pullBranch has two overloads. In the string-arg style pullBranch(projectId, repoPath), + // the second positional carries repoPath (not the 3rd parameter), so resolve it from `projectId` + // to avoid dropping repoPath; otherwise multi-repo workspace pulls hit the wrong repo. + const isStringForm = typeof optionsOrProjectId === "string"; + const options = isStringForm ? undefined : optionsOrProjectId; + const resolvedProjectId = isStringForm ? optionsOrProjectId : projectId; + const resolvedRepoPath = isStringForm ? projectId : repoPath; - return api(withRepoPath(withProjectId("/git/pull", resolvedProjectId), repoPath), { + return api(withRepoPath(withProjectId("/git/pull", resolvedProjectId), resolvedRepoPath), { method: "POST", body: JSON.stringify({ rebase: options?.rebase ?? false }), }); diff --git a/packages/dashboard/app/components/GitManagerModal.tsx b/packages/dashboard/app/components/GitManagerModal.tsx index b2e1f04966..e50f21bb78 100644 --- a/packages/dashboard/app/components/GitManagerModal.tsx +++ b/packages/dashboard/app/components/GitManagerModal.tsx @@ -898,16 +898,30 @@ export function GitManagerModal({ isOpen, onClose, tasks: _tasks, addToast, proj }, [projectId]); // Fetch workspace repos on mount to determine if this is a multi-repo project. + /* + FNXC:Workspace 2026-06-24-21:30: + Revalidate selectedRepo against the freshly fetched repo list. When projectId + changes (or a project has no workspace repos), a stale selection from the prior + project would otherwise persist and keep sending a stale repoPath to git + endpoints. Keep the current selection only if it still exists in the new list; + otherwise fall back to repos[0], or clear to null when the list is empty (and on + fetch error). The functional updater lets us revalidate without depending on + selectedRepo in the effect deps, preserving the projectId-keyed intent. + */ useEffect(() => { fetchWorkspaceRepos(projectId) .then((result) => { - setWorkspaceRepos(result.repos); - if (result.repos.length > 0 && !selectedRepo) { - setSelectedRepo(result.repos[0]!); - } + const repos = result.repos; + setWorkspaceRepos(repos); + setSelectedRepo((current) => + current && repos.includes(current) ? current : (repos[0] ?? null), + ); }) - .catch(() => setWorkspaceRepos([])); - }, [projectId]); // intentionally omit selectedRepo to avoid resetting on repo switch + .catch(() => { + setWorkspaceRepos([]); + setSelectedRepo(null); + }); + }, [projectId]); // keyed on projectId; selectedRepo is revalidated via the functional updater const handleSyncIntegrationTip = useCallback(async () => { if (!status?.integrationBranch || status.isOnIntegrationBranch === false) return; diff --git a/packages/dashboard/src/routes/register-git-github.ts b/packages/dashboard/src/routes/register-git-github.ts index 0fa820f3b7..0e08a14aa3 100644 --- a/packages/dashboard/src/routes/register-git-github.ts +++ b/packages/dashboard/src/routes/register-git-github.ts @@ -1,5 +1,5 @@ import { type NextFunction, type Request, type Response } from "express"; -import { isAbsolute, resolve, relative, join } from "node:path"; +import { isAbsolute, resolve, relative } from "node:path"; import { realpathSync } from "node:fs"; import { exec as execCb, spawn } from "node:child_process"; import { promisify } from "node:util"; @@ -17,7 +17,7 @@ import type { Task, TaskStore, } from "@fusion/core"; -import { classifyGhError, getCurrentRepo, isGhAuthenticated } from "@fusion/core"; +import { classifyGhError, getCurrentRepo, isGhAuthenticated, loadWorkspaceConfig } from "@fusion/core"; import { dropAutostashHandle, generateSyntheticRunId, @@ -2474,11 +2474,25 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { In workspace mode (multi-repo), git operations target a specific sub-repo. The `repoPath` query param selects which sub-repo. When absent, the project root directory is used (existing single-repo behavior). + + FNXC:Workspace 2026-06-24-22:30: + `repoPath` is caller-supplied and untrusted. It must resolve to a directory + contained within the project root; a `../`-prefixed or absolute value would + otherwise redirect every git endpoint (read remote URLs, commit/push/discard) + at an arbitrary repo on disk. Resolve to an absolute path and reject anything + that escapes `projectRoot` via the shared `isPathWithin` containment check + (the empty / `.` / exact-root case stays allowed — that is the root itself). */ function resolveGitDir(req: Request, projectRoot: string): string { const repoPath = req.query.repoPath; if (typeof repoPath === "string" && repoPath.trim()) { - return join(projectRoot, repoPath.trim()); + const resolved = resolve(projectRoot, repoPath.trim()); + if (!isPathWithin(projectRoot, resolved)) { + throw new ApiError(400, "Invalid repoPath: resolves outside the project root", { + reason: "repo-path-escape", + }); + } + return resolved; } return projectRoot; } @@ -2492,7 +2506,6 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { try { const { store: scopedStore } = await getProjectContext(req); const rootDir = resolveGitDir(req, scopedStore.getRootDir()); - const { loadWorkspaceConfig } = await import("@fusion/core"); const config = await loadWorkspaceConfig(rootDir); res.json({ repos: config?.repos ?? [] }); } catch (err: unknown) {