Addons-server: .tgz file is accepted on Upload but causes silent error in Describe step

[d86leader]

Describe the problem and steps to reproduce it:

Creating this issue after a brief discussion on discourse.mozilla.org

I'm trying to submit an addon. I follow the steps from extensionworkshop.com, and all goes well, no errors when uploading anything. On the "Describe Add-on" web page, when I fill all the required fields and press submit, I'm directed to the same page with no changes. The exact page url is https://addons.mozilla.org/en-US/developers/addon/pure-image-redirecter/submit/details

What happened?

Nothing happens when I press the "Submit" button

What did you expect to happen?

I expected my addon to be submitted

Anything else we should know?

The discussion happened here: discourse
In that discussion you can find a full screenshot of the submit page with all the fields filled out.

The addon name is "pure-image-redirecter", and the source code is here

[jvillalobos]

Have you tried with a different Firefox profile or a different browser? Maybe you changed something in your JavaScript or security settings?

[d86leader]

I did change thing in my security settings, basically enabled all of the firefox privacy stuff, and set cookies to be remembered only for one session.

Tried it again in a fresh virtual machine with a fresh firefox 75.0 (linux) with all default settings, and still the same result

[jvillalobos]

Okay, I just tried it myself and I get the same behavior. No error messages and nothing stands out in the Web Console.

@d86leader could you attach the file that you uploaded?

[d86leader]

This is the extension itself: pure_image_redirecter-1.0.zip

This is the sources archive: image_redirecter.tar.gz - it had tgz extension when I uploaded it to amo, but github disallows it so I changed it to tar.gz just for github.

[AlexandraMoga]

As far as I can tell, this issue is caused by the .tgz archive type uploaded as source file.
If I'm uploading the extension mentioned above without source files or with a different archive type as source files (i.e .zip, .tar.gz) , the issue will not reproduce.
I've also tried with a different add-on and .tgz source file and I was able to reproduce the issue.

[d86leader]

I tried it with .tar.gz and it worked indeed! So, is it intended behaviour? I myself like calling my archives with a simpler extension.

[jvillalobos]

I'm not sure what the intended behavior should be, but either we should support this format or show an error in the upload step.

[wagnerand]

@jvillalobos What are your opinions on only allowing .zip files as source packages?

[jvillalobos]

.tar.gz is pretty common, so I would like us to continue supporting it. But if it's more trouble keeping it than dropping it, I'm okay with it.

[krupa-raj]

What type is it? How frequently will users hit this issue?
Bug because of missing file type/extension support/Missed error handling
Missed error handling - we should show a proper error message when we don’t accept a certain file extension
Frequency: low
How was this found?
Filed by an add-on developer after discussing the issue on discourse.mozilla.org
Issue was found on linux
What caused it?
Where was the gap which caused this issue
We need to clarify what filetypes/extensions we actually support
Why didn’t we catch it sooner?
This file type/extension is not so common
How do we ensure it doesn’t repeat?
If the uploaded file has a type/extension which we don’t support, we fail with a graceful error message.
Is this a symptom of a broader issue
Our filetype/extension detection is not as robust as it should be. Something for us to look into.

[Lusito]

10254 seems to be about this. Has this changed? I was just looking into if this was supported or not.

The code line changed back then seems to be still accepting tgz:
https://github.com/mozilla/addons-server/blob/master/src/olympia/devhub/forms.py#L734

I haven't tried to upload a tgz yet.

[diox]

It should have worked, as you pointed out the form still accepts it. The error gotta be somewhere else.

[Lusito]

Just wanted to say, I can reproduce this.
I just tried uploading a tgz file as source and the final step where you write down release notes does not allow submitting.. It actually creates a post, but it just keeps showing the same form afterwards.

I then tried editing the version manually (via the "view all") and there I was able to set the text and upload the source file.

There is a big BUT here though: When I click the "view current" link to the source-code, it gives me a download filename "forget_me_not-2.2.8-src.tar_eVDiNjo.bz2". The file content is identical to the byte, but the extension is wrong.

[Lusito]

So, I checked the code again and found this line where tgz is not checked:

https://github.com/mozilla/addons-server/blob/32eae5e1ae414f348fae3e272c9fc99dad0a9ec0/src/olympia/versions/models.py#L50

[diox]

Yeah, I see the bug now.

What's happening is that the form does accept .tgz, so the upload goes through, but when it comes to storing it, source_upload_path() loops over VALID_SOURCE_EXTENSIONS, breaking the loop when the file extension matches, assuming it will break, and... it never does, and it unfortunately ends up picking the last extension in the list of VALID_SOURCE_EXTENSIONS. So the .tgz ends up being uploaded as .tar.bz2.

I'm still not clear why this is causing the page to be shown again - I assume the form must be considered invalid while not showing the actual error, but I just don't know yet why that happens. I'm also not sure why we get this weird tar_eVDiNjo.bz2 instead of .tar.bz2 - it's wrong in both cases, but not what I'd expect. There must be some additional check somewhere, maybe in django.

VALID_SOURCE_EXTENSIONS should include .tgz for sure, but also source_upload_path() should be written more defensively. And we should figure out why the actual error is hidden.

[diox]

'm also not sure why we get this weird tar_eVDiNjo.bz2 instead of .tar.bz2 - it's wrong in both cases, but not what I'd expect. There must be some additional check somewhere, maybe in django.

Ah, that's probably a red herring - just the storage class trying to find an alternate filename with the same extension because the file already exists - and it only looks at the .bz2 part.

[jvillalobos]

VALID_SOURCE_EXTENSIONS should include .tgz for sure

Can that part be split into a good first bug? Would that fix the problem?

[diox]

I've finished investigating. The TL;DR version is that yes, changing VALID_SOURCE_EXTENSIONS to include .tgz would fix the bug. Below are more details:

I think QA and I never noticed it before because this bug only manifests in this form at initial submission time:
When providing sources for a version, we still get the bug causing the source to be considered a .tar.bz2, but everything still works - the source file just get saved with the wrong extension.

At submission time however, in the details step, we accidentally recheck that the source is correct because we re-use the full form for the approval notes + the Version instance we already have, which has the source set already. So we try to read the source again, but since the extension is incorrect we throw an error, making the form invalid, but never show it to the user. Since the form is invalid so we keep showing the details step.

I'll fix it myself now that I've gone full Sherlock Holmes on this.

[AlexandraMoga]

Verified fixed on -dev.
Submissions of new add-ons that contain a .tgz archive as source file can be successfully finalized now. The other types supported by VALID_SOURCE_EXTENSIONS continue to work as expected.