I have not tested this yet, but we should probably follow the style set forth in other preferences so instead of "query_kw" it should be "{query_kw}" maybe even in all caps.  That's how it's done on the other preferences where we enter placeholders.

Nicole

I agree with Nicole. Using a literal string is inconsistent and just asking for trouble.

I like the idea of this patch, but unfortunately it adds an xss vulnerability.

For example, if I searched on 
><script type="text/javascript" src="http://link/to/evil.js"></script> that would be substituted and output (and run).

OPACNoResult is not piped through the html filter, because then it couldn't have links in it, so it's not a simple fix to just change that.

Probably the easiest fix is to run the $query_kw through HTML::Scrubber before substituting it in the syspref.

Well, I try to see how to proceed with C4::Scrubber and how it had been already used in other koha scripts like opac-tags.pl but I don't really get how to adapt it to the present case. However, if it's a question of sanitizing the $query_kw string before substitution, you 'll note that the regexp used to extract keywords does it since it keeps only alphanumeric characters (plus -):
To pick your example, it goes like this :
my $query_kw='<script type="text/javascript" src="http://link/to/evil.js"></script>';
then after theese lines :
my @query_kw=($query_kw=~ /([-\w]+\b)(?:[^,:]|$)/g);
my $query_kw=join('+',@query_kw);
the output to be substituted will be :
my $query_kw='script+type+text+javascript+src+link+to+evil+js+script'

may be, that could do the trick anyway ?

The patch does not apply :

OPACNoResults, add keyword to search string
Apply? [yn] y

Applying: OPACNoResult, Add keyword to query string
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Failed to merge in the changes.
Patch failed at 0001 OPACNoResult, Add keyword to query string
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/OPACNoResults-add-keyword-to-search-string-cfak79.patch

Ok,I think I see the reason why now :
The \w class can match accentuated characters (as it does at lyon3) but unfortunately only if the adequate set of locales is installed on the system.

I am sorry, but I think should be "needs signoff" for the improved patch. Thx for fixing it!

Do you mean I declared "Signed Off" status instead of "Needs Signoff" ? If so, I'm sorry.

I just tested, and this patch does not work for me. Adding {QUERY_KW} to OPACNoResultsFound does not change the output at all (without this patch, the text {QUERY_KW} showed up).

(In reply to comment #13)
> I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> OPACNoResultsFound does not change the output at all (without this patch,
> the text {QUERY_KW} showed up).

strange... it works perfectly on our system, could you mention the query you had tested in OPACNoResultsFound and the keywords used ?

(In reply to comment #14)
> (In reply to comment #13)
> > I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> > OPACNoResultsFound does not change the output at all (without this patch,
> > the text {QUERY_KW} showed up).
> 
> strange... it works perfectly on our system, could you mention the query you
> had tested in OPACNoResultsFound and the keywords used ?

asflfsjsfja (no, that was not a typo)

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> > > OPACNoResultsFound does not change the output at all (without this patch,
> > > the text {QUERY_KW} showed up).
> > 
> > strange... it works perfectly on our system, could you mention the query you
> > had tested in OPACNoResultsFound and the keywords used ?
> 
> asflfsjsfja (no, that was not a typo)

Thanks for the keyword but what was your full query (the one you put in OPACNoResultsFound) ?

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> > > > OPACNoResultsFound does not change the output at all (without this patch,
> > > > the text {QUERY_KW} showed up).
> > > 
> > > strange... it works perfectly on our system, could you mention the query you
> > > had tested in OPACNoResultsFound and the keywords used ?
> > 
> > asflfsjsfja (no, that was not a typo)
> 
> Thanks for the keyword but what was your full query (the one you put in
> OPACNoResultsFound) ?

Sorry, I thought you meant the query I entered into Koha. I tried it with the following in OPACNoResultsFound: Search elsewhere: <a href="www.google.com/?q={QUERY_KW}">Google</a> for {QUERY_KW}.

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> > > OPACNoResultsFound does not change the output at all (without this patch,
> > > the text {QUERY_KW} showed up).
> > 
> > strange... it works perfectly on our system, could you mention the query you
> > had tested in OPACNoResultsFound and the keywords used ?
> 
> asflfsjsfja (no, that was not a typo)

Thanks for the keyword but what was your full query (the one you put in OPACNoResultsFound) ?(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > (In reply to comment #13)
> > > > > I just tested, and this patch does not work for me. Adding {QUERY_KW} to
> > > > > OPACNoResultsFound does not change the output at all (without this patch,
> > > > > the text {QUERY_KW} showed up).
> > > > 
> > > > strange... it works perfectly on our system, could you mention the query you
> > > > had tested in OPACNoResultsFound and the keywords used ?
> > > 
> > > asflfsjsfja (no, that was not a typo)
> > 
> > Thanks for the keyword but what was your full query (the one you put in
> > OPACNoResultsFound) ?
> 
> Sorry, I thought you meant the query I entered into Koha. I tried it with
> the following in OPACNoResultsFound: Search elsewhere: <a
> href="www.google.com/?q={QUERY_KW}">Google</a> for {QUERY_KW}.

Your url and keyword works on our system, the substitution is done :
www.google.com/?q=asflfsjsfja
I have no idea why it doesn't on your system.
But I noted another point to be fixed : without the http mention in href attribute (href="http://www.google...) the address is searched in the web directory of koha.

Hi, 

I retested this on my system and it works nicely - could it be some kind of configuration issue? I tried:

- simple keyword searches
- searches from the advanced search page with different indexes
- Umlaut searches

The content of my system preference is: Something {QUERY_KW}

"It works for me"

But (qa comment): it is useless to generate $query_kw if the syspref does not match {QUERY_KW}, please use if( $var =~ m/{QUERY_KW}/ ).

QA Comments:
Now it is ok for me.
Marked as Passed QA.

Created attachment 14240 [details] [review]
OPACNoResult, Add keyword to query string

When search failed, this patch gets the keyword(s) and add them
to the string entered by the user to OPACNoResultFound

http://bugs.koha-community.org/show_bug.cgi?id=7620
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

This patch has been pushed to master.

As a reminder for future patches, please be sure to include the bug number in the patch subject (first line of the commit message)