search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

Reviewers browse api doesn't bring up the the addon signature details (META-INF) #6540

Closed AlexandraMoga closed 5 years ago

AlexandraMoga commented 5 years ago

STR:

  1. Authenticate with a reviewer account
  2. Check the details for the following addon: https://addons-dev.allizom.org/api/v4/reviewers/addon/616628/versions/1655510/
  3. Search for the add-on signature details - i.e the META-INF contents

Actual result: The META-INF folder is missing completely

Expected result: The META-INF contents should be retrieved, once they are available.

Notes:

EnTeQuAk commented 5 years ago

The problem here is that the git-extraction actually happens before we sign the files and we don't update the extracted data after signing.

To be honest, I'm unsure if this matters but I think to have the exact data there we should probably re-extract the file after signing and maybe commit a new version?

AlexandraMoga commented 5 years ago

Maybe @wagnerand can confirm if having this information is actually useful for reviewers. I've raised the issue only because I noticed the old file viewer has it.

EnTeQuAk commented 5 years ago

I think this is actually something we'll have to do. Autograph returns a completely new XPI file so we'd have to make sure that reviewers see exactly that file - because that's what is served to the user.

What I'm wondering is… assuming we delay git-extraction till after the signing happened (with all the consequences of forwarding author-data all the way to signing-code), would that work for all possible scenarios, add-on types etc?

cc @diox @eviljeff @kewisch for some input.

wagnerand commented 5 years ago

A small-ish but important subset of add-ons have auto-approval disabled or will do so very soon.

I agree with @EnTeQuAk that reviewers should see what's being served to users (if available).

Does that mean we have to extract twice or do you see another way?

eviljeff commented 5 years ago

the biggest downside would be the file viewer and diff wouldn't be available until after the file was approved (i.e. when it's signed). For most add-ons I don't imagine that's a huge problem because they're auto approved after a few minutes.
But, the edge cases.... add-ons that failed signing; needing to inspect in those first few minutes; add-ons with auto-approved disabled (that will include all the ~curated~recommended add-ons); etc?

You would work around the non-auto-approval by checking that on submission and just extracting those. But I suppose you'd then need to extract those twice. Or just extract all of them twice.

EnTeQuAk commented 5 years ago

Does that mean we have to extract twice or do you see another way?

The more I think about this the more I think this will be what we'll have to do. I don't think it'll be much of a problem, on the contrary, it represents the life of an XPI file nicely and if we must we can inspect all the changes made by autograph in the future too.

diox commented 5 years ago

A small-ish but important subset of add-ons have auto-approval disabled or will do so very soon.

This. I agree, you do need to extract twice unfortunately.

AlexandraMoga commented 5 years ago

Verified fixed on -dev with FF66, WIn10x64

The META-INF file is now included in the package after the add-on is signed. This applies only to new addons/new versions.

https://code.addons-dev.allizom.org/en-US/browse/616653/versions/1655540/ image