security: reject symlink roots before canonicalization - #227
Conversation
|
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:
📝 WalkthroughWalkthrough5개 읽기 전용 감사·복구·materialization API가 Changes바운드 읽기 루트
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The PR hardens symlink/reparse root admission, but macOS read-only audit and recovery paths can still follow a replaced caller path and produce empty or degraded evidence, weakening the intended security boundary. The required validation checks are also still pending, so this should not merge until the macOS contract and replacement regression coverage are added and all required checks pass. Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReadOnlyAPI
participant BoundReadRoot
participant Filesystem
Caller->>ReadOnlyAPI: source_root 전달
ReadOnlyAPI->>BoundReadRoot: BoundReadRoot::open(source_root)
BoundReadRoot->>Filesystem: no-follow 루트 핸들 바인딩
ReadOnlyAPI->>BoundReadRoot: 상대 경로 탐색 및 파일 열기
BoundReadRoot-->>ReadOnlyAPI: 검증된 파일 핸들
ReadOnlyAPI->>Filesystem: canonical 경로에서 active-use probe
ReadOnlyAPI->>BoundReadRoot: 완료 후 canonical_path() 재검증
BoundReadRoot-->>ReadOnlyAPI: 결과 또는 *-root-unsafe 오류
ReadOnlyAPI-->>Caller: 감사·복구·materialization 결과
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkResolution macOS를 포함한 모든 플랫폼에서 child traversal과 파일 읽기에 read_dir_names, entry_kind, open_file 등 identity-bound BoundReadRoot API만 사용하십시오. stable_path()를 child I/O 또는 외부 probe 경로로 사용하지 마십시오. 최종 canonical_path() identity 검사는 유지하고, root replacement 및 관련 보안·회귀 테스트를 다시 실행하십시오. Full details: Docstring CoverageExplanation Docstring coverage is 73.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 12 files. (1 skipped: 1 unsupported.) ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
|
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== checked-out HEAD =='
git rev-parse HEAD
printf '%s\n' '== requested commit =='
git rev-parse f4cf0024b9975af1b1c98b21c6ac1e8ec0696b28
printf '%s\n' '== changed files =='
git diff-tree --no-commit-id --name-status -r f4cf0024b9975af1b1c98b21c6ac1e8ec0696b28
printf '%s\n' '== exact diff =='
git diff --find-renames --find-copies f4cf0024b9975af1b1c98b21c6ac1e8ec0696b28^ f4cf0024b9975af1b1c98b21c6ac1e8ec0696b28 -- \
src-tauri/src/bound_read_root.rs \
src-tauri/src/duplicate_audit.rs \
src-tauri/src/multipart_archive.rs \
src-tauri/src/incomplete_download.rs \
src-tauri/src/incomplete_download_recovery.rs \
src-tauri/src/incomplete_download_materialization.rs \
src-tauri/tests/root_symlink_admission.rs \
src-tauri/tests/root_identity_binding_contract.rs
printf '%s\n' '== module outlines =='
ast-grep outline src-tauri/src/bound_read_root.rs --items all --view expanded
for f in duplicate_audit.rs multipart_archive.rs incomplete_download.rs incomplete_download_recovery.rs incomplete_download_materialization.rs; do
found="$(fd -a -t f "^${f}$" src-tauri/src | head -n 1 || true)"
if [ -n "$found" ]; then
ast-grep outline "$found" --items all --view expanded
fi
doneLength of output: 38449 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== bound root implementation =='
nl -ba src-tauri/src/bound_read_root.rs | sed -n '1,460p'
printf '%s\n' '== crate module ownership =='
nl -ba src-tauri/src/lib.rs | sed -n '1,120p'
for spec in \
'src-tauri/src/duplicate_audit.rs:400:805' \
'src-tauri/src/multipart_archive.rs:420:568' \
'src-tauri/src/incomplete_download.rs:280:952' \
'src-tauri/src/incomplete_download_recovery.rs:300:1036' \
'src-tauri/src/incomplete_download_materialization.rs:140:579' \
'src-tauri/tests/root_identity_binding_contract.rs:1:240' \
'src-tauri/tests/root_symlink_admission.rs:1:180'
do
IFS=: read -r file start end <<EOF
$spec
EOF
printf '\n== %s, lines %s-%s ==\n' "$file" "$start" "$end"
nl -ba "$file" | sed -n "${start},${end}p"
done
printf '%s\n' '== all BoundReadRoot use sites =='
rg -n -C 3 --glob '*.rs' '\b(BoundReadRoot::open|\.read_dir_names\(|\.entry_kind\(|\.open_file\(|\.canonical_path\(|\.stable_path\()' src-tauriLength of output: 1366 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_range() {
local file="$1" start="$2" end="$3"
printf '\n== %s, lines %s-%s ==\n' "$file" "$start" "$end"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d\t%s\n", NR, $0 }' "$file"
}
show_range src-tauri/src/bound_read_root.rs 1 460
show_range src-tauri/src/lib.rs 1 120
show_range src-tauri/src/duplicate_audit.rs 400 805
show_range src-tauri/src/multipart_archive.rs 420 568
show_range src-tauri/src/incomplete_download.rs 280 952
show_range src-tauri/src/incomplete_download_recovery.rs 300 1036
show_range src-tauri/src/incomplete_download_materialization.rs 140 579
show_range src-tauri/tests/root_identity_binding_contract.rs 1 240
show_range src-tauri/tests/root_symlink_admission.rs 1 180
printf '%s\n' '== all BoundReadRoot use sites =='
rg -n -C 3 --glob '*.rs' '\b(BoundReadRoot::open|\.read_dir_names\(|\.entry_kind\(|\.open_file\(|\.canonical_path\(|\.stable_path\()' src-tauriLength of output: 170708
Root replacement can still redirect macOS child I/O before the final fail-closed check.
On macOS, an attacker can rename the admitted root and replace the original pathname after Migrate these consumers to You are interacting with an AI system. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
opencode-agent
Bot
added
area: security
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Pull request was converted to draft
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Purpose
Fix #226 by rejecting caller-supplied symlink/reparse roots before canonicalization and preserving an opened no-follow root identity for read-only audit/recovery/materialization boundaries.
Exact current state
457c9ebb2a69a6b234ef9ce8d882fcc3355892ec.main:7eb131c4b4ac628226b262d6406a663fc7dc632f.32727270416, Release32727270516, Security Scan32727270533, and SAST Semgrep32727270436are queued; none is passing evidence yet.Test-first security lineage
The branch retains the existing fail-closed symlink-root diagnostics at the public read-only boundaries and now restores the Unix descriptor-relative root primitive that had regressed out of the current head:
read_dir_names()enumerates through a duplicated directory descriptor andfdopendir/readdir;entry_kind()walks components withopenat(O_DIRECTORY|O_NOFOLLOW)and inspects the leaf withfstatat(..., AT_SYMLINK_NOFOLLOW);open_file()opens the leaf relative to the bound descriptor withopenat(..., O_NOFOLLOW|O_NONBLOCK);F_GETPATHonly for legacy/display-compatiblestable_path()exposure, not as the only available child-I/O primitive.A new executable Unix regression,
bound_read_root_descriptor_replacement.rs, opens a real root, renames it, installs a replacement directory at the caller pathname, and proves descriptor-relative enumeration/type inspection/file reads still return the original object while finalcanonical_path()publication authority fails closed.The RED commit was
2081f7784e96e91dc7eec7b7e7ac39266dd384de; the narrow primitive repair isa3dc596e5229e469dab87e3588edc61598e67cec.Compatibility-path note
Not every consumer in this PR has been rewritten to descriptor-relative child I/O.
duplicate_auditand some incomplete-download compatibility paths still operate through the identity-checkedstable_path()namespace and final caller-path revalidation. Those read-only paths do not gain mutation authority, but this PR no longer claims that every child access is descriptor-relative. The restored primitive and replacement regression prevent the stronger Unix authority boundary from silently disappearing again and allow remaining consumers to migrate without reintroducing an unsafe root primitive.Scope
Production/security changes remain bounded to root admission and read-only root-object binding. No delete, rename, extraction, recovery execution, upload, discard, or broader mutation authority is added. Existing descendant no-follow checks, lineage/fingerprint validation, coverage, security, and release gates remain fail-closed.
Required before merge
Keep Draft until an unchanged exact head has every applicable required native/central workflow passing, current valid findings are addressed, live-base ancestry remains current, any required independent review is satisfied, effective branch/ruleset enforcement is restored, and repository-wide exact 100% owned-production statement/region-equivalent, branch, function, and line coverage is proven without exclusions or threshold weakening. Pending, queued, skipped-required, cancelled, absent, failed, stale, predecessor, synthetic, diagnostic-only, status-only, model-only, author-only, or no-source-scanner evidence is non-passing.
Summary by CodeRabbit
보안 및 안정성 개선
테스트