From a2967bfa24275a95139662902595282325ff146c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 22:52:14 -0400 Subject: [PATCH] fix(3589)(security): validate workstream in relPlanningPath against traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relPlanningPath(workstream)` previously called `posix.join('.planning', 'workstreams', workstream)` without validating the workstream argument. Direct SDK callers — and `planningPaths` / `ContextEngine` which both forward through `relPlanningPath` — could pass values like `'../../../outside'`, `'foo/bar'`, or `'foo\\bar'` and route planning operations outside the intended `.planning/workstreams/` subtree. The env-sourced workstream code path in `planningPaths` already validated via `validateWorkstreamName` (line 444-445, pre-filtering to `null` on failure per the #2791 silent-fallback contract). Explicit SDK arguments had no equivalent gate. Fix: validate inside `relPlanningPath` using the same shared `validateWorkstreamName` policy. Every caller — direct SDK use, `planningPaths`, `ContextEngine` — fails closed at the same seam. Empty/undefined workstream still returns `.planning` for back-compat (treated as "no workstream provided"); non-empty invalid names throw a synchronous Error with the offending value in the message. Env-sourced behaviour is unchanged: `planningPaths` continues to filter invalid env values to `null` before they reach `relPlanningPath`, so the silent-fallback path for malformed `GSD_WORKSTREAM` env still works. Regression test (sdk/src/bug-3589-planning-paths-validation.test.ts): - 9 traversal/invalid cases (.., /, \\, spaces, .hidden, /abs, -leading-hyphen) all throw with a `/workstream/i`-matching message. - Valid names (`frontend`, `api_v2`, `alpha.beta-1`) continue to produce the expected `.planning/workstreams/` path. - `planningPaths('/tmp', '../../../outside')` rejects before path construction (proven via try/catch — resultPath stays null). - Valid workstream + `planningPaths` produces the expected subtree (`.planning/workstreams/frontend/STATE.md` etc.). - Omitted workstream still returns root `.planning` with no `workstreams` segment. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...89-planning-paths-workstream-validation.md | 5 ++ ...bug-3589-planning-paths-validation.test.ts | 88 +++++++++++++++++++ sdk/src/workstream-utils.ts | 15 ++++ 3 files changed, 108 insertions(+) create mode 100644 .changeset/3589-planning-paths-workstream-validation.md create mode 100644 sdk/src/bug-3589-planning-paths-validation.test.ts diff --git a/.changeset/3589-planning-paths-workstream-validation.md b/.changeset/3589-planning-paths-workstream-validation.md new file mode 100644 index 000000000..9bc011e5c --- /dev/null +++ b/.changeset/3589-planning-paths-workstream-validation.md @@ -0,0 +1,5 @@ +--- +type: Security +issue: 3589 +--- +**`relPlanningPath()` now validates explicit workstream names** — direct SDK callers passing a workstream argument to `relPlanningPath`, `planningPaths`, or `ContextEngine` previously had no path-traversal gate. A value like `'../../../outside'` flowed through `posix.join('.planning', 'workstreams', name)` and routed planning operations outside the intended `.planning/workstreams/` subtree. The fix runs the shared `validateWorkstreamName` policy inside `relPlanningPath` so every consumer fails closed at the same seam. Env-sourced workstreams continue to fall back silently to root `.planning/` per the #2791 contract (they are filtered to `null` by `planningPaths` before reaching `relPlanningPath`). diff --git a/sdk/src/bug-3589-planning-paths-validation.test.ts b/sdk/src/bug-3589-planning-paths-validation.test.ts new file mode 100644 index 000000000..7c2112eb2 --- /dev/null +++ b/sdk/src/bug-3589-planning-paths-validation.test.ts @@ -0,0 +1,88 @@ +/** + * Bug #3589 (security): SDK `planningPaths(projectDir, workstream)` and + * `relPlanningPath(workstream)` accepted unvalidated explicit workstream + * names from direct SDK callers. Path-traversal segments (`..`, `/`, `\\`) + * would flow through `posix.join('.planning', 'workstreams', name)` and + * route planning operations outside the intended `.planning/workstreams/` + * subtree. + * + * Env-sourced workstreams are pre-validated inside `planningPaths` and fall + * back to root .planning/ silently (#2791 contract). Explicit SDK arguments + * had no such gate. + * + * Fix: validate inside `relPlanningPath` so every caller — direct SDK use, + * `planningPaths`, `ContextEngine` — is protected at the same seam. + * Explicit invalid names throw; env-sourced ones still silently fall back + * because `planningPaths` filters them to `null` before calling + * `relPlanningPath`. + */ + +import { describe, it, expect } from 'vitest'; +import { relPlanningPath } from './workstream-utils.js'; +import { planningPaths } from './query/helpers.js'; + +// Empty string is treated as "no workstream provided" (returns `.planning`) +// for back-compat with pre-fix behaviour; only non-empty invalid names throw. +const TRAVERSAL_CASES = [ + '../../../outside', + '../escape', + '..', + 'foo/bar', + 'foo\\bar', + 'foo bar', + '.hidden', + '/abs', + '-leading-hyphen', +]; + +describe('bug #3589: relPlanningPath rejects path-traversal and invalid workstream names', () => { + it('returns .planning when workstream is omitted (unchanged)', () => { + expect(relPlanningPath()).toBe('.planning'); + expect(relPlanningPath(undefined)).toBe('.planning'); + }); + + it('returns .planning/workstreams/ for valid workstream names (unchanged)', () => { + expect(relPlanningPath('frontend')).toBe('.planning/workstreams/frontend'); + expect(relPlanningPath('api_v2')).toBe('.planning/workstreams/api_v2'); + expect(relPlanningPath('alpha.beta-1')).toBe('.planning/workstreams/alpha.beta-1'); + }); + + for (const bad of TRAVERSAL_CASES) { + it(`throws for invalid workstream name ${JSON.stringify(bad)}`, () => { + expect(() => relPlanningPath(bad)).toThrow(/workstream/i); + }); + } + + it('throws BEFORE constructing the path (no partial side effect)', () => { + let resultPath: string | null = null; + try { + resultPath = relPlanningPath('../../../outside'); + } catch { + /* expected */ + } + expect(resultPath).toBeNull(); + }); +}); + +describe('bug #3589: planningPaths rejects explicit invalid workstream names', () => { + it('throws for explicit ../../../outside (was silently constructing a traversal path)', () => { + expect(() => planningPaths('/tmp/projectDir', '../../../outside')).toThrow(/workstream/i); + }); + + it('throws for explicit slash-bearing names', () => { + expect(() => planningPaths('/tmp/projectDir', 'foo/bar')).toThrow(/workstream/i); + }); + + it('accepts valid explicit names and constructs the expected planning subtree', () => { + const paths = planningPaths('/tmp/projectDir', 'frontend'); + expect(paths.planning.endsWith('.planning/workstreams/frontend')).toBe(true); + expect(paths.state.endsWith('.planning/workstreams/frontend/STATE.md')).toBe(true); + expect(paths.roadmap.endsWith('.planning/workstreams/frontend/ROADMAP.md')).toBe(true); + }); + + it('still returns root .planning when workstream is omitted', () => { + const paths = planningPaths('/tmp/projectDir'); + expect(paths.planning.endsWith('.planning')).toBe(true); + expect(paths.planning).not.toContain('workstreams'); + }); +}); diff --git a/sdk/src/workstream-utils.ts b/sdk/src/workstream-utils.ts index abb1c9f81..2f805d8db 100644 --- a/sdk/src/workstream-utils.ts +++ b/sdk/src/workstream-utils.ts @@ -6,6 +6,7 @@ */ import { posix } from 'node:path'; +import { validateWorkstreamName } from './workstream-name-policy.js'; export { validateWorkstreamName, toWorkstreamSlug } from './workstream-name-policy.js'; /** @@ -13,9 +14,23 @@ export { validateWorkstreamName, toWorkstreamSlug } from './workstream-name-poli * * - Without workstream: `.planning` * - With workstream: `.planning/workstreams/` + * + * #3589 (security): validates the explicit workstream name against the + * shared `validateWorkstreamName` policy before path construction. Path + * traversal segments (`..`, `/`, `\\`) and other invalid identifiers throw + * synchronously, so every caller — direct SDK use, `planningPaths`, + * `ContextEngine` — fails closed at the same seam. Env-sourced workstreams + * are still pre-filtered to `null` by `planningPaths` (the #2791 silent + * fallback contract), so this guard does NOT change env-sourced behaviour. */ export function relPlanningPath(workstream?: string): string { if (!workstream) return '.planning'; + if (!validateWorkstreamName(workstream)) { + throw new Error( + `Invalid workstream name: ${JSON.stringify(workstream)}. ` + + `Workstream names must match /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/ and may not contain '..'.`, + ); + } // Use POSIX segments so the same logical path string is used on all platforms (Windows included). return posix.join('.planning', 'workstreams', workstream); }