diff --git a/.changelog/next/fixed-issue-4293.md b/.changelog/next/fixed-issue-4293.md new file mode 100644 index 0000000000..f9068e6350 --- /dev/null +++ b/.changelog/next/fixed-issue-4293.md @@ -0,0 +1 @@ +- [issue-4293] Workspace-root rejection logs now identify the rejected path and checked roots. diff --git a/server/lib/README.md b/server/lib/README.md index 4e36c77ce5..7f08e16fa4 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -364,7 +364,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `uuid.js` | `v4()` thin wrapper over `crypto.randomUUID()`. | | `versionUtils.js` | `compareSemver(a, b)` — semver ordering (-1/0/1) with pre-release precedence and build-metadata stripping. Shared by the self-update checker (`updateChecker.js`) and the local-LLM Ollama update detector (`localLlm.js`). Inputs must be `v`-stripped. | | `workTracker.js` | `WORK_TRACKERS`/`CONCRETE_WORK_TRACKERS`/`DEFAULT_WORK_TRACKER`, `workTrackerLabel`, `hostToWorkTracker`, `isGithubHost` (GitHub-family host test — github.com + enterprise github.*; enterprise-aware replacement for the github.com-only `isGithub` gate), `githubRepoSpec(origin)` (host-qualified `HOST/OWNER/REPO` selector for `gh --repo`, or null for a non-GitHub origin — pairs the isGithubHost gate with the selector so prWatcher/branchReconcile/issueReconcile share one "resolvable GitHub repo" definition), `forgeCliForTracker`, `isFileTracker` (true when the tracker records work as repo files — PLAN.md — so an agent's proposal necessarily dirties the worktree; false for github/gitlab/jira), `trackerToClaimTaskType`, `hostFromOriginUrl` (subgroup-tolerant host parse), pure `resolveWorkTracker({configured,host})`, async `resolveAppWorkTracker(app)` — resolves a managed app's autonomous work source (PLAN.md / GitHub / GitLab / JIRA), defaulting `'auto'` to the git origin host. Async `resolveRepoForgeTarget(repoPath)` — the ONE definition of "which forge can we query for this checkout", returning `{ forge, fullName, repoSpec, apiHost }` (enterprise-aware `gh --repo` selector for GitHub; `repoSpec: null` for GitLab, which `glab` resolves from its cwd) or null for a non-forge origin; shared by `issueReconcile.js` and `appIssues.js`. Async `resolveAppForgeTarget(app, {repoPath})` — the composed `resolveAppWorkTracker` + `resolveRepoForgeTarget` for callers holding the managed-app record, returning `{ tracker, target }` with the app's github/gitlab pin threaded in as `preferredForge` (so a self-hosted forge on a hostname matching neither pattern still resolves); use this instead of re-threading the pin by hand. Also owns the `{trackerInstructions}` prompt block shared by the TRACKER-FILING task types (types that read the app read-only and deliver findings as tracker items, not a commit): `TRACKER_FILING_PRESETS` (per-task-type slug prefix / label / body requirements — `reference-watch`, `ux`, `repo-study`), `TRACKER_FILING_TASK_TYPES` (derived from the presets, so a gated type always has wording), and `formatTrackerInstructions(tracker, options)` which renders the plan/github/gitlab/jira block (reference-watch is the default option set, so a bare call stays byte-identical for it). Consumed by the `claim-work` router in `cosTaskGenerator.js`, `referenceRepos.js`, and `routes/apps.js`. | -| `workspaceRoots.js` | Shared allow-list for routes that take a caller-supplied filesystem path. `isWithinAllowedRoots(realPath)` is the single complete test — on Windows it also allows any lettered non-system drive (`D:\code`), which appears in no root, so never render `ALLOWED_WORKSPACE_ROOTS` as "the directories you may use". Also `ALLOWED_WORKSPACE_ROOTS` (defaults + `PORTOS_WORKSPACE_ROOTS`, split on the platform path delimiter — `;` on Windows — and symlink-resolved), `isWithinRoot(resolvedPath, root)` (separator-safe containment), and `WORKSPACE_ROOTS_CONFIGURED` (true when the operator set the env var — lets a permissive-by-default route like `routes/detect.js` opt into confinement). Defaults cover home plus wherever the platform mounts secondary volumes: `/tmp` + `/Users` + `/Volumes` + `/mnt` + `/media` + `/opt` on POSIX; home + the temp dir plus the non-system-drive rule on Windows, where those POSIX literals would resolve to whatever drive the process happens to be on. Used by `routes/commands.js`, `routes/scaffold.js`, and `routes/git.js` (always scoped) and `routes/detect.js` (scoped only when configured). | +| `workspaceRoots.js` | Shared allow-list for routes that take a caller-supplied filesystem path. `isWithinAllowedRoots(realPath)` is the single complete test — on Windows it also allows any lettered non-system drive (`D:\code`), which appears in no root, so never render `ALLOWED_WORKSPACE_ROOTS` as "the directories you may use". `outsideAllowedRootsMessage(realPath, { field })` formats a redacted server-only diagnostic with the rejected realpath and every checked root. Also `ALLOWED_WORKSPACE_ROOTS` (defaults + `PORTOS_WORKSPACE_ROOTS`, split on the platform path delimiter — `;` on Windows — and symlink-resolved), `isWithinRoot(resolvedPath, root)` (separator-safe containment), and `WORKSPACE_ROOTS_CONFIGURED` (true when the operator set the env var — lets a permissive-by-default route like `routes/detect.js` opt into confinement). Defaults cover home plus wherever the platform mounts secondary volumes: `/tmp` + `/Users` + `/Volumes` + `/mnt` + `/media` + `/opt` on POSIX; home + the temp dir plus the non-system-drive rule on Windows, where those POSIX literals would resolve to whatever drive the process happens to be on. Used by `routes/commands.js`, `routes/scaffold.js`, and `routes/git.js` (always scoped) and `routes/detect.js` (scoped only when configured). | | `zodCompat.js` | Zod 4 compatibility helpers. `partialWithoutDefaults(objectSchema)` — like `.partial()` but strips inner field defaults first, so a PATCH/update schema doesn't inject (and clobber) the stored values of fields the caller didn't send. Use for any update schema derived from a defaulted base. | ## Test support diff --git a/server/lib/workspaceRoots.js b/server/lib/workspaceRoots.js index 772edaa951..10ac8cb503 100644 --- a/server/lib/workspaceRoots.js +++ b/server/lib/workspaceRoots.js @@ -1,5 +1,5 @@ import { realpathSync } from 'fs'; -import { resolve, relative, isAbsolute, delimiter } from 'path'; +import { resolve, relative, isAbsolute, delimiter, basename } from 'path'; import { homedir, tmpdir } from 'os'; // Allowed workspace roots shared by routes that accept a caller-supplied @@ -80,3 +80,93 @@ export function isWithinAllowedRoots(realPath) { if (isOnWindowsWorkspaceDrive(realPath)) return true; return ALLOWED_WORKSPACE_ROOTS.some(root => isWithinRoot(realPath, root)); } + +const HOME_ROOT = ALLOWED_WORKSPACE_ROOTS[0]; + +const singleLine = (value) => String(value).replace(/[\r\n]+/g, ' '); + +const PRIVATE_HOME_SEGMENT = /(^|[\\/])((?:Users|home))([\\/])[^\\/]+(?=([\\/]|$))/gi; + +const HOME_USERNAME = basename(homedir()); +const HOME_USERNAME_KEY = HOME_USERNAME.toLowerCase(); + +const redactUncHost = (path) => { + const leading = path.startsWith('//') + ? '//' + : path.charCodeAt(0) === 92 && path.charCodeAt(1) === 92 + ? path.slice(0, 2) + : null; + if (!leading) return path; + + const firstSegmentEndOffset = path.slice(leading.length).search(/[\\/]/); + if (firstSegmentEndOffset < 0) return `${leading}`; + const firstSegmentEnd = leading.length + firstSegmentEndOffset; + const firstSegment = path.slice(leading.length, firstSegmentEnd); + + // Extended-length UNC paths (`\\?\\UNC\\server\\share`) put the marker and + // `UNC` segments before the actual host. Extended local paths (`\\?\\C:\\…`) + // have no host to redact and fall through to the home-segment redaction. + let hostStart = leading.length; + if (firstSegment === '?' || firstSegment === '.') { + const secondSegmentStart = firstSegmentEnd + 1; + const secondSegmentEndOffset = path.slice(secondSegmentStart).search(/[\\/]/); + if (secondSegmentEndOffset < 0) return path; + const secondSegmentEnd = secondSegmentStart + secondSegmentEndOffset; + const secondSegment = path.slice(secondSegmentStart, secondSegmentEnd); + if (secondSegment.toUpperCase() !== 'UNC') return path; + hostStart = secondSegmentEnd + 1; + } + + const hostEndOffset = path.slice(hostStart).search(/[\\/]/); + if (hostEndOffset < 0) return `${path.slice(0, hostStart)}`; + const hostEnd = hostStart + hostEndOffset; + return `${path.slice(0, hostStart)}${path.slice(hostEnd)}`; +}; + +const redactKnownUsername = (path) => { + if (!HOME_USERNAME) return path; + let result = ''; + let segmentStart = 0; + for (let index = 0; index <= path.length; index += 1) { + const atBoundary = index === path.length || path[index] === '/' || path[index] === '\\'; + if (!atBoundary) continue; + const segment = path.slice(segmentStart, index); + const matches = IS_WINDOWS + ? segment.toLowerCase() === HOME_USERNAME_KEY + : segment === HOME_USERNAME; + result += matches ? '' : segment; + if (index < path.length) result += path[index]; + segmentStart = index + 1; + } + return result; +}; + +const redactPrivatePath = (path) => redactKnownUsername( + path.replace(PRIVATE_HOME_SEGMENT, (_match, prefix, root, separator) => `${prefix}${root}${separator}`) +); + +const formatLogPath = (value) => { + const path = singleLine(value); + if (path === HOME_ROOT) return '~'; + const redactedUncPath = redactUncHost(path); + const isWindowsStylePath = path.startsWith('//') + || path.charCodeAt(0) === 92 + || /^[A-Za-z]:[\\/]/.test(path); + if (isWindowsStylePath) return redactPrivatePath(redactedUncPath); + const relativeHomePath = relative(HOME_ROOT, path); + if (relativeHomePath && !relativeHomePath.startsWith('..') && !isAbsolute(relativeHomePath)) { + return `~/${redactPrivatePath(relativeHomePath)}`; + } + return redactPrivatePath(redactedUncPath); +}; + +/** + * Build the redacted, server-only diagnostic for a path rejected by isWithinAllowedRoots. + * Callers keep their existing terse HTTP error so a real filesystem path never + * appears in an API response or a value a user might paste into a public issue. + */ +export function outsideAllowedRootsMessage(realPath, { field = 'path' } = {}) { + const roots = ALLOWED_WORKSPACE_ROOTS.map(formatLogPath); + if (IS_WINDOWS) roots.push('any non-system drive'); + return `${singleLine(field)} is outside allowed directories: ${formatLogPath(realPath)} (allowed: ${roots.join(', ')})`; +} diff --git a/server/lib/workspaceRoots.test.js b/server/lib/workspaceRoots.test.js index ffda3ef88c..8f44251c77 100644 --- a/server/lib/workspaceRoots.test.js +++ b/server/lib/workspaceRoots.test.js @@ -1,9 +1,10 @@ import { describe, it, expect, afterEach, vi } from 'vitest'; import { homedir } from 'os'; -import { join, delimiter } from 'path'; +import { join, basename, delimiter } from 'path'; import { isWithinRoot, isWithinAllowedRoots, + outsideAllowedRootsMessage, ALLOWED_WORKSPACE_ROOTS, DEFAULT_WORKSPACE_ROOTS, } from './workspaceRoots.js'; @@ -47,6 +48,65 @@ describe('isWithinAllowedRoots', () => { }); }); +describe('outsideAllowedRootsMessage', () => { + it('names the rejected realpath and the roots checked', () => { + const message = outsideAllowedRootsMessage('/etc/shadow'); + + expect(message).toMatch(/^path is outside allowed directories: \/etc\/shadow \(allowed: .+\)$/); + expect(message).toContain('allowed: ~'); + if (IS_WINDOWS) expect(message).toContain('any non-system drive'); + }); + + it('uses a caller-provided field name', () => { + expect(outsideAllowedRootsMessage('/etc/shadow', { field: 'workspacePath' })) + .toMatch(/^workspacePath is outside allowed directories: \/etc\/shadow \(allowed: .+\)$/); + }); + + it('redacts usernames in other home-directory paths', () => { + const message = outsideAllowedRootsMessage('/home/alice/private-repo'); + + expect(message).toContain('/home//private-repo'); + expect(message).not.toContain('alice'); + }); + + it('redacts the current username in nonstandard path segments', () => { + const username = basename(homedir()); + const message = outsideAllowedRootsMessage(`/srv/${username}/repo`); + + expect(message).toContain(''); + expect(message).not.toContain(username); + }); + + it('redacts the current username in nested home-directory segments', () => { + const username = basename(homedir()); + const message = outsideAllowedRootsMessage(join(homedir(), 'nested', username, 'repo')); + + const expectedNestedPath = IS_WINDOWS + ? String.raw`nested\\repo` + : '~/nested//repo'; + expect(message).toContain(expectedNestedPath); + expect(message).not.toContain(username); + }); + + it('redacts hosts in UNC paths', () => { + const message = outsideAllowedRootsMessage(String.raw`\\server\share\repo`); + + expect(message).toContain(String.raw`\\\share\repo`); + expect(message).not.toContain('server'); + }); + + it('redacts nested usernames and extended UNC hosts', () => { + const nestedMessage = outsideAllowedRootsMessage(String.raw`\\server\share\Users\alice\repo`); + const extendedMessage = outsideAllowedRootsMessage(String.raw`\\?\UNC\server\share\repo`); + + expect(nestedMessage).toContain(String.raw`\\\share\Users\\repo`); + expect(nestedMessage).not.toContain('server'); + expect(nestedMessage).not.toContain('alice'); + expect(extendedMessage).toContain(String.raw`\\?\UNC\\share\repo`); + expect(extendedMessage).not.toContain('server'); + }); +}); + // Windows repos routinely live off the system drive (D:\code, E:\projects), and // the POSIX defaults (/tmp, /Users, /Volumes, /opt) resolve to nothing useful // there — so a non-system lettered drive is allowed, the way /Volumes is on macOS. diff --git a/server/routes/commands.js b/server/routes/commands.js index e424f3ca0b..9f864c7549 100644 --- a/server/routes/commands.js +++ b/server/routes/commands.js @@ -4,7 +4,7 @@ import { resolve } from 'path'; import * as commands from '../services/commands.js'; import * as pm2Service from '../services/pm2.js'; import { asyncHandler, ServerError } from '../lib/errorHandler.js'; -import { isWithinAllowedRoots } from '../lib/workspaceRoots.js'; +import { isWithinAllowedRoots, outsideAllowedRootsMessage } from '../lib/workspaceRoots.js'; const router = Router(); @@ -41,6 +41,7 @@ router.post('/execute', asyncHandler(async (req, res) => { throw new ServerError('workspacePath is not accessible', { status: 400, code: 'INVALID_PATH' }); } if (!isWithinAllowedRoots(realPath)) { + console.error(`❌ ${outsideAllowedRootsMessage(realPath, { field: 'workspacePath' })}`); throw new ServerError('workspacePath is outside allowed directories', { status: 400, code: 'INVALID_PATH' }); } } diff --git a/server/routes/detect.js b/server/routes/detect.js index 60370ee998..bf32c8f022 100644 --- a/server/routes/detect.js +++ b/server/routes/detect.js @@ -8,7 +8,7 @@ import { execPm2 } from '../services/pm2.js'; import { detectAppWithAi } from '../services/aiDetect.js'; import { asyncHandler, ServerError } from '../lib/errorHandler.js'; import { safeJSONParse, tryReadFile } from '../lib/fileUtils.js'; -import { isWithinAllowedRoots, WORKSPACE_ROOTS_CONFIGURED } from '../lib/workspaceRoots.js'; +import { isWithinAllowedRoots, outsideAllowedRootsMessage, WORKSPACE_ROOTS_CONFIGURED } from '../lib/workspaceRoots.js'; const execAsync = promisify(exec); const router = Router(); @@ -68,6 +68,7 @@ router.post('/repo', asyncHandler(async (req, res) => { return res.json({ valid: false, error: 'Path is not accessible' }); } if (!isWithinAllowedRoots(realPath)) { + console.error(`❌ ${outsideAllowedRootsMessage(realPath)}`); return res.json({ valid: false, error: 'Path is outside the configured workspace roots (PORTOS_WORKSPACE_ROOTS)' diff --git a/server/routes/git.js b/server/routes/git.js index b42234f711..1f262139b7 100644 --- a/server/routes/git.js +++ b/server/routes/git.js @@ -5,7 +5,7 @@ import * as git from '../services/git.js'; import * as appsService from '../services/apps.js'; import { getAgents } from '../services/cosAgentLifecycle.js'; import { asyncHandler, ServerError } from '../lib/errorHandler.js'; -import { isWithinAllowedRoots } from '../lib/workspaceRoots.js'; +import { isWithinAllowedRoots, outsideAllowedRootsMessage } from '../lib/workspaceRoots.js'; import { validateRequest, submoduleStatusQuerySchema, submoduleUpdateSchema } from '../lib/validation.js'; /** @@ -32,6 +32,7 @@ function assertAllowedWorkspace(path) { throw new ServerError('path is not accessible', { status: 400, code: 'INVALID_PATH' }); } if (!isWithinAllowedRoots(realPath)) { + console.error(`❌ ${outsideAllowedRootsMessage(realPath)}`); throw new ServerError('path is outside allowed directories', { status: 403, code: 'FORBIDDEN' }); } } diff --git a/server/routes/git.test.js b/server/routes/git.test.js index c3cb8f556f..37cd80c80f 100644 --- a/server/routes/git.test.js +++ b/server/routes/git.test.js @@ -44,7 +44,8 @@ vi.mock('../services/cosAgentLifecycle.js', () => ({ // Mock workspace-roots so we control which paths are "allowed" without // touching the real filesystem. vi.mock('../lib/workspaceRoots.js', () => ({ - isWithinAllowedRoots: vi.fn() + isWithinAllowedRoots: vi.fn(), + outsideAllowedRootsMessage: vi.fn((realPath, { field = 'path' } = {}) => `${field} is outside allowed directories: ${realPath}`) })); // Mock fs functions used by assertAllowedWorkspace. @@ -59,7 +60,7 @@ vi.mock('fs', async (importOriginal) => { }); import { existsSync, statSync, realpathSync } from 'fs'; -import { isWithinAllowedRoots } from '../lib/workspaceRoots.js'; +import { isWithinAllowedRoots, outsideAllowedRootsMessage } from '../lib/workspaceRoots.js'; import * as cosAgentLifecycleService from '../services/cosAgentLifecycle.js'; import * as gitService from '../services/git.js'; @@ -99,6 +100,7 @@ describe('git routes — workspace root validation', () => { expect(res.status).toBe(403); expect(res.body.code).toBe('FORBIDDEN'); + expect(outsideAllowedRootsMessage).toHaveBeenCalledWith(body.path); }); }); diff --git a/server/routes/scaffold.js b/server/routes/scaffold.js index d5342daec0..6d0974b92d 100644 --- a/server/routes/scaffold.js +++ b/server/routes/scaffold.js @@ -10,7 +10,7 @@ import { createApp, getReservedPorts } from '../services/apps.js'; import { asyncHandler, ServerError } from '../lib/errorHandler.js'; import { validateRequest, scaffoldSchema } from '../lib/validation.js'; import { ensureDir, expandHome } from '../lib/fileUtils.js'; -import { isWithinAllowedRoots } from '../lib/workspaceRoots.js'; +import { isWithinAllowedRoots, outsideAllowedRootsMessage } from '../lib/workspaceRoots.js'; import { scaffoldVite } from './scaffoldVite.js'; import { scaffoldExpress } from './scaffoldExpress.js'; import { scaffoldIOS } from './scaffoldIOS.js'; @@ -259,6 +259,7 @@ async function scaffoldApp(req, res) { throw new ServerError('parentDir is not accessible', { status: 400, code: 'INVALID_PARENT' }); } if (!isWithinAllowedRoots(realParentDir)) { + console.error(`❌ ${outsideAllowedRootsMessage(realParentDir, { field: 'parentDir' })}`); throw new ServerError('parentDir is outside allowed directories', { status: 403, code: 'FORBIDDEN' }); } diff --git a/server/routes/scaffold.test.js b/server/routes/scaffold.test.js index 4f19e369ea..c2d7800394 100644 --- a/server/routes/scaffold.test.js +++ b/server/routes/scaffold.test.js @@ -45,7 +45,8 @@ vi.mock('../services/apps.js', () => ({ })); vi.mock('../lib/workspaceRoots.js', () => ({ - isWithinAllowedRoots: vi.fn(() => true) + isWithinAllowedRoots: vi.fn(() => true), + outsideAllowedRootsMessage: vi.fn((realPath, { field = 'path' } = {}) => `${field} is outside allowed directories: ${realPath}`) })); vi.mock('./scaffoldVite.js', () => ({ scaffoldVite: vi.fn().mockResolvedValue(undefined) }));