web-auth / webauthn-framework

FIDO-U2F / FIDO2 / Webauthn Framework
MIT License
403 stars 53 forks source link

The icon field of PublicKeyCredentialEntity has been removed from the specification #634

Closed abcang closed 1 month ago

abcang commented 1 month ago

Version(s) affected

5.0.0

Description

The icon field of PublicKeyCredentialEntity has been removed from the specification (https://github.com/w3c/webauthn/pull/1337). So it can be removed from the implementation.

When using the WebauthnSerializerFactory serializer to serialize a PublicKeyCredentialRpEntity with an unspecified icon to JSON, the icon field appears as null. I am concerned that this may cause unexpected behavior.

How to reproduce

$factory = new Webauthn\Denormalizer\WebauthnSerializerFactory(
    new Webauthn\AttestationStatement\AttestationStatementSupportManager()
);
$serializer = $factory->create();

echo $serializer->serialize(
    Webauthn\PublicKeyCredentialRpEntity::create(
        'rp name',
        'rp.id'
    ),
    'json'
);

Output:

{"id":"rp.id","name":"rp name","icon":null}

Possible Solution

A tentative fix would be to create a Denormalizer to filter out nulls, similar to the PublicKeyCredentialUserEntity. A radical fix would be to remove the icon property from PublicKeyCredentialEntity.

Additional Context

No response

Spomky commented 1 month ago

Hi @abcang,

Indeed, I missed removing the icon field. This will be deprecated in the next minor release. To fix that issue without any changes in this library, I recomment the use of the skip_null_values options:

<?php
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;

$serializer->serialize(
    Webauthn\PublicKeyCredentialRpEntity::create(
        'rp name',
        'rp.id'
    ),
    'json',
    [AbstractObjectNormalizer::SKIP_NULL_VALUES => true]
);

The output will be as expected:

{"id":"rp.id","name":"rp name"}
abcang commented 1 month ago

Thank you very much! I will tentatively use that option.

Spomky commented 1 month ago

The icon property will be marked as deprecated in 5.1.0 and removed in 6.0.0.

abcang commented 1 month ago

Thank you!

github-actions[bot] commented 3 weeks ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.