Skip to content

[rb] Fix credential issue with private key#17188

Open
aguspe wants to merge 3 commits intoSeleniumHQ:trunkfrom
aguspe:rb-fix-credential-from-json
Open

[rb] Fix credential issue with private key#17188
aguspe wants to merge 3 commits intoSeleniumHQ:trunkfrom
aguspe:rb-fix-credential-from-json

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 7, 2026

🔗 Related Issues

#16916

💥 What does this PR do?

This commit fix the bug #16916 where the type of private key is wrong, I also updated the test

🔧 Implementation Notes

I did it this way because the actual type should be an array of bytes, not a string

🔄 Types of changes

  • Bug fix (backwards compatible)

This commit fix the bug
SeleniumHQ#16916 where the type of
private key is wrong, I also updated the test
Copilot AI review requested due to automatic review settings March 7, 2026 22:21
@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Fix private key decoding in credential from_json method

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix private key type handling in credential deserialization
• Decode base64-encoded private key from JSON input
• Update test expectations to match decoded value

Grey Divider

File Changes

1. rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb 🐞 Bug fix +1/-1

Decode base64 private key in credential deserialization

• Changed private_key parameter to decode base64-encoded value using decode() method
• Ensures private key is stored as bytes array instead of string

rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb


2. rb/spec/unit/selenium/webdriver/common/credentials_spec.rb 🧪 Tests +1/-1

Update test assertion for decoded private key

• Updated test assertion to expect decoded private key value
• Aligns test with corrected implementation behavior

rb/spec/unit/selenium/webdriver/common/credentials_spec.rb


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 7, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Non-defensive privateKey decode 🐞 Bug ⛯ Reliability
Description
Credential.from_json now unconditionally base64-decodes opts['privateKey'], which will raise if
the value is nil or already a byte array. This introduces a new runtime failure mode (and potential
behavioral break) for any caller that previously passed through an already-decoded value.
Code

rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb[50]

+              private_key: decode(opts['privateKey']),
Evidence
decode requires a base64 string (no nil/type guard), and from_json now always calls it for
privateKey. If privateKey is not a valid base64 string (e.g., nil or an Array), this will raise
before creating a Credential.

rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb[41-53]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Credential.from_json` now always runs `decode(opts['privateKey'])`. This will raise if `privateKey` is nil or not a String (e.g., already a byte array). Improve robustness by accepting either Array-of-bytes or base64 String, and raising an explicit `ArgumentError` when missing/invalid.

### Issue Context
- `Credential.decode` calls `Base64.urlsafe_decode64` directly.
- `from_json` is used when deserializing credentials returned by the remote end (`VirtualAuthenticator#credentials`).

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb[41-53]

### Suggested approach
- In `from_json`, compute `private_key` like:
 - if `opts['privateKey'].is_a?(Array)` -> use as-is
 - elsif `opts['privateKey']` -> `decode(opts['privateKey'])`
 - else -> raise `ArgumentError, "Missing privateKey"`
- (Optional) Apply similar defensive logic to `credentialId` for consistency, if desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

2. PrivateKey roundtrip untested 🐞 Bug ✓ Correctness
Description
The updated unit test asserts decoding of a raw base64 string, but it still doesn’t validate the
as_jsonfrom_json round-trip for privateKey. This can miss regressions where
encoding/decoding are not true inverses (padding/urlsafe differences).
Code

rb/spec/unit/selenium/webdriver/common/credentials_spec.rb[98]

+          expect(credential.private_key).to eq(described_class.decode(base64_encoded_pk))
Evidence
The spec constructs JSON with a hard-coded privateKey string and only checks decode(privateKey);
it does not assert that the value produced by Credential#as_json['privateKey'] can be fed back
into from_json to recover the original byte array. Since production serialization uses
Credential.encode(private_key), a round-trip test would better match real behavior.

rb/spec/unit/selenium/webdriver/common/credentials_spec.rb[83-100]
rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb[37-39]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Current unit coverage checks `from_json` decoding a specific base64 string, but doesn’t validate the real-world round-trip: `credential.as_json` (which uses `Credential.encode`) followed by `Credential.from_json` reproduces the original byte array.

### Issue Context
`Credential#as_json` serializes `private_key` via `Credential.encode(private_key)`.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/credentials_spec.rb[83-120]

### Suggested approach
- Add an example like:
 - build a `Credential` with `private_key` as byte array
 - `json = credential.as_json`
 - `roundtripped = described_class.from_json(json)`
 - `expect(roundtripped.private_key).to eq(private_key)`
- Optionally update the existing `from_json` test to set `'privateKey' => described_class.encode(private_key)` to mirror production serialization.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Contributor

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 fixes Ruby Virtual Authenticator credential deserialization so Credential#private_key is returned as a byte array (as expected by Credential.encode and by callers), aligning behavior with how credentials are serialized and with the reported issue.

Changes:

  • Decode privateKey in Credential.from_json so the in-memory type is an Array<Integer> (bytes) rather than a Base64 string.
  • Update the unit spec to assert the decoded private_key value.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
rb/lib/selenium/webdriver/common/virtual_authenticator/credential.rb Fixes deserialization by decoding privateKey when constructing a Credential from JSON.
rb/spec/unit/selenium/webdriver/common/credentials_spec.rb Updates expectation to match the corrected private_key type returned by from_json.

You can also share your feedback on Copilot code review. Take the survey.

@aguspe aguspe self-assigned this Mar 7, 2026
Copilot AI review requested due to automatic review settings March 10, 2026 09:43
Copy link
Contributor

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 2 out of 2 changed files in this pull request and generated 1 comment.


You can also share your feedback on Copilot code review. Take the survey.

expect(credential.rp_id).to eq('localhost')
expect(credential.user_handle).to eq(user_handle)
expect(credential.private_key).to eq(base64_encoded_pk)
expect(credential.private_key).to eq(described_class.decode(base64_encoded_pk))
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

This expectation re-decodes base64_encoded_pk inline even though the spec already defines let(:private_key) { described_class.decode(base64_encoded_pk) }. Reusing private_key here would avoid duplication and keep the test aligned with the other examples in this file.

Suggested change
expect(credential.private_key).to eq(described_class.decode(base64_encoded_pk))
expect(credential.private_key).to eq(private_key)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants