38694 - Can't query properly on a product with an ampersand in it.

Status: RESOLVED WORKSFORME

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Devin Weaver]

<p>I added a product called "L & amp; M" (Thats the ampersand + 'amp;' no
space)</p>

<p>The query page accepted it but when I clicked it no version or components
showed up.</p>

<p>I then renamed it with out the ampersand and I got two warnings about an
argument not the right type?</p>

<p>Anyways Things when back to normal after that. Thought you'd like to know
:-)</p>

Comment 1

[Dave Miller [:justdave]]

moving to real milestones...

Comment 2

[Matthew Tuck [:CodeMachine]]

See also bug #95290.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 4

[Christian Reis]

I'm gonna try and reproduce this on landfill. 

Comment 5

[Christian Reis]

Okay, the query _Javascript_ currently works (you can see at
http://landfill.tequilarista.org/bz96534/query.cgi ). However, I can see a
couple of related bugs in Bugzilla. For instance, posting a bug to the last
product (starts with $*$) gives me a:

 No recipient addresses found in header bugzilla-daemonTo:
[e-mail]: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
[e-mail] ReportedBy: [e-mail]... Unbalanced '('
bugzilla-daemonTo: [e-mail]: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
[e-mail] ReportedBy: [e-mail]... Unbalanced '('
bugzilla-daemonTo: [e-mail]: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
[e-mail] ReportedBy: [e-mail]... Unbalanced '('
bugzilla-daemonTo: [e-mail]: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
[e-mail] ReportedBy: [e-mail]... Unbalanced '(' Email sent to:
[e-mail]

Which is no good. This isn't specifically about &, though.

Comment 6

[Christian Reis]

Okay, and searching for that last bug (which is
http://landfill.tequilarista.org/bz96534/show_bug.cgi?id=286)didn't yield good
results either.

However, I was able to add a bug to product "this&that" (with that error) and
also query for it without problems - see
http://landfill.tequilarista.org/bz96534/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&product=this%26thar&component=%40%23%21%40%26%25%24%40%21%24*%28%24_%40%21%28%40*%26%2B_%21%24%26*_%25%25%AF%21%40*%23_%24%21%23%28%40%21%2B%40%24%29%23&short_desc=&short_desc_type=allwordssubstr&long_desc=&long_desc_type=allwordssubstr&bug_file_loc=&bug_file_loc_type=allwordssubstr&keywords=&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=doit&newqueryname=&order=Reuse+same+sort+as+last+time

I'm quite inclined to mark this WORKSFORME and file another bug with larger
scope (fixing symbol handling 100%).

Comment 7

[Christian Reis]

Damn. No dice, I can reproduce this here. Javascript error on line 136. Looking.

Comment 8

[Christian Reis]

This is an interesting problem we're adding A&B to an array as a key.
However, when I add to an option and pull the value out,it's been transmuted
into A&B. This makes it scary matching. AFAICT, there's no good fix without
changing the indexes.

a) We could use a simple html_unquote() around the product names. However, doing
that would make components['A&B'] clobber ['A&B']; we would of course also
be us vulnerable to clobbers with _any_ html entity in the name.

b) We could index products by position, reducing the size of the generated
JavaScript, and making us impervious to javascript transformations of the
product name. This is trés cool!

I've implemented b) and will attach the patch.

Comment 9

[Christian Reis]

Attachment: changes the arrays to index by product number, not name.

Comment 10

[Christian Reis]

Okay, I've altered query.cgi to be impervious to problems with option value
transformation that happens when we insert a product with html entities into it.
I would like to point out that this makes the code smaller also (though I've
probably made it up with extra comments). This is a fix for this patch, and
won't break anything else.

However, I am concerned with fixing problems like this: this problem had a
simple fix, because I always have the product position inside the SELECT.
However, if somebody decides to move products around, this causes problems. Of
course, nobody plans to move products inside the SELECT control. Fine.

However, we are _still_ open to bugs that will occur when the component name is
"Foo & Bar", because when I merge the arrays, I use the actual SELECT option
value to compare against the array contents. This means I will end up trying to
compare "Foo & Bar" with "Foo & Bar" and we will probably end up adding two
items. I have no easy solution for _that_ problem (which is outside the scope of
this bug) other than running html_dequote() on components, versions and
milestones and allowing a (very rare) clobber to happen. Because of the way the
code behaves, the clobber would mean things would work fine, _but_ it's a HACK,
because it's only an implementation detail. However, the chance of this clobber
occuring is rare and the user has to be on a lot of dope to register a component
called "Foo & Bar" and another "Foo & Bar".

My recommendation to the reviewers: r= this patch (if the code seems decent, I'm
not sure if $#array is a good way to find length) and commit it, since the
effect of it is quite evil, BUT: consider the problems that will occur with
entities in product and version names. The original code did work, so in part
it's bug 96534's fault that they stopped working. I'd either do html_dequote()
and allow rare clobbers to occur, or prohibit symbols going into product names,
or WONTFIX these bugs. It's your call.

Comment 11

[Christian Reis]

See this in action on

http://landfill.tequilarista.org/bz96534/query.cgi

Click on the L&M product, and you'll see component M&L selected. Yeah, I
didn't expect that to happen. Reviewer, reported, what should we do?
html_unquote() stuff? Let me know.

Comment 12

[Christian Reis]

I'd like to point out that even though this patch works (and IMHO should be
applied since it makes things better for us in the future), it won't solve the
problem at hand. The problem is that when we submit the form, the Product's
value is converted from L&M to L&M in the query string, and it breaks.

Proper fix = block entering products with entities in it, or silly characters
altogether (nicer, not best solution). What do we do?

Comment 13

[Christian Reis]

If bug 83033 is checked in this will be fixed at least in part so i'll add the dep.

Comment 14

[Gervase Markham [:gerv]]

I think the solution here is to only allow a certain range of characters in a
Product and Component name at creation, i.e. disallowing ", <, > and &. That
would save a lot of hassle.

Gerv

Comment 15

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 16

[Christian Reis]

Filed new bug for cleanups that make this bug much easier to fix.

Comment 17

[Jouni Heikniemi]

Comment on attachment 47916 [details] [diff] [review]
changes the arrays to index by product number, not name.

Needs a templatization rewrite. Kiko, is this still an issue?

Comment 18

[Christian Reis]

This is fixed in CVS head; I think the JS modifications and proper urlencoding
did it. Thanks for reminding me of it.

Comment 19

[Dave Miller [:justdave]]

clearing target in DUPLICATE/WORKSFORME/INVALID/WONTFIX bugs so they'll show up
as untriaged if they get reopened.