290642 - [Submission] Additem must not error for unrecognized GUIDs

Status: RESOLVED FIXED

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

Description

[alanjstr]

Error! The MinAppVer for Thunderbird of 1.0 in install.rdf is invalid.
Error! The MaxAppVer for Thunderbird of 1.0 in install.rdf is invalid.
Errors were encountered during install.rdf checking...
Aborting...

Comment 1

[alanjstr]

The problem is that it gets to Nvu, can't find it, and dies.  Instead it should
realize that zero rows are returned during app lookup and just move on.

Comment 2

[alanjstr]

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

Comment 3

[Wolf [:wolf]]

Attachment: Proposed Fix

Proposed patch, based on Comment #1 and my own research. Adds a check to see if
there's any rows returned matching the guid and skips the app if there's not.

This isn't a problem since the code in step2 generates the compatibility list
based on recognized GUIDs in the applications table, so if somebody puts in a
bogus Firefox GUID by accident, it'll get ignored just like a whole new app
would.
Also fixed the bogus error messages by unseting $appname at the beginning of
the loop and making a null $appname use the guid.

I don't have a dev enviroment to test this though, so it *should* be tested
before being used on the live site.

Comment 4

[alanjstr]

Do you need to unset them since 2 lines later you set them?  Its already
established that you found a valid row.

Comment 5

[Wolf [:wolf]]

They only get set /if/ # of rows is greater than 0, in the bug before it wasn't
getting set as null since the while loop was never entered.

Comment 6

[Michael Morgan [:morgamic]]

Comment on attachment 180987 [details] [diff] [review]
Proposed Fix

- patch does not fix problem described
- it looks like a GROUP BY AppName fixes the stated problem
- need to look at this more and determine what the real source of the problem
is -- Wolf, could you get set up with an account on chameleon and hammer on
this?

Comment 7

[Colin Ogilvie [:cso]]

(In reply to comment #6)
> - patch does not fix problem described

The patch fixes the problem described on my test install?

> - it looks like a GROUP BY AppName fixes the stated problem

That makes matters much much worse

> - need to look at this more and determine what the real source of the problem
> is

The cause of the problem is that an invalid GUID returns 0 rows in the query,
but the variables still contain the previous application names. This, of course,
reports a bogus error message.

This patch fixes the bogus error message, and means extensions with GUIDs that
we do not support can be added to UMO for the applications that we DO support.

What is the actual problem with the patch you found, as my testing shows it
works great?

Comment 8

[alanjstr]

Comment on attachment 180987 [details] [diff] [review]
Proposed Fix

The patch looks good to me.  Colin says the patch works. 

Please take another look at this.

Comment 9

[Wolf [:wolf]]

Any progress on actually reviewing the attached patch? :-)

Comment 10

[Wolf [:wolf]]

after 7 months waiting for review, this simple patch really should be done with by now...

morgamic: Is there a plan to review this in the known future, if not, mind bouncing this to somebody who can? Its senseless for a simple change to wait this long to be reviewed... :-)

Comment 11

[Michael Morgan [:morgamic]]

Comment on attachment 180987 [details] [diff] [review]
Proposed Fix

- bit-rotted
- flawed logic -- you could potentially submit an addon with no valid applications; shouldn't we have at least one valid application?

Comment 12

[Michael Morgan [:morgamic]]

Wolf - this fell through the cracks - thanks for reminding me.

Comment 13

[Michael Morgan [:morgamic]]

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

Comment 14

[Michael Morgan [:morgamic]]

Dan - do you see support for unrecognized GUIDs as a security risk?

Comment 15

[Michael Morgan [:morgamic]]

Attachment: Band-aid for a massive head wound.

Patch to require at least one valid GUID, and for all matching GUIDs min and max versions are still enforced.

Legacy code still needs to be stripped out, and the entire 800+ line file needs to be rewritten, time permitting.

Comment 16

[Michael Morgan [:morgamic]]

Attachment: Forgot -up8N.

Forgot -up8N.

Comment 17

[Robert William Vesterman]

I'm guessing, from the fact that 319074 has been marked as a duplicate of this bug, that Flock is going to become an "unrecognized" GUID?

Could that also be done for Sunbird? That one always causes min/maxAppVersion reject problems too, due to various reasons.

What exactly will be the list of "recognized" GUIDs?

Thanks.

Comment 18

[Michael Morgan [:morgamic]]

Flock is a non-Mozilla application, Sunbird is.  Non-recognized GUIDs are applications that are not Mozilla projects.

The Sunbird version issue will be handled by using 'understandable' versioning as was discussed in that bug.

Comment 19

[alanjstr]

The application manager says that AMO knows about Flock.  

Comment 20

[Lloyd D Budd]

(In reply to comment #18)
> Flock is a non-Mozilla application, Sunbird is.  Non-recognized GUIDs are
> applications that are not Mozilla projects.

Flock is a mozilla application, as in built on the Mozilla platform.

Why was it removed from "Valid App Versions for Addon Developers"
https://addons.mozilla.org/faq.php
it is no less valid than Netscape nor Nvu.


NVU and Netscape are 

Comment 21

[Michael Morgan [:morgamic]]

This has been checked in and will be pushed this morning.

Comment 22

[Michael Morgan [:morgamic]]

Comment on attachment 205123 [details] [diff] [review]
Forgot -up8N.

Don't want to wait 7 months.  :)

Comment 23

[Wolf [:wolf]]

Oops. I looked at it and it looked fine. :-) Forgot to mark it though.

Comment 24

[Michael Morgan [:morgamic]]

Suuure.  ;)