feat(#128-B2): 워크스페이스 무결성 link-guard 하드닝 — symlink/junction 탈출 차단 (advisory)#130
Conversation
설계+구현계획 — ignored-baseline 의 statSync(링크 추종) 5지점을 lstat/link-aware 로 전환해 symlink/junction 통한 워크스페이스 밖 read/write/삭제를 차단(advisory), path-guard helper 로 workspace-tools 와 통일. Codex 설계·계획 2라운드 독립 리뷰 반영(created-삭제 isLinkSync 정정· walk 비정형 표면화·새 symlink rollback·collect distinguisher). finding5 는 별도 PR 분리. Refs #128. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…(미존재 tail·case-fold·정확 predicate) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…수집 차단 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…us fallthrough 주석
두 새 테스트(symlink POSIX / junction Windows)에 `expect(base.skipped).toContainEqual({path:'link',reason:'symlink'})` 추가 — side-effect 뿐 아니라 보안 메커니즘 자체를 단언.
ignored-baseline.ts top-level 링크 분기에 'suspicious'/'missing' 은 pushFile로 낙하해 capture lstat(Task 4)이 not-regular 로 표면화한다는 주석 추가.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
… 방지) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…ad modified Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…제 차단 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…(startsWith 오거부 수정) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
…k f.dat 복원 단언 + 주석 정정 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ChangesB2 Link-Guard Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36f3e17f43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isLinkSync(resolve(root, rel)) === 'link') { | ||
| skipped.push({ path: rel, reason: 'symlink' }) | ||
| continue |
There was a problem hiding this comment.
When git reports an ignored sensitive link such as .env, this early link skip diverts it into baseline.skipped before pushFile/captureIgnoredBaseline can apply the sensitive lstat branch, so capture no longer throws and the agent can run despite Fleet having no restorable backup for that sensitive path. Either throw here for policy.sensitiveRe.test(rel) or let sensitive links reach the capture path while still skipping non-sensitive links.
Useful? React with 👍 / 👎.
| if (existsSync(abs)) { | ||
| const st = statSync(abs) | ||
| const st = lstatSync(abs) // [#128-B2] 비추종 — symlink leaf 는 isFile()=false 로 제거 대상 |
There was a problem hiding this comment.
When a baseline ignored file is replaced with a dangling symlink whose target is outside the workspace, existsSync(abs) returns false because it follows the missing target, so this new lstatSync guard is skipped and the later writeFileSync(abs, entry.backup) writes through the symlink if the outside parent exists. Use lstatSync directly and only ignore ENOENT so broken symlink leaves are unlinked before restoring.
Useful? React with 👍 / 👎.
| for (const part of relDir.split(/[\\/]/).filter(Boolean)) { | ||
| cur = resolve(cur, part) | ||
| if (existsSync(cur) && !statSync(cur).isDirectory()) { | ||
| if (!existsSync(cur)) continue |
There was a problem hiding this comment.
When restoring a baseline path like a/b/c.txt after the agent replaces a or a/b with a dangling symlink/junction, existsSync(cur) follows the missing target and returns false, so this cleanup never removes the link. The subsequent mkdirSync(dirname(abs), { recursive: true }) then fails instead of recreating the in-workspace directory chain, leaving ignored files unrestored; probe with lstatSync directly and only treat ENOENT as absent.
Useful? React with 👍 / 👎.
| const git = fakeGitIgnored(['f.dat', 'esc']) // f.dat=baseline, esc=created link | ||
| writeFileSync(join(root, 'f.dat'), 'orig') | ||
| const base = await captureIgnoredBaseline(root, git, DEFAULT_IGNORED_POLICY) |
There was a problem hiding this comment.
This fixture reports esc during baseline capture even though the test creates esc only later, so captureIgnoredBaseline records esc as a baseline read-failed skipped path. restoreIgnoredBaseline intentionally skips current paths already in baseline.skipped, meaning the symlink remains and the assertion at line 784 fails on POSIX; use a baseline git runner with only f.dat and a restore-time runner with f.dat plus esc.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/core/tools/workspace-tools.ts`:
- Around line 167-168: The rootReal variable is derived from fs.realpath(root)
which uses an async non-native implementation, while the start variable obtained
from resolveWithin uses fs.realpathSync.native internally. This inconsistency
causes problems on Windows with junctions and short names. To fix this, ensure
rootReal is derived from the same root resolution source used by resolveWithin.
Either modify resolveWithin to return or expose the resolved root path it
computes, or refactor to use the same native sync realpath method for computing
rootReal that resolveWithin uses internally, so that path.relative(rootReal,
file) at line 178 works correctly and SENSITIVE_FILE.test(rel) exclusion at line
180 functions as intended.
In `@src/main/core/workspace/ignored-baseline.test.ts`:
- Around line 775-783: The test is incorrectly using a single git object that
includes both 'f.dat' and 'esc' for the captureIgnoredBaseline call, but the
'esc' symlink doesn't exist at capture time, causing it to be recorded as a
read-failed entry in baseline.skipped. This exempts 'esc' from removal during
restore, making the assertion fail. Create a separate git object for
captureIgnoredBaseline that only includes 'f.dat', then use the original git
object with both 'f.dat' and 'esc' for restoreIgnoredBaseline, following the
pattern shown in the sibling test at lines 792-808.
In `@src/main/core/workspace/ignored-baseline.ts`:
- Around line 148-155: The symlink check in the else branch routes every link
directly into skipped and continues before the entry can reach
captureIgnoredBaseline's fail-closed throw guard at lines 185-189. For sensitive
symlinks like .env targeting ~/.ssh/id_rsa, this causes a silent skip instead of
the intended throw, breaking the test at ignored-baseline.test.ts:746. Remove or
bypass the early symlink skip logic for sensitive paths so entries flow through
to captureIgnoredBaseline where its lstat guard can properly fail closed. Ensure
sensitive symlink paths are not added to the skipped array and allow them to
reach the pushFile or capture logic that validates them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa7b0070-694c-439a-871e-4e2d9406e595
📒 Files selected for processing (9)
brain.mddocs/superpowers/plans/2026-06-23-issue128-B2-link-guard.mddocs/superpowers/specs/2026-06-23-issue128-B2-link-guard-design.mdsrc/main/core/tools/workspace-tools.test.tssrc/main/core/tools/workspace-tools.tssrc/main/core/workspace/ignored-baseline.test.tssrc/main/core/workspace/ignored-baseline.tssrc/main/core/workspace/path-guard.test.tssrc/main/core/workspace/path-guard.ts
| const rootReal = await fs.realpath(root) | ||
| const start = await resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.') | ||
| const start = resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Do Node.js fs.realpath and fs.realpathSync.native return identical paths on Windows for junctions and short (8.3) names?
💡 Result:
Node.js fs.realpath and fs.realpathSync.native do not always return identical paths on Windows, particularly regarding how they handle the resolution process and their reliance on underlying system APIs [1]. The primary technical differences are: 1. Implementation Mechanism: The.native variants (fs.realpath.native and fs.realpathSync.native) bind directly to system-level realpath functions (specifically GetFinalPathNameByHandle on Windows) [1]. In contrast, the standard fs.realpath and fs.realpathSync functions implement a more complex, Node.js-specific logic that resolves paths step-by-step [1][2]. 2. Error Handling: The two methods handle non-existent paths differently. fs.realpath returns an error with the path property set to the last successfully resolved sub-path, whereas the.native versions return an error with the path property set to the entire original input path [3]. 3. Path Lengths and Normalization: While both functions generally resolve NTFS junctions and handle path normalization, they have historically diverged in edge cases [4][2]. For example, the.native functions have previously required explicit handling (such as the \?\ prefix) to correctly process long paths on Windows, whereas standard fs.realpath handled this differently [4][5]. Additionally, users have reported inconsistent behaviors regarding drive letter capitalization when resolving junctions between these methods [6]. Because these functions rely on different code paths—one being a thin wrapper around a system API and the other a higher-level Node.js implementation—they are not guaranteed to return identical results for all edge cases involving Windows-specific filesystem features like junctions, 8.3 short names, or long path handling [4][1][3].
Citations:
- 1: fs: expose realpath(3) bindings nodejs/node#15776
- 2: Windows Container: fs.realpathSync() is broken on shared volumes nodejs/node#8897
- 3: fs.realpath vs fs.realpath.native differences not documented nodejs/node#45067
- 4: win: fix fs.realpath.native for long paths nodejs/node#44536
- 5: What is fs.realpath.native() for? nodejs/help#3695
- 6: On Windows, node inconsistently capitalizes drive letters when resolving junctions nodejs/node#38459
🏁 Script executed:
# First, find the file and explore its structure
ast-grep outline src/main/core/tools/workspace-tools.ts --view expanded | head -100Repository: pdw96/fleet
Length of output: 1339
🏁 Script executed:
# Search for resolveWithin definition
rg -n "resolveWithin" src/main/core/tools/workspace-tools.ts -A 15Repository: pdw96/fleet
Length of output: 3387
🏁 Script executed:
# Find and examine the resolveWithin function
find src/main/core/workspace -name "*.ts" -o -name "*.js" | head -20Repository: pdw96/fleet
Length of output: 390
🏁 Script executed:
# Look at path-guard file
cat -n src/main/core/workspace/path-guard.ts | head -150Repository: pdw96/fleet
Length of output: 3563
Unify the root realpath source used by grep.
rootReal currently comes from fs.realpath(root) (async, non-native), while start — and therefore every file yielded by walk(start) — derives from fs.realpathSync.native(root) inside resolveWithin. On Windows, these two implementations can produce divergent results for junctions, 8.3 short names, and drive letter capitalization. If the prefixes differ, path.relative(rootReal, file) at line 178 won't strip the root cleanly, corrupting the displayed rel and breaking the SENSITIVE_FILE.test(rel) exclusion at line 180.
♻️ Suggested fix — derive rootReal from the same helper
- const rootReal = await fs.realpath(root)
- const start = resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.')
+ const rootReal = resolveWithin(root, '.')
+ const start = resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rootReal = await fs.realpath(root) | |
| const start = await resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.') | |
| const start = resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.') | |
| const rootReal = resolveWithin(root, '.') | |
| const start = resolveWithin(root, asStr((input as { path?: unknown })?.path) ?? '.') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/core/tools/workspace-tools.ts` around lines 167 - 168, The rootReal
variable is derived from fs.realpath(root) which uses an async non-native
implementation, while the start variable obtained from resolveWithin uses
fs.realpathSync.native internally. This inconsistency causes problems on Windows
with junctions and short names. To fix this, ensure rootReal is derived from the
same root resolution source used by resolveWithin. Either modify resolveWithin
to return or expose the resolved root path it computes, or refactor to use the
same native sync realpath method for computing rootReal that resolveWithin uses
internally, so that path.relative(rootReal, file) at line 178 works correctly
and SENSITIVE_FILE.test(rel) exclusion at line 180 functions as intended.
…eaf/ancestor 추종 차단·테스트 fixture 분리 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — P1×3 + P2×1 수정 반영( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9448e791bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // [#128-B2 P1] listIgnored 가 링크를 skipped{symlink} 로 우회시키므로 민감-명 링크가 아래 | ||
| // sensitive→throw 분기에 도달하지 못한다. 여기서 fail-closed(민감 경로 백업 불가 → hard-stop). | ||
| for (const s of enumSkipped) { | ||
| if (s.reason === 'symlink' && policy.sensitiveRe.test(s.path)) { |
There was a problem hiding this comment.
When git reports an ignored directory symlink such as .ssh/, listIgnored strips the slash before recording the symlink skip, and this check tests only .ssh. With the default SENSITIVE_FILE, .ssh/ is sensitive but .ssh is not, so a sensitive .ssh symlink/junction present at capture time is only added to baseline.skipped instead of triggering the intended fail-closed throw, leaving an unbacked sensitive link in scope. Consider preserving/testing the directory form (for example s.path and ${s.path}/) for symlink skips.
Useful? React with 👍 / 👎.
…형 테스트)
listIgnored top-level 루프가 '.ssh/' → '.ssh'(trailing slash 제거)로 skipped에 push.
SENSITIVE_FILE .ssh 절은 `(^|[/\])\.ssh[/\]` — trailing slash 필수이므로
s.path='.ssh' 단독 테스트 시 false → fail-open 취약.
captureIgnoredBaseline fail-closed 루프에서 s.path 와 `${s.path}/` 양형을 OR 테스트하도록 수정.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — 재리뷰 P1(디렉터리 symlink) 반영( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f726d93fdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| // 존재하지 않거나 접근 불가 → walk 로 넘겨 fail-closed 처리 | ||
| walk(dir) |
There was a problem hiding this comment.
When a git-ignored directory entry is a reparse point or other path that lstatSync cannot classify, this catch falls back to walk(dir), whose first step is readdirSync(resolve(root, relDir)). In environments where the OS can still open that directory target, capture will follow the path the link guard just failed to classify and can enumerate/read outside-workspace ignored contents instead of failing closed; non-ENOENT lstat failures should be skipped/escalated rather than walked.
Useful? React with 👍 / 👎.
| } catch (e) { | ||
| if ((e as NodeJS.ErrnoException).code === 'ENOENT') continue // 진짜 부재 | ||
| continue // 기타(race 등) fail-safe(advisory) |
There was a problem hiding this comment.
If an existing ancestor of a baseline file is a reparse point or other path where lstatSync fails for a non-ENOENT reason, this catch continues and restore then proceeds to mkdirSync(dirname(abs))/writeFileSync(abs, ...) through that ancestor. In those environments the restore path can follow the unclassified ancestor outside the workspace instead of removing or refusing it, so non-ENOENT lstat failures here should abort or otherwise prevent the subsequent write.
Useful? React with 👍 / 👎.
| continue | ||
| } | ||
| if (fileSt.isSymbolicLink()) { | ||
| skipped.push({ path: rel, reason: 'symlink' }) |
There was a problem hiding this comment.
Recording a baseline ignored symlink only as skipped makes later code treat that path as protected by path alone. If the agent replaces this symlink with a regular ignored file, collectIgnoredChanges skips it because skippedPaths contains the path, and restoreIgnoredBaseline also skips deleting it, so a rejected/failed task can leave the agent-created ignored file behind instead of rolling it back. The skipped entry needs enough type/state checking to distinguish the original symlink from a new regular file at the same path.
Useful? React with 👍 / 👎.
…감 skip 광역화·baseline symlink 치환 롤백 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — round-3 P1 4건 반영( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 558ca83fd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (baseline.entries.has(path) || (skippedPaths.has(path) && !baselineSymlinkPaths.has(path))) | ||
| continue | ||
| removeCreated(path) |
There was a problem hiding this comment.
When an ignored baseline symlink/junction points at a directory and the agent replaces it with a real ignored directory, git status --ignored reports child paths like link/file (or nothing if the replacement is empty), not the exact link entry stored in baselineSymlinkPaths. This loop therefore calls removeCreated only for the children and leaves the agent-created directory in place, so a rejected task can leave a regular directory shadowing a pre-existing skipped link. Consider checking each baseline symlink path after the created pass and deleting it when it now lstat's as a non-symlink.
Useful? React with 👍 / 👎.
| } | ||
| // [#128-B2] 파일 + 비정형(FIFO/socket/device) → pushFile. capture/collect 의 lstat 가 | ||
| // regular 면 백업, 아니면 'not-regular' 로 표면화한다(B1 동작 보존 — silent drop 금지). | ||
| pushFile(rel) |
There was a problem hiding this comment.
On filesystems that return DT_UNKNOWN for directory entries, such as some network/FUSE mounts, the Dirent type predicates can all be false even for real directories. In that environment this fallback treats an ignored directory as a file, and capture then skips it as not-regular instead of walking it, so ignored files below it are omitted from the baseline and won't be detected or restored. Please lstat the entry when none of the dirent type predicates identify it before calling pushFile.
Useful? React with 👍 / 👎.
| st = lstatSync(abs) | ||
| } catch { | ||
| return |
There was a problem hiding this comment.
When rollback sees a newly-created ignored path whose lstat fails for a non-ENOENT reason, such as an exotic reparse point or a permission/erroring filesystem entry, this catch returns silently. That lets restoreIgnoredBaseline report success while leaving the agent-created ignored entry behind; the earlier scan already treats unclassifiable paths as fail-closed, so removal should likewise ignore only true ENOENT and otherwise throw or surface the rollback failure.
Useful? React with 👍 / 👎.
| if (ent.isSymbolicLink()) { | ||
| // [#128-B2] symlink/junction 은 따라가지 않는다 — 밖을 가리켜 읽거나 재귀하지 않음. | ||
| skipped.push({ path: rel.replace(/\\/g, '/'), reason: 'symlink' }) | ||
| continue |
There was a problem hiding this comment.
If an ignored directory contains a very large number of symlinks or junctions, each one is appended to skipped without incrementing generalCount or tripping the existing maxFiles/SCAN_CAPPED guard. Because these paths are later copied into baseline/unrestorable data and surfaced in risk reasons, an ignored tree made mostly of links can still force an unbounded scan and large in-memory result despite the cap meant to bound traversal. Consider counting link skips against the same scan budget or collapsing them once the cap is reached.
Useful? React with 👍 / 👎.
…t 폴백·removeCreated fail-closed·링크 skip cap 카운트 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — round-4 P2 4건 반영( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23e067ea68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fileSt.isSymbolicLink()) { | ||
| skipped.push({ path: rel, reason: 'symlink' }) |
There was a problem hiding this comment.
When git status --ignored reports ignored symlinks as top-level records (for example many ignored root-level links, or a symlinked directory reported as link/), this branch appends them directly to skipped instead of going through pushSkipSymlink, so non-sensitive links do not consume maxFiles and SCAN_CAPPED is never emitted. That reopens the unbounded skipped growth/incomplete-scan-signal gap that the new helper is intended to close; the directory-symlink branch just above has the same bypass.
Useful? React with 👍 / 👎.
| if ((e as NodeJS.ErrnoException).code === 'ENOENT') continue | ||
| throw e | ||
| } | ||
| if (!st.isSymbolicLink()) removeCreated(p) // 여전히 symlink 면 baseline 상태=그대로 둠 |
There was a problem hiding this comment.
If an ignored symlink present in the baseline is replaced with a different symlink, this check treats the path as unchanged because lstat still reports a symlink, and collectIgnoredChanges also deduplicates current skips by path. In that scenario a rejected round can leave an agent-created symlink target in place even though the rollback path ran; the sweep needs some way to distinguish the original link from a replacement or otherwise remove changed baseline-link paths.
Useful? React with 👍 / 👎.
| const baselineSymlinkPaths = new Set( | ||
| baseline.skipped.filter((s) => s.reason === 'symlink').map((s) => s.path), | ||
| ) |
There was a problem hiding this comment.
This set includes every baseline skip recorded as reason === 'symlink', but listIgnored also uses that same reason when lstat fails and the path was never confirmed to be a symlink. If that failure is transient for a real ignored directory and lstat succeeds during restore, the new sweep will treat the pre-existing directory as a replaced baseline symlink and recursively remove it; unclassified lstat failures need a separate reason or must be excluded from this replacement sweep.
Useful? React with 👍 / 👎.
| if (baseline.entries.has(path) || (skippedPaths.has(path) && !baselineSymlinkPaths.has(path))) | ||
| continue | ||
| changes.push({ path, change: 'created', sensitive: policy.sensitiveRe.test(path) }) |
There was a problem hiding this comment.
Because baseline symlink paths are only reconsidered when they appear in the current files list, replacing a baseline symlink-to-directory with an empty ignored directory (or one containing only pruned denylist entries) produces no changes for the destructive gate or ignoredTouched. On the sequential success path no rollback runs, so that ignored replacement is silently left in the workspace; collectIgnoredChanges needs the same baseline-symlink sweep that restore now has.
Useful? React with 👍 / 👎.
…차단)·top-level skip cap 카운트·collect symlink-치환 sweep·symlink→symlink 한계 문서화 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — round-5 P2 4건 반영( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8a193ade4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| st = lstatSync(abs) | ||
| } catch (e) { | ||
| const code = (e as NodeJS.ErrnoException).code | ||
| if (code === 'ENOENT') continue // 사라짐 — 위 entries 루프가 다룸(entries 에 없으면 변경 없음) |
There was a problem hiding this comment.
When an ignored symlink/junction was present in the baseline and the agent deletes it, baselineSymlinkPaths is the only scan that can detect that path because skipped symlinks are not in baseline.entries; this ENOENT branch continues as if the baseline state is unchanged. Callers therefore get no deleted ignored change for the removed link (only the static baseline unrestorable entry), and rollback leaves the symlink absent, so the actual destructive change is not visible in the ignored-change list.
Useful? React with 👍 / 👎.
| if (generalCount >= policy.maxFiles) { | ||
| if (!capped) { | ||
| capped = true | ||
| skipped.push({ path: SCAN_CAPPED, reason: 'over-cap' }) | ||
| } | ||
| return // 비민감 링크/미분류는 cap 초과 시 생략(SCAN_CAPPED 가 불완전 신호) |
There was a problem hiding this comment.
When a second non-sensitive symlink/unclassified entry exceeds maxFiles, this branch sets capped and returns; the top-level ignored-record loop then breaks before later records are inspected. For example, with maxFiles=1 and ignored records link1/, link2/, .ssh/, the .ssh link never reaches the sensitive fail-closed check, so capture proceeds with only scan-capped instead of rejecting the sensitive link. The scanner should still process already-enumerated top-level sensitive records after cap.
Useful? React with 👍 / 👎.
| if (baseline.entries.has(path) || (skippedPaths.has(path) && !baselineSymlinkPaths.has(path))) | ||
| continue |
There was a problem hiding this comment.
When a baseline path was skipped as unclassified because lstat failed, a later ignored file at that exact path is still treated as whitelisted here because only baseline symlinks are excluded from skippedPaths. If the baseline failure was transient or ENOENT and the agent creates ghost at that path, collectIgnoredChanges reports no created change and restoreIgnoredBaseline uses the same condition to skip deletion, leaving the new ignored file behind while rollback appears successful.
Useful? React with 👍 / 👎.
… baseline symlink deleted 표면화·unclassified whitelist 근거 문서화 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LksLaP3AYvAtbcoKkatwF
|
@codex review — round-6 P2 3건 반영( |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
요약
#128-B1(PR #129) 위에 워크스페이스 무결성 advisory link-guard 하드닝을 올린다. Fleet 자체 FS 연산이 symlink/junction/reparse 를 따라가 워크스페이스 밖을 읽거나 쓰지 않도록
lstat-aware 로 전환하고, realpath containment helper 를workspace-tools와 단일화한다.배경 / 보안 동기
ignored-baseline.ts의 scan/capture/collect/restore 가 전부statSync(링크 추종)를 썼다. 워크스페이스 안에~/.ssh/id_rsa를 가리키는 ignored symlink/junction 을 심으면:readFileSync로 밖의 비밀을 백업 Buffer 에 읽어들이고,writeFileSync/rmSync로 워크스페이스 밖에 쓰거나 삭제한다.B1 의 non-regular 가드는
statSync가 링크를 추종해isFile()=true로 보고하므로 이 벡터를 못 잡았다. 본 PR 이 닫는다.변경
src/main/core/workspace/path-guard.ts(sync helper):isLinkSync(abs)—lstat기반 종류판정. POSIX symlink + Windows junction 모두'link'(실증: Node 24.16.0=Electron 42 에서 junctionlstat.isSymbolicLink()=true). exotic reparse(OneDrive/AppExecLink lstat EINVAL/UNKNOWN)→'suspicious'(fail-closed).resolveWithin(root,p)—realpathSync.native정준화 + 최근접 존재 조상 realpath + 미존재 tail 재부착 + 정확 predicate(..foo비-오거부) + win32 case-fold. lexical 비교(symlink 비해소) 무력 문제 해결.ignored-baseline.ts—statSync5지점(walk·capture·collect·ancestor·leaf restore)을 link-aware 로:walk:readdirSync(withFileTypes)+Dirent.isSymbolicLink()→ 링크 재귀·수집 차단(skipped{reason:'symlink'}). 비정형(FIFO 등)은pushFile→capture 가not-regular표면화(B1 동작 보존).lstatSync→ 링크는 B1 non-regular 분기에 흡수(sensitive→throw / else→skip). 링크 target read 안 함.clearNonDirAncestorslstat(!isDirectory()||isSymbolicLink()제거)·leaf lstat(symlink leaf 제거 후 root 안 write)·created 삭제는 lexical containment +isLinkSync(realpath 추종 금지 — 탈출 링크 unlink 막힘 방지).workspace-tools.ts— 인라인resolveWithin(부정확startsWith('..')·JS realpath) 제거 → 공유 helper.classify도 helper 경유. (sync 전환에 따라await3곳 제거.)위협모델 (advisory — 정직한 한계)
탐지·완화 모델. lstat-refusal 은 Fleet 자체 FS 경로가 링크를 안 따라가게 할 뿐, 스폰된 CLI 의 직접 쓰기는 막지 않으며 lstat→write 사이 TOCTOU 창이 남는다(순수 Node 는 크로스플랫폼
openat2/O_NOFOLLOW부재 — Windows 엔O_NOFOLLOW자체 없음). 강한 격리 = OS/CLI 샌드박스(향후 B-tier) —path-guard.tsdocblock·스펙에 명시. (참고: Codex 편집은 이미-s workspace-write로 OS 쓰기 경계 부분 적용 = 별개 층.)테스트
win32 보안 회귀required check 에서 실행(walk 비추종·created junction unlink·junction 조상·junction leaf 교체). POSIX symlink 테스트는 POSIX CI(leaf write-through·sensitive throw·같은내용 교체 distinguisher).체크포인트 (코딩 전 독립 리뷰)
@codex review→ 승인·반영 (comment)@codex review→ 승인·반영(created-삭제isLinkSync정정·walk 비정형 표면화·새 symlink rollback·collect distinguisher) (comment)비목표
OS/CLI 샌드박스 신규 배선 · denylist 내부 sensitive(B1 확정) · TOCTOU 완전차단(불가·문서화) · 적대자 보장 · 순차→worktree 전환. finding5(
workspace.ignored_discardedlive emit)는 별도 PR 로 분리(UX 표면화·OrchestratorEventTypeunion 확장 별건).Refs #128 (잔존: finding5 live-emit 별도 PR). 선행 #123-A(PR #127)·#128-B1(PR #129).
🤖 Generated with Claude Code
Summary by CodeRabbit