Skip to content

[java] Enhance ScriptKey.toString() and mask script content in UnpinnedScriptKey#17159

Merged
asolntsev merged 5 commits intoSeleniumHQ:trunkfrom
seethinajayadileep:enhancement/scriptkey-add-tostring-clean
Mar 2, 2026
Merged

[java] Enhance ScriptKey.toString() and mask script content in UnpinnedScriptKey#17159
asolntsev merged 5 commits intoSeleniumHQ:trunkfrom
seethinajayadileep:enhancement/scriptkey-add-tostring-clean

Conversation

@seethinajayadileep
Copy link
Contributor

@seethinajayadileep seethinajayadileep commented Mar 2, 2026

💥 What does this PR do?

Overrides toString() in org.openqa.selenium.ScriptKey to return the underlying identifier instead of relying on the default Object.toString() implementation.

Previously, logging or printing a ScriptKey instance produced output such as:

org.openqa.selenium.ScriptKey@6d06d69c

This output is not helpful for debugging. Since ScriptKey represents an identifier wrapper, its string representation should reflect that identifier directly.

After this change, printing a ScriptKey will produce:

my-script-id

Additionally, UnpinnedScriptKey now overrides toString() to prevent exposing the full JavaScript source in logs. Since UnpinnedScriptKey internally stores the raw script as its identifier, returning it directly could result in large log output or unintended disclosure. The override ensures a safe, concise debug representation without leaking script content.

This improves debugging clarity while avoiding potential log bloat or sensitive script exposure.


🔧 Implementation Notes

  • Implemented toString() in ScriptKey to return the identifier field.
  • Overrode toString() in UnpinnedScriptKey to avoid logging raw script content.
  • No changes were made to equals() or hashCode().
  • JSON serialization via toJson() and fromJson() remains unaffected.
  • No functional behavior changes were introduced.

💡 Additional Considerations

  • This is a non-breaking change.
  • Prevents potential script leakage in logs for UnpinnedScriptKey.
  • Improves overall debugging readability without altering API behavior.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality)

@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Add toString() override to ScriptKey for better debugging

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Adds toString() override to ScriptKey class
• Returns the underlying identifier for improved debugging
• Replaces unhelpful default Object.toString() output
• Non-breaking change with no functional impact

Grey Divider

File Changes

1. java/src/org/openqa/selenium/ScriptKey.java ✨ Enhancement +5/-0

Override toString() to return script identifier

• Adds @Override annotated toString() method
• Returns the identifier field directly
• Improves debugging output clarity in logs and exceptions
• No changes to equals(), hashCode(), or JSON serialization

java/src/org/openqa/selenium/ScriptKey.java


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the C-java Java Bindings label Mar 2, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 2, 2026

Code Review by Qodo

🐞 Bugs (8) 📘 Rule violations (4) 📎 Requirement gaps (0)

Grey Divider


Action required

1. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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


2. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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


3. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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


View more (1)
4. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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



Remediation recommended

5. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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


6. Misleading handle in toString🐞 Bug ✓ Correctness
Description
UnpinnedScriptKey.toString() includes the per-instance random scriptHandle, but equals/hashCode
ignore scriptHandle (they only consider the script/identifier). This can confuse debugging because
two keys that are equal (and interchangeable in Sets/Maps) may print different handles, implying
they are different keys.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R116-121]

+  @Override
+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return String.format(
+        "UnpinnedScriptKey{handle=%s, scriptId=%s, length=%d}",
+        scriptHandle, Objects.toString(scriptId, "unset"), script.length());
Evidence
The class generates a random handle per instance, but equality is based on ScriptKey.identifier
(which is the script string for UnpinnedScriptKey) and the script field—scriptHandle is never
compared. Meanwhile, toString prominently prints scriptHandle, so string representations can differ
for equal objects.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-109]
java/src/org/openqa/selenium/ScriptKey.java[41-48]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-122]

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

## Issue description
`UnpinnedScriptKey.toString()` currently prints a per-instance random `scriptHandle`, but `equals()`/`hashCode()` ignore `scriptHandle` and treat keys as equal based on script content. This can mislead debugging/log analysis because equal keys can render different strings.
### Issue Context
- `scriptHandle` is generated randomly per instance.
- Equality is based on `ScriptKey.identifier` (which is the script string for `UnpinnedScriptKey`) and `script`.
- `toString()` prints `handle=...`, suggesting object uniqueness.
### Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-122]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
Suggested direction:
- Prefer making `toString()` reflect the equality identity: e.g., include `length` + `scriptHash` (or other stable digest) and either omit `handle` or label it explicitly as per-instance (e.g., `instanceHandle=`).

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


7. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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


View more (4)
8. Content-derived script fingerprint 🐞 Bug ⛨ Security
Description
UnpinnedScriptKey.toString() includes script.hashCode(), which is a deterministic value derived from
the full script content; this can leak a stable fingerprint/metadata and can be brute-forced for
short/guessable scripts. It also forces computing the String hash (potentially scanning the whole
script at least once) just to log/debug, despite having a safe random handle already available.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R117-126]

+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return "UnpinnedScriptKey{"
+        + "scriptHash="
+        + script.hashCode()
+        + ", scriptId="
+        + Objects.toString(scriptId, "unset")
+        + ", length="
+        + script.length()
+        + "}";
Evidence
UnpinnedScriptKey already has a random per-instance handle (scriptHandle) that does not depend on
script contents, but the new toString() instead derives a fingerprint from the raw script via
script.hashCode() while also referencing script length. Since the class stores and uses the raw
script in other methods, scripts can be non-trivial, making content-derived hashing in toString() an
avoidable cost and metadata disclosure.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[32-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[81-88]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-126]

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

## Issue description
`UnpinnedScriptKey.toString()` currently includes `script.hashCode()`, which is deterministic and derived from the full script contents. This can leak a stable fingerprint/metadata of the script and can also trigger avoidable work (hash computation) just for logging.
## Issue Context
`UnpinnedScriptKey` already maintains a per-instance random `scriptHandle` that can be used as a safe, concise identifier for debugging and correlation without inspecting script contents.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-126]
## Suggested change (direction)
- Replace `scriptHash=...script.hashCode()` with something like `handle=<scriptHandle>` (and optionally keep `scriptId` and `length`).
- Example shape: `UnpinnedScriptKey{handle=<handle>, scriptId=<id|unset>, length=<n>}`

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


9. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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


10. Misleading handle in toString🐞 Bug ✓ Correctness
Description
UnpinnedScriptKey.toString() includes the per-instance random scriptHandle, but equals/hashCode
ignore scriptHandle (they only consider the script/identifier). This can confuse debugging because
two keys that are equal (and interchangeable in Sets/Maps) may print different handles, implying
they are different keys.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R116-121]

+  @Override
+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return String.format(
+        "UnpinnedScriptKey{handle=%s, scriptId=%s, length=%d}",
+        scriptHandle, Objects.toString(scriptId, "unset"), script.length());
Evidence
The class generates a random handle per instance, but equality is based on ScriptKey.identifier
(which is the script string for UnpinnedScriptKey) and the script field—scriptHandle is never
compared. Meanwhile, toString prominently prints scriptHandle, so string representations can differ
for equal objects.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-109]
java/src/org/openqa/selenium/ScriptKey.java[41-48]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-122]

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

## Issue description
`UnpinnedScriptKey.toString()` currently prints a per-instance random `scriptHandle`, but `equals()`/`hashCode()` ignore `scriptHandle` and treat keys as equal based on script content. This can mislead debugging/log analysis because equal keys can render different strings.
### Issue Context
- `scriptHandle` is generated randomly per instance.
- Equality is based on `ScriptKey.identifier` (which is the script string for `UnpinnedScriptKey`) and `script`.
- `toString()` prints `handle=...`, suggesting object uniqueness.
### Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-122]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
Suggested direction:
- Prefer making `toString()` reflect the equality identity: e.g., include `length` + `scriptHash` (or other stable digest) and either omit `handle` or label it explicitly as per-instance (e.g., `instanceHandle=`).

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


11. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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



Advisory comments

12. Leak test too permissive 🐞 Bug ⛯ Reliability
Description
The new UnpinnedScriptKeyTest only asserts that toString() does not contain the entire script
string; it would not catch partial leakage (e.g., a token substring) if toString() later changes to
include a prefix/suffix. Strengthening this test would better prevent future regressions.
Code

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[R26-32]

+  @Test
+  void toStringDoesNotExposeRawScript() {
+    String script = "return 'SECRET_TOKEN';";
+    UnpinnedScriptKey key = new UnpinnedScriptKey(script);
+
+    assertThat(key.toString()).doesNotContain(script);
+  }
Evidence
The test checks only that the full script value is not present, but not that sensitive substrings
(like SECRET_TOKEN) are absent; a future change that logs part of the script could slip by.

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[26-32]

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

## Issue description
The test guarding against script leakage in `UnpinnedScriptKey.toString()` only checks that the full script string is not present; partial leakage could still pass.
## Issue Context
We want the test to fail if any meaningful script content (especially secrets) is accidentally included in `toString()` output.
## Fix Focus Areas
- java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[26-32]
## Suggested change (direction)
- Add assertions like:
- `doesNotContain("SECRET_TOKEN")`
- optionally `doesNotContain("return")`
- Add positive assertions that the output contains expected safe fields/prefix.

ⓘ 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

@seethinajayadileep seethinajayadileep changed the title [JAVA] Add toString() to ScriptKey for improved debugging readability [JAVA] Enhance ScriptKey.toString() and mask script content in UnpinnedScriptKey Mar 2, 2026
@cgoldberg cgoldberg changed the title [JAVA] Enhance ScriptKey.toString() and mask script content in UnpinnedScriptKey [java] Enhance ScriptKey.toString() and mask script content in UnpinnedScriptKey Mar 2, 2026
@cgoldberg
Copy link
Member

Many RBE CI tests are failing on this branch.. I haven't investigated if they are related to these changes.

@asolntsev asolntsev added this to the 4.42.0 milestone Mar 2, 2026
@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

[java] Add toString() to ScriptKey and mask script content in UnpinnedScriptKey

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add toString() to ScriptKey returning the identifier directly
• Override toString() in UnpinnedScriptKey to prevent raw script exposure in logs
• Add unit tests for both ScriptKey and UnpinnedScriptKey toString behavior

Grey Divider

File Changes

1. java/src/org/openqa/selenium/ScriptKey.java ✨ Enhancement +5/-0

Override toString() to return identifier in ScriptKey

• Added toString() override that returns the identifier field directly
• Improves debugging readability over default Object.toString() output

java/src/org/openqa/selenium/ScriptKey.java


2. java/src/org/openqa/selenium/UnpinnedScriptKey.java ✨ Enhancement +8/-0

Mask raw script content in UnpinnedScriptKey toString()

• Added toString() override to avoid exposing raw JavaScript source in logs
• Returns a structured format with scriptHandle, scriptId, and script length
• Prevents log bloat and potential sensitive script content leakage

java/src/org/openqa/selenium/UnpinnedScriptKey.java


3. java/test/org/openqa/selenium/ScriptKeyTest.java 🧪 Tests +33/-0

Add unit tests for ScriptKey toString() behavior

• New test class for ScriptKey
• Tests that toString() returns the identifier string
• Tests edge case with empty identifier string

java/test/org/openqa/selenium/ScriptKeyTest.java


View more (1)
4. java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java 🧪 Tests +45/-0

Add unit tests for UnpinnedScriptKey toString() behavior

• New test class for UnpinnedScriptKey
• Verifies raw script content is not exposed in toString() output
• Verifies structured format includes handle and scriptId fields

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 2, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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



Remediation recommended

2. Misleading handle in toString 🐞 Bug ✧ Quality ⭐ New
Description
UnpinnedScriptKey.toString() includes the per-instance random scriptHandle, but equals/hashCode
ignore scriptHandle (they only consider the script/identifier). This can confuse debugging because
two keys that are equal (and interchangeable in Sets/Maps) may print different handles, implying
they are different keys.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R116-121]

+  @Override
+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return String.format(
+        "UnpinnedScriptKey{handle=%s, scriptId=%s, length=%d}",
+        scriptHandle, Objects.toString(scriptId, "unset"), script.length());
Evidence
The class generates a random handle per instance, but equality is based on ScriptKey.identifier
(which is the script string for UnpinnedScriptKey) and the script field—scriptHandle is never
compared. Meanwhile, toString prominently prints scriptHandle, so string representations can differ
for equal objects.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-109]
java/src/org/openqa/selenium/ScriptKey.java[41-48]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-122]

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

### Issue description
`UnpinnedScriptKey.toString()` currently prints a per-instance random `scriptHandle`, but `equals()`/`hashCode()` ignore `scriptHandle` and treat keys as equal based on script content. This can mislead debugging/log analysis because equal keys can render different strings.

### Issue Context
- `scriptHandle` is generated randomly per instance.
- Equality is based on `ScriptKey.identifier` (which is the script string for `UnpinnedScriptKey`) and `script`.
- `toString()` prints `handle=...`, suggesting object uniqueness.

### Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-122]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]

Suggested direction:
- Prefer making `toString()` reflect the equality identity: e.g., include `length` + `scriptHash` (or other stable digest) and either omit `handle` or label it explicitly as per-instance (e.g., `instanceHandle=`).

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


3. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

ⓘ 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

@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

[java] Add toString() to ScriptKey and mask script content in UnpinnedScriptKey

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add toString() to ScriptKey returning the identifier string
• Override toString() in UnpinnedScriptKey to avoid exposing raw script content in logs
• Add unit tests for both ScriptKey and UnpinnedScriptKey toString behavior

Grey Divider

File Changes

1. java/src/org/openqa/selenium/ScriptKey.java ✨ Enhancement +5/-0

Override toString() to return identifier in ScriptKey

• Added toString() override that returns the identifier field directly
• Improves debugging readability over default Object.toString() output

java/src/org/openqa/selenium/ScriptKey.java


2. java/src/org/openqa/selenium/UnpinnedScriptKey.java ✨ Enhancement +13/-0

Mask raw script content in UnpinnedScriptKey toString()

• Added toString() override to prevent raw JavaScript script content from appearing in logs
• Returns a structured string with scriptHash, scriptId, and length fields instead

java/src/org/openqa/selenium/UnpinnedScriptKey.java


3. java/test/org/openqa/selenium/ScriptKeyTest.java 🧪 Tests +33/-0

Add unit tests for ScriptKey toString()

• New test file for ScriptKey
• Tests that toString() returns the identifier for a normal and empty identifier

java/test/org/openqa/selenium/ScriptKeyTest.java


View more (1)
4. java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java 🧪 Tests +45/-0

Add unit tests for UnpinnedScriptKey toString()

• New test file for UnpinnedScriptKey
• Tests that toString() does not expose raw script content
• Tests that toString() includes scriptId when set and always includes length

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 2, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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


2. ScriptKey.toString() untested 📘 Rule violation ⛯ Reliability
Description
The PR changes observable behavior by overriding toString() but does not add a corresponding unit
test to lock in the new string representation. This risks regressions and violates the requirement
to include tests for behavior changes.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
PR Compliance ID 4 requires behavior changes to be accompanied by appropriate tests, preferring
small unit tests. The diff introduces a new toString() behavior returning identifier, but no
test changes are present to validate this behavior.

AGENTS.md
java/src/org/openqa/selenium/ScriptKey.java[36-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
`ScriptKey` now overrides `toString()` to return `identifier`, but there is no unit test asserting this behavior.
## Issue Context
This is an observable behavior change intended to improve debugging readability; a small unit test should pin the expected output and prevent regressions, preferably without relying on browser/integration infrastructure.
## Fix Focus Areas
- java/src/org/openqa/selenium/ScriptKey.java[36-39]

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



Remediation recommended

3. Content-derived script fingerprint 🐞 Bug ⛨ Security ⭐ New
Description
UnpinnedScriptKey.toString() includes script.hashCode(), which is a deterministic value derived from
the full script content; this can leak a stable fingerprint/metadata and can be brute-forced for
short/guessable scripts. It also forces computing the String hash (potentially scanning the whole
script at least once) just to log/debug, despite having a safe random handle already available.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R117-126]

+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return "UnpinnedScriptKey{"
+        + "scriptHash="
+        + script.hashCode()
+        + ", scriptId="
+        + Objects.toString(scriptId, "unset")
+        + ", length="
+        + script.length()
+        + "}";
Evidence
UnpinnedScriptKey already has a random per-instance handle (scriptHandle) that does not depend on
script contents, but the new toString() instead derives a fingerprint from the raw script via
script.hashCode() while also referencing script length. Since the class stores and uses the raw
script in other methods, scripts can be non-trivial, making content-derived hashing in toString() an
avoidable cost and metadata disclosure.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[32-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[81-88]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-126]

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

## Issue description
`UnpinnedScriptKey.toString()` currently includes `script.hashCode()`, which is deterministic and derived from the full script contents. This can leak a stable fingerprint/metadata of the script and can also trigger avoidable work (hash computation) just for logging.

## Issue Context
`UnpinnedScriptKey` already maintains a per-instance random `scriptHandle` that can be used as a safe, concise identifier for debugging and correlation without inspecting script contents.

## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-126]

## Suggested change (direction)
- Replace `scriptHash=...script.hashCode()` with something like `handle=<scriptHandle>` (and optionally keep `scriptId` and `length`).
- Example shape: `UnpinnedScriptKey{handle=<handle>, scriptId=<id|unset>, length=<n>}`

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


4. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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


5. Misleading handle in toString 🐞 Bug ✓ Correctness
Description
UnpinnedScriptKey.toString() includes the per-instance random scriptHandle, but equals/hashCode
ignore scriptHandle (they only consider the script/identifier). This can confuse debugging because
two keys that are equal (and interchangeable in Sets/Maps) may print different handles, implying
they are different keys.
Code

java/src/org/openqa/selenium/UnpinnedScriptKey.java[R116-121]

+  @Override
+  public String toString() {
+    // Avoid dumping raw JavaScript into logs: in UnpinnedScriptKey the identifier is the script.
+    return String.format(
+        "UnpinnedScriptKey{handle=%s, scriptId=%s, length=%d}",
+        scriptHandle, Objects.toString(scriptId, "unset"), script.length());
Evidence
The class generates a random handle per instance, but equality is based on ScriptKey.identifier
(which is the script string for UnpinnedScriptKey) and the script field—scriptHandle is never
compared. Meanwhile, toString prominently prints scriptHandle, so string representations can differ
for equal objects.

java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-109]
java/src/org/openqa/selenium/ScriptKey.java[41-48]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[116-122]

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

## Issue description
`UnpinnedScriptKey.toString()` currently prints a per-instance random `scriptHandle`, but `equals()`/`hashCode()` ignore `scriptHandle` and treat keys as equal based on script content. This can mislead debugging/log analysis because equal keys can render different strings.
### Issue Context
- `scriptHandle` is generated randomly per instance.
- Equality is based on `ScriptKey.identifier` (which is the script string for `UnpinnedScriptKey`) and `script`.
- `toString()` prints `handle=...`, suggesting object uniqueness.
### Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[94-122]
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
Suggested direction:
- Prefer making `toString()` reflect the equality identity: e.g., include `length` + `scriptHash` (or other stable digest) and either omit `handle` or label it explicitly as per-instance (e.g., `instanceHandle=`).

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


View more (1)
6. Unpinned key dumps script 🐞 Bug ⛨ Security
Description
ScriptKey.toString() now returns the internal identifier; UnpinnedScriptKey passes the full
JavaScript source as that identifier, so printing/logging a ScriptKey from the default pin() path
can output the entire script (large/noisy and potentially sensitive). This is an API-behavior change
beyond “readable identifier” and can cause log bloat or unintended disclosure in exception
messages/logging.
Code

java/src/org/openqa/selenium/ScriptKey.java[R36-39]

+  @Override
+  public String toString() {
+    return identifier;
+  }
Evidence
ScriptKey now renders itself as its identifier. While ScriptKey’s constructor enforces non-null
identifiers, UnpinnedScriptKey sets that identifier to the raw script string (super(script)), and
JavascriptExecutor’s default pin() implementation returns an UnpinnedScriptKey. This means callers
who log/format the returned ScriptKey (or code that concatenates it into exception messages) may now
emit the full script contents.

java/src/org/openqa/selenium/ScriptKey.java[24-40]
java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-63]
java/src/org/openqa/selenium/JavascriptExecutor.java[144-159]
java/src/org/openqa/selenium/chromium/ChromiumDriver.java[264-270]

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

## Issue description
`ScriptKey.toString()` now returns `identifier`. `UnpinnedScriptKey` sets the parent `identifier` to the full JavaScript source via `super(script)`, so `toString()` can emit the entire script content when a `ScriptKey` is logged or interpolated into messages.
## Issue Context
`JavascriptExecutor#pin(String)` default implementation returns an `UnpinnedScriptKey`, and other code may concatenate a `ScriptKey` into exception messages. Returning a full script body from `toString()` can bloat logs and may expose sensitive script content.
## Fix Focus Areas
- java/src/org/openqa/selenium/UnpinnedScriptKey.java[58-93]
- java/src/org/openqa/selenium/ScriptKey.java[24-40]
- java/src/org/openqa/selenium/JavascriptExecutor.java[144-170]
## Suggested change
- Add `@Override public String toString()` in `UnpinnedScriptKey` returning something like:
- `scriptId` if non-null, else `scriptHandle`, optionally prefixed (e.g., `"UnpinnedScriptKey{" + scriptHandle + "}"`).
- (Optional) Add/adjust a small unit test to ensure `UnpinnedScriptKey#toString()` does not return the full script string.

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



Advisory comments

7. Leak test too permissive 🐞 Bug ⛯ Reliability ⭐ New
Description
The new UnpinnedScriptKeyTest only asserts that toString() does not contain the entire script
string; it would not catch partial leakage (e.g., a token substring) if toString() later changes to
include a prefix/suffix. Strengthening this test would better prevent future regressions.
Code

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[R26-32]

+  @Test
+  void toStringDoesNotExposeRawScript() {
+    String script = "return 'SECRET_TOKEN';";
+    UnpinnedScriptKey key = new UnpinnedScriptKey(script);
+
+    assertThat(key.toString()).doesNotContain(script);
+  }
Evidence
The test checks only that the full script value is not present, but not that sensitive substrings
(like SECRET_TOKEN) are absent; a future change that logs part of the script could slip by.

java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[26-32]

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

## Issue description
The test guarding against script leakage in `UnpinnedScriptKey.toString()` only checks that the full script string is not present; partial leakage could still pass.

## Issue Context
We want the test to fail if any meaningful script content (especially secrets) is accidentally included in `toString()` output.

## Fix Focus Areas
- java/test/org/openqa/selenium/UnpinnedScriptKeyTest.java[26-32]

## Suggested change (direction)
- Add assertions like:
 - `doesNotContain("SECRET_TOKEN")`
 - optionally `doesNotContain("return")`
- Add positive assertions that the output contains expected safe fields/prefix.

ⓘ 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

@asolntsev asolntsev enabled auto-merge (squash) March 2, 2026 20:51
@asolntsev asolntsev merged commit 86dc705 into SeleniumHQ:trunk Mar 2, 2026
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants