security: reject symlinked cache cleanup roots - #169
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough카탈로그 루트를 실제 디렉터리로 검증한다. 심링크와 Windows reparse point를 거부한다. 열린 핸들로 캐시 후보와 정리 대상을 조회한다. 캐시 정리는 원자적 휴지통 작업이 없으면 실패한다. UI는 캐시를 읽기 전용으로 표시한다. Changes카탈로그 및 캐시 정리
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CleanupUI
participant Tauri
participant CacheCleanup
participant FileSystem
Operator->>CleanupUI: select developer artifacts
CleanupUI->>Tauri: cleanPaths(artifact paths)
Tauri->>CacheCleanup: clean_cache_contents(cache path)
CacheCleanup->>FileSystem: validate catalog root and filesystem identity
FileSystem-->>CacheCleanup: validation result
CacheCleanup-->>Tauri: success or fail-closed error
Tauri-->>CleanupUI: cleanup result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-tauri/src/rules.rs`:
- Around line 84-97: Replace the separate is_real_directory validation and later
path-based use with a single no-follow root-opening operation, then pass the
acquired directory handle into scanner::scan_dir_with_interval and read_dir for
scanning and enumeration. Ensure the handle cannot be redirected by concurrent
symlink replacement; on platforms where no-follow opening is unavailable, reject
the root fail-closed instead of continuing with path-based access.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fdd950e-2582-490f-81e6-409adb3b9162
📒 Files selected for processing (1)
src-tauri/src/rules.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src-tauri/src/rules.rs (2)
194-224: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff재귀 깊이 제한과 중복 메타데이터 조회를 검토하십시오.
현재 구현은 하위 디렉터리마다
CatalogRoot::open을 호출합니다.open은lstat2회와 디렉터리 열기 2회를 수행하고,stable_path는 추가로 핸들 복제와 재열기를 수행합니다. 깊은 캐시 트리에서 syscall 비용이 크게 증가합니다. 또한 재귀에 깊이 제한이 없어 매우 깊은 트리에서 스택이 고갈될 수 있습니다.각 엔트리에서
entry.file_type()대신symlink_metadata를 다시 호출하는 부분도 중복입니다. Unix에서DirEntry::file_type()은 심링크를 따라가지 않습니다. 크기 계산에는 파일에서만metadata가 필요합니다.추가로
scanner기반 계산이 제거되면서 취소·간격 제어가 사라졌습니다.cache_candidates는 IPC 스레드에서 호출되므로(src-tauri/src/commands.rs:401) 큰 캐시에서 응답이 지연될 수 있습니다. 깊이 상한 또는 취소 신호 도입을 검토하십시오.🤖 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-tauri/src/rules.rs` around lines 194 - 224, Update directory_size to recurse directly through directory entries instead of calling CatalogRoot::open and stable_path for every child, while enforcing a finite recursion-depth limit. Use each DirEntry’s file_type to skip symlinks and identify directories, fetching metadata only for regular files when adding sizes. Preserve saturating accumulation and ensure cache_candidates remains bounded or cancellable when invoked from the IPC thread.
146-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value지원되지 않는 플랫폼에서 결과가 조용히 비워집니다.
handle_namespace_path가None을 반환하는 플랫폼(예: FreeBSD)에서는stable_path가 항상 실패합니다. 그러면directory_size는 0을 반환하고child_paths는 빈 목록을 반환합니다. 반면cache_candidates의exists는true이고is_catalog_path도true입니다. 사용자에게는 "존재하지만 0바이트, 정리 대상 없음"으로 보입니다.동작 자체는 fail-closed 이므로 안전합니다. 다만 원인을 알 수 없는 상태입니다. 컴파일 타임에 미지원 플랫폼을 명시적으로 차단하거나, 최소한 한 번 경고 로그를 남기는 방식을 검토하십시오.
Also applies to: 194-196
🤖 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-tauri/src/rules.rs` around lines 146 - 149, Make unsupported targets explicit in the cfg-gated handle_namespace_path path instead of silently returning None: add a compile-time rejection for platforms outside Linux, macOS, and Windows, while preserving the existing supported-platform implementations and behavior.
🤖 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-tauri/src/rules.rs`:
- Around line 118-121: Update the Unix open_directory_handle function to open
directories with kernel-level O_NOFOLLOW and O_DIRECTORY flags, rather than
relying on Handle::from_path, so symlink swaps fail closed and non-directories
are rejected during open. Use the existing direct libc dependency if available,
and return None on unsupported platforms or flag failures.
- Around line 226-249: Revalidate the catalog root immediately before deletion
in clean_paths, rather than trusting paths produced by expandCleanTargets.
Verify the root is still the expected directory and has not become a symlink or
been replaced, or perform child deletion relative to an already validated root
handle before calling trash_delete. Preserve existing behavior only after this
validation succeeds.
---
Nitpick comments:
In `@src-tauri/src/rules.rs`:
- Around line 194-224: Update directory_size to recurse directly through
directory entries instead of calling CatalogRoot::open and stable_path for every
child, while enforcing a finite recursion-depth limit. Use each DirEntry’s
file_type to skip symlinks and identify directories, fetching metadata only for
regular files when adding sizes. Preserve saturating accumulation and ensure
cache_candidates remains bounded or cancellable when invoked from the IPC
thread.
- Around line 146-149: Make unsupported targets explicit in the cfg-gated
handle_namespace_path path instead of silently returning None: add a
compile-time rejection for platforms outside Linux, macOS, and Windows, while
preserving the existing supported-platform implementations and behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4267b9f3-9ce7-4208-aa4a-83f85d90e1e3
📒 Files selected for processing (1)
src-tauri/src/rules.rs
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head0fdfed2f31f3f3466153e905a4b6e16d159217c0. -
Head SHA:
0fdfed2f31f3f3466153e905a4b6e16d159217c0 -
Workflow run: 31483369754
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (7 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (7 files)"]
R1 --> V1["required checks"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (7 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (7 files)"]
R1 --> V1["required checks"]
|
|
@coderabbitai review Please review the unchanged exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 169 \
--repo ContextualWisdomLab/disksage \
--json number,state,isDraft,headRefOid,baseRefName,baseRefOid \
--jq '{number, state, isDraft, headRefOid, baseRefName, baseRefOid}'
gh api repos/ContextualWisdomLab/disksage/git/ref/heads/main \
--jq '.object.sha'Length of output: 668
I will review this exact range. I will not transfer findings or approval from an earlier head.
|
1 participant
Purpose
Test-first hardening for cache cleanup scope. A catalog path can currently be a symlink to an unrelated directory: lexical catalog admission succeeds,
read_dirfollows the root symlink, and cleanup enumeration can expose children outside the intended cache root. Child-symlink filtering does not protect the root itself.RED
Exact protected-main base:
7b81efd43c48439d7a4a5508f6d09ac15f141546.RED head:
9a69606d68244a78ecf78deea6437d7aa8c03744.The added Unix regressions require both catalog scope admission and child enumeration to reject a symlinked cache root. The current implementation is expected to fail those assertions because it performs lexical equality and calls
read_dirdirectly on the symlink path.Intended fix
Fail closed before enumeration: require the exact catalog root itself to be a real directory according to
symlink_metadata, reject a root symlink, and retain the existing child-symlink exclusion. No deletion authority, cache catalog breadth, or protected-path policy is broadened.Keep Draft until RED is observed, the narrow fix makes the same regressions green, unchanged exact-head Test/Release/Security/SAST evidence is acquired, and live-base/review state is revalidated.
Summary by CodeRabbit
버그 수정
변경 사항