Skip to content

fix(security): resolve symlink write-guard bypass on Python 3.10#1256

Merged
kovtcharov-amd merged 1 commit into
mainfrom
fix/symlink-security-3.10
May 29, 2026
Merged

fix(security): resolve symlink write-guard bypass on Python 3.10#1256
kovtcharov-amd merged 1 commit into
mainfrom
fix/symlink-security-3.10

Conversation

@kovtcharov-amd
Copy link
Copy Markdown
Collaborator

Symlinks pointing into blocked directories (.ssh, C:\Windows, /etc, …) were not detected by is_write_blocked() on Python <3.12 — the write guardrail returned False when it should have returned True. The Python 3.10/3.11 version matrix (#1247) surfaced this via test_symlink_to_blocked_directory_is_blocked. On 3.12+ the test passed by accident because Path.resolve() internally delegates to os.path.realpath; on older versions it uses a separate resolver that can disagree after symlink traversal.

The fix drops the redundant .resolve() call so os.path.realpath is the single source of truth for symlink resolution across all supported Python versions.

Test plan

  • python -m pytest tests/unit/test_security_edge_cases.py tests/unit/test_file_write_guardrails.py -xvs — all 132 pass, 6 skipped (platform-specific)
  • CI matrix passes on Python 3.10, 3.11, 3.12, 3.13

`is_write_blocked()` chained `Path(os.path.realpath(path)).resolve()`, but
on Python <3.12 `Path.resolve()` uses a separate code path that can disagree
with `os.path.realpath()` — symlinks pointing into blocked directories were
not detected as blocked.

Drop the redundant `.resolve()` call so `os.path.realpath` is the single
source of truth for symlink resolution across all supported Python versions.
@github-actions github-actions Bot added tests Test changes security Security-sensitive changes labels May 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Summary

This is a correct and minimal security fix. Chaining Path(os.path.realpath(path)).resolve() was idempotent on Python 3.12+ (where resolve() internally delegates to realpath) but not on Python 3.10/3.11, where resolve() uses a separate resolver that can disagree after symlink traversal — allowing symlinks into blocked directories (.ssh, /etc, C:\Windows) to bypass is_write_blocked(). The fix makes os.path.realpath() the single, authoritative resolver for the security-critical path. The test update correctly tracks the new code path.

Issues Found

🟢 Minor — Two sibling call sites still chain .resolve() after realpath (security.py:508, security.py:575)

Both validate_write() (line 508) and create_backup() (line 575) retain the old double-resolution pattern:

real_path = Path(os.path.realpath(path)).resolve()

Neither is in the write-block decision path — is_write_blocked() is called first at line 493, before line 508 is reached, so there is no security bypass. But on Python 3.10/3.11 the exists() call and backup destination in those methods could operate on a path that differs from the realpath() result, which is confusing and fragile.

This is out of scope for this PR (which correctly targets only the security-critical path) and could be cleaned up in a follow-up without urgency.

Strengths

  • Correctly scoped. Only the security-critical code path (is_write_blocked) is changed; surrounding functions are left alone so the diff is narrow and reviewable.
  • Comment earns its lines. The 3-line comment on lines 413–415 explains a non-obvious Python version quirk that would otherwise look like an unnecessary simplification — exactly the "WHY" comment CLAUDE.md asks for.
  • Test update is accurate. Replacing the Path.resolve patch (a dead code path post-fix) with an os.path.normpath patch correctly targets the first fallible operation that remains in the try-block, preserving coverage of the except branch.

Verdict

Approve. The fix is correct, minimal, and well-tested. The minor note above is a cleanup opportunity for a follow-up, not a blocking concern.

@kovtcharov-amd kovtcharov-amd added bug Something isn't working p0 high priority labels May 29, 2026
@kovtcharov-amd kovtcharov-amd self-assigned this May 29, 2026
@kovtcharov-amd kovtcharov-amd enabled auto-merge May 29, 2026 09:50
Copy link
Copy Markdown
Collaborator

@itomek itomek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against PR-head security.py: is_write_blocked now resolves symlinks solely via os.path.realpath, and the fail-closed except still returns (True, ...), so the test swap from patching Path.resolve to patching os.path.normpath correctly keeps coverage of the blocked-on-error branch. One addition to the bot's sibling-site note inline. Approving.


Generated by Claude Code

Comment thread src/gaia/security.py
kovtcharov-amd pushed a commit that referenced this pull request May 29, 2026
Path.resolve() chained after os.path.realpath() re-resolves via a
different code path on Python <3.12, causing symlinks to blocked
directories to slip through. Use os.path.realpath exclusively.

Cherry-picked from fix/symlink-security-3.10 (PR #1256).
@kovtcharov-amd kovtcharov-amd added this pull request to the merge queue May 29, 2026
Merged via the queue into main with commit 10740b9 May 29, 2026
33 checks passed
@kovtcharov-amd kovtcharov-amd deleted the fix/symlink-security-3.10 branch May 29, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working p0 high priority security Security-sensitive changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants