Skip to content

Allow unexpected fields #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 4, 2025
Merged

Conversation

AlfioEmanueleFresta
Copy link
Contributor

@AlfioEmanueleFresta AlfioEmanueleFresta commented May 25, 2025

We're using serde-indexed in libwebauthn, which implements the platform side of the CTAP spec.

When deserializing, the CTAP spec mandates unknown fields should be ignored. Without this, we just can't use serde-indexed, in favour of parsing structures manually.

I think this change will be safe to merge as backwards compatible because:

  • All working inputs will continue working;
  • Ignoring unexpected fields is the default behaviour of #[derive(Deserialize)], so it can be considered expected behaviours;
  • Behaviour for unexpected inputs currently does not specify otherwise.

Cc @robin-nitrokey @sosthene-nitrokey for review

@sosthene-nitrokey
Copy link
Contributor

Relevant part of the CTAP spec:

fido-client-to-authenticator-protocol-v2.2 § 8

if map keys are present that an implementation does not understand, they MUST be ignored. Note that this
enables additional fields to be used as new features are added without breaking existing implementations

@robin-nitrokey
Copy link
Member

Thanks for the PR! The change looks good to me, I just want to check if it has any effects on the binary size of our firmware. Potentially, this could pull in parts of the deserialization library that are otherwise unused.

@sosthene-nitrokey
Copy link
Contributor

It would be nice to have an opt-out (like #[serde(deny_unknown_fields)].

@robin-nitrokey
Copy link
Member

This change only has a small impact on the firmware binary size. I’d also like to have a deny_unknown_fields options but that can also be implemented in a separate PR.

@AlfioEmanueleFresta Please update the changelog for this change.

@AlfioEmanueleFresta AlfioEmanueleFresta changed the title Allow unexpected fields Allow unexpected fields & update changelog for 0.1.2 May 26, 2025
@AlfioEmanueleFresta
Copy link
Contributor Author

Thank you both!

@robin-nitrokey done - I took the liberty of drafting the 0.1.2 changelog for release later today, I hope that works.

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @sosthene-nitrokey Are you okay with considering this a bugfix and releasing it as v0.1.2 or do you see it as a breaking change?

@sosthene-nitrokey
Copy link
Contributor

I think it should be a breaking release but since according to lib.rs we're the only users I'm fine with a minor release. Might be great to include #14 and #16 in it too though.

@robin-nitrokey
Copy link
Member

@sosthene-nitrokey Then let’s do it properly and make a breaking release v0.2.0 with this change. Then we can also pull in #21.

@robin-nitrokey robin-nitrokey changed the title Allow unexpected fields & update changelog for 0.1.2 Allow unexpected fields Jun 4, 2025
@robin-nitrokey robin-nitrokey merged commit df9e76b into trussed-dev:main Jun 4, 2025
4 checks passed
AlfioEmanueleFresta added a commit to linux-credentials/libwebauthn that referenced this pull request Jun 16, 2025
* Part fixes #66 for unknown CBOR structures decoded via
DeserializeIndexed from serde-indexed.
* Remaining to allow unknown enum variants, to be fixed in a later PR.
 
Depends on trussed-dev/serde-indexed#19.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants