Search results should not expose private information
hutch - June 25, 2006 - 13:29
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | search.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | David Lesieur |
| Status: | patch (code needs review) |
Description
The attached patch is on
search.module,v 1.176.2.2 2006/05/07 17:51:24 killes
It adds some checkboxes to the search settings page to control the display of Date, Author, Node type and Extras in the search results page.
HTH
| Attachment | Size |
|---|---|
| search.module_display_results_settings.patch.txt | 5.34 KB |
#1
search keywords module is not search.module.
#2
we only accept new features against drupal CVS HEAD (the upcoming 4.8)
#3
a lot of changes were made between drupal HEAD and 4.7
--> i guess it wont apply
#4
reapplied
#5
Not sure how important this is. One some sites it is probably useful to hide the date and author. And on static sites, the content type might be confusing. We'll want to come up with a better description for "extras". Extra's is not exactly helpful for a user. If you mean 'result snippets', write 'result snippets' (or something equivalent). Otherwise I'm cool with this patch.
#6
Search results are already themable, do we really need explicit toggles?
The texts need work though. A description à la "Show the date in the results" is a bit redundant if you do it for every checkbox. How about naming the fieldset "Show in search results:" and removing the descriptions? "Node type" should be "Content type". And "extras" can be better too... perhaps "module-provided fields"?
Shouldn't we use #type => checkboxes for this as well?
#7
I agree this is a non-important feature, but for some people this kind of tweaks can be quite usefull... Anyway, I fixed the texts as you suggested, but I also moved the form to the theme settings page following the example of post information settings.
#8
Tweaking your site using settings is always easier and accessible to non-developers / non-designers. It's a useful feature IMO, and the code looks fast and elegant.
At least, the first patch looked elegant. The second patch seems rather complex ... not sure this ought to be a theme setting.
#9
It's back on the search settings page...
#10
...and ready for review...
#11
This patch works well for me. I like it.
However, I'd like the search module to also care about the theme settings (as in
theme_get_setting('toggle_node_info_' . $node->type)). If a node type requires not to display post information, I think the search results should not show such information. Maybe this is a separate issue though.#12
A bit late to try to revive this issue, but who knows? ;-)
#13
#14
Re-rolled the patch (not taking into account my comment in #11).
#15
Marking http://drupal.org/node/107506 as a duplicate.
Furthermore, per content type setting from theme configuration could be imposed, as pointed on above link?
#16
Yes, I'll make a new patch to take the theme setting into account. I also need to rename the new 'search_result_node_info' variable; it is misnamed because this setting does not apply only to nodes. I should have kept the same name as the previous patch. ;-)
#17
This new patch takes theme settings into account and uses a proper variable name.
#18
Removed an unwanted whitespace.
#19
If anyone is interested in getting this frequently requested feature in Drupal 6, please review and test the patch before the code freeze (June 1st). It's a small and easy patch! :-)
#20
<?php'#description' => t('Select which additional information, besides title and snippet, should be displayed for each search result. This setting will have no effect on content types whose post information is never displayed (as per your !theme).', array('!theme' => l(t('theme settings'), 'admin/build/themes/settings/global'))),
?>
This usage of t()/l() is incorrect.
#21
Fixed!
#22
Umm, use this patch instead.
#23
Targetting D7, although the patch still works on D6 at the moment.
#24
Would consider the 'post information' displayed on search result pages as a bug. It's really odd, because you don't want to show who created a content page; and if you have search module enabled, anyone can do a query and find the actual author and date of publishing. An example: Handbook pages on drupal.org.
If we can at least create a patch against search.module for Drupal 6.x-dev for respecting the "Display post information" settings in theme configuration, as a part of bug fix actually.
#25
Good point! Here's the patch. ;-)
#26
The patch is to the point for the actual bug fix!! If there's anything else demanded(except a comment?), that would be a feature request most probably. The patch attempts the minimal and best so far:
<?phpif ($type != 'node' || theme_get_setting('toggle_node_info_'. $item['node']->type)) {
// do the post info. stuff
}
?>
Hopefully, this is getting in soon. :)
#27
Brilliant idea to apply the node toggles here, it is in fact a bugfix this way :) A line of comment would be good though.
#28
Added a comment.
#29
If the patch still applies, anyone going to commit? :)
#30
It still applies. ;-)
#31
Not any more it doesn't.
#32
I looked at a few of the patches in the comments above, and the latest patch now looks incomplete. Fixing the latest patch to use search-result.tpl.php is trivial (patch attached), but someone else should add the settings form back. Was this dropped intentionally?
#33
As of today, the settings form is there (in admin/build/themes/settings). However, the above patch did not work because
$info_split['type']is a "search type", and the patch seemed to expect a node type instead.It could be nice to keep the template simple, so I'm proposing this new (and tested) patch that does not affect the tpl file.
#34
tested:
1. tried mixed search, result contains node types with and without post info
2. tried to write print $info_split['user'] into tpl file, to get php error
works without any problem and code looks good
changed patch to apply from drupal root
#35
no, sorry it does show php notice for test #2
#36
search-result.tpl.php
#37
this is the minimal set of keys required.
#38
#39
@Pasqualle: Thanks for the testing! However, the conditions you've added in your last patch are redundant, and I don't think they're of any use. As you have quoted above regarding $info_split:
Checking for existence means using
isset()in the tpl file if it needs to use a value from $info_split. The default search-result.tpl.php as it stands now does not use $info_split, and there are no PHP warnings or notices.My recommendation would be to go with the patch at #34.
#40
read further please
As I read it, the whole description, there are keys like comment and upload and other possible module generated keys which are not guaranteed to be set, but these 3 are always set.
Of course if you examine the code closely, it was not guaranteed even before the patch, but that is a bug, as I understand.
It is basic requirement for css only themes to set all variables used, otherwise you will get php notices.
If you want to go with patch #34, then comment in search-result.tpl.php must be modified also.
#41
Ah, yes... Good catch!
Except for 'type', 'user' and 'date', any value in $info_split already has to be checked for existence, so it seems more consistent to do the same for 'type', 'user' and 'date' as well. At least that's what the patch at #34 implied. This new one changes the comments in search-result.tpl.php to reflect this logic.
#42
#43
still applies.
#44
LIVE FROM THE MINNESOTA SEARCH SPRINT: This patch still applies and works.
#45
LIVE FROM THE MINNESOTA SEARCH SPRINT
Changing the title to better reflect the issue.
Currently, search results reveal authorship and other 'post' information, even if a site admin has elected to hide this information in the theme settings. This could be construed as an information leak.
This patch fixes that issue, by checking whether the theme allows post information to be displayed before populating the search results.
#46
LIVE FROM THE MINNESOTA SEARCH SPRINT
Reviewed code; variable setting code in template_preprocess_search_result() is now executed conditionally for non 'node' type results or node types for which 'toggle_node_info_' is set.
Tested as follows:
* Before the patch is applied, all post information is displayed for all node types in search results
* With patch applied, enabled 'display post information' for a given set of types. Typed a search term which displays results encompassing all node types. Only the types for which 'display post information' is enabled now have post information displayed in search results. Tried this for several different combinations of types.
#47
puregin: can you try writing a simpletest for this? It shouldn't be too hard.
#48
puregin: can you try writing a simpletest for this? It shouldn't be too hard.
#49
#50
Hi,
This weekend PHP Quebec organized a Code Fest; I used this occasion to write a simpletest to test the presence of node info in the test result. Here is the new patch for the module.
#51