Skip to content

fix(security): add HMAC integrity verification to PickleHandler and module allowlist for agent repository imports - #6871

Open
Varshith-Kali wants to merge 4 commits into
crewAIInc:mainfrom
Varshith-Kali:fix/security-pickle-integrity-and-module-allowlist
Open

fix(security): add HMAC integrity verification to PickleHandler and module allowlist for agent repository imports#6871
Varshith-Kali wants to merge 4 commits into
crewAIInc:mainfrom
Varshith-Kali:fix/security-pickle-integrity-and-module-allowlist

Conversation

@Varshith-Kali

Copy link
Copy Markdown

Summary

Resolves #6798

Two unsafe primitives identified in the training and agent-repository paths:

  1. PickleHandler.load()pickle.load() with no integrity check. Any actor that can write the working directory (shared CI, multi-user host) can plant a malicious pickle file that executes arbitrary code on the next trained crew kickoff.

  2. load_agent_from_repository()importlib.import_module(tool["module"]) with no allowlist. A compromised AMP endpoint or MITM can supply an arbitrary module path, achieving RCE without any local file write.

Changes

PickleHandler (file_handler.py)

  • Added HMAC-SHA256 integrity verification: a signature file (.pkl.sig) is written alongside the pickle file on every save() call
  • On load(), the signature is verified using hmac.compare_digest() before deserialization
  • Legacy files without a signature file load with a UserWarning and can be re-saved to generate one
  • The HMAC key is auto-generated (32 bytes via secrets.token_bytes) and stored in .crewai_key with 0600 permissions

Module allowlist (agent_utils.py)

  • Added _ALLOWED_TOOL_MODULES frozenset containing permitted tool module prefixes
  • load_agent_from_repository() now raises AgentRepositoryError if a tool's module is not in the allowlist
  • Currently allowed: crewai.tools, crewai_tools, crewai.tools.base_tool, crewai.tools.structured_tool, crewai.tools.tool_usage

Tests

test_file_handler.py (6 new tests)

  • test_save_creates_signature_file — verifies .sig file is created with correct size
  • test_load_tampered_file_raises_error — verifies tampered files are rejected
  • test_load_legacy_file_without_signature — verifies backward-compatible loading with warning
  • test_overwrite_preserves_signature — verifies re-saving updates the signature correctly
  • test_initialize_file_creates_valid_signature — verifies initialize_file() creates valid signatures

test_agent_utils.py (1 new test class)

  • TestModuleAllowlist — verifies blocked modules are not in allowlist, allowlist is immutable frozenset

All 11 tests pass. ruff check and ruff format are clean.

Notes

  • The allowlist is intentionally conservative. If maintainers want to support additional tool modules, they can be added to _ALLOWED_TOOL_MODULES. Users who need custom tool modules from the agent repository can override the allowlist or the maintainers can expose a configuration mechanism.
  • The HMAC key is stored per-working-directory in .crewai_key. This protects against pickle tampering but does not protect against an attacker who can also write the key file — that threat model requires OS-level file permissions or a key derived from a user-provided secret.

Per CONTRIBUTING.md, this PR was prepared with AI assistance. I reviewed every changed line, ran the full test suite locally, and verified code style with ruff and mypy. I do not have permission to apply the llm-generated label as an external contributor — could a maintainer please apply it?

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds HMAC-SHA256 protection for PickleHandler data and rejects repository tools from modules outside a CrewAI allowlist. Tests cover key security, signatures, tampering, unsigned files, overwrites, initialization, disappearing signatures, and blocked modules.

Changes

Pickle integrity protection

Layer / File(s) Summary
Pickle key and signature setup
lib/crewai/src/crewai/utilities/file_handler.py
PickleHandler loads or creates a restricted 32-byte HMAC key. It validates key ownership, symlink status, and permissions.
Signed pickle save and load
lib/crewai/src/crewai/utilities/file_handler.py, lib/crewai/tests/utilities/test_file_handler.py
save() writes HMAC-SHA256 signatures. load() verifies signatures before deserialization and raises ValueError for missing or mismatched signatures. Tests cover the updated behavior.

Tool module allowlist

Layer / File(s) Summary
Repository tool module validation
lib/crewai/src/crewai/utilities/agent_utils.py, lib/crewai/tests/utilities/test_agent_utils.py
The utility defines five permitted tool module paths in a frozenset. Repository loading rejects disallowed modules before import. Tests cover blocked modules and agents without tools.

Sequence Diagram(s)

sequenceDiagram
  participant PickleHandler
  participant PickleFile
  participant SignatureFile
  PickleHandler->>PickleFile: read serialized pickle data
  PickleHandler->>SignatureFile: read HMAC signature
  PickleHandler->>PickleHandler: verify signature
  PickleHandler->>PickleHandler: deserialize verified data
Loading

Suggested reviewers: lorenzejay

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies both security changes: HMAC integrity verification for PickleHandler and a module allowlist for agent repository imports.
Description check ✅ Passed The description directly explains the two security risks, the implemented mitigations, and the related tests.
Linked Issues check ✅ Passed The changes address both issue objectives by verifying pickle integrity before deserialization and rejecting disallowed repository tool modules before import.
Out of Scope Changes check ✅ Passed The code and test changes are limited to the two linked security objectives and their required validation coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 157-174: Update the key-loading logic in the visible
key-generation method so the HMAC key uses a protected store or explicitly
configured path outside os.getcwd(), rather than .crewai_key beside the data
files. Create the key atomically with mode 0600, and propagate an error when key
creation, persistence, or permission hardening fails instead of silently
continuing with an unprotected key.
- Around line 227-239: The file-loading path must reject unsigned pickle data
before deserialization. In
lib/crewai/src/crewai/utilities/file_handler.py#L227-L239, replace the
missing-signature warning with an integrity error and ensure pickle.load() is
never reached without a valid signature; in
lib/crewai/tests/utilities/test_file_handler.py#L54-L61, assert that integrity
error instead of an unpickling error; in
lib/crewai/tests/utilities/test_file_handler.py#L73-L88, replace automatic
legacy loading coverage with rejection-by-default coverage, leaving migration
only behind explicit operator approval if retained.

In `@lib/crewai/tests/utilities/test_agent_utils.py`:
- Around line 1349-1356: Update test_blocked_module_raises_error to construct a
minimal repository definition using "os" as the tool module, invoke
load_agent_from_repository(), and assert that it raises AgentRepositoryError.
Remove the implementation-only _ALLOWED_TOOL_MODULES assertions so the test
verifies the loader’s public security 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: df9d3e99-a328-4fdd-910f-4766a28ca816

📥 Commits

Reviewing files that changed from the base of the PR and between 92012ae and 1d1aed3.

📒 Files selected for processing (4)
  • lib/crewai/src/crewai/utilities/agent_utils.py
  • lib/crewai/src/crewai/utilities/file_handler.py
  • lib/crewai/tests/utilities/test_agent_utils.py
  • lib/crewai/tests/utilities/test_file_handler.py

Comment thread lib/crewai/src/crewai/utilities/file_handler.py Outdated
Comment thread lib/crewai/src/crewai/utilities/file_handler.py Outdated
Comment thread lib/crewai/tests/utilities/test_agent_utils.py Outdated
@Varshith-Kali

Copy link
Copy Markdown
Author

Addressed all three CodeRabbit comments in the latest commit:

  1. Key storage — HMAC key now lives in ~/.crewai/.hmac_key with mode 0600, written atomically via tempfile + rename. No longer in the working directory alongside pickle data.

  2. Fail-closed on unsigned picklesload() now raises ValueError when no signature file is found, instead of loading with a warning. pickle.load() is never reached without a verified signature. Tests updated to assert rejection.

  3. End-to-end allowlist test — Replaced frozenset introspection with a test that calls load_agent_from_repository() with "os" as the tool module and asserts AgentRepositoryError is raised. Added a positive test verifying that an agent with no tools loads successfully.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
lib/crewai/src/crewai/utilities/file_handler.py (1)

167-187: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate existing HMAC key storage before use.

Line 167 accepts any readable 32-byte key. Line 177 does not harden an existing ~/.crewai directory.

If another user can write the existing directory, that user can install a known key and sign a malicious pickle. load() will then accept and deserialize it.

Before reading the key, verify that the directory and key are owned by the current user, are not symlinks, and have restrictive modes. Fail closed or securely harden unsafe storage. Use 0700 for the directory and 0600 for the key. Add regression coverage for pre-existing insecure storage.

Based on the PR objective, the HMAC key must remain outside attacker control.

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py` around lines 167 - 187,
Harden key storage validation in the key-loading flow before accepting the
existing 32-byte key: verify the key directory and file are owned by the current
user, are not symlinks, and use directory mode 0700 and key mode 0600. If
validation fails, do not use the existing key; securely harden or recreate the
storage before generating and atomically persisting a replacement. Add
regression coverage for pre-existing insecure storage.
lib/crewai/tests/utilities/test_file_handler.py (1)

37-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Isolate the HMAC key store from the user home.

setUp() creates PickleHandler, which now reads or creates ~/.crewai/.hmac_key. tearDown() does not remove or isolate that state.

A test run can create persistent files in a developer or CI user home. It can also depend on, or replace, an existing invalid key.

Patch the home directory to a temporary directory before constructing PickleHandler. Clean up that directory after each test. This also enables direct tests for key creation and permissions.

As per coding guidelines, tests for new functionality must focus on behavior without external user-state dependencies.

🤖 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 `@lib/crewai/tests/utilities/test_file_handler.py` around lines 37 - 42, Update
the test fixture setup around PickleHandler so the home directory is redirected
to a per-test temporary directory before construction, and ensure that directory
is cleaned up during teardown. Keep key-related assertions isolated from real or
pre-existing user-home state, enabling deterministic coverage of key creation
and permissions.

Source: Coding guidelines

🧹 Nitpick comments (1)
lib/crewai/tests/utilities/test_agent_utils.py (1)

1378-1396: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Align the positive-path test with its fixture.

test_allowed_module_proceeds_past_allowlist sets "tools": []. It does not exercise an allowlisted module, module import, or tool construction. Rename the test and docstring to describe the no-tools case, or add a real allowlisted-tool fixture with a patched constructor.

Suggested rename
-    def test_allowed_module_proceeds_past_allowlist(self):
-        """A tool referencing an allowlisted module should not trigger the allowlist rejection."""
+    def test_agent_without_tools_loads_successfully(self):
+        """An agent with no tools should load without tool-module validation."""

As per coding guidelines, unit tests for new functionality should focus on the behavior under test.

🤖 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 `@lib/crewai/tests/utilities/test_agent_utils.py` around lines 1378 - 1396,
Align test_allowed_module_proceeds_past_allowlist with its fixture by either
renaming the test and docstring to describe loading an agent with no tools, or
replacing the empty tools list with a real allowlisted-tool fixture and patching
its constructor so the allowlist path is exercised.

Source: Coding guidelines

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 182-187: Update the key initialization flow around os.replace() to
prevent concurrent processes from overwriting an existing key: use
synchronization or atomic no-clobber creation, and when another process wins,
reload the installed key instead of retaining the obsolete in-memory key. Add a
multi-process regression test verifying both processes use the same persisted
key and data remains verifiable after restart.

---

Outside diff comments:
In `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 167-187: Harden key storage validation in the key-loading flow
before accepting the existing 32-byte key: verify the key directory and file are
owned by the current user, are not symlinks, and use directory mode 0700 and key
mode 0600. If validation fails, do not use the existing key; securely harden or
recreate the storage before generating and atomically persisting a replacement.
Add regression coverage for pre-existing insecure storage.

In `@lib/crewai/tests/utilities/test_file_handler.py`:
- Around line 37-42: Update the test fixture setup around PickleHandler so the
home directory is redirected to a per-test temporary directory before
construction, and ensure that directory is cleaned up during teardown. Keep
key-related assertions isolated from real or pre-existing user-home state,
enabling deterministic coverage of key creation and permissions.

---

Nitpick comments:
In `@lib/crewai/tests/utilities/test_agent_utils.py`:
- Around line 1378-1396: Align test_allowed_module_proceeds_past_allowlist with
its fixture by either renaming the test and docstring to describe loading an
agent with no tools, or replacing the empty tools list with a real
allowlisted-tool fixture and patching its constructor so the allowlist path is
exercised.
🪄 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: ce8cae30-fa7d-4f97-a9e0-ee9c55448f8e

📥 Commits

Reviewing files that changed from the base of the PR and between 1d1aed3 and 4f4a848.

📒 Files selected for processing (4)
  • lib/crewai/src/crewai/utilities/agent_utils.py
  • lib/crewai/src/crewai/utilities/file_handler.py
  • lib/crewai/tests/utilities/test_agent_utils.py
  • lib/crewai/tests/utilities/test_file_handler.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/crewai/src/crewai/utilities/agent_utils.py

Comment thread lib/crewai/src/crewai/utilities/file_handler.py Outdated
…odule allowlist for agent repository imports

PickleHandler.load() now verifies an HMAC-SHA256 signature before
deserializing pickle files, preventing arbitrary code execution via
tampered training data. Legacy files without signatures load with a
warning and can be re-saved to generate one.

load_agent_from_repository() now checks tool module names against an
allowlist before calling importlib.import_module(), preventing RCE via
compromised AMP endpoints that supply arbitrary module paths.
…st loader behavior

- Store HMAC key in ~/.crewai/.hmac_key with 0600 perms instead of
  the working directory, keeping it separate from pickle data files
- Write key atomically via tempfile + rename to avoid partial writes
- Reject pickle files without a valid signature instead of loading
  with a warning, preventing deserialization of untrusted data
- Replace frozenset assertions with end-to-end test that calls
  load_agent_from_repository with a blocked module and verifies
  AgentRepositoryError is raised
- Validate key dir/file ownership, reject symlinks, enforce mode 0700/0600
- Use O_CREAT|O_EXCL for atomic no-clobber key creation across concurrent processes
- Patch home directory to temp dir in test fixtures to avoid polluting ~/.crewai
- Rename test_allowed_module_proceeds_past_allowlist to test_agent_without_tools_loads_successfully
@Varshith-Kali
Varshith-Kali force-pushed the fix/security-pickle-integrity-and-module-allowlist branch from 4f4a848 to c1443aa Compare August 9, 2026 05:43
@Varshith-Kali

Copy link
Copy Markdown
Author

Addressed all CodeRabbit round 2 feedback in the latest commit:

  1. Concurrent key replacement — Replaced \ empfile.mkstemp\ + \os.replace\ with \os.open(O_CREAT | O_EXCL)\ for atomic no-clobber creation. If another process wins the race (\FileExistsError), the installed key is validated and loaded instead of using the in-memory copy.

  2. Key storage hardening — Added _validate_key_storage()\ that checks directory and file ownership (must match current UID), rejects symlinks, and enforces mode 0700 (dir) / 0600 (key). Raises \PermissionError\ if any check fails — fails closed, never silently uses insecure storage.

  3. Test isolation — \setUp\ now patches \os.path.expanduser\ to a per-test temp directory created via \ empfile.mkdtemp(). \ earDown\ cleans it up with \shutil.rmtree. Tests no longer create ~/.crewai/.hmac_key\ on developer or CI machines.

  4. Test naming — Renamed \ est_allowed_module_proceeds_past_allowlist\ → \ est_agent_without_tools_loads_successfully\ with matching docstring, since the test uses an empty tools list rather than an allowlisted module.

Also rebased onto latest \main\ (v1.15.14).

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 201-206: Update the key persistence logic in the surrounding
key-generation method: ensure all bytes in key are written despite short
os.write results, flush the file descriptor before returning, and propagate an
error when writing or flushing fails. Preserve the existing os.close cleanup and
only return key after complete persistence succeeds.
- Around line 293-313: Update the load() exception handling around the signature
read and primary pickle read so a FileNotFoundError for self._sig_path is
converted to the existing integrity-check ValueError instead of returning {}.
Preserve {} only when the primary file is absent before loading begins, and add
a regression test covering the signature disappearing between existence check
and open.
- Around line 198-206: Update the key-loading flow around the invalid-key
fallback and the `os.open` call so an existing key with an invalid length raises
an error instead of using `O_TRUNC` to replace it. Only create a key when the
key file is absent; preserve the existing valid-key path and require explicit
recovery or rotation for invalid keys.
- Around line 182-206: Validate key_dir using non-following metadata before
first-time creation, rejecting symlinks, foreign ownership, and
group/world-accessible permissions before proceeding; after writing, validate
the completed directory and key file before returning from the key-generation
flow in the key-storage method. Add corresponding rejection tests in
lib/crewai/tests/utilities/test_file_handler.py lines 11-37 for existing
symlinked, foreign-owned, and group/world-accessible directories.
🪄 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: 66725da8-b172-4e40-95f2-83abc9cbd163

📥 Commits

Reviewing files that changed from the base of the PR and between f7ba8e3 and c1443aa.

📒 Files selected for processing (4)
  • lib/crewai/src/crewai/utilities/agent_utils.py
  • lib/crewai/src/crewai/utilities/file_handler.py
  • lib/crewai/tests/utilities/test_agent_utils.py
  • lib/crewai/tests/utilities/test_file_handler.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/crewai/src/crewai/utilities/agent_utils.py
  • lib/crewai/tests/utilities/test_agent_utils.py

Comment thread lib/crewai/src/crewai/utilities/file_handler.py
Comment thread lib/crewai/src/crewai/utilities/file_handler.py Outdated
Comment thread lib/crewai/src/crewai/utilities/file_handler.py
Comment on lines 293 to 313
if not os.path.exists(self._sig_path):
raise ValueError(
f"Integrity check failed for {self.file_path}: "
"no signature file found. Re-save the data to generate one."
)

with open(self._sig_path, "rb") as f:
stored_sig = f.read()

expected_sig = hmac.new(self._key, payload, hashlib.sha256).digest()

if not hmac.compare_digest(stored_sig, expected_sig):
raise ValueError(
f"Integrity check failed for {self.file_path}: "
"signature mismatch - file may have been tampered with"
)

import io

return pickle.load(io.BytesIO(payload)) # noqa: S301
except (FileNotFoundError, EOFError):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject a signature that disappears during loading.

If the signature exists at Line 293 but is removed before Line 299 opens it, FileNotFoundError reaches Line 313 and load() returns {}. This accepts an unsigned-file condition instead of reporting the required integrity failure.

Handle a missing signature file as ValueError. Only return {} when the primary pickle file is absent before loading starts. Add a regression test for this race.

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 311-311: pickle.load/loads executes arbitrary code when the data is untrusted (a model file, cache, or request payload). Use a safe format like JSON, or only unpickle data from a trusted, integrity-checked source.
Context: pickle.load(io.BytesIO(payload))
Note: [CWE-502] Deserialization of Untrusted Data.

(pickle-deserialization-python)


[warning] 298-298: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self._sig_path, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)

🪛 OpenGrep (1.26.0)

[ERROR] 312-312: pickle.load/loads deserializes arbitrary Python objects and can execute arbitrary code. Use a safe format like JSON instead.

(coderabbit.deserialization.python-pickle)

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py` around lines 293 - 313,
Update the load() exception handling around the signature read and primary
pickle read so a FileNotFoundError for self._sig_path is converted to the
existing integrity-check ValueError instead of returning {}. Preserve {} only
when the primary file is absent before loading begins, and add a regression test
covering the signature disappearing between existence check and open.

…TOU race

- Validate key directory before first-time creation, not just when key exists
- Raise ValueError for invalid-length key instead of silently replacing it
- Handle short writes with full-write loop and fsync before return
- Catch FileNotFoundError on signature read as integrity error, not empty return
- Add regression test for signature disappearing during load
@Varshith-Kali

Copy link
Copy Markdown
Author

Addressed all CodeRabbit round 3 feedback in commit 3f4968a:

  1. Validate key directory before first-time creation — When the key file is absent but the directory already exists, _validate_key_storage() now runs on the directory before creating a key. The method also handles the case where the key file doesn't exist yet (skips file-level checks, validates directory only). Uses os.makedirs(exist_ok=False) for new directories.

  2. Don't replace existing invalid key — An existing key with invalid length now raises ValueError with instructions to remove the file or restore from backup. The O_TRUNC fallback path is removed entirely. The concurrent-creation path also raises ValueError if the winner's key is invalid.

  3. Short write safety — Replaced bare os.write(fd, key) with a write loop (while offset < len(key): offset += os.write(fd, key[offset:])) followed by os.fsync(fd). The key is only returned after complete persistence is confirmed.

  4. Signature TOCTOU race — Separated the FileNotFoundError handler for the signature file read from the general exception flow. If the signature file disappears between the existence check and open(), it now raises ValueError ("signature file disappeared during loading") instead of returning {}. Removed the broad except (FileNotFoundError, EOFError): return {} that was masking integrity failures. Added test_load_rejects_disappearing_signature regression test.

All 10 tests pass. Ruff clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/utilities/file_handler.py (1)

295-299: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject symlink writes for the pickle and signature outputs.

PickleHandler saves to paths under the current directory, and open(path, "wb") follows symlinks. If <pickle>.pkl.sig is a symlink, this with open(self._sig_path, "wb") can overwrite the symlink target or create a non-signature file. store_lock() does not prevent a non-cooperating actor from adding or swapping the symlink, so the pickle output should use the same no-follow no-clobber protection.

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py` around lines 295 - 299,
Update PickleHandler’s pickle and signature write paths to reject symlinks and
avoid clobbering existing files, using no-follow, exclusive creation semantics
for both outputs. Apply the same protection to the pickle write and the
signature write around self._sig_path, while preserving the existing payload and
HMAC generation flow.
🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 326-333: Update the test_load_rejects_disappearing_signature setup
so its patched os.path.exists returns True after deleting the signature,
allowing the subsequent open(self._sig_path, "rb") in the load path to raise
FileNotFoundError and exercise the handler’s missing-signature-during-loading
error.
- Around line 188-194: Update the key-directory initialization around
_validate_key_storage to catch FileExistsError from os.makedirs when another
process creates key_dir first, then validate the existing directory and continue
into the existing no-clobber key creation flow. Add a multi-process test that
begins with ~/.crewai absent and verifies concurrent initialization succeeds
without overwriting the key file.

---

Outside diff comments:
In `@lib/crewai/src/crewai/utilities/file_handler.py`:
- Around line 295-299: Update PickleHandler’s pickle and signature write paths
to reject symlinks and avoid clobbering existing files, using no-follow,
exclusive creation semantics for both outputs. Apply the same protection to the
pickle write and the signature write around self._sig_path, while preserving the
existing payload and HMAC generation flow.
🪄 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: b6108455-f781-4fc3-a8ef-008838eede4f

📥 Commits

Reviewing files that changed from the base of the PR and between c1443aa and 3f4968a.

📒 Files selected for processing (2)
  • lib/crewai/src/crewai/utilities/file_handler.py
  • lib/crewai/tests/utilities/test_file_handler.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/crewai/tests/utilities/test_file_handler.py

Comment on lines +188 to +194
# Validate directory before first-time key creation: os.makedirs with
# exist_ok=True does not tighten an existing insecure directory.
if os.path.exists(key_dir):
self._validate_key_storage(key_dir, key_path)
else:
os.makedirs(key_dir, mode=0o700, exist_ok=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle the first-use directory race.

If two processes initialize key storage at the same time, both can observe that key_dir is absent. One process creates it. The other process raises FileExistsError at Line 193 before the key-file race handling at Line 202 can run.

Catch this FileExistsError, validate the directory, and continue to the existing no-clobber key creation path. Add a multi-process test that starts without ~/.crewai.

Proposed fix
         if os.path.exists(key_dir):
             self._validate_key_storage(key_dir, key_path)
         else:
-            os.makedirs(key_dir, mode=0o700, exist_ok=False)
+            try:
+                os.makedirs(key_dir, mode=0o700, exist_ok=False)
+            except FileExistsError:
+                self._validate_key_storage(key_dir, key_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.

Suggested change
# Validate directory before first-time key creation: os.makedirs with
# exist_ok=True does not tighten an existing insecure directory.
if os.path.exists(key_dir):
self._validate_key_storage(key_dir, key_path)
else:
os.makedirs(key_dir, mode=0o700, exist_ok=False)
# Validate directory before first-time key creation: os.makedirs with
# exist_ok=True does not tighten an existing insecure directory.
if os.path.exists(key_dir):
self._validate_key_storage(key_dir, key_path)
else:
try:
os.makedirs(key_dir, mode=0o700, exist_ok=False)
except FileExistsError:
self._validate_key_storage(key_dir, key_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 `@lib/crewai/src/crewai/utilities/file_handler.py` around lines 188 - 194,
Update the key-directory initialization around _validate_key_storage to catch
FileExistsError from os.makedirs when another process creates key_dir first,
then validate the existing directory and continue into the existing no-clobber
key creation flow. Add a multi-process test that begins with ~/.crewai absent
and verifies concurrent initialization succeeds without overwriting the key
file.

Comment on lines 326 to +333
try:
with open(self.file_path, "rb") as file:
return pickle.load(file) # noqa: S301
except (FileNotFoundError, EOFError):
return {}
with open(self._sig_path, "rb") as f:
stored_sig = f.read()
except FileNotFoundError:
raise ValueError(
f"Integrity check failed for {self.file_path}: "
"signature file disappeared during loading."
) from None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise the disappearing-signature handler.

test_load_rejects_disappearing_signature removes the signature and returns False from its patched os.path.exists. Line 320 then raises the missing-signature error, so this FileNotFoundError handler is not tested.

Return True after deleting the signature. This forces the signature open at Line 327 and verifies the intended race behavior.

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 326-326: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self._sig_path, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)

🤖 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 `@lib/crewai/src/crewai/utilities/file_handler.py` around lines 326 - 333,
Update the test_load_rejects_disappearing_signature setup so its patched
os.path.exists returns True after deleting the signature, allowing the
subsequent open(self._sig_path, "rb") in the load path to raise
FileNotFoundError and exercise the handler’s missing-signature-during-loading
error.

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.

training data pickle.load (no integrity check) + Agent Repository importlib.import_module on remote JSON (no allowlist)

1 participant