fix(memory): escape record_ids in lancedb delete filter expression#4930
Closed
wishhyt wants to merge 1 commit intocrewAIInc:mainfrom
Closed
fix(memory): escape record_ids in lancedb delete filter expression#4930wishhyt wants to merge 1 commit intocrewAIInc:mainfrom
wishhyt wants to merge 1 commit intocrewAIInc:mainfrom
Conversation
The `delete()` method interpolated `record_ids` directly into a
LanceDB filter string without escaping single quotes, while every
other method that builds id-based filters (`touch_records`,
`get_record`, `update`) already applied `.replace("'", "''")`
correctly. This was a copy-paste omission that could allow a
crafted record_id to break out of the string literal.
Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| before = int(self._table.count_rows()) | ||
| ids_expr = ", ".join(f"'{rid}'" for rid in record_ids) | ||
| safe_ids = [str(rid).replace("'", "''") for rid in record_ids] | ||
| ids_expr = ", ".join(f"'{rid}'" for rid in safe_ids) |
There was a problem hiding this comment.
Incomplete fix: second delete path still unescaped
Medium Severity
The delete method has two code paths that build IN (...) filter expressions. The PR escapes record_ids in the first branch (line 408–409), but the second branch at line 431 builds ids_expr directly from to_delete (which contains record.id values read from the database) without applying the same .replace("'", "''") escaping. A record previously stored with a single quote in its ID would break the filter expression in this path.
Additional Locations (1)
Contributor
|
Duplicate of #4835. Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
Changes
Test plan
Note
Medium Risk
Adjusts how
record_idsare interpolated into LanceDB delete filter expressions to prevent malformed queries or injection via unescaped quotes. Low functional risk, but impacts deletion behavior for IDs containing special characters.Overview
Fixes
LanceDBStorage.delete()to escape single quotes inrecord_idsbefore constructing theid IN (...)filter expression.This aligns deletion-by-ID behavior with other methods (e.g.
touch_records,get_record,update) and prevents crafted IDs from breaking the LanceDB filter syntax.Written by Cursor Bugbot for commit 8e8fb7a. This will update automatically on new commits. Configure here.