Skip to content

[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName - #17898

Open
diemol wants to merge 5 commits into
trunkfrom
fix/17845-slot-matcher-automation-name-direction
Open

[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName#17898
diemol wants to merge 5 commits into
trunkfrom
fix/17845-slot-matcher-automation-name-direction

Conversation

@diemol

@diemol diemol commented Aug 10, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

Fixes #17845

💥 What does this PR do?

DefaultSlotMatcher.extensionCapabilitiesMatch() only inspects extension capability names the stereotype declares. If a stereotype declares no extension (:-namespaced) capabilities at all — e.g. a plain browser node — the check short-circuits to true regardless of what the request asks for. This lets a request differentiated solely by an identity extension capability (appium:automationName) match a completely unrelated, non-Appium node, as long as browserName is absent from the request and platformName resolves to the same Platform family.

This adds a new automationNameMatch() check, called alongside the existing extension-capability checks in matches(). It only rejects a match when the stereotype shows no existing Appium-awareness at all (no relevant extension capability, and no non-W3C-compliant platformVersion, which the code already treats as an Appium signal) while the request specifies an automationName. Stereotypes that already show some Appium-awareness (e.g. relay nodes advertising appium:platformVersion but omitting automationName) are left untouched, preserving existing relay-node matching behavior.

🔧 Implementation Notes

The issue proposed an unconditional bidirectional automationName check. That approach was tested against the existing test suite and found to break two passing relay-node tests, where Appium relay nodes intentionally omit automationName while still being valid targets for varied automation sessions. The fix here gates the check on stereotype Appium-awareness instead, using the same platformVersion signal the codebase already relies on elsewhere in this file, so relay-node flexibility is preserved while the reported misroute is closed.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: Initial implementation of automationNameMatch() and the two regression tests, drafted from the issue's analysis and root-caused/iterated against the existing test suite to avoid regressing relay-node matching.
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

None.

🔄 Types of changes

  • Bug fix (backwards compatible)

…ionName

extensionCapabilitiesMatch() only inspected extension capability names the
stereotype declared, so a stereotype declaring none at all (e.g. a plain
browser node) matched any requested automationName by default. This let a
native-automation request differentiated solely by automationName match an
unrelated browser-only node.

Add automationNameMatch(), gated on the stereotype showing some existing
Appium-awareness (a relevant extension capability, or a non-W3C-compliant
platformVersion), so relay-node matching keeps working while a stereotype
with no such awareness no longer matches on an undeclared automationName.

Fixes #17845

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Aug 10, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Grid: Prevent slot matching on undeclared appium:automationName

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Prevent plain browser stereotypes matching requests differentiated only by
 appium:automationName.
• Add automationNameMatch() gating based on whether the stereotype is Appium-aware.
• Add regression tests for #17845 and preserve relay-node matching behavior.
Diagram

graph TD
  A["Stereotype + request caps"] --> B["DefaultSlotMatcher.matches()"] --> C{"Extension caps match?"} -->|"pass"| D{"AutomationName allowed?"} -->|"pass"| E{"platformVersion match?"} -->|"pass"| F["Final browser/platform match"] --> G["Match"]
  C -->|"fail"| H["No match"]
  D -->|"fail"| H
  E -->|"fail"| H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Unconditional bidirectional automationName match
  • ➕ Very clear rule: stereotype and request must agree on automationName presence/value
  • ➖ Breaks existing relay-node behavior where nodes intentionally omit automationName to stay flexible
  • ➖ Would require either relaxing relay behavior or expanding stereotype declarations across deployments
2. Fold automationName into extensionCapabilitiesMatch()
  • ➕ Keeps all extension-capability matching rules in one place
  • ➕ Avoids adding another top-level check in matches()
  • ➖ Harder to express the required nuance (only enforce when stereotype is not Appium-aware) without complicating extension-cap matching semantics
  • ➖ Blurs the intent between 'match declared extensions' vs 'guard against Appium-only differentiation'

Recommendation: Keep the current gated automationNameMatch() approach: it closes the misroute when a stereotype is clearly non-Appium, while preserving existing relay-node flexibility by treating other Appium signals (e.g., platformVersion or relevant extension caps) as sufficient awareness.

Files changed (2) +65 / -0

Bug fix (1) +24 / -0
DefaultSlotMatcher.javaAdd Appium automationName gating to slot matching +24/-0

Add Appium automationName gating to slot matching

• Adds 'automationNameMatch()' and invokes it during 'matches()' to prevent non-Appium stereotypes from matching requests differentiated only by 'automationName'. The check is intentionally gated so Appium-aware stereotypes (e.g., those signaling via 'platformVersion' or relevant extensions) continue matching even if they omit 'automationName'.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

Tests (1) +41 / -0
DefaultSlotMatcherTest.javaRegression tests for automationName-only request matching +41/-0

Regression tests for automationName-only request matching

• Adds a regression test ensuring a plain browser stereotype does not match a request containing only 'appium:automationName'. Adds a companion test verifying that an Appium-aware stereotype (via 'appium:platformVersion') still matches requests that include 'appium:automationName' even if the stereotype omits it.

java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

@qodo-code-review

qodo-code-review Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Appium-aware heuristic too broad ✓ Resolved 🐞 Bug ≡ Correctness
Description
automationNameMatch() treats any non-vendor extension capability key (any : capability not in
goog:/moz:/ms:/safari:/se: and not *options*) as “Appium-aware”, so a non-Appium node
advertising an unrelated custom extension capability will bypass the automationName gate. This can
re-allow Appium requests differentiated only by automationName to be considered eligible for the
wrong slot whenever other non-extension checks match.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R203-207]

+                name ->
+                    name.contains("platformVersion")
+                        || (name.contains(":")
+                            && !name.toLowerCase().contains("options")
+                            && EXTENSION_CAPABILITIES_PREFIXES.stream().noneMatch(name::contains)));
Evidence
The new logic marks a stereotype as Appium-aware based on the presence of any : capability not in
a short vendor-prefix ignore list; this is not equivalent to Appium-awareness. The test suite
demonstrates generic non-vendor extension keys like prefixed:cheese are used for non-Appium
extension matching, so treating them as Appium signals will bypass the new automationName protection
for unrelated nodes.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[48-55]
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[165-178]
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[195-213]
java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[526-571]

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

### Issue description
`automationNameMatch()` currently infers “Appium-aware” from *any* non-vendor extension capability key. This is overly broad and causes unrelated custom extension caps to disable the new automationName protection.

### Issue Context
- `EXTENSION_CAPABILITIES_PREFIXES` only lists vendor browser prefixes (`goog:`, `moz:`, `ms:`, `safari:`, `se:`). Many non-Appium nodes can still advertise other extension keys (e.g. `prefixed:cheese` in tests), which should not imply Appium support.
- The intent of this PR is to treat *Appium signals* as Appium-awareness (e.g., `appium:*` keys and the existing non-W3C `platformVersion` signal), not arbitrary extension caps.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[48-55]
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[195-213]
- java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[526-571]

### Suggested change
Constrain `stereotypeIsAppiumAware` to true only when the stereotype declares Appium-specific keys (e.g. `name.startsWith("appium:")`) and/or the existing `platformVersion` signal (preferably exact key match rather than substring). This preserves relay flexibility while preventing unrelated extension caps from bypassing the new automationName gate.

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



Remediation recommended

2. Options automationName ignored 🐞 Bug ≡ Correctness ⭐ New
Description
automationNameMatch only inspects top-level capability keys, so a request that specifies
automationName inside an *:options map (e.g. appium:options) will bypass the new guard and may
still match a non-Appium-aware stereotype. This leaves the reported misrouting hole open for a
capability format that this matcher explicitly treats as “handled by the remote endpoint”.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R206-208]

+    return capabilities.getCapabilityNames().stream()
+        .noneMatch(name -> name.equals("automationName") || name.endsWith(":automationName"));
+  }
Evidence
The new guard only checks top-level capability names; capability names are the keyset of asMap(),
so nested maps (like appium:options) are invisible to this check. The Grid session payload parser
constructs ImmutableCapabilities directly from merged JSON maps, preserving nested maps as values,
and the matcher explicitly filters out *options* extension capabilities elsewhere, so
automationName nested under options bypasses both extension matching and the new automation-name
matching.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[165-208]
java/src/org/openqa/selenium/Capabilities.java[98-108]
java/src/org/openqa/selenium/remote/NewSessionPayload.java[214-223]

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

### Issue description
`DefaultSlotMatcher.automationNameMatch()` only checks `capabilities.getCapabilityNames()` (top-level keys) for `automationName` / `*:automationName`. If a client sends automationName nested under an `*:options` capability (e.g. `appium:options: { automationName: ... }`), the key never appears in `getCapabilityNames()`, so the new gate does not block matching against non-Appium-aware stereotypes.

### Issue Context
- This matcher already intentionally skips matching extension capabilities whose name contains `options`, implying these payload shapes are expected to appear and be forwarded.
- `NewSessionPayload` builds `ImmutableCapabilities` directly from merged JSON maps, without flattening nested maps.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[165-208]
- java/src/org/openqa/selenium/Capabilities.java[98-108]
- java/src/org/openqa/selenium/remote/NewSessionPayload.java[214-223]

### Suggested fix
Extend `automationNameMatch()` to also detect automationName when it is provided inside an options map (at least `appium:options`, and ideally any `*:options` where the value is a map). If options contains `automationName` (or `appium:automationName`), treat it as “request specifies automationName” for the purpose of rejecting non-Appium-aware stereotypes.

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


3. automationName key substring match ✓ Resolved 🐞 Bug ≡ Correctness
Description
automationNameMatch() rejects requests by checking whether any requested capability key contains
the substring automationName, which can accidentally match unrelated custom keys. This can cause
false negatives (unexpected no-match) for non-Appium requests that happen to use a different
capability name containing that substring.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R211-213]

+    return capabilities.getCapabilityNames().stream()
+        .noneMatch(name -> name.contains("automationName"));
+  }
Evidence
The new check uses substring matching on capability *names* rather than matching a specific
capability key, so any custom capability containing that substring will trigger the Appium-specific
rejection logic.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[211-213]

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 request-side check uses `name.contains("automationName")`, which is overly broad and can treat unrelated custom capability names as an Appium automationName request.

### Issue Context
This method is intended to special-case Appium’s `automationName` differentiation. Substring matching risks blocking unrelated capabilities like `my:automationNameOverride`.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[211-213]

### Suggested change
Replace substring matching with exact key matching against the intended keys (e.g. `"appium:automationName"` and possibly legacy/non-namespaced `"automationName"` if supported), using `equals` (or `equalsIgnoreCase` only if the project treats capability names case-insensitively elsewhere).

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


4. matches missing Javadoc 📘 Rule violation ✧ Quality
Description
The public method matches(Capabilities, Capabilities) was modified but still has no Javadoc block
documenting parameters/return value, which violates the requirement for complete Javadoc on changed
public API methods. This reduces API clarity and makes behavior changes harder to audit and consume
correctly.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R78-81]

+    if (!automationNameMatch(stereotype, capabilities)) {
+      return false;
+    }
+
Evidence
PR Compliance ID 330201 requires a Javadoc block for each changed public method, including @param
tags for all parameters and a @return tag when non-void. In the updated source, `public boolean
matches(Capabilities stereotype, Capabilities capabilities) has no /** ... */` Javadoc immediately
above it.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[59-61]

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

## Issue description
A changed public method (`matches`) lacks a Javadoc block, but the checklist requires complete Javadoc (including `@param` and `@return`) for all changed public API methods.

## Issue Context
`DefaultSlotMatcher.matches(...)` is `public` and was modified in this PR (new `automationNameMatch(...)` gating). The method should be documented so callers and maintainers can understand matching semantics and expectations for the two `Capabilities` parameters.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[59-103]

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


Grey Divider

Context
✅ Compliance rules (platform): 18 rules

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous review results

Review updated until commit a5511ed

Results up to commit 8e4d708 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Appium-aware heuristic too broad ✓ Resolved 🐞 Bug ≡ Correctness
Description
automationNameMatch() treats any non-vendor extension capability key (any : capability not in
goog:/moz:/ms:/safari:/se: and not *options*) as “Appium-aware”, so a non-Appium node
advertising an unrelated custom extension capability will bypass the automationName gate. This can
re-allow Appium requests differentiated only by automationName to be considered eligible for the
wrong slot whenever other non-extension checks match.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R203-207]

+                name ->
+                    name.contains("platformVersion")
+                        || (name.contains(":")
+                            && !name.toLowerCase().contains("options")
+                            && EXTENSION_CAPABILITIES_PREFIXES.stream().noneMatch(name::contains)));
Evidence
The new logic marks a stereotype as Appium-aware based on the presence of any : capability not in
a short vendor-prefix ignore list; this is not equivalent to Appium-awareness. The test suite
demonstrates generic non-vendor extension keys like prefixed:cheese are used for non-Appium
extension matching, so treating them as Appium signals will bypass the new automationName protection
for unrelated nodes.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[48-55]
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[165-178]
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[195-213]
java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[526-571]

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

### Issue description
`automationNameMatch()` currently infers “Appium-aware” from *any* non-vendor extension capability key. This is overly broad and causes unrelated custom extension caps to disable the new automationName protection.

### Issue Context
- `EXTENSION_CAPABILITIES_PREFIXES` only lists vendor browser prefixes (`goog:`, `moz:`, `ms:`, `safari:`, `se:`). Many non-Appium nodes can still advertise other extension keys (e.g. `prefixed:cheese` in tests), which should not imply Appium support.
- The intent of this PR is to treat *Appium signals* as Appium-awareness (e.g., `appium:*` keys and the existing non-W3C `platformVersion` signal), not arbitrary extension caps.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[48-55]
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[195-213]
- java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[526-571]

### Suggested change
Constrain `stereotypeIsAppiumAware` to true only when the stereotype declares Appium-specific keys (e.g. `name.startsWith("appium:")`) and/or the existing `platformVersion` signal (preferably exact key match rather than substring). This preserves relay flexibility while preventing unrelated extension caps from bypassing the new automationName gate.

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



Remediation recommended
2. matches missing Javadoc 📘 Rule violation ✧ Quality
Description
The public method matches(Capabilities, Capabilities) was modified but still has no Javadoc block
documenting parameters/return value, which violates the requirement for complete Javadoc on changed
public API methods. This reduces API clarity and makes behavior changes harder to audit and consume
correctly.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R78-81]

+    if (!automationNameMatch(stereotype, capabilities)) {
+      return false;
+    }
+
Evidence
PR Compliance ID 330201 requires a Javadoc block for each changed public method, including @param
tags for all parameters and a @return tag when non-void. In the updated source, `public boolean
matches(Capabilities stereotype, Capabilities capabilities) has no /** ... */` Javadoc immediately
above it.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[59-61]

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

## Issue description
A changed public method (`matches`) lacks a Javadoc block, but the checklist requires complete Javadoc (including `@param` and `@return`) for all changed public API methods.

## Issue Context
`DefaultSlotMatcher.matches(...)` is `public` and was modified in this PR (new `automationNameMatch(...)` gating). The method should be documented so callers and maintainers can understand matching semantics and expectations for the two `Capabilities` parameters.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[59-103]

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


3. automationName key substring match ✓ Resolved 🐞 Bug ≡ Correctness
Description
automationNameMatch() rejects requests by checking whether any requested capability key contains
the substring automationName, which can accidentally match unrelated custom keys. This can cause
false negatives (unexpected no-match) for non-Appium requests that happen to use a different
capability name containing that substring.
Code

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[R211-213]

+    return capabilities.getCapabilityNames().stream()
+        .noneMatch(name -> name.contains("automationName"));
+  }
Evidence
The new check uses substring matching on capability *names* rather than matching a specific
capability key, so any custom capability containing that substring will trigger the Appium-specific
rejection logic.

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[211-213]

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 request-side check uses `name.contains("automationName")`, which is overly broad and can treat unrelated custom capability names as an Appium automationName request.

### Issue Context
This method is intended to special-case Appium’s `automationName` differentiation. Substring matching risks blocking unrelated capabilities like `my:automationNameOverride`.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[211-213]

### Suggested change
Replace substring matching with exact key matching against the intended keys (e.g. `"appium:automationName"` and possibly legacy/non-namespaced `"automationName"` if supported), using `equals` (or `equalsIgnoreCase` only if the project treats capability names case-insensitively elsewhere).

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


Qodo Logo

Comment thread java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
Comment thread java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java Outdated
Comment thread java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
Fixes two issues found in CI/review of #17898:
- Format / Check Format was failing on ImmutableCapabilities argument
  wrapping in the new tests; re-ran ./go format to match.
- Qodo correctly flagged that treating any non-vendor extension
  capability as "Appium-aware" was too broad -- a node advertising an
  unrelated custom extension capability (e.g. prefixed:cheese) would
  incorrectly bypass the automationName gate. Narrow the signal to
  appium:-prefixed capabilities and platformVersion specifically, and
  tighten the automationName check to an exact/suffix match instead of
  a bare substring. Added a regression test for the narrowed case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit e62d621

google-java-format wants the noneMatch lambda collapsed onto one line;
the previous manual edit split it across two.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 97ccbef

Per Qodo review feedback on #17898: document the public matches()
method's parameters and return value, since its behavior changed with
the new automationName gating.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0d2c0c8

@qodo-code-review

qodo-code-review Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

No code changes since the last review — review skipped

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related C-java Java Bindings

Projects

None yet

2 participants