Skip to content

fix: script injection in element locators with parameterized scripts#477

Open
Dor-bl wants to merge 2 commits into
serhatbolsu:masterfrom
Dor-bl:fix-script-injection-locators-3234679202092105971
Open

fix: script injection in element locators with parameterized scripts#477
Dor-bl wants to merge 2 commits into
serhatbolsu:masterfrom
Dor-bl:fix-script-injection-locators-3234679202092105971

Conversation

@Dor-bl

@Dor-bl Dor-bl commented May 31, 2026

Copy link
Copy Markdown
Contributor

This pull request refactors the way JavaScript is executed in the element finding methods to improve security and maintainability. The main change is replacing string interpolation in JavaScript execution with the use of argument passing. This helps prevent potential injection issues and makes the code more robust.

JavaScript execution improvements:

  • Updated the _find_by_dom method to use eval(arguments[0]) with argument passing instead of directly interpolating the criteria into the JavaScript string.
  • Modified the _find_by_sizzle_selector method to use jQuery(arguments[0]).get() and pass the criteria as an argument, rather than interpolating it into the JavaScript string.
  • Updated the call to application.execute_script in _find_by_sizzle_selector to pass the criteria as an argument.

Direct string interpolation was used to construct scripts for `execute_script`.
This was replaced with parameterized scripts using `arguments[0]`.
@Dor-bl

Dor-bl commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@emanlove @serhatbolsu
Can someone please have a look into this one?

@emanlove emanlove self-requested a review July 5, 2026 23:34
@emanlove emanlove self-assigned this Jul 5, 2026
@emanlove emanlove added Acknowledge Acknowledge within Release Notes needs review labels Jul 5, 2026
@emanlove

emanlove commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

@emanlove @serhatbolsu Can someone please have a look into this one?

Reviewing now ..

@chsinger

chsinger commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR @Dor-bl!

Two points from my side:

  1. As far as I can tell, _find_by_dom is not used.
    It is not registered in the _strategies dict (there is no dom= locator prefix) and has no callers anywhere in the repo. I think we can delete it completely.

  2. It would be great to add a small regression test for the fix, so a future refactoring can't silently reintroduce string interpolation.

@Dor-bl

Dor-bl commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the feedback: removed the unused _find_by_dom method and added a regression test for the script injection fix.

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

Labels

Acknowledge Acknowledge within Release Notes needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants