Conversation
kbrock
force-pushed
the
pr-22921
branch
2 times, most recently
from
51420d7 to
83d7e16
Compare
|
update:
update:
update:
|
kbrock
force-pushed
the
pr-22921
branch
2 times, most recently
from
3356960 to
ac8aad8
Compare
| # finds by name or namespace. not domain | ||
| # @param [nil,String] search | ||
| scope :name_path_search, lambda { |search| | ||
| search.present? ? where(arel_table[:relative_path].matches("%#{search}%")) : where({}) |
There was a problem hiding this comment.
I think this is a potential security issue? We try to avoid interpolation and use parameter binding instead. What would happen if the search itself had a % ?
Perhaps use sanitize_sql_like ?
There was a problem hiding this comment.
In core I see:
- 1: use of
sanitize_sql_like(actually across all our code) - 15: uses of the way I presented it. (
grep for 'matches.*%') - 24: uses of
"like ?", ...%(grep 'like *(*\?') - 44: uses of
where... #{}(grep where.*#{)
Do we actually care if someone uses % in the phrase?
I'll look into making these changes but since we are adding in a wild character, not sure why it buys us.
There was a problem hiding this comment.
It's more of a best practices thing? I thought like ? with bindings was typically the way to do it, but if matches with interpolation also works, then I'm fine with it.
Do we actually care if someone uses % in the phrase?
% in the phrase is really a question to you? Will name_path_search break if someone has a literal % in their namespace path? Is that even a valid character? Same goes for ?. It might be worth having a test case for that particular scenario to make sure it works and that this code can find those edge cases.
There was a problem hiding this comment.
- Verified that this pattern is not susceptible to sql injection attacks. interpolation in the main sql is, but the arguments are secure
- If searching for a string with a %, it finds it, but treats it as a wild character
- Method names can not have a
%in them, but I feel our conversation may be about the bigger picture, so I'm trying not to get bound down by this
I added a test case. Was not able to add a method with a % in the name, but came as close as I could.
|
update:
update:
|
|
Checked commits kbrock/manageiq@a4e86f0~...5d2ddfe with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
3 participants
Overview
Part of ManageIQ/manageiq-ui-classic#9047
Adds scopes for: ManageIQ/manageiq-ui-classic#9059
This PR is an RnD work related to the search feature for the selection of the method.
Alternative to #22921
First commit deletes a bunch of code from the spec
Second commit is method and specs
Discussion: do we want to have conditional ids logic here or over in the ui?