1214363 - We don't block users from submitting the same add-on over and over again

Status: VERIFIED FIXED

(Marketplace Graveyard :: Developer Pages, defect, P2)

Description

[krupa raj[:krupa]]

On AMO, we uniquely identify add-ons with GUIDs and stop duplicate submission. On Marketplace I can submit the same add-on enough times to cause a Denial of Service attack.

Comment 1

[Chuck Harmston [:chuck]]

Is the concern a UX one or a security one? If it's a security one, we can add some reasonable rate limiting to these endpoints. 60/hour would do fine. The UX concern would be more tricky, as there is nothing uniquely identifiable about an addon file.

Comment 2

[Andrew Williamson [:eviljeff]]

This is blocked on the blocklisting/unique-id decision platform has to make.  If it goes down the desktop-like route then there will be a developer specified guid in the manifest; otherwise I guess we do what's done for apps (nothing? stop duplicate addon names by the same developer?)

Comment 3

[David Durst [:ddurst]]

FxOS will pull the UUID from the mini-manifest.

But this doesn't help us solve this problem. How do we identify a duplicate submission? Rate limiting is one thing, database pollution (and integrity) is another.

Maybe the safest route *is* to require the developer to submit it and *also* put it in the mini-manifest.

Comment 4

[Kevin Ngo [:ngoke]]

For duplicate submissions, maybe we could compute a checksum of the add-on files.

Comment 5

[Mathieu Pillard [:mat]]

As mentioned by email, I think a checksum of the zip file itself would be enough. I really don't like the idea of requiring developers to submit their own uuid.

Comment 6

[Mark Striemer [:mstriemer]]

I feel as though there should be something even if it's a slug. I haven't looked at what sort of manifest file is here but if you look at other package managers like npm's package.json [1] and python's setup.py [2] a name is required and probably a version.

I'm currently adding a new submission API to AMO that will support command-line and if we didn't use the UUID from the manifest we'd have no way of identifying the add-on without asking the developer each time.

So forcing the developer to come up with a UUID doesn't sound awesome but making them give it a unique name is probably a good idea.

[1] https://github.com/mozilla/fireplace/blob/9c3b682ae8ded6140f1658ef923886f2faf34089/package.json#L2
[2] https://github.com/mozilla/app-validator/blob/7d45c525ae912db833a2423062bc3559b7cece13/setup.py#L5

Comment 7

[David Durst [:ddurst]]

Hrm, well we don't let them set their own slug either, do we? (For add-ons, in marketplace.)

Mat had a good point in email that UUID is a barrier (albeit an established convention) but that it doesn't do any better than a checksum in terms of preventing a version being submitted as a new add-on.

Comment 8

[Andrew Williamson [:eviljeff]]

the scenarios we're looking at are:
a) developer accidentally uploading the same identical zip file twice
b) developer accidentally uploading a subsequent version of an existing add-on as a new add-on
c) malicious/deviant developer trying to work around our checks

I think the most likely scenario is (b) and checksums won't solve it.  The only reason I can think of for a developer to do (a) is a server problem on Marketplace side; otherwise there would be a minor change in the zip file and the checksum would be different. 

A unique name would work for (b) -and AMO does this already-, but I can't remember why we didn't enforce it for apps.  The error message could be something like 'an add-on with this name already exists' and/or 'you already have an add-on with this name - upload a new version to it or delete it first'.  Wouldn't work if the developer decides to slightly change the add-on name of course.

Forcing the developer to add a UUID and extra cruft to the manifest is non-awesome, sure.  But is something they're unlikely to change in the manifest unknowingly so would be the most effective against (b).  Wouldn't really stop (c) but nothing automated will.

Comment 9

[David Durst [:ddurst]]

We actively want to allow the non-accidental version of b), right? An add-on goes as far is it can (for whatever reason), but the new "version" needs to be separate.

Nothing prevents c), sure.

And a) seems like the kind of thing we could check against and ask for confirmation for.

I don't love the idea of checking the name, but it's not terrible. I agree that adding the UUID to the manifest is a needless barrier, but one that isn't going away anytime soon on AMO either. In light of those facts -- and because once we have them supplying the UUID, it's really hard to go back -- I think we should leave the UUID as being generated and do some due diligence dupe-checking however possible. [Bonus points if we let the user know about possible dupes, but I doubt that happens for MVP.]

Comment 10

[Mathieu Pillard [:mat]]

My suggestions for this:

- In content tools, if the developer already has an add-on, maybe add a link to the new version submission. Something subtle, just saying "hey, did you want to upload a new version instead?"

- At validation time, the API should extract the name in the zip and look for addons with the same name, in the same language, with the same developer in the authors. If one is found, return a warning that the content tools can display to the developer.

- Show the number of add-ons submitted by the developer somewhere in reviewer tools. Maybe a click to quickly show their names and links without changing the page.

These additions should cover the main non malicious use cases, and we can iterate on this later.

Comment 11

[Mathieu Pillard [:mat]]

Fixed in https://github.com/mozilla/zamboni/commit/ae12f5b7538486a9330c5166dde926d388b1aa2e

I decided to block the creation of the add-on completely in the API, returning a 400.

STR:
- Try to upload a new add-on. It should just work :)
- Try to upload a new add-on (not a new version) with the same name. It should display an error saying you already uploaded an add-on with this name
- Try to upload a new version of an existing add-on, keeping the same name. It should just work.

Comment 12

[Valentina Virlics [:vvalentina], Ecosystem QA]

Verified as fixed on MP-dev FF46(Win 7).
Postfix screenshot: http://screencast.com/t/Ij3Ejo4cC74
Closing bug.