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

RESOLVED WORKSFORME

Status

()

Bugzilla
Query/Bug List
P3
trivial
RESOLVED WORKSFORME
18 years ago
5 years ago

People

(Reporter: Devin Weaver, Assigned: Christian Reis)

Tracking

Details

(Whiteboard: 2.16)

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
<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>
Whiteboard: 2.14

Updated

17 years ago
Whiteboard: 2.14 → 2.16
moving to real milestones...
Target Milestone: --- → Bugzilla 2.16
See also bug #95290.
Mass moving to new product Bugzilla...
Assignee: tara → endico
Component: Bugzilla → Query/Bug List
Product: Webtools → Bugzilla
Version: other → 2.13
(Assignee)

Comment 4

17 years ago
I'm gonna try and reproduce this on landfill. 
(Assignee)

Comment 5

17 years ago
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:
kiko@async.com.brSubject: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
kiko@async.com.br ReportedBy: kiko@async.com.br... Unbalanced '('
bugzilla-daemonTo: kiko@async.com.brSubject: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
kiko@async.com.br ReportedBy: kiko@async.com.br... Unbalanced '('
bugzilla-daemonTo: kiko@async.com.brSubject: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
kiko@async.com.br ReportedBy: kiko@async.com.br... Unbalanced '('
bugzilla-daemonTo: kiko@async.com.brSubject: [Bug 286] New: we hate
symbolshttp://landfill.tequilari...$*$(!@%(!@%#(@&#@#*#&*&# AssignedTo:
kiko@async.com.br ReportedBy: kiko@async.com.br... Unbalanced '(' Email sent to:
kiko@async.com.br

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

Comment 6

17 years ago
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%).
Assignee: endico → kiko
(Assignee)

Comment 7

17 years ago
Damn. No dice, I can reproduce this here. Javascript error on line 136. Looking.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

17 years ago
This is an interesting problem we're adding A&amp;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&amp;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.

(Assignee)

Comment 9

17 years ago
Created attachment 47916 [details] [diff] [review]
changes the arrays to index by product number, not name.
(Assignee)

Comment 10

17 years ago
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 &amp; 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 &amp; 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 &amp; 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.
Keywords: patch, review
(Assignee)

Comment 11

17 years ago
See this in action on

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

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

Comment 12

17 years ago
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&amp;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?
(Assignee)

Comment 13

16 years ago
If bug 83033 is checked in this will be fixed at least in part so i'll add the dep.
Depends on: 83033
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
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
(Assignee)

Comment 16

16 years ago
Filed new bug for cleanups that make this bug much easier to fix.
Depends on: 122154
No longer depends on: 83033

Comment 17

16 years ago
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?
Attachment #47916 - Flags: review-
(Assignee)

Comment 18

16 years ago
This is fixed in CVS head; I think the JS modifications and proper urlencoding
did it. Thanks for reminding me of it.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WORKSFORME
clearing target in DUPLICATE/WORKSFORME/INVALID/WONTFIX bugs so they'll show up
as untriaged if they get reopened.
Target Milestone: Bugzilla 2.18 → ---
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.