JSON.parse error in test.pypi.org due to HTTP/2 500 Internal Server Error during registration of 2FA security device

rolandog commented 3 years ago

Describe the bug

When trying to add a 2FA security device, I get a JSON.parse error due to a 500 Internal Server Error from a XHR POST to during validation. (I think it's not related to #9086)

Expected behavior

I get confirmation of 2FA security device.

To Reproduce

Go to https://test.pypi.org/manage/account/webauthn-provision
Name security device
Read prompt for anonymization (this may be due to enhanced tracking protection):
test.pypi.org is requesting extended information about your security key, which may affect your privacy.

Firefox can anonymize this for you, but the website might decline this key. If declined, you can try again.

Learn more
- [ ] Anonymize anyway
Click on Proceed (I tested without anonymization, so it shouldn't have been a problem; I will test with anonymization to confirm if this is also an issue).
- Update: if I do anonymize I get a different error, which I think is not applicable to the scope of this bug report.
  
  Registration rejected. Error: Authenticator attestation is required.
Read notification

test.pypi.org wants to register an account with one of your security keys. You can connect and authorize one now, or cancel.
Insert security device, which should begin flashing.
Press button on security device to validate.
Get following error:
JSON.parse: unexpected character at line 1 column 1 of the JSON data

My Platform
- Browser: Firefox 88.0b2 (64-bit)
- OS: Ubuntu 20.10
- Connectivity: VPN through WireGuard.
- Add-ons: I tried this with all add-ons---except NoScript---disabled (but I allowed all scripts to run temporarily in NoScript).

Additional context

There are other JS errors in the console, mainly due to some conflicts due to the Content Security Policy: Additional JS errors in the console due to CSP

di commented 3 years ago

Looks like this is https://sentry.io/organizations/python-software-foundation/issues/1806073874/, and results from us not correctly handling duplicate labels. Here's the stacktrace:

UniqueViolation: duplicate key value violates unique constraint "user_security_keys_label_key"
DETAIL:  Key (label)=(YubiKey 5 NFC) already exists.

  File "sqlalchemy/engine/base.py", line 1276, in _execute_context
    self.dialect.do_execute(
  File "sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)
IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "user_security_keys_label_key"
DETAIL:  Key (label)=(YubiKey 5 NFC) already exists.

[SQL: INSERT INTO user_security_keys (user_id, label, credential_id, public_key, sign_count) VALUES (%(user_id)s, %(label)s, %(credential_id)s, %(public_key)s, %(sign_count)s) RETURNING user_security_keys.id]
[parameters: {'user_id': UUID('c724dbf2-2e42-4b06-a9f2-d4c15f0ee1d8'), 'label': 'YubiKey 5 NFC', 'credential_id': 'Ys8FqP-BZVJCjU_Bue5...
  File "pyramid/router.py", line 270, in __call__
    response = self.execution_policy(environ, self)
  File "__init__.py", line 127, in retry_policy
    response = router.invoke_request(request)
  File "pyramid/router.py", line 249, in invoke_request
    response = handle_request(request)
  File "warehouse/sanity.py", line 69, in sanity_tween_ingress
    return handler(request)
  File "warehouse/referrer_policy.py", line 16, in referrer_policy_tween
    response = handler(request)
  File "warehouse/csp.py", line 31, in content_security_policy_tween
    resp = handler(request)
  File "warehouse/config.py", line 83, in require_https_tween
    return handler(request)
  File "__init__.py", line 178, in tm_tween
    reraise(*exc_info)
  File "pyramid_tm/compat.py", line 36, in reraise
    raise value
  File "__init__.py", line 143, in tm_tween
    response = handler(request)
  File "warehouse/utils/compression.py", line 92, in compression_tween
    response = handler(request)
  File "pyramid/tweens.py", line 43, in excview_tween
    response = _error_handler(request, exc)
  File "pyramid/tweens.py", line 17, in _error_handler
    reraise(*exc_info)
  File "pyramid/compat.py", line 179, in reraise
    raise value
  File "pyramid/tweens.py", line 41, in excview_tween
    response = handler(request)
  File "warehouse/cache/http.py", line 74, in conditional_http_tween
    response = handler(request)
  File "warehouse/sanity.py", line 76, in sanity_tween_egress
    return unicode_redirects(handler(request))
  File "pyramid/router.py", line 147, in handle_request
    response = _call_view(
  File "pyramid/view.py", line 683, in _call_view
    response = view_callable(context, request)
  File "pyramid/config/views.py", line 188, in attr_view
    return view(context, request)
  File "pyramid/config/views.py", line 214, in predicate_wrapper
    return view(context, request)
  File "warehouse/cache/http.py", line 33, in wrapped
    return view(context, request)
  File "warehouse/cache/http.py", line 33, in wrapped
    return view(context, request)
  File "pyramid/viewderivers.py", line 514, in csrf_view
    return view(context, request)
  File "pyramid/viewderivers.py", line 325, in secured_view
    return view(context, request)
  File "pyramid/viewderivers.py", line 275, in wrapper
    response = view(context, request)
  File "warehouse/metrics/views.py", line 34, in wrapper_view
    return view(context, request)
  File "pyramid/viewderivers.py", line 436, in rendered_view
    result = view(context, request)
  File "pyramid/viewderivers.py", line 116, in _class_requestonly_view
    response = getattr(inst, attr)()
  File "warehouse/manage/views.py", line 607, in validate_webauthn_provision
    self.user_service.add_webauthn(
  File "warehouse/accounts/services.py", line 457, in add_webauthn
    self.db.flush()  # flush the db now so webauthn.id is available
  File "sqlalchemy/orm/session.py", line 2540, in flush
    self._flush(objects)
  File "sqlalchemy/orm/session.py", line 2682, in _flush
    transaction.rollback(_capture_exception=True)
  File "sqlalchemy/util/langhelpers.py", line 68, in __exit__
    compat.raise_(
  File "sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
  File "sqlalchemy/orm/session.py", line 2642, in _flush
    flush_context.execute()
  File "sqlalchemy/orm/unitofwork.py", line 422, in execute
    rec.execute(self)
  File "sqlalchemy/orm/unitofwork.py", line 586, in execute
    persistence.save_obj(
  File "sqlalchemy/orm/persistence.py", line 239, in save_obj
    _emit_insert_statements(
  File "sqlalchemy/orm/persistence.py", line 1135, in _emit_insert_statements
    result = cached_connections[connection].execute(
  File "sqlalchemy/engine/base.py", line 1011, in execute
    return meth(self, multiparams, params)
  File "sqlalchemy/sql/elements.py", line 298, in _execute_on_connection
    return connection._execute_clauseelement(self, multiparams, params)
  File "sqlalchemy/engine/base.py", line 1124, in _execute_clauseelement
    ret = self._execute_context(
  File "sqlalchemy/engine/base.py", line 1316, in _execute_context
    self._handle_dbapi_exception(
  File "sqlalchemy/engine/base.py", line 1510, in _handle_dbapi_exception
    util.raise_(
  File "sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
  File "sqlalchemy/engine/base.py", line 1276, in _execute_context
    self.dialect.do_execute(
  File "sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)

rolandog commented 3 years ago

Hi! Thanks for the stacktrace!

I don't know if it's a bug or a feature to not have duplicate names for the security keys used in the testing and real PyPI (since I thought they were completely separate instances... including the webauthn side), but---for now---knowing that has helped me to resolve the issue on my end.

Should I close this issue? Or, leave it open in case a fix to catch the error and display an informative message is needed.

di commented 3 years ago

I'm not 100% sure whether we should allow duplicate labels or not, but we definitely shouldn't be responding with a 500 error here -- that's a bug. We can leave this open until that's resolved.

pypi / warehouse