1013347 - Signing breaks on Metro y Metrobus lite app - UnicodeEncodeError: 'ascii' codec can't encode character u'\xfa'

Status: RESOLVED FIXED

(Marketplace Graveyard :: Reviewer Tools, defect, P1)

Description

[Andrew Williamson [:eviljeff]]

http://sentry.mktmon.services.phx1.mozilla.com/mkt/marketplacefirefoxcom/group/6616/

https://marketplace.firefox.com/reviewers/apps/review/metro-y-metrobus-lite

Generating the reviewer signed zip is failing.  Sentry shows "UnicodeEncodeError: 'ascii' codec can't encode character u'\xfa' " indicates it is chocking on something in the zip file.

Comment 1

[Kumar McMillan [:kumar]]

Stacktrace (most recent call last):

  File "django/core/handlers/base.py", line 114, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "newrelic/hooks/framework_django.py", line 492, in wrapper
    return wrapped(*args, **kwargs)
  File "addons/decorators.py", line 32, in wrapper
    return f(request, addon, *args, **kw)
  File "mkt/reviewers/views.py", line 835, in wrapper
    return f(request, addon, *args, **kw)
  File "mkt/reviewers/views.py", line 904, in get_signed_packaged
    path = addon.sign_if_packaged(version.pk, reviewer=True)
  File "mkt/webapps/models.py", line 947, in sign_if_packaged
    return packaged.sign(version_pk, reviewer=reviewer)
  File "newrelic/hooks/application_celery.py", line 61, in wrapper
    return wrapped(*args, **kwargs)
  File "celery/app/task.py", line 329, in __call__
    return self.run(*args, **kwargs)
  File "nuggets/celeryutils.py", line 35, in wrapped
    return fun(*args, **kw)
  File "lib/crypto/packaged.py", line 161, in sign
    sign_app(file_obj.file_path, path, ids, reviewer)
  File "lib/crypto/packaged.py", line 29, in sign_app
    return _sign_app(src, dest, ids, reviewer, tempname)
  File "lib/crypto/packaged.py", line 60, in _sign_app
    log.info('App signature contents: %s' % jar.signatures)
  File "signing_clients/apps.py", line 261, in signatures
    self._sig = Signature([self._sign(f) for f in self._digests],
  File "signing_clients/apps.py", line 245, in _sign
    digests = _digest(str(item))

Comment 2

[Andy McKay]

The problem is this file name: FIRFOX-CPMM01_Casos Prueba Metro - Metrobús_20140423.pdf, if the developer want's to get approved while we figure this out, just changing that to an ascii name will unblock them.

Comment 3

[Andy McKay]

Anything we do here has to match whatever the client does when it reads in the files and creates the digests to ensure that they are the same. I'm not sure what that does, I read some docs and couldn't find anything. rtilder, any ideas?

Comment 4

[Ryan Tilder [:rtilder]]

(In reply to Kumar McMillan [:kumar] (needinfo for quickness) from comment #1)
> Stacktrace (most recent call last):
>
>   [snip]
> 
>   File "signing_clients/apps.py", line 245, in _sign
>     digests = _digest(str(item))

Do you have the actual exception?  I think I know how to fix it but it occurred to me that I should double check.

Comment 5

[Ryan Tilder [:rtilder]]

Ignore me.  Just saw the full bug title.  Still adjusting to Australis's lack of title bar.

Comment 6

[Andy McKay]

rtilder says he's got a handle on this one.

Comment 7

[Ryan Tilder [:rtilder]]

Attachment: UTF-8 support and test

The spec for zip files only supports extended ASCII and UTF-8 for filenames and comments.  See http://www.pkware.com/documents/casestudies/APPNOTE.TXT and search for "language encoding" for details

Comment 8

[Kumar McMillan [:kumar]]

Comment on attachment 8434204 [details] [diff] [review]
UTF-8 support and test

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

r+wc but it looks like some files are missing from the patch

::: signing_clients/apps.py
@@ +88,5 @@
> +        # and search for "language encoding" for details
> +        #
> +        # See https://bugzilla.mozilla.org/show_bug.cgi?id=1013347
> +        if type(self.name) == unicode:
> +            name = "Name: %s" % self.name.encode("utf-8")

Seems like the formatting should be separate:

if ...:
    name = self.name.encode("utf-8")
else:
    name = self.name
name = "Name: %s" % name

::: signing_clients/tests/__init__.py
@@ +75,4 @@
>  SHA1-Digest: lIbbwE8/2LFOD00+bJ/Wu80lR/I=
>  """
> +# Test for Unicode
> +UNICODE_MANIFEST = """Manifest-Version: 1.0

nit: I'd call this non_ascii_manifest since unicode isn't really an encoding

@@ +125,5 @@
>          self.assertEqual(str(extracted.manifest), VERY_LONG_MANIFEST)
> +
> +    def test_08_unicode(self):
> +        extracted = JarExtractor('signing_clients/tests/test-jar-unicode.zip',
> +                                 'signing_clients/tests/test-jar-unicode-signed.jar',

I don't see test-jar-unicode.zip or test-jar-unicode-signed.jar in this diff. Maybe bugzilla didn't show it?

Comment 9

[Ryan Tilder [:rtilder]]

> 
> I don't see test-jar-unicode.zip or test-jar-unicode-signed.jar in this
> diff. Maybe bugzilla didn't show it?
>

git's diff didn't include the archives for some reason.  It is included in the pending commit & push.  The '-signed' file is created by the extractor only when signing happens.  Which it doesn't in this test case so it's effectively a no-op.

Comment 10

[Ryan Tilder [:rtilder]]

Attachment: UTF-8 support and test, take 2

I will ignore your nit though it is entirely correct.

Comment 11

[Kumar McMillan [:kumar]]

Comment on attachment 8434251 [details] [diff] [review]
UTF-8 support and test, take 2

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

r+. I still don't see the missing files but it sounds like they'll go into the merge so I'm not worried.

Comment 12

[Andy McKay]

Bumped zamboni: https://github.com/mozilla/zamboni/pull/2145, let's test it out.