fix(core): harden workspace path containment - #534
Conversation
Resolve workspace roots and candidates through realpath-aware containment checks so traversal and symlink escapes are rejected consistently across platforms. Closes #528
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
hojinzs
left a comment
There was a problem hiding this comment.
Request changes: dangling symlink bypass in workspace containment
Evidence: https://evidence-browser.dev0.daniworks.kr/w/github-symphony/b/project14-pr-534-20260805-155820z
realpathWithMissingTail() in packages/core/src/workspace/path-safety.ts uses existsSync() to find the existing prefix. existsSync() returns false for a dangling symlink, so a symlink inside the configured root pointing to a not-yet-created path outside the root is treated as an ordinary missing tail and accepted.
I reproduced this against commit 22dda9381901aac976894dba38084ad82243ff8e: resolveWorkspaceDirectory(root, "linked") accepted root/linked when linked was a dangling symlink to an outside path. If that target is created after validation, subsequent workspace operations can follow the link outside the configured root, defeating the stated symlink-escape protection.
Please fail closed for dangling symlink components (for example, inspect links with lstat/equivalent handling) and add a regression test. The core tests passed, but the required full test command also has an unrelated CLI doctor failure; details and raw logs are in the Evidence Browser bundle.
Fail closed when a missing path component is a dangling symlink, so later creation cannot redirect workspace I/O outside the configured root. Closes #528
|
리뷰 피드백을 반영했습니다.
|
hojinzs
left a comment
There was a problem hiding this comment.
✅ Approve — 완성도 검증 완료
원본 이슈 #528 의 수용 조건과 코드 품질, 스모크 테스트를 검증했습니다.
완료 조건 대조 (#528)
| 완료 조건 | 상태 | 근거 |
|---|---|---|
경로 이탈 검사를 path.relative 기반으로 교체 (구분자 하드코딩 제거) |
✅ | isPathWithinRoot 가 relative/isAbsolute 사용, safety.ts·identity.ts 양쪽에서 ${root}/ 하드코딩 제거 |
| realpath 기반 symlink 이탈 검사 추가 (양쪽) | ✅ | realpathWithMissingTail (realpath + 존재하는 최근접 상위 기준), 두 리졸버에 모두 적용 |
TC: .. 이탈 거부 / symlink 이탈 거부 / Windows 구분자 통과 / 정상 회귀 |
✅ | 4종 모두 + dangling symlink 회귀까지 포함 |
| changeset 필수 | ✅ | .changeset/528-workspace-path-safety.md |
추가 강점: dangling symlink 를 lstat 로 fail-closed 처리 — 검사 통과 후 대상이 생성되어도 root 밖 I/O 로 우회되지 않습니다 (TOCTOU 완화).
스모크 테스트 (로컬, preview 환경 없음 → 라이브러리 변경이라 로컬 실행)
pnpm --filter @gh-symphony/core test→ 210 passed- 실제 함수 대상 보안 시나리오 스모크:
../../etc거부 ✅ / symlink(evil) 거부 ✅ / symlink 하위(evil/sub) 거부 ✅ / 정상 미존재 하위(ws-1) 허용 ✅ - CI:
Test✅ /Container Smoke✅ (green)
코드 리뷰
- 인라인 코멘트 1건 (
path-safety.ts:57):startsWith("..")가..config같은 정상 이름을 오탐 거부하는 low-severity 가용성 nit. fail-closed 방향이라 보안 취약점이 아니고, 원본 이슈가 제시한 공식을 그대로 따른 것이라 blocking 아님 — 후속 처리 권장.
이탈을 통과시키는 위험은 발견되지 않았고 수용 조건이 빠짐없이 반영되어 승인합니다. 인라인 nit 은 선택적 후속으로 남겨도 됩니다 — 불필요한 핑퐁을 피하기 위해 이 건으로 재요청하지 않습니다.
Generated by Claude Code
|
|
||
| return ( | ||
| (allowRoot && rel === "") || | ||
| (rel !== "" && !rel.startsWith("..") && !isAbsolute(rel)) |
There was a problem hiding this comment.
[nit · low severity] !rel.startsWith("..") 는 이탈이 아닌 정상 이름도 오탐(false positive)으로 거부합니다.
relative(root, root/"..config") 는 "..config" 를 반환하는데, 이 값은 startsWith("..") 에 걸려 이탈로 판정됩니다. 실제로 ..config 는 root 하위의 정상 디렉터리입니다. 로컬 스모크 테스트로 확인:
resolveWorkspaceDirectory(root, "..config") → throws // 정상 이름인데 거부됨
- 방향 자체는 fail-closed(과도하게 엄격) 라서 보안 취약점은 아닙니다 — 이탈을 통과시키는 게 아니라 정상 이름을 막는 가용성 이슈입니다.
resolveIssueWorkspaceDirectory에서는startsWith(".")reserved 검사에 먼저 걸려 가려지지만,resolveWorkspaceDirectory(safety.ts) 경로에는 그 방어막이 없습니다.- 실무에서 workspace id/key 는 대부분 정제된 슬러그라 트리거 가능성은 낮습니다. 원본 이슈가 제시한 공식(
!rel.startsWith(".."))을 그대로 따른 것이라 blocking 은 아닙니다.
정밀하게 하려면 세그먼트 경계까지 확인하는 형태를 권장합니다:
import { sep } from "node:path";
// ...
rel !== "" && rel !== ".." && !rel.startsWith(`..${sep}`) && !isAbsolute(rel)선택 사항이며 후속 처리로 남겨도 무방합니다.
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
This PR hardens workspace path containment in @gh-symphony/core (workspace filesystem safety; primarily Execution-layer behavior) by replacing string-prefix checks with a realpath-aware, cross-platform containment validator and adding regression coverage for traversal and symlink escapes (Issue #528).
Changes:
- Introduces
isPathWithinRoot()inworkspace/path-safety.tsusingrealpathSync+path.relativecontainment logic, including handling for missing descendant tails and fail-closed dangling symlink components. - Applies the shared validator to both workspace directory resolvers (
workspace/safety.ts,workspace/identity.ts) to remove/-hardcoded prefix logic. - Adds traversal/symlink/dangling-symlink and Windows-separator regression tests in core test suites.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.changeset/528-workspace-path-safety.md |
Adds a patch changeset documenting the path-safety hardening. |
packages/core/src/workspace/path-safety.ts |
New centralized realpath-aware containment helper used by workspace resolvers. |
packages/core/src/workspace/safety.ts |
Switches workspace directory containment check to isPathWithinRoot(). |
packages/core/src/workspace/identity.ts |
Switches issue workspace directory containment check to isPathWithinRoot() (disallowing root). |
packages/core/src/workspace-safety.test.ts |
Adds traversal/symlink/dangling symlink and Windows-separator regression tests for workspace roots. |
packages/core/src/core-conformance.test.ts |
Adds regression tests for issue workspace symlink/dangling symlink escapes and Windows-style separators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| (allowRoot && rel === "") || | ||
| (rel !== "" && !rel.startsWith("..") && !isAbsolute(rel)) | ||
| ); |
TL;DR
path.relativechecks.lstatand rejected fail-closed.변경 지점 다이어그램
여기부터 보세요
packages/core/src/workspace/path-safety.tspackages/core/src/workspace/safety.tspackages/core/src/workspace/identity.tspackages/core/src/workspace-safety.test.tspackages/core/src/core-conformance.test.tsIssues
Summary
Workspace paths are rejected when traversal or symlink resolution escapes the configured root, without hardcoding
/as the separator.Changes
Evidence
pnpm lint— passedpnpm -r --workspace-concurrency=1 test— passed (all workspace packages; core 208 tests, orchestrator 233 tests, worker 152 tests, CLI 497 tests)pnpm --filter @gh-symphony/orchestrator exec vitest run src/index.test.ts -t "gracefully shuts down on SIGTERM"— passed in isolationpnpm typecheck— passedpnpm build— passedpnpm exec prettier --check <changed files>— passedpnpm --filter @gh-symphony/core test— passed (210 tests, including both dangling symlink regressions)TestandContainer Smoke— passed위험 & 롤백
d23e536and22dda93to restore the previous containment implementation.변경 파일
.changeset/528-workspace-path-safety.mdpackages/core/src/workspace/path-safety.tspackages/core/src/workspace/safety.tspackages/core/src/workspace/identity.tspackages/core/src/workspace-safety.test.tspackages/core/src/core-conformance.test.ts머지 후/사람 확인
Human Validation
Risks