Skip to content

Conversation

@AlfioEmanueleFresta
Copy link
Member

@AlfioEmanueleFresta AlfioEmanueleFresta commented Jul 27, 2025

No description provided.

@AlfioEmanueleFresta AlfioEmanueleFresta changed the title [WIP] Web IDL support (make credentials); [WIP] Web IDL support Jul 27, 2025
@AlfioEmanueleFresta
Copy link
Member Author

AlfioEmanueleFresta commented Aug 15, 2025

This now covers both MC and GA requests, but not responses. I'll probably tidy this up and keep responses for a separate PR.

Note to self, the failures are due to Base64UrlString being used in CTAP models, which are serialized as a Base64 string rather than URL with serde_cbor. The next step is to separate these models into two, a WebAuthn JSON model using Base64UrlString, and an equivalent CTAP model using ByteBuf. Must also review all usages of Base64UrlString.

@AlfioEmanueleFresta AlfioEmanueleFresta changed the title [WIP] Web IDL support Web IDL support 1/N: Get assertion and make credentials basic parsing Sep 7, 2025
@AlfioEmanueleFresta AlfioEmanueleFresta changed the title Web IDL support 1/N: Get assertion and make credentials basic parsing Web IDL support 1/N: GA, MC basic parsing Sep 7, 2025
@AlfioEmanueleFresta
Copy link
Member Author

Marking this as ready for review as this is getting large, keeping track of further work in #134.

@AlfioEmanueleFresta AlfioEmanueleFresta marked this pull request as ready for review September 7, 2025 21:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the foundation for Web IDL support by implementing basic parsing for GetAssertion (GA) and MakeCredential (MC) operations. The changes enable the library to parse WebAuthn operations from JSON format, which is a key requirement for Web IDL compliance.

  • Introduces a new IDL module with JSON parsing capabilities for WebAuthn operations
  • Refactors extension handling to use proper typed structures instead of enums
  • Adds support for parsing WebAuthn requests from JSON format with proper validation

Reviewed Changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
libwebauthn/src/ops/webauthn/mod.rs Adds IDL module exports and updates core WebAuthn type definitions
libwebauthn/src/ops/webauthn/idl/* New IDL parsing infrastructure with JSON deserialization support
libwebauthn/src/ops/webauthn/make_credential.rs Refactors extension handling and adds JSON parsing support
libwebauthn/src/ops/webauthn/get_assertion.rs Updates extension structures and adds JSON parsing capabilities
libwebauthn/src/proto/ctap2/model/* Updates extension handling to work with new typed structures
libwebauthn/src/tests/basic_ctap2.rs Updates test to use new extension format
Examples and other files Updates to use new extension APIs
Comments suppressed due to low confidence (1)

libwebauthn/src/proto/ctap2/model/make_credential.rs:1

  • The Default trait was removed from Ctap2GetAssertionRequestExtensions but line 142 in the same file still calls skip_serializing_extensions which expects a default value check. This could cause compilation issues.
use super::{

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +15 to +17
impl Into<String> for RelyingPartyId {
fn into(self) -> String {
self.0
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

Consider implementing From<RelyingPartyId> for String instead of Into<String> for RelyingPartyId. The From trait automatically provides the Into implementation and is the preferred approach in Rust.

Suggested change
impl Into<String> for RelyingPartyId {
fn into(self) -> String {
self.0
impl From<RelyingPartyId> for String {
fn from(rpid: RelyingPartyId) -> String {
rpid.0

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +43
impl Into<Ctap2PublicKeyCredentialDescriptor> for PublicKeyCredentialDescriptorJSON {
fn into(self) -> Ctap2PublicKeyCredentialDescriptor {
Ctap2PublicKeyCredentialDescriptor {
r#type: self.r#type,
id: ByteBuf::from(self.id),
transports: self.transports,
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

Consider implementing From<PublicKeyCredentialDescriptorJSON> for Ctap2PublicKeyCredentialDescriptor instead of Into. The From trait automatically provides the Into implementation and is the preferred approach in Rust.

Suggested change
impl Into<Ctap2PublicKeyCredentialDescriptor> for PublicKeyCredentialDescriptorJSON {
fn into(self) -> Ctap2PublicKeyCredentialDescriptor {
Ctap2PublicKeyCredentialDescriptor {
r#type: self.r#type,
id: ByteBuf::from(self.id),
transports: self.transports,
impl From<PublicKeyCredentialDescriptorJSON> for Ctap2PublicKeyCredentialDescriptor {
fn from(value: PublicKeyCredentialDescriptorJSON) -> Self {
Ctap2PublicKeyCredentialDescriptor {
r#type: value.r#type,
id: ByteBuf::from(value.id),
transports: value.transports,

Copilot uses AI. Check for mistakes.
Comment on lines +57 to +59
impl Into<Vec<u8>> for Base64UrlString {
fn into(self) -> Vec<u8> {
self.0
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

Consider implementing From<Base64UrlString> for Vec<u8> instead of Into<Vec<u8>>. The From trait automatically provides the Into implementation and is the preferred approach in Rust.

Suggested change
impl Into<Vec<u8>> for Base64UrlString {
fn into(self) -> Vec<u8> {
self.0
impl From<Base64UrlString> for Vec<u8> {
fn from(b64: Base64UrlString) -> Vec<u8> {
b64.0

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +29
let json =
format!("{{\"type\":\"{op_str}\",\"challenge\":\"{challenge_str}\",\"origin\":\"{origin_str}\",\"crossOrigin\":{cross_origin_str}}}");
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

The JSON string construction using format! is error-prone and hard to maintain. Consider using serde_json to serialize a proper struct to ensure valid JSON format.

Copilot uses AI. Check for mistakes.
Some(inner.exclude_credentials)
},
extensions: inner.extensions,
timeout: timeout,
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

Redundant field initialization. When the field name matches the variable name, you can use the shorthand syntax: timeout instead of timeout: timeout.

Suggested change
timeout: timeout,
timeout,

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@msirringhaus msirringhaus left a comment

Choose a reason for hiding this comment

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

Looking good overall, I think. I haven't had time for a full deep dive, though. I only have some preliminary questions.

(signed_extensions.hmac_secret, None)
}
MakeCredentialHmacOrPrfInput::Prf => (
if let Some(_hmac_create_secret) = incoming_ext.hmac_create_secret {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using if {} else if {} here means you can either have HMAC or PRF, and if you request both, HMAC will override PRF. Is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think you're right. Whilst I wouldn't expect a browser to request this, I'll change it so that if your makeCredential includes both HMAC & PRF, you will get both responses back.


use super::{DowngradableRequest, RegisterRequest, UserVerificationRequirement};

pub const DEFAULT_TIMEOUT: Duration = Duration::from_secs(60);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use the defintion from libwebauthn/src/ops/webauthn/timeout.rs here? (Same in the other files where this is defined again)

pub hash: Vec<u8>,
pub allow: Vec<Ctap2PublicKeyCredentialDescriptor>,
pub extensions: Option<GetAssertionRequestExtensions>,
pub extensions: GetAssertionRequestExtensions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I confess, I'm currently a bit lost in the details of this huge PR, but if I recall correctly, we used to use the Option<> here to decide if we have to include an extensions-map in the output. Some portals like demo.yubico.com are rather strict on what they expect, e.g. if they request extensions, they expect extensions in the response, even if the map is empty. And they do not want to see that empty map, if they didn't request one.
Is this still possible with this change?

Copy link
Member Author

@AlfioEmanueleFresta AlfioEmanueleFresta Dec 18, 2025

Choose a reason for hiding this comment

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

Thanks for spotting this, addressing it so we can represent both states. Also adding a test for both cases (extensions: {} and no etensions).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

let resident_key = if inner
.authenticator_selection
.as_ref()
.and_then(|s| Some(s.require_resident_key))
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The .and_then(|s| Some(s.require_resident_key)) is incorrect. Using and_then with a closure that always returns Some is redundant - this should be .map(|s| s.require_resident_key) instead.

Suggested change
.and_then(|s| Some(s.require_resident_key))
.map(|s| s.require_resident_key)

Copilot uses AI. Check for mistakes.
Comment on lines +84 to +97
let hmac_or_prf = match inner.extensions.clone() {
Some(ext) => {
if let Some(prf) = ext.prf {
let prf_input = PrfInput::try_from(prf)?;
Some(GetAssertionHmacOrPrfInput::Prf(prf_input))
} else if let Some(hmac) = ext.hamc_get_secret {
let hmac_input = HMACGetSecretInput::try_from(hmac)?;
Some(GetAssertionHmacOrPrfInput::HmacGetSecret(hmac_input))
} else {
None
}
}
None => None,
};
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The .clone() on line 84 followed by accessing .as_ref() on line 102 is inefficient. The extensions should only be accessed once (using .as_ref()) and then the necessary fields cloned individually if needed.

Copilot uses AI. Check for mistakes.
let mut channel = device.channel().await?;
channel.wink(TIMEOUT).await?;

// Relying
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The comment "// Relying" on line 79 appears to be incomplete. It should probably say "// Relying Party ID" or similar.

Suggested change
// Relying
// Relying Party ID

Copilot uses AI. Check for mistakes.
pub id: Base64UrlString,
pub r#type: Ctap2PublicKeyCredentialType,

#[serde(skip_serializing_if = "Option::is_none")]
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The skip_serializing_if attribute is for serialization but is applied to a field in a struct that only derives Deserialize, not Serialize. This attribute has no effect and should be removed.

Suggested change
#[serde(skip_serializing_if = "Option::is_none")]

Copilot uses AI. Check for mistakes.
} else {
"false"
};
let json =
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

Trailing whitespace on this line should be removed.

Suggested change
let json =
let json =

Copilot uses AI. Check for mistakes.
if let Some(prf) = ext.prf {
let prf_input = PrfInput::try_from(prf)?;
Some(GetAssertionHmacOrPrfInput::Prf(prf_input))
} else if let Some(hmac) = ext.hamc_get_secret {
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The field name hamc_get_secret should be hmac_get_secret (typo is propagated from the struct definition). This will cause the extension to be silently ignored rather than parsed, which is a critical bug.

Suggested change
} else if let Some(hmac) = ext.hamc_get_secret {
} else if let Some(hmac) = ext.hmac_get_secret {

Copilot uses AI. Check for mistakes.
#[serde(rename = "largeBlobKey")]
pub large_blob: Option<LargeBlobInputJson>,
#[serde(rename = "hmacCreateSecret")]
pub hamc_get_secret: Option<HmacGetSecretInputJson>,
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

Typo in the field name: "hamc_get_secret" should be "hmac_get_secret" (missing the 'a' in 'hmac').

Suggested change
pub hamc_get_secret: Option<HmacGetSecretInputJson>,
pub hmac_get_secret: Option<HmacGetSecretInputJson>,

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@msirringhaus msirringhaus left a comment

Choose a reason for hiding this comment

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

Looking good so far as I can tell. A few nitpicks below.

},
);
let hmac_or_prf = GetAssertionHmacOrPrfInput::Prf {
let hmac_or_prf: GetAssertionHmacOrPrfInput = GetAssertionHmacOrPrfInput::Prf(PrfInput {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need the double type marker here

Suggested change
let hmac_or_prf: GetAssertionHmacOrPrfInput = GetAssertionHmacOrPrfInput::Prf(PrfInput {
let hmac_or_prf = GetAssertionHmacOrPrfInput::Prf(PrfInput {

.transpose()
.ok()
.flatten()
.flatten(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be done a little bit simpler? Feels like transpose + 2x flatten points to an overly complicated data structure. E.g. does the try_from() function need to be implemented for Option<GetAssertionLargeBlobExtension> instead of GetAssertionLargeBlobExtension or &GetAssertionLargeBlobExtension? Then we could maybe do and_then() instead of map() here.

},
))
})
.collect::<Result<HashMap<String, PRFValue>, GetAssertionRequestParsingError>>()?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can probably be shortened by omitting at least the error type

Suggested change
.collect::<Result<HashMap<String, PRFValue>, GetAssertionRequestParsingError>>()?,
.collect::<Result<HashMap<String, PRFValue>, _>>()?,

The compiler should be able to figure it out (same for String and PRFValue, but those are not overly long)


#[derive(Debug, Clone, Deserialize)]
pub struct AuthenticatorSelectionCriteria {
#[serde(rename = "authenticatorAttachment")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use #[serde(rename_all = "camelCase")] here instead of renaming each field individually.

#[serde(default)]
pub require_resident_key: bool,
#[serde(rename = "userVerification")]
#[serde(default = "default_user_verification")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could think about deriving Default for UserVerificationRequirement and marking Preferred with #[default] and remove default_user_verification(). But maybe we don't want a default impl for that. Would be nice if serde would finally implement the ability to add default values here.

}

#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, Deserialize, PartialEq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could also be done with rename_all

pub enum MakeCredentialLargeBlobExtension {
#[default]
None,
#[serde(rename = "preferred")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename_all

}

#[derive(Debug, Default, Clone)]
#[derive(Debug, Default, Clone, Deserialize, PartialEq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename_all

GetAssertion,
}

#[derive(Debug, Clone, Copy, Deserialize, PartialEq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename_all

#[derive(Debug, Clone, Deserialize, PartialEq)]
pub struct MakeCredentialPrfInput {
#[serde(rename = "eval")]
pub _eval: Option<JsonValue>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a comment here, why it's _eval, aka not used at the moment?

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