fix(security): validate file_id and partition names to block Milvus filter injection

[EnjoyBacon7]

Issue (High)

file_id only forbade / (FORBIDDEN_CHARS_IN_FILE_ID = set("/")) and partition names were not validated at all. Both are interpolated by f-string into Milvus filter expressions:

filter=f'partition == "{partition}" and file_id == "{file_id}"'

A file_id such as x" or partition == "other (no /, previously valid) breaks out of the quoted literal and injects boolean logic into delete/query/relationship-expansion filters — reading or deleting chunks outside the intended partition. A maliciously named partition gives the same primitive.

Fix

• Restrict file_id and partition names to a safe identifier allowlist [A-Za-z0-9._:-].
• Enforce the partition allowlist in ensure_partition_role (covers every role-gated partition operation) and in create_partition.
• Apply the existing validate_file_id dependency to the delete_file, get_file, get_file_ancestors, and search_file endpoints, which previously accepted a raw file_id path param.

Notes

Defense-in-depth follow-up: convert the remaining f-string Milvus filters to parameterized expressions (filter_params=), the pattern already used by list_all_chunk / get_file_chunk_ids. Validation here closes the injection; parameterization would make it structurally impossible.

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Strengthened input validation for file identifiers and partition names across API endpoints.
  • Invalid identifiers are now rejected with HTTP 400 errors and descriptive messages.
  • Validation uses an allowlist of permitted characters to ensure safer operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces a shared allowlist-based regex (_VALID_IDENTIFIER_RE) in openrag/routers/utils.py to validate file IDs and partition names used in Milvus filter expressions. Updates validate_file_id and is_file_id_valid to use this regex, adds assert_valid_partition_name, and wires both validators into the delete_file, get_file, get_file_ancestors, search_file, and create_partition endpoints via FastAPI Depends or direct assertions.

Changes

Allowlist Identifier Validation

Layer / File(s)	Summary
Allowlist regex and validator functions openrag/routers/utils.py	Adds re import and _VALID_IDENTIFIER_RE allowlist regex; reworks is_file_id_valid and validate_file_id to use the allowlist with empty-check-first ordering and updated HTTP 400 error detail; adds assert_valid_partition_name helper; updates ensure_partition_role to call assert_valid_partition_name before role checks.
Router endpoint wiring openrag/routers/indexer.py, openrag/routers/partition.py, openrag/routers/search.py	Imports validate_file_id and assert_valid_partition_name in relevant routers; replaces bare file_id: str with file_id: str = Depends(validate_file_id) in delete_file, get_file, get_file_ancestors, and search_file; adds assert_valid_partition_name(partition) call at the start of create_partition.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A sneaky string tried to slip right through,
But the regex said, "Only safe chars, it's true!"
With allowlists set and 400s in store,
No injection shall pass through my warren's front door.
Hoppity-hop, the IDs are now pure! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: implementing validation for file_id and partition names to prevent Milvus filter injection attacks.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch security/id-injection-validation

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (1)

openrag/routers/indexer.py (1)

401-402: ⚠️ Potential issue | 🟠 Major

source_file_id bypasses validation and reaches Milvus filter expressions, creating an injection vulnerability.

The source_file_id Form parameter is passed directly to indexer.copy_file.remote() without validation. The copy operation calls vectordb.get_file_chunks(file_id, partition), which constructs a Milvus filter expression at line 938 of vectordb.py:

filter_expr = f'partition == "{partition}" and file_id == "{file_id}"'

Since source_file_id is unvalidated, it can reach this f-string filter and be exploited for injection attacks. The destination file_id parameter on line 399 correctly uses Depends(validate_file_id), but source_file_id lacks the same validation.

Apply the validation pattern used for the destination file_id:

 async def copy_file_between_partitions(
     partition: str,
     file_id: str = Depends(validate_file_id),
     metadata: Any | None = Depends(validate_metadata),
     source_partition: str = Form(...),
-    source_file_id: str = Form(...),
+    source_file_id: str = Depends(validate_file_id),
     indexer=Depends(get_indexer),

🤖 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 `@openrag/routers/indexer.py` around lines 401 - 402, The `source_file_id`
parameter lacks validation and is passed directly to the copy operation,
eventually reaching an unvalidated Milvus filter expression that constructs an
f-string, creating an injection vulnerability. Apply the same validation pattern
used for the destination file_id parameter by adding `Depends(validate_file_id)`
to the `source_file_id` Form parameter definition, ensuring the input is
properly validated before reaching the filter expression construction in
vectordb operations.

🧹 Nitpick comments (1)

openrag/routers/indexer.py (1)

43-43: 💤 Low value

Dead code: FORBIDDEN_CHARS_IN_FILE_ID appears unused.

This constant was part of the previous validation approach that only forbade specific characters. Now that validation uses the allowlist regex in utils.py, this constant is no longer referenced.

🧹 Suggested removal

-FORBIDDEN_CHARS_IN_FILE_ID = set("/")  # set('"<>#%{}|\\^`[]')

🤖 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 `@openrag/routers/indexer.py` at line 43, Remove the unused constant
FORBIDDEN_CHARS_IN_FILE_ID from openrag/routers/indexer.py. This constant
represents the old blacklist-based validation approach and is no longer
referenced now that validation has been refactored to use the allowlist regex
pattern in utils.py. Simply delete the line containing this constant definition.

🤖 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.

Outside diff comments:
In `@openrag/routers/indexer.py`:
- Around line 401-402: The `source_file_id` parameter lacks validation and is
passed directly to the copy operation, eventually reaching an unvalidated Milvus
filter expression that constructs an f-string, creating an injection
vulnerability. Apply the same validation pattern used for the destination
file_id parameter by adding `Depends(validate_file_id)` to the `source_file_id`
Form parameter definition, ensuring the input is properly validated before
reaching the filter expression construction in vectordb operations.

---

Nitpick comments:
In `@openrag/routers/indexer.py`:
- Line 43: Remove the unused constant FORBIDDEN_CHARS_IN_FILE_ID from
openrag/routers/indexer.py. This constant represents the old blacklist-based
validation approach and is no longer referenced now that validation has been
refactored to use the allowlist regex pattern in utils.py. Simply delete the
line containing this constant definition.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d1b7b39f-aef8-4c3d-800f-41cfb4306256

📥 Commits

Reviewing files that changed from the base of the PR and between bea69b0 and ddfe748.

📒 Files selected for processing (4)

• openrag/routers/indexer.py
• openrag/routers/partition.py
• openrag/routers/search.py
• openrag/routers/utils.py