Fix/5840 escape search wildcards#7270
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEscapes SQL/LIKE wildcard characters in transaction quick search and updates LIKE→regex translation to respect backslash-escaped Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/desktop-client/src/queries/index.ts (1)
87-87:⚠️ Potential issue | 🟠 MajorIncomplete escaping: backslashes in user input are not escaped.
The CodeQL warning is valid. If a user searches for a literal backslash (e.g.,
a\b), the backslash passes through unescaped and will be interpreted as an escape character byunicodeLike, potentially causing unexpected behavior.The backslash must be escaped first, before escaping the wildcards:
🐛 Proposed fix to escape backslashes
- const escapedSearch = search.replace(/[%_?]/g, '\\$&'); + const escapedSearch = search.replace(/\\/g, '\\\\').replace(/[%_?]/g, '\\$&');Alternatively, escape all special characters in a single pass with proper ordering:
const escapedSearch = search.replace(/[\\%_?]/g, '\\$&');Note: The single-pass version works because
$&inserts the matched character, so\becomes\\,%becomes\%, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/queries/index.ts` at line 87, The current escapedSearch calculation in queries/index.ts misses escaping backslashes, so update the replace to escape backslashes as well (either escape backslashes first or use a single-pass regex that includes backslash) so that inputs like "a\b" don't misbehave when passed to unicodeLike; modify the code that defines escapedSearch (and any callers that pass it to unicodeLike) to use a pattern that replaces backslash, percent, underscore, and question-mark (e.g., include '\\' in the character class) ensuring backslashes become '\\\\' in the resulting string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/platform/server/sqlite/unicodeLike.ts`:
- Around line 5-29: In likePatternToRegex, update the escape handling in the
likePatternToRegex function so it only treats '%' and '?' as escapable wildcards
(remove '_' from the condition that checks next === '%' || next === '_' || next
=== '?'); adjust the conditional to next === '%' || next === '?' and keep the
existing logic that escapes the literal character and advances i by 2, ensuring
plain '_' continues to be handled as a normal literal via the default branch.
---
Duplicate comments:
In `@packages/desktop-client/src/queries/index.ts`:
- Line 87: The current escapedSearch calculation in queries/index.ts misses
escaping backslashes, so update the replace to escape backslashes as well
(either escape backslashes first or use a single-pass regex that includes
backslash) so that inputs like "a\b" don't misbehave when passed to unicodeLike;
modify the code that defines escapedSearch (and any callers that pass it to
unicodeLike) to use a pattern that replaces backslash, percent, underscore, and
question-mark (e.g., include '\\' in the character class) ensuring backslashes
become '\\\\' in the resulting string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e4bc05e-f038-4fdb-964a-b2888a1e798f
📒 Files selected for processing (3)
packages/desktop-client/src/queries/index.tspackages/loot-core/src/platform/server/sqlite/unicodeLike.tsupcoming-release-notes/7270.md
actual-github-bot
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/platform/server/sqlite/unicodeLike.ts`:
- Around line 10-17: The escape handling currently only treats "\%" and "\?" as
escaped wildcards; update the logic in unicodeLike
(packages/loot-core/src/platform/server/sqlite/unicodeLike.ts) so that "\_" is
also recognized as an escaped wildcard: extend the condition that checks next
(currently next === '%' || next === '?') to include next === '_' and ensure the
replacement/escaping logic that appends to regexStr still treats the underscore
correctly (add '_' to the list if you maintain a custom escape character class,
or note that '_' is not a special regex char so simply allowing it through is
fine); adjust the variables pattern, next, and regexStr handling accordingly so
"\_" becomes a literal underscore in the generated regex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 430f0d74-56d1-4c2c-98e1-9198edd315e9
📒 Files selected for processing (2)
packages/desktop-client/src/queries/index.tspackages/loot-core/src/platform/server/sqlite/unicodeLike.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/queries/index.ts
matt-fidd
left a comment
matt-fidd
left a comment
There was a problem hiding this comment.
Thanks for taking a look at this!
Could you have a look at my comments, and add some tests in for this new behaviour please?
Removed |
|
Wait, I considered this a feature, not a bug. How often do payees actually contain special characters? |
I guess, in the "notes" you can put wathever you want. |
|
This is true, but even more of a reason to include wildcard characters. In that case, can you make it escape only for the notes field? https://github.com/actualbudget/actual/pull/7270/changes#diff-d8fa4e0507344fcd8454e77d6dec4adb052acaea5abb19682dc861998cb2f4c5R103 |
@StephenBrown2 I understand the value of having wildcards for advanced searches! However, the quick search uses an $or condition across all fields. If we leave the notes field unescaped, typing a ? in the search bar will still return all transactions (because the unescaped notes field will match everything). This would defeat the purpose of fixing the bug reported in #5840. @matt-fidd since you reviewed the initial fix, what is your take on this? I'm happy to adjust the code, but I want to make sure we don't accidentally reintroduce the bug for the global search. Let me know how you'd like to proceed! |
|
Hi @matt-fidd! I would love to see this closed. Can you take a look? |
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
Hey, thanks for your work on this. I understand Stephen's points here, and also what you've said about the or operator making it return everything regardless. I've been tossing this one about in my head, and i think that most users will expect it to be escaped and won't be using the wildcards, so we can go ahead with this. The search is pretty rudimentary at the moment, some sort of fuzzy search algorithm would be great down the road. |
✅ Deploy Preview for actualbudget-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
matt-fidd
left a comment
matt-fidd
left a comment
There was a problem hiding this comment.
Brilliant, thank you! And sorry for the delay getting this in
4 participants
Description
When a user types special characters such as ? or % into the transaction quick search bar, all transactions were returned instead of only those matching the literal character. This happened because these characters were being passed unescaped into $like query patterns, where they acted as regex wildcards inside the custom UNICODE_LIKE function.
The fix has two parts:
Related issue(s)
Fixes #5840
Testing
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/queries/index.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/platform/server/sqlite/unicodeLike.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/platform/server/sqlite/unicodeLike.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged