[py] Fix default rpId in virtual authenticator by cgoldberg · Pull Request #16428 · SeleniumHQ/selenium

PR Code Suggestions ✨

Latest suggestions up to 8899e13

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Validate and default missing rp_id

In the init method, validate that rp_id is provided for non-resident
credentials and default it to an empty string if it is None to ensure type
consistency.

py/selenium/webdriver/common/virtual_authenticator.py [79-104]

 def __init__(
     self,
     credential_id: bytes,
     is_resident_credential: bool,
     rp_id: Optional[str],
     user_handle: Optional[bytes],
     private_key: bytes,
     sign_count: int,
 ):
     """Constructor. A credential stored in a virtual authenticator.
     https://w3c.github.io/webauthn/#credential-parameters.
 
     :Args:
         - credential_id (bytes): Unique base64 encoded string.
         - is_resident_credential (bool): Whether the credential is client-side discoverable.
-        - rp_id (str): Relying party identifier.
+        - rp_id (str | None): Relying party identifier. Optional for resident credentials.
         - user_handle (bytes): userHandle associated to the credential. Must be Base64 encoded string. Can be None.
         - private_key (bytes): Base64 encoded PKCS#8 private key.
         - sign_count (int): initial value for a signature counter.
     """
     self._id = credential_id
     self._is_resident_credential = is_resident_credential
-    self._rp_id = rp_id
+    # Default to empty string if rp_id is None to keep property type stable
+    # Alternatively, enforce presence for non-resident credentials
+    if not is_resident_credential and rp_id is None:
+        raise ValueError("rp_id is required for non-resident credentials")
+    self._rp_id = rp_id or ""
     self._user_handle = user_handle
     self._private_key = private_key
     self._sign_count = sign_count

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that allowing rp_id to be None introduces a type inconsistency with the rp_id property which is hinted to return str, and proposes a robust fix in the __init__ method to prevent this invalid state.

Medium
Possible issue
Default missing rpId on parse

In Credential.from_dict, default a missing rpId to an empty string instead of
None to prevent passing None to the constructor.

py/selenium/webdriver/common/virtual_authenticator.py [179-188]

 @classmethod
 def from_dict(cls, data: dict[str, Any]) -> "Credential":
     _id = urlsafe_b64decode(f"{data['credentialId']}==")
     is_resident_credential = bool(data["isResidentCredential"])
-    rp_id = data.get("rpId", None)
+    rp_id_val = data.get("rpId") or ""
     private_key = urlsafe_b64decode(f"{data['privateKey']}==")
     sign_count = int(data["signCount"])
-    user_handle = urlsafe_b64decode(f"{data['userHandle']}==") if data.get("userHandle", None) else None
+    user_handle = urlsafe_b64decode(f"{data['userHandle']}==") if data.get("userHandle") else None
 
-    return cls(_id, is_resident_credential, rp_id, user_handle, private_key, sign_count)
+    return cls(_id, is_resident_credential, rp_id_val, user_handle, private_key, sign_count)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: This suggestion correctly points out that from_dict can pass None for rp_id to the constructor. While this is a valid concern, the primary fix should be in the __init__ method to handle the None case centrally, as suggested by another suggestion. This change is beneficial but somewhat redundant if the constructor is fixed.

Low
Learned
best practice
Align docstring with optional rp_id

Update the docstring to reflect that rp_id can be optional and document when it
may be None to keep the API contract accurate.

py/selenium/webdriver/common/virtual_authenticator.py [79-98]

 def __init__(
         self,
         credential_id: bytes,
         is_resident_credential: bool,
         rp_id: Optional[str],
         user_handle: Optional[bytes],
         private_key: bytes,
         sign_count: int,
     ):
         """Constructor. A credential stored in a virtual authenticator.
         https://w3c.github.io/webauthn/#credential-parameters.
 
         :Args:
             - credential_id (bytes): Unique base64 encoded string.
             - is_resident_credential (bool): Whether the credential is client-side discoverable.
-            - rp_id (str): Relying party identifier.
-            - user_handle (bytes): userHandle associated to the credential. Must be Base64 encoded string. Can be None.
+            - rp_id (Optional[str]): Relying party identifier. May be None in scenarios where RP ID is not applicable.
+            - user_handle (Optional[bytes]): userHandle associated to the credential. Must be Base64 encoded string. Can be None.
             - private_key (bytes): Base64 encoded PKCS#8 private key.
-            - sign_count (int): initial value for a signature counter.
+            - sign_count (int): Initial value for a signature counter.
         """

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Keep API and documentation accurate and consistent with parameter types and behavior.

Low
General
Make string output robust

In Credential.str, handle the case where rp_id is None to ensure the string
representation is always robust and does not output "None".

py/selenium/webdriver/common/virtual_authenticator.py [190-192]

 def __str__(self) -> str:
-    return f"Credential(id={self.id}, is_resident_credential={self.is_resident_credential}, rp_id={self.rp_id},\
-        user_handle={self.user_handle}, private_key={self.private_key}, sign_count={self.sign_count})"
+    rp = self.rp_id if self.rp_id is not None else ""
+    return (
+        f"Credential(id={self.id}, is_resident_credential={self.is_resident_credential}, "
+        f"rp_id={rp}, user_handle={self.user_handle}, private_key={self.private_key}, "
+        f"sign_count={self.sign_count})"
+    )

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly notes that rp_id could be None in the __str__ output. However, if the __init__ method is fixed to normalize None to an empty string, this change becomes unnecessary as self.rp_id would never be None. It's a valid point but addresses a symptom rather than the root cause.

Low
  • More

Previous suggestions

Suggestions up to commit c56ad9a
CategorySuggestion                                                                                                                                    Impact
Possible issue
Revert change to prevent type mismatch

Revert the use of data.get("rpId", None) to data["rpId"]. The Credential
constructor expects rp_id to be a str, but the change allows it to be None,
which would cause a runtime error.

py/selenium/webdriver/common/virtual_authenticator.py [183]

 -        rp_id = data["rpId"]
-+        rp_id = data.get("rpId", None)
++        rp_id = data["rpId"]
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug introduced by the PR, where passing None as rp_id to the Credential constructor would cause a TypeError due to a type hint mismatch.

High