Skip to content

test(security): skip symlink-escape tests when Windows lacks privilege - #7432

Open
iam-kira wants to merge 1 commit into
crewAIInc:mainfrom
iam-kira:test/skip-symlink-tests-without-privilege
Open

iam-kira wants to merge 1 commit into
crewAIInc:mainfrom
iam-kira:test/skip-symlink-tests-without-privilege

Conversation

@iam-kira

Copy link
Copy Markdown

What

Two path-containment security tests in lib/crewai-tools create a symlink in
setup with no guard, so they error out on Windows for any user who is not
elevated and does not have Developer Mode on:

OSError: [WinError 1314] A required privilege is not held by the client
  • tests/utilities/test_safe_path.pytest_rejects_symlink_escape
  • tests/tools/test_file_writer_tool.pytest_blocks_symlink_escape

These are exactly the tests that guard escape-via-symlink rejection (the
behaviour hardened in #6248 / #6249), so today an ordinary Windows contributor
can't run the suite that protects that path — it goes red in setup.

Fix

Wrap each os.symlink() call: if it fails with WinError 1314, pytest.skip
that one test; re-raise any other OSError so a genuine failure never turns
into a silent skip. Where symlinks work — every current CI job, and any elevated
or Developer-Mode machine — behaviour is unchanged and the escape assertions
still run. The file-writer test also cleans up its mkdtemp() dir on the skip
path so nothing leaks.

Guard is inlined at the two call sites (13 lines total) rather than adding a
shared helper, to keep it a two-file, dependency-free diff — happy to factor it
into a shared helper if you'd prefer.

Why CI doesn't catch it

The workflow matrix runs ubuntu-latest and macos-*; no job runs the suite on
Windows. GitHub's Windows runners can create symlinks anyway, so even adding a
Windows job wouldn't surface this — the gap is the privilege, not just the OS.

Product code is unaffected

This is tests-only. validate_file_path (safe_path.py) uses os.path.realpath()

  • a prefix check, which resolves junctions as well as symlinks on Windows, so
    containment itself is fine.

Notes

Fixes #7431

This PR was authored with AI assistance (Claude Code). Per CONTRIBUTING,
please apply the llm-generated label — I don't have permission to add it
myself.

Verified locally by py_compile and code inspection; I don't have an
unprivileged-Windows box wired into CI to execute the skip path, so a maintainer
confirming the skip fires there (and still runs everywhere else) would be
welcome.

🤖 Generated with Claude Code

os.symlink raises WinError 1314 on unprivileged Windows, so these two escape-rejection tests error in setup. Skip only that case; re-raise other OSErrors so real failures aren't masked.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fc7c9c12-8aa1-4100-99bf-26c2c98d131c

📥 Commits

Reviewing files that changed from the base of the PR and between 894898f and edd3ccb.

📒 Files selected for processing (2)
  • lib/crewai-tools/tests/tools/test_file_writer_tool.py
  • lib/crewai-tools/tests/utilities/test_safe_path.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Two symlink escape tests now skip on Windows when symlink creation fails with privilege error 1314. Other symlink creation errors still fail the tests.

Changes

Windows symlink test handling

Layer / File(s) Summary
Symlink setup handling
lib/crewai-tools/tests/tools/test_file_writer_tool.py, lib/crewai-tools/tests/utilities/test_safe_path.py
The tests catch OSError during symlink creation. They skip Windows error 1314 and re-raise other errors. The file writer test also cleans up its temporary directory before skipping.

Suggested reviewers: thecybertech

Priority: ⬇️ Low

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to edd3c

The security tests now skip only where Windows lacks symlink privileges and continue validating supported environments, with no product-code risk introduced.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the Windows privilege-error issue, the test-only fix, preserved error handling, cleanup behavior, verification performed, linked issue, and additional context. It uses differe…
Title check ✅ Passed The title clearly and concisely describes the main change: skipping symlink-escape tests when Windows lacks the required privilege.
Linked Issues check ✅ Passed PR #7431 requires both symlink security tests to skip only for Windows winerror 1314, re-raise other OSError values, and continue the security assertions when symlink creation succeeds. The diff a…
Out of Scope Changes check ✅ Passed The diff changes only the two test setup paths named in issue #7431. The changes provide the required privilege-specific skip, failure preservation, and temporary-directory cleanup. No unrelated produ…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Two path-containment security tests fail on Windows without symlink privilege

1 participant