Fixed query with hidden search fields. #7
Conversation
| */ | ||
| public function hookItemsBrowseSql($args) | ||
| { | ||
| if ($this->_overrideFilter() || !isset($this->_settings['search']) || empty($this->_settings['search'])) { |
There was a problem hiding this comment.
The !isset() on this line is redundant, it should be entirely covered by the empty().
|
Have you thought about removing the offending advanced "rows" using the browse params filter instead? So the search would, instead of returning zero results, just act like those fields weren't part of the search, and you wouldn't have to touch the SQL. At any rate, I'll have to think about this one. It may need to be toggleable by some specific setting, because I think it's reasonable to merely hide from the search, rather than totally remove the ability. For example, you could want to remove elements from the search form seen by users but still want something like SearchByMetadata to keep working (which makes direct "search" browse links that use the advanced search). |
|
Hi, For this patch, my priority was a security one, but yours is an interface one. I don't want that a user can search an internal field or can guess what it is inside with multiple queries via the direct manipulation of the url. So in that case, a forbidden field should return an empty result. But it's ok if each field may be enable or not. For the choice between the hook and the filter, I think it was simpler to use the last used, specially because the method should filter not only advanced params, but the main simple "Search for words" field too. Sincerely, Daniel Berthereau |
Hi,
When a search field is removed from the advanced form, the search can always be done via direct manipulation of the url. This patch forbids it.
Sincerely,
Daniel Berthereau
Infodoc & Knowledge management