291616 - Works With query should use vID, not URI for query

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[alanjstr]

URI isn't indexed.  vID is.  This also will avoid us throwing a mySQL error when
the URI has a single quote in it.

Comment 1

[alanjstr]

Attachment: Replace URI with vID

Comment 2

[Michael Morgan [:morgamic]]

Comment on attachment 181647 [details] [diff] [review]
Replace URI with vID

- array indexes are referenced as constants ($array[foo] instead of
$array['foo']

- are you sure $row['vID'] is escaped properly?  Is it safe to assume that?

Comment 3

[Michael Morgan [:morgamic]]

After taking a closer look, I think the real problem comes from the building of
the URI and filename validation / correction on addition.  Really, a ' should
not be an allowed character, unless you can think of a good reason why it should
be there.

I took a dump of the current DB and the two versions of Zicklepop's Buttons are
the only instances where the URI contains '.

I'd say let's analyze additem.php to adjust these URI entries.

Comment 4

[alanjstr]

> - are you sure $row['vID'] is escaped properly?  Is it safe to assume that?

if its in our DB, it had better be an integer.

The problem isn't just with single quotes.  There are exclamation points and
slashes coming in, too.

Comment 5

[alanjstr]

> The problem isn't just with single quotes.  There are exclamation points and
> slashes coming in, too.

Sorry, that's a different bug.

In this situation, we're querying on a non-indexed field, so this would also
help performance.

URI building is bug 291562

Comment 6

[alanjstr]

Attachment: Replace URI search with vID search

The missing quotes were because the original code was missing them.

Comment 7

[Michael Morgan [:morgamic]]

Did you _actually_ test the patch?

This throws a parse error:
+    echo"".ucwords($row3['shortname'])." $row3['MinAppVer']-$row3['MaxAppVer'] \n";

Error:
Parse error: parse error, unexpected T_ENCAPSED_AND_WHITESPACE, expecting
T_STRING or T_VARIABLE or T_NUM_STRING in
/home/morgamic/public_html/tip/developers/approval.php on line 160

If you adjust it to this, it does not:
    160     echo ucwords($row3['shortname'])."
$row3['MinAppVer']}-{$row3['MaxAppVer']}\n";

Also, this patch needs to work with bug 290958, and I was having problems
merging the two.  I have rewritten the other patch, and will submit a unified diff.

Comment 8

[Michael Morgan [:morgamic]]

Attachment: Fixes lack of escaping in URI query, also adds Thunderbird download link

Comment 9

[Michael Morgan [:morgamic]]

*** Bug 290958 has been marked as a duplicate of this bug. ***

Comment 10

[Michael Morgan [:morgamic]]

Comment on attachment 182090 [details] [diff] [review]
Fixes lack of escaping in URI query, also adds Thunderbird download link

Combining this bug with bug 290958.  The current patch covers both.

Comment 11

[Michael Morgan [:morgamic]]

Comment on attachment 182073 [details] [diff] [review]
Replace URI search with vID search

obselete.

Comment 12

[Colin Ogilvie [:cso]]

Comment on attachment 182090 [details] [diff] [review]
Fixes lack of escaping in URI query, also adds Thunderbird download link

>Index: developers/approval.php
>+    // If our item is linked to thunderbird, show a download now link.
>+    if (in_array('tb',$apps)) {

Not review related but:
I think we need to look at an alternative way of doing this in the future
(v2.0) - Hardcoding this doesn't seem sensible if the application is to be "not
mozilla.org" specific.

>+        echo '<span>(<a href="' . $row['URI'] . '">Download Now</a>)</span>'."\n";
>+    } 
>+
>+    // If our item is linked to a browser, also show the install now link.
>+    if (in_array('mz',$apps) || in_array('fx',$apps)) {
>+        if ($type=="T") {
>+            echo "<span>(<a href=\"javascript:void(InstallTrigger.installChrome(InstallTrigger.SKIN,'{$row['URI']}','{$row['Name']} {$row['Version']}'))\">Install Now</a>)</span>\n";
>+        } else {
>+            echo "<span>(<a href=\"javascript:void(InstallTrigger.install({'".addslashes($row['Name'].$row['Version'])."':'{$row['URI']}'}))\">Install Now</a>)</span>";
>+        }

Review Related:
The Zicklepop's Buttons extension produces a URL of:

javascript:void(InstallTrigger.installChrome(InstallTrigger.SKIN,'http://addons
.update.mozilla.org/developers/approvalfile.php/zicklepop's_buttons-1.0-fx+mz.j
ar','ZicklePop's Buttons 1.0'))

Which gives a JS error message of:

Error: missing ) after argument list
Source File:
javascript:void(InstallTrigger.installChrome(InstallTrigger.SKIN,'http://addons
.update.mozilla.org/developers/approvalfile.php/zicklepop's_buttons-1.0-fx+mz.j
ar','ZicklePop's Buttons 1.0'))
Line: 1, Column: 126
Source Code:
void(InstallTrigger.installChrome(InstallTrigger.SKIN,'http://addons.update.moz
illa.org/developers/approvalfile.php/zicklepop's_buttons-1.0-fx+mz.jar','Zickle
Pop's Buttons 1.0'))

and I don't get the window offering to install it.

Comment 13

[Michael Morgan [:morgamic]]

Attachment: Patch that adds javascript escaping.

Comment 14

[alanjstr]

Comment on attachment 182108 [details] [diff] [review]
Patch that adds javascript escaping.

You still didn't fix the one in listmanager

Warning: mysql_fetch_array(): supplied argument is not a valid MySQL result
resource in /home/alanjstr/public_html/developers/listmanager.php on line 560
zicklepop's_buttons-1.0-fx+mz.jar (699kb)

Comment 15

[Michael Morgan [:morgamic]]

Attachment: Additional fix for itemoverview.php

Should fix error in itemoverview.php.  Please doublecheck listmanager -- my
copy works.  cvs up -C and then re-patch if necessary.

Comment 16

[Colin Ogilvie [:cso]]

Comment on attachment 182380 [details] [diff] [review]
Additional fix for itemoverview.php

Seems to work, in all cases I've tried.

Comment 17

[alanjstr]

Please make sure that we're not going to blow up on the client side, too, like
on moreinfo.

http://lxr.mozilla.org/update1.0/source/core/inc_global.php#169

Comment 18

[Michael Morgan [:morgamic]]

What are you talking about?

Comment 19

[alanjstr]

Bug 291546.  We need to make sure we escape it properly when building the
install links.

Comment 20

[Michael Morgan [:morgamic]]

Attachment: Additional fix for moreinfo.php

Check to see if this fixes the javascript error caused by an unescaped ' in
themes/moreinfo.php.

Comment 21

[Jan Dittmer]

+    if (in_array('tb',$apps)) {
+        echo '<span>(<a href="' . $row['URI'] . '">Download Now</a>)</span>'."\n";
+    } 

Can you please show the download link unconditionally in approve.php? I'm mostly
working with 2 browser sessions an downloading in the one, installing in the
other. Doesn't hurt - does it?

Comment 22

[Michael Morgan [:morgamic]]

I don't see any reason why not.  I'll adjust the patch and repost it along with
modifications to showlist.php.

Comment 23

[alanjstr]

You should be able to right click the install link to save to disk.

Comment 24

[Michael Morgan [:morgamic]]

Attachment: showlist.php fixes, download link always shows.

Should make approval queue display download and install links regardless of
app.  Added t/e/showlist.php to the mix, so the install() calls do not break
there either due to an '.

Comment 25

[Colin Ogilvie [:cso]]

It looks as if it should work - but I'm having problems testing as whenever I
approve the problematic theme on my test install, the URI gets set to '' which
then breaks the install links for another reason (even running the SQL query
manually results in ''). 

Comment 26

[Michael Morgan [:morgamic]]

Attachment: Added additional checking in inc_approval.php to prevent blank URIs.

inc_approval.php was causing problems because of lack of escaping; this should
be fixed.  As is the norm, it should be rewritten along with approval.php for
multiple reasons:

    -the regex/str_replace operations are not the best way to do this
    -error checking is not correct
    -if the approval file is not there, URI will be blanked out instead of just
throwing an error
    -the UPDATE statement is not protected, so it gets executed whether it has
the proper args or not (bad logic)

The patch fixes errors caused by unescaped queries, which was the point of this
bug.  Additional problems were caused by hard-coded paths in the (completely
unnecessary) str_replace calls, which was tainting the destination path, which
then affected the URI.

While testing this I realized that the dump from prod is not happening as often
as the files, which might cause the dev approval queues to contain more items
than the server has approval files (so there is no approval file to migrate to
/ftp/$type/$filename for an approval).	We should try to get the file and DB
updates synchronized.

Comment 27

[Colin Ogilvie [:cso]]

Comment on attachment 182787 [details] [diff] [review]
Added additional checking in inc_approval.php to prevent blank URIs.

Works for me in my test cases.

Note that Zicklepop's Buttons that currently exists in the approval queue is
probably going to need to be denied.

Comment 28

[alanjstr]

Landed in CVS my morgamic