[py] Fix default rpId in virtual authenticator by cgoldberg · Pull Request #16428 · SeleniumHQ/selenium
PR Code Suggestions ✨
Latest suggestions up to 8899e13
| Category | Suggestion | Impact |
| Incremental [*] |
Validate and default missing rp_idIn the 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
Suggestion importance[1-10]: 8__ Why: The suggestion correctly identifies that allowing | Medium |
| Possible issue |
Default missing rpId on parseIn 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)
Suggestion importance[1-10]: 6__ Why: This suggestion correctly points out that | Low |
| Learned best practice |
Align docstring with optional rp_idUpdate the docstring to reflect that 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.
"""
Suggestion importance[1-10]: 6__ Why: | Low |
| General |
Make string output robustIn 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})" + )
Suggestion importance[1-10]: 4__ Why: The suggestion correctly notes that | Low |
| ||
Previous suggestions
Suggestions up to commit c56ad9a
| Category | Suggestion | Impact |
| Possible issue |
Revert change to prevent type mismatchRevert the use of 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 | High |