Deprecation of setters decreases safety and developer experience

[TimWolla]

Version(s) affected

4.7.0

Description

While working up the upgrade from 4.6.x to 4.7.x, specifically the replacement of setters by property access, I noticed that the safety of the library decreased. Specifically it is much less misuse resistant.

Let me take the PublicKeyCredentialRequestOptions::$userVerification property as an example. The setUserVerification() setter performed validation the the passed value is actually a valid value and by extension the named createFromArray() constructor / deserializer also performed this validation.

Now any value may be passed, valid or not, allowing garbage data to enter the library instead of being rejected at the boundary.

The $userVerification property could be fixed by leveraging a native PHP enum, but likewise nothing (except perhaps a static analyzer) stops me from inserting garbage into PublicKeyCredentialRequestOptions::$allowCredentials property, because PHP does not support typed arrays. Previously the typed variadic parameter prevented inserting invalid values.

How to reproduce

Look at the code.

Possible Solution

No response

Additional Context

No response

[Spomky]

Hi @TimWolla,

Many thanks for your feedback. I understand your concern and I agree that developers may misuse some of the properties and set invalid values.
With this intermediate version, I would like to get rid of createFromArray and createFromJson methods and go to the use of the symfony components Serializer and Validator. The idea behind this is to simplify the classes to the bear minimum i.e. hold the data and have the minimum of logic, which is not the case at the moment.

Now any value may be passed, valid or not, allowing garbage data to enter the library instead of being rejected at the boundary.

Yes indeed. A developer may use plop as a User Verification method. This is because when we read the specification, the enum types are not listed either. See https://www.w3.org/TR/webauthn-2/#sct-domstring-backwards-compatibility and the note in 5.8.6. User Verification Requirement Enumeration: Note: The [UserVerificationRequirement](https://www.w3.org/TR/webauthn-2/#enumdef-userverificationrequirement) enumeration is deliberately not referenced

I would like to reflect this choice in this library. Months ago, we had a (small) problem with the new enterprise attestation conveyance mode and few years ago with the internal transport method. I would like to avoid this in the future.

Again, I understand that could be a terrible approach, especially because the support for the two symfony components is not finished at the moment and developers have to be very careful when assigning the properties. I will do my best for issuing the next minor release in the next weeks.

[TimWolla]

Thank you for your response!

I would like to reflect this choice in this library. Months ago, we had a (small) problem with the new enterprise attestation conveyance mode and few years ago with the internal transport method. I would like to avoid this in the future.

As a user of this library I would prefer a stricter validation. While browsers need to gracefully handle unknown values, because you can't really control whether the users actually update their browsers or not, this is different for a server-side library. If a new value arrives that I want to use then I need to update my code anyway. It is reasonable to also update the library at the same time.

I'm specifically using a library, because WebAuthn is a complicated topic that is also relevant for my application's security. I don't want to learn all the important details and instead trust the library developer to perform all the necessary checks and wrap them into a safe API that makes it hard to introduce (security) issues. And your library really does a great job here (or did until 4.6). The API was easy to use and I get an exception whenever I do something dumb.

With the removal of the validation in the setters this unfortunately is no longer the case. When preparing an internal PR to replace the use of setters by direct property access I needed to very carefully check that I access the right properties and that the setters didn't do something else that I might miss. Here's an example of such a gotcha:

webauthn-framework/src/webauthn/src/PublicKeyCredentialRequestOptions.php

Lines 75 to 79 in 87895ca

	if ($userVerification === null) {
	$this->rpId = null;
	return $this;
	}

When setting the user verification to null the setter would also clear the rpId. I don't know why that happens (perhaps it is a bug?), but it was there. If this clearing is important, then might easily miss this when performing the changes.

[TimWolla]

and go to the use of the symfony components Serializer and Validator.

I can totally get behind that from a PoV of a library developer that is a Symfony expert, so please don't take this as a complaint or request for change or reconsideration.

However I'd like to note that Symfony's Serializer and Validator are two components that are fairly large. Newly introducing them to a non-Symfony codebase that does not yet use them is not ideal, because it's one more dependency to keep track of with regard to PHP version compatibility / security updates / etc.

[Spomky]

🤔 All this makes me reconsider the latest changes. Give me a moment to think about it. Your arguments are valuable and my intention was not to reduce the DX but to simplify the logic of the classes.

[Spomky]

I slept on the idea and come to a conclusion.
To me, the issue is limited to the DTOs/Value objects created for the ceremonies from the webauthn-lib.
As an example, there is no risk

• with services (e.g. AuthenticatorAssertionResponseValidator) as they usually expose no parameters
• with the other packages (MDS, Symfony and Stimulus) e.g. StatusReport and MetadataStatement as most of the properties are readonly and the developers are not supposed to instantiate those classes, but have them from services.

I will focus on the the following classes:

• AuthenticatorSelectionCriteria: to be updated
• PublicKeyCredential: no changes as loaded by services and properties are readonly
• PublicKeyCredentialCreationOptions: to be updated
• PublicKeyCredentialDescriptor: no changes as loaded by PublicKeyCredential/PublicKeyCredentialSource and properties are readonly
• PublicKeyCredentialEntity: no changes (only getters of readonly properties
• PublicKeyCredentialParameters: to be updated no changes (only getters of readonly properties
• PublicKeyCredentialRequestOptions: to be updated
• PublicKeyCredentialRpEntity: no changes (only getters of readonly properties
• PublicKeyCredentialSource: to be updated
• PublicKeyCredentialUserEntity: no changes (only getters of readonly properties
• PublicKeyCredentialDescriptorCollection: to be moved to the Symfony bundle package

WDYT?

[TimWolla]

I agree that no change is needed when just dealing with readonly properties, as the constructor can (and should) perform the required validation.

With regard to your list: I'm mostly concerned about AuthenticatorSelectionCriteria, PublicKeyCredentialCreationOptions and PublicKeyCredentialRequestOptions, as these are the ones that I needed to actively interact with. You've listed these as “to be updated”, no that would match.

I've also had a quick look through the library. What about PublicKeyCredentialDescriptorCollection? It appears that this class is entirely unused (at least in our application), however by asking the user to go through direct property access, correct handling of the keys (i.e. the ID) of the inner array cannot be enforced, leading to accidental bugs.

[TimWolla]

I've now updated to 4.7.1 and it's looking much better. Thank you!

[github-actions]

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.