Cannot install image-uploader and filatours packaged apps with reviewer certs - receive INVALID_SIGNATURE on install

RESOLVED FIXED in Firefox 24

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: cviecco)

Tracking

Trunk
mozilla24
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:tef+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

Details

(Whiteboard: [tef-triage][target:05/15])

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Build:

Device: Unagi
Build: B2G 18 2/14/2013
Reviewer Certs Included

Steps:

1. Try installing https://marketplace.firefox.com/reviewers/apps/review/image-uploader or https://marketplace.firefox.com/reviewers/apps/review/filatours

Expected:

The app should install successfully.

Actual:

The app fails to install with INVALID_SIGNATURE.

Additional Notes:

The investigation I've done so far hasn't shown any obvious evidence that this is a client-side bug. Could be a signing bug on the marketplace side.
(Reporter)

Comment 1

5 years ago
Mentioned this to Ryan in IRC a couple of days ago to see if he could investigate why this is failing. Have you found why this is failing?
Blocks: 777048
Flags: needinfo?(rtilder)
ping?
Per my conversation with Jason on IRC last week, I can't access those Marketplace URLs since I am not a reviewer.  He was going to send me the necessary data.
Flags: needinfo?(rtilder)
(Reporter)

Comment 4

5 years ago
(In reply to Ryan Tilder [:rtilder] from comment #3)
> Per my conversation with Jason on IRC last week, I can't access those
> Marketplace URLs since I am not a reviewer.  He was going to send me the
> necessary data.

I just emailed you the zip files from reviewer tools.
-> rtilder I think.  Let me know if you need something
Assignee: nobody → rtilder
Whiteboard: p=
Is it possible for you to re-submit the packages for review and thereby have them re-signed, Jason?  I'm curious to see if a newer signature attempt will have the same problem.
Flags: needinfo?(jsmith)
(Reporter)

Comment 7

5 years ago
(In reply to Ryan Tilder [:rtilder] from comment #6)
> Is it possible for you to re-submit the packages for review and thereby have
> them re-signed, Jason?  I'm curious to see if a newer signature attempt will
> have the same problem.

Redirecting to Lisa - can we try resubmitting these apps and seeing if we can install it after we resubmit?
Flags: needinfo?(jsmith) → needinfo?(adora)
Please check the expiration date of the certificates used for the signing.

Over the weekend, the test certificates used for the Gecko unit tests expired because I put "expires in 90 days" in those certificates. I don't remember what expiration date we picked for the test certs used on the servers, but I wouldn't be surprised if there is a similar problem with them.

Attach one of the signed apps to this bug if you are still having trouble after confirming the test certs aren't expired.
As asked by lisa, I've resubmitted both the applications.
Still unable to install.  Alexandre, by chance did you delete the old versions and resubmit with the same version number?  If so, we're also seeing Bug 843419, and you'll need to change the version number.
Flags: needinfo?(adora) → needinfo?(lissyx+mozillians)
Yesterday, I've resubmitted as version 1.1. I did not deleted the old versions.
Flags: needinfo?(lissyx+mozillians)
I've deleted old 1.0 that were marked as « Disabled by Mozilla ».
(Reporter)

Updated

5 years ago
Duplicate of this bug: 845263
(Reporter)

Comment 14

5 years ago
So we're stuck on resolution in this bug.

bsmith - Can help shed some light on what you think we need to do here to diagnose and resolve this bug?
Flags: needinfo?(bsmith)
(Reporter)

Comment 15

5 years ago
Working with rtilder on this bug, the signatures for packaged apps that work vs. not work don't have distinct differences.
Updated to new certs, still unable to install these apps.
I'm removing the dependency on the packaged apps bug and closing that bug out.  Packaged apps are live, I'm treating this as a separate bug.
No longer blocks: 777048
Created attachment 732174 [details]
Package for Image Uploader
Camilo, can you take a look at this?

Probably the fastest way to figure out what to do would be to replace the privileged-app-test-1.0.zip file with the image-uploader ZIP, make the marketplace reviewer root cert (root-ca-reviewers-marketplace.der in the github repo below) trusted, and then run the test_signed_apps-marketplace.js or similar test in a debugger to find out what the problem with the package is.

In order to test it out end-to-end, you would have to use B2G Desktop (not Firefox) or a B2G device and do this craziness:

https://github.com/briansmith/marketplace-certs

And, I guess in order for cviecco to be able to test everything, he needs to have a reviewer account on marketplace.firefox.com so that these things can be accessible.

https://marketplace.firefox.com/reviewers/apps/review/image-uploader or https://marketplace.firefox.com/reviewers/apps/review/filatours

Ask cgalimidi who you need to ask for help for the marketplace.firefox.com account.
Flags: needinfo?(bsmith) → needinfo?(cviecco)
Nominating tef+ because this is a potential bug in the client that could affect app installation.
blocking-b2g: --- → tef?
(Reporter)

Comment 21

5 years ago
Bumping over to core under the assumption that this is now a client-side bug. Will bump back if it turns out this isn't right.
Assignee: rtilder → nobody
Component: Reviewer Tools → DOM: Apps
Product: Marketplace → Core
Version: 1.0 → Trunk
(Reporter)

Updated

5 years ago
Blocks: 756729
Over to Camilio to explore this a bit further. If it turns out that this can be worked around w/o a client side change, we wouldn't need to block on this, but until we know that, we'll block on this.
Assignee: nobody → cviecco
Camilo, send eviljeff (awilliamson@mozilla.com) the email address you're using for Marketplace and he can get you reviewer privs.
We want this so that we don't have to provide marketplace reviewers with a custom build. Marking as tef+ for now.

Camilo - if simple fixes can be made to the submitted apps, please mark this as [NPOTB].
blocking-b2g: tef? → tef+
Whiteboard: p= → p= [status: no solution yet]

Updated

5 years ago
Whiteboard: p= [status: no solution yet] → p= [status: no solution yet][madrid]
(Assignee)

Comment 25

5 years ago
Got access to the marketplace. Now tring to solve.
Flags: needinfo?(cviecco)
See Also: → bug 861284
(Reporter)

Updated

5 years ago
Blocks: 863032
No longer blocks: 756729
Whiteboard: p= [status: no solution yet][madrid] → p= [status: in progress][madrid]
Camilo, any estimation for this bug ?
(Assignee)

Comment 27

5 years ago
Internal error->  NS_FILE_CORRUPTED.

Besides those two apps, any other apps with the problem?

ETA, tomorrow with p=.90
today: p=0.3
(Reporter)

Comment 28

5 years ago
(In reply to Camilo Viecco (:cviecco) from comment #27)
> Internal error->  NS_FILE_CORRUPTED.
> 
> Besides those two apps, any other apps with the problem?

IQ FitFun Lite is another app. I think I'm also hitting the problem with WP Photos now too.

Updated

5 years ago
Target Milestone: --- → Madrid (19apr)
(Assignee)

Comment 29

5 years ago
The manifests are invalid. The maximum data lenght for each line cannot exceed 72 bytes,

For the app the initiated the problem this is the problem line:
Name: style/bb/menus-dialogs/valueselector/date/images/ui/shadow-invert.png



http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/JARSignatureVerification.cpp#248
(Reporter)

Comment 30

5 years ago
I'm assuming you are referring to the manifest.mf file in the META-INF directory with that line you are referring, right?

Does that imply then this is no longer a client-side bug then, since we're following the spec here?
(Assignee)

Comment 31

5 years ago
Correct it is still a bug, but not a client side bug.
(Reporter)

Comment 32

5 years ago
Thanks Camilo. I think we've reached the conclusion here that this now back to being a signing bug. Sending this back over to marketplace to rtilder.
Assignee: cviecco → nobody
No longer blocks: 863032
blocking-b2g: tef+ → ---
Component: DOM: Apps → General
Product: Core → Marketplace
Whiteboard: p= [status: in progress][madrid]
Target Milestone: Madrid (19apr) → ---
Version: Trunk → 1.0
(Reporter)

Comment 33

5 years ago
(In reply to Jason Smith [:jsmith] from comment #32)
> Thanks Camilo. I think we've reached the conclusion here that this now back
> to being a signing bug. Sending this back over to marketplace to rtilder.

When I meant "signing bug," I meant a bug on the signing service on the marketplace side.
(Reporter)

Comment 34

5 years ago
Ryan - Can you take a look?
Flags: needinfo?(rtilder)
Good work!

See http://docs.oracle.com/javase/7/docs/technotes/guides/jar/jar.html

"Line length: No line may be longer than 72 bytes (not characters), in its UTF8-encoded form. If a value would make the initial line longer than this, it should be continued on extra lines (each starting with a single SPACE)."

For example:

Name: style/bb/menus-dialogs/valueselector/date/images/ui/shadow-invert
 .png
(In reply to Camilo Viecco (:cviecco) from comment #27)
> Internal error->  NS_FILE_CORRUPTED.

Camilo, why did the openSignedJarFileAsync return NS_FILE_CORRUPTED instead of NS_ERROR_SIGNED_JAR_MANIFEST_INVALID?

In ParseAttribute in JARSignatireVerification.cpp:

  if (len > 72) {
    // The spec says "No line may be longer than 72 bytes (not characters)"
    // in its UTF8-encoded form. This check also ensures that len < INT32_MAX,
    // which is required below.
    return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
  }

and that error code seems to be propogated all the way back. Is nsJAR.cpp also doing its own parsing/checking of the manifest? If so, then that might be a bug in the client. The intent is that the nsJAR.cpp code should ignore all the stuff in META-INF because I re-implemented all that code in JARSignatureVerification.cpp in a better way.
Flags: needinfo?(cviecco)
(Assignee)

Comment 37

5 years ago
Was a bad test file during testing (Ctrl + C) during copy.
Flags: needinfo?(cviecco)
(Reporter)

Comment 38

5 years ago
Talked with Ryan in IRC - he said he would take a look into this today.
Flags: needinfo?(rtilder)

Updated

5 years ago
Assignee: nobody → rtilder
Still no news, we are now nearly at 4 months since I submitted my apps ... and no idea of the issue ?
I did work on the application this weekend, tried to take a chance and submit a new version on the Marketplace ...

After uploading this v1.2 (and removing any previous version), I'm able to download the manifest which is at https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/manifest.webapp (this URL is listed in the edit listing page).

However, this manifest is empty, and contains only "{}".
(In reply to Alexandre LISSY :gerard-majax from comment #40)
> I did work on the application this weekend, tried to take a chance and
> submit a new version on the Marketplace ...
> 
> After uploading this v1.2 (and removing any previous version), I'm able to
> download the manifest which is at
> https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/
> manifest.webapp (this URL is listed in the edit listing page).
> 
> However, this manifest is empty, and contains only "{}".

Further attemps to download the file results in either 404 or 500 errors.
Here is logcat matching this:

I/Gecko   ( 5684): -*-*- Webapps.jsm : Got http status=500 for https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/manifest.webapp
E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file: "resource://gre/modules/Webapps.jsm" line: 1687}]
E/GeckoConsole( 5982): Content JS LOG at https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min.js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR
(In reply to Alexandre LISSY :gerard-majax from comment #42)
> Here is logcat matching this:
> 
> I/Gecko   ( 5684): -*-*- Webapps.jsm : Got http status=500 for
> https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/
> manifest.webapp
> E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app
> from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file:
> "resource://gre/modules/Webapps.jsm" line: 1687}]
> E/GeckoConsole( 5982): Content JS LOG at
> https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min.
> js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR

You can't install a packaged app from the public listing page until its been approved.  The bug is about being able to install via our reviewer tools before approval.
(In reply to Andrew Williamson [:eviljeff] from comment #43)
> (In reply to Alexandre LISSY :gerard-majax from comment #42)
> > Here is logcat matching this:
> > 
> > I/Gecko   ( 5684): -*-*- Webapps.jsm : Got http status=500 for
> > https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/
> > manifest.webapp
> > E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app
> > from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file:
> > "resource://gre/modules/Webapps.jsm" line: 1687}]
> > E/GeckoConsole( 5982): Content JS LOG at
> > https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min.
> > js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR
> 
> You can't install a packaged app from the public listing page until its been
> approved.  The bug is about being able to install via our reviewer tools
> before approval.

I'm not talking about public listing, I'm talking about application for which I'm developper, thus I have access even if it's not yet approved.
You /are/ talking about the public listing page, even though the app isn't publicly installable yet.  A packaged app is only signed on approval.  The app isn't approved so isn't signed so isn't installable.
This bug is about a separate file; only available from the reviewer pages, with an account with appropriate permissions; which is signed with a different certificate that isn't present on FirefoxOS by default.  This /is/ available before approval.
(In reply to Andrew Williamson [:eviljeff] from comment #45)
> You /are/ talking about the public listing page, even though the app isn't
> publicly installable yet.  A packaged app is only signed on approval.  The
> app isn't approved so isn't signed so isn't installable.
> This bug is about a separate file; only available from the reviewer pages,
> with an account with appropriate permissions; which is signed with a
> different certificate that isn't present on FirefoxOS by default.  This /is/
> available before approval.

Then there is another bug here: that I'm being able to see the app with an « Install » button.
Created attachment 743011 [details]
The application is installable while it should not ?

Please find attached a screenshot showing that I can actually install the app from my Nexus S, after adding the Marketplace account used on the device as a owner team member and accepting the developper agreement.
Patched and will work with ops to make sure it gets deployed as soon as possible. https://github.com/mozilla/signing-clients/pull/5
Status: NEW → ASSIGNED

Comment 49

5 years ago
Just for information:
I am the author of app "IQ Fit Fun Lite" and got my Peak today.
I was able to install the app using simulator's new push feature.
Macbook Retina -> FF 20.0 -> simulator 3.0pre8
(In reply to michbier from comment #49)
> Just for information:
> I am the author of app "IQ Fit Fun Lite" and got my Peak today.
> I was able to install the app using simulator's new push feature.
> Macbook Retina -> FF 20.0 -> simulator 3.0pre8

Seems consistent since the issue is with signing on marketplace.
Status: ASSIGNED → NEW
(In reply to Ryan Tilder [:rtilder] from comment #48)
> Patched and will work with ops to make sure it gets deployed as soon as
> possible. https://github.com/mozilla/signing-clients/pull/5

I see this has been merged, any idea when it will be live ?
Status: NEW → ASSIGNED
(In reply to Alexandre LISSY :gerard-majax from comment #50)
> (In reply to michbier from comment #49)
> > Just for information:
> > I am the author of app "IQ Fit Fun Lite" and got my Peak today.
> > I was able to install the app using simulator's new push feature.
> > Macbook Retina -> FF 20.0 -> simulator 3.0pre8
> 
> Seems consistent since the issue is with signing on marketplace.

Right. The push feature skips all the JAR integrity and signature checking, so signature-related errors will never be caught with it.
(In reply to Alexandre LISSY :gerard-majax from comment #51)
> I see this has been merged, any idea when it will be live ?
>

It should be pushed to prod with tomorrow regularly scheduled release.  At that point affected packages can be re-signed with the production key.

--Ryan

Comment 54

5 years ago
I am a little bit confused, do the authors of the apps like mine (IQ FitFun) need to upload a new app version or just wait until approval will continue?
(Reporter)

Comment 55

5 years ago
Wil - Should this be closed, given that a fix has been pushed and merged?
Flags: needinfo?(clouserw)
btw, I've just tried to install filatour, and IQ Fitfun Lite, and installing is failing for both of them.
(Reporter)

Comment 57

5 years ago
(In reply to Andrew Williamson [:eviljeff] from comment #56)
> btw, I've just tried to install filatour, and IQ Fitfun Lite, and installing
> is failing for both of them.

Well, I guess that answers my needinfo request. This isn't fixed.
Flags: needinfo?(clouserw)
I'm not sure if the reviewer cert signing is done on upload, on first use, or on-demand each time.  If its anything other than on-demand something might need to be run to force re-signing on existing apps.
Okay, all the cached reviewer cert signed packaged apps have been cleared off the disk now so Marketplace should be signing with the new signing code.

They're still failing though for me though.

Ryan, do they work for you?  (You now have reviewer access on Marketplace)
Flags: needinfo?(rtilder)
(In reply to Andrew Williamson [:eviljeff] from comment #58)
> I'm not sure if the reviewer cert signing is done on upload, on first use,
> or on-demand each time.  If its anything other than on-demand something
> might need to be run to force re-signing on existing apps.

For the record, they are currently signed on demand and then saved on disk permanently.  Another bug is filed to gc the cached versions on disk once a day.
The re-signed packages look correct for the 72 byte line length limit.  I fear we're going to need Brian to see what the error being generated is now.
Flags: needinfo?(rtilder) → needinfo?(bsmith)
Camilo, you investigated this before. Do you remember anything else wrong other than the 72-byte line issue? Is there any way you can investigate this again?
Flags: needinfo?(bsmith) → needinfo?(cviecco)
(Assignee)

Comment 63

5 years ago
Sure, but it will have to wait until tomorrow morning. Wait for more info.
Flags: needinfo?(cviecco)
(Assignee)

Comment 64

5 years ago
it is possible to get a raw copy of the new app?
Camilo, what do you mean by "raw copy of the new app?"

Btw, the same INVALID_SIGNATURE error is also showing in logcat when trying to install the packaged version of the preinstalled Notes app.

https://marketplace.firefox.com/reviewers/apps/review/notes-packaged
Flags: needinfo?(cviecco)
Blocks: 869212
(Reporter)

Comment 66

5 years ago
FYI - with those meta bugs like bug 869212, I was only intending to track app-specific issues there, not other issues such as this bug.
No longer blocks: 869212
Alexandre: can you attempt a possible workaround for us to get your app listed?  If this bug is still caused by long file paths, shorter ones will not be affected.  So can you rename some files/folders so no file path is longer than 67 characters?
Flags: needinfo?(lissyx+mozillians)

Comment 68

5 years ago
IQ Fit Fun manifest looks like this:

Any recommendation how to change it would be great:

{
  "name": "IQ FitFunLite",
  "version": "1.2.6.1",

  "description": "Have fun with 43 free brain teasers for young and old (lite version).",
  "icons": {
     "16": "/IQFitFun/res/drawable/iqfitfun_lite_icon_round_16.png",
    "128": "/IQFitFun/res/drawable/iqfitfun_lite_icon_round_128.png"
  },
  "developer": {
    "name": "DsignMatters, Michael Biermann",
    "url": "http://sites.google.com/site/dsignmatters/"
  },
  "default_locale": "en",
  "launch_path": "/IQFitFun/index.html",
  "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'",
  "installs_allowed_from": ["*"]
}
Flags: needinfo?(lissyx+mozillians)
(In reply to michbier from comment #68)
> IQ Fit Fun manifest looks like this:
> 
> Any recommendation how to change it would be great:

There isn't any issue in the app manifest.  Its the file paths - i.e. folders+filename.  From a quick look I think its the filenames of the image assets e.g. IQFitFun/res/drawable/background_blackboard_desc_164x59_wo_shadow.png
(Assignee)

Comment 70

5 years ago
Line size restrictions where being enforced the decoded line not on a per line basis. Patch coming soon.
Flags: needinfo?(cviecco)
(Reporter)

Comment 71

5 years ago
(In reply to Camilo Viecco (:cviecco) from comment #70)
> Line size restrictions where being enforced the decoded line not on a per
> line basis. Patch coming soon.

This needs a gecko patch I take it, right?

If so, I'd close this bug for the work rtilder did and open a new bug in the core to track the gecko work. Nom that also for tef.
(Assignee)

Comment 72

5 years ago
Created attachment 749140 [details] [diff] [review]
move size check from parseline to readline
(Assignee)

Updated

5 years ago
Attachment #749140 - Flags: review?(bsmith)
(Reporter)

Updated

5 years ago
Assignee: rtilder → nobody
Component: General → Security: PSM
Product: Marketplace → Core
Version: 1.0 → Trunk
(Reporter)

Comment 73

5 years ago
tef? since we now know there's a client change required here. Marketplace piece is finished per an earlier comment.
blocking-b2g: --- → tef?
Comment on attachment 749140 [details] [diff] [review]
move size check from parseline to readline

Review of attachment 749140 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test for this to test_signed_apps.js (*not* test_signed_apps-marketplace.js). It should be easy to modify the existing test case to include an entry with a path that causes the manifest entry to wrap.

::: security/manager/ssl/src/JARSignatureVerification.cpp
@@ +197,5 @@
>  // On input, nextLineStart is the start of the current line. On output,
>  // nextLineStart is the start of the next line.
>  nsresult
>  ReadLine(/*in/out*/ const char* & nextLineStart, /*out*/ nsCString & line,
> +         bool allowContinuations = true, size_t lineLimit = 72 )

Why is lineLimit a parameter? AFAICT, it should be a local static constant, or even just use a literal "72" below with a comment that says this restriction is from the spec.

@@ +222,5 @@
> +    }
> +    
> +    // We can probably be more strict as the spec says: "Implementations 
> +    // should support 65535-byte (not character) header values..."
> +    if (currentLength >  INT32_MAX) {

whitespace around ">". Also, I suggest we compare against 65535. Even 65535 is ridiculously long for our needs. Also, IIRC, elsewhere in JarSignatureVerification.cpp we limit the total size of the manifest file to be fairly small to prevent OOM since we read the entire file into memory.
Attachment #749140 - Flags: review?(bsmith) → review-

Updated

5 years ago
Whiteboard: [tef-triage]
(Assignee)

Comment 75

5 years ago
Created attachment 749561 [details] [diff] [review]
move size check (v2)
Attachment #749140 - Attachment is obsolete: true
(Assignee)

Comment 76

5 years ago
Created attachment 749566 [details] [diff] [review]
move size check (v2.1)
Attachment #749561 - Attachment is obsolete: true
(Assignee)

Comment 77

5 years ago
Comment on attachment 749566 [details] [diff] [review]
move size check (v2.1)

Review of attachment 749566 [details] [diff] [review]:
-----------------------------------------------------------------

previous review comments addressed
Attachment #749566 - Flags: review?(bsmith)
tef+ since this is blocking marketplace testing.
Assignee: nobody → cviecco
blocking-b2g: tef? → tef+

Updated

5 years ago
Whiteboard: [tef-triage] → [tef-triage][target:05/15]
Comment on attachment 749566 [details] [diff] [review]
move size check (v2.1)

Review of attachment 749566 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good. Please add the test.

::: security/manager/ssl/src/JARSignatureVerification.cpp
@@ +215,5 @@
> +    currentLength = line.Length();
> +
> +    // The spec says "No line may be longer than 72 bytes (not characters)"
> +    // in its UTF8-encoded form.
> +    const size_t lineLimit = 72;

nit: make this static.

@@ +726,5 @@
>      NS_ENSURE_TRUE(signerCert, NS_ERROR_OUT_OF_MEMORY);
>  
>      signerCert.forget(aSignerCert);
>    }
> + 

Undo this whitespace change.
Attachment #749566 - Flags: review?(bsmith) → review-
(Assignee)

Comment 80

5 years ago
push to try with tests before cleanup:
https://tbpl.mozilla.org/?tree=Try&rev=fc00df66c7cb
(Assignee)

Comment 81

5 years ago
Created attachment 750096 [details] [diff] [review]
move size check (v3)
Attachment #749566 - Attachment is obsolete: true
Attachment #750096 - Flags: review?(bsmith)
(Assignee)

Comment 82

5 years ago
Created attachment 750098 [details]
move size check (v3.1)

now with tests!
Attachment #750096 - Attachment is obsolete: true
Attachment #750096 - Flags: review?(bsmith)
Attachment #750098 - Flags: review?(bsmith)
(Assignee)

Comment 83

5 years ago
Created attachment 750100 [details] [diff] [review]
move size check (v3.1)
Attachment #750098 - Attachment is obsolete: true
Attachment #750098 - Flags: review?(bsmith)
Attachment #750100 - Flags: review?(bsmith)
Comment on attachment 750100 [details] [diff] [review]
move size check (v3.1)

Review of attachment 750100 [details] [diff] [review]:
-----------------------------------------------------------------

There is no test that verifies that we reject a line longer than 72 bytes. However, that isn't the important thing here; I don't actually care whether we accept lines that are too long or not, as long as we correctly accept the continuation form of lines, which is what this new test *does* test.

::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py
@@ +14,5 @@
>  max_entry_count = 100 * 1000
>  max_entry_filename_len = 1024
>  max_mf_len = max_entry_count * 50
>  max_sf_len = 1024
>  

Add a comment along the lines of "Lines in a JAR manifest file are required to be 72 bytes or less."

@@ +18,5 @@
>  
> +def reformat_line_to_JAR_line(inline):
> +  newline = inline[0:72]
> +  leftover = inline[72:]
> +  while len(leftover) > 0 :

s/ :/:/

Add a comment along the lines of "On continuation lines, the leading space allows only 71 bytes from the original line to be used."

Nit: The words "newline" and "inline" both have completely different meanings from what you are using them here and those names don't match the naming convention used throughout the file. I suggest using "input_line" and "output_line".
Attachment #750100 - Flags: review?(bsmith) → review+
(Assignee)

Comment 85

5 years ago
Created attachment 750144 [details] [diff] [review]
move size check (v4)

comments addressed. keeping r+ from bsmith
Attachment #750100 - Attachment is obsolete: true
Attachment #750144 - Flags: review+
(Assignee)

Comment 86

5 years ago
v4 push to try

https://tbpl.mozilla.org/?tree=Try&rev=53c8c83a85bc

Updated

5 years ago
Whiteboard: [tef-triage][target:05/15] → [status: needs landing][tef-triage][target:05/15]
(In reply to Camilo Viecco (:cviecco) from comment #86)
> v4 push to try
> 
> https://tbpl.mozilla.org/?tree=Try&rev=53c8c83a85bc

Please land or add checkin-needed if the try results look good.
(Assignee)

Comment 88

5 years ago
try failed on win32 due to filename length limitations. Since this only affects the test there will be a new bug on the tests and land the fix. ASAP

try: https://tbpl.mozilla.org/?tree=Try&rev=c641101def5c
(Assignee)

Comment 89

5 years ago
Created attachment 750813 [details] [diff] [review]
move size check (v5)

removal of tests so tha we can land this.
keeping r+ from bsmith
Attachment #750813 - Flags: review+
(In reply to Camilo Viecco (:cviecco) from comment #89)
> Created attachment 750813 [details] [diff] [review]
> move size check (v5)
> 
> removal of tests so tha we can land this.
> keeping r+ from bsmith

Are we good to land this now? If so I think we just need to request check-in
status-b2g18: --- → affected
status-b2g18-v1.0.1: --- → affected
https://hg.mozilla.org/mozilla-central/rev/cb242a1cccb2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [status: needs landing][tef-triage][target:05/15] → [status: needs uplift][tef-triage][target:05/15]
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b87559cef261
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/67ac7c9e74fb
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: affected → fixed
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
Whiteboard: [status: needs uplift][tef-triage][target:05/15] → [tef-triage][target:05/15]
(Reporter)

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith
(Reporter)

Comment 93

5 years ago
So Andrew ended up testing this today on a 5/21 build and this isn't working for him. I'm following up with Rob & Ryan to find out why.
Keywords: verifyme
(Reporter)

Comment 94

5 years ago
Looks like comment 93 is a false alarm. We didn't have the reviewer certs installed when we tested this. The two apps in question here can now be installed on 1.01. Marking as verified.
status-b2g18-v1.0.1: fixed → verified
My application has now been validated :)

Comment 96

5 years ago
My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1 working day", but nothing seems to happen for more than 40 days.

Comment 97

5 years ago
(In reply to michbier from comment #96)
> My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1
> working day", but nothing seems to happen for more than 40 days.

My app "IQ FitFun Lite" has been approved today and is listed on the Firefox Marketplace.
Many thx
    Michael
(In reply to michbier from comment #96)
> My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1
> working day", but nothing seems to happen for more than 40 days.

bug 845263 was tracking that app - but it seems to have slipped through the cracks.  I've reviewed it now.
You need to log in before you can comment on or make changes to this bug.