_WD_SetElementValue - $_WD_OPTION_Advanced - $sValue is not escaped

mlipok commented 1 year ago

Bug report

Describe the bug

_WD_SetElementValue() in $_WD_OPTION_Advanced mode does not use any StringEscape action on $sValue https://github.com/Danp2/au3WebDriver/blob/f00d0d6a62f101fb2e8ff184750bdb05baefcfa2/wd_helper.au3#L2617-L2624

How to reproduce

check with: wd_demo.au3: UserFile :

Using file: UserTesting_EscapeString.au3 with following content:

ConsoleWrite("! Start: UserTesting_EscapeString.au3" & @CRLF)

ConsoleWrite("- Test 1:" & @CRLF)
_WD_Navigate($sSession, 'https://www.google.com')
_WD_LoadWait($sSession)
MsgBox($MB_OK + $MB_TOPMOST + $MB_ICONWARNING, "Warning #" & @ScriptLineNumber, "If you see COOKIE accept panel close them before you will continue.")

ConsoleWrite("- Test 2:" & @CRLF)
; REMARK:  __SetVAR($IDX_VAR, $value) will set  $_VAR[$IDX_VAR] = $value
__SetVAR(0, _WD_FindElement($sSession, $_WD_LOCATOR_ByXPath, "//textarea[@name='q']"))

ConsoleWrite("- Test 3:" & @CRLF)
__SetVAR(1, ' " ! @ # $ % ^ & * ( ) - _ + = { } [ ] ; : \ | , . < > / ? ~ ' & " ' " & ' @CR' & @CR & ' @CRLF' & @CRLF & ' @LF' & @LF & ' @TAB' & @TAB & 'END')
;~ _WD_ElementAction($sSession, $_VAR[0], "VALUE", $_VAR[1])
_WD_SetElementValue($sSession, $_VAR[0], $_VAR[1], $_WD_OPTION_Advanced)

ConsoleWrite("! $_VAR[1] -" & @CRLF & $_VAR[1] & @CRLF)

ConsoleWrite("! __WD_EscapeString($_VAR[0]) -" & @CRLF & __WD_EscapeString($_VAR[1]) & @CRLF)
ConsoleWrite("! END: UserTesting_EscapeString.au3" & @CRLF)

results:

__WD_Post ==> Invalid argument [5] : HTTP status = 400
_WD_SetElementValue ==> Invalid argument [5] : Parameters:    Element=ca424034-1f02-4f2a-9af1-b5fee062a7b2    Value=<masked>    Style=1

! $_VAR[1] -
 " ! @ # $ % ^ & * ( ) - _ + = { } [ ] ; : \ | , . < > / ? ~  '  @CR
 @CRLF
 @LF
 @TAB   END
! __WD_EscapeString($_VAR[0]) -
 \" ! @ # $ % ^ & * ( ) - _ + = { } [ ] ; : \\ | , . < > / ? ~  '  @CR
 @CRLF
 @LF
 @TAB   END
! END: UserTesting_EscapeString.au3
+ wd_demo.au3: Finished: UserFile

Expected behavior

String should be escaped the same way as $_WD_OPTION_Standard (internally in _WD_ElementAction)

Screenshots

none

Additional context

none

System under test

not related

mlipok commented 1 year ago

UserTesting_EscapeString.au3 changed

mlipok commented 1 year ago

REPRO SCRIPT updated, and require: https://github.com/Danp2/au3WebDriver/pull/464

Danp2 commented 1 year ago

REPRO SCRIPT updated, and require: https://github.com/Danp2/au3WebDriver/pull/464

Please don't do this in the future. You could have easily reproduced this without requiring another PR.

Danp2 commented 1 year ago

Taking a look at the two options for _WD_SetElementValue --

$_WD_OPTION_Standard calls _WD_ElementAction, which formats the string with __WD_EscapeString
$_WD_OPTION_Advanced calls _WD_ExecuteScript, which formats the script with __WD_EscapeString, but not the arguments

I wonder if there would be any undesirable effects if we updated _WD_ExecuteScript to format the arguments. 🤔

mlipok commented 1 year ago

Do you plan to resolve this ISSUE with me ... before you release new version ? Or release will not necessarily be related with this issue.

Danp2 commented 1 year ago

As you can see from #468, my plan is to release the update tomorrow. Currently, we haven't decided on the best method to address this issue. Any fix most likely won't make the next release because --

this issue isn't critical
the fix could produce unwanted side effects

Danp2 commented 1 year ago

I wonder if there would be any undesirable effects if we updated _WD_ExecuteScript to format the arguments.

Based upon some basic tests, I can conclude that it would not be safe to format the arguments of _WD_ExecuteScript.

without encoding

    __WD_Post: URL=HTTP://127.0.0.1:4444/session/6d16a944-058c-40df-bac9-5951fb54fecc/execute/sync; Data={"script":"return arguments[0].second;", "args":[{"first": "1st", "second": "2nd", "third": "3rd"}]}
    __WD_Post ==> Success [0] : HTTP status = 200 ResponseText={"value":"2nd"}
    _WD_ExecuteScript ==> Success [0]

with encoding

    __WD_Post: URL=HTTP://127.0.0.1:4444/session/30b6951c-dc25-43c6-85ba-9c7d19d7e381/execute/sync; Data={"script":"return arguments[0].second;", "args":[{\"first\": \"1st\", \"second\": \"2nd\", \"third\": \"3rd\"}]}
    __WD_Post ==> Invalid argument [5] : HTTP status = 400 ResponseText={"value":{"error":"invalid argument","message":"Failed to decode request as JSON: {\"script\":\"return arguments[0].second;\", \"args\":[{\\\"first\\\": \\\"1st\\\", \\\"second\\\": \\\"2nd\\\", \\\"third\\\": \\\"3rd\\\"}]}","stacktrace":"Syntax error at :1:51"}}
    _WD_ExecuteScript ==> Invalid argument [5] : Failed to decode request as JSON: {"script":"return arguments[0].second;", "args":[{\"first\": \"1st\", \"second\": \"2nd\", \"third\": \"3rd\"}]}

Danp2 commented 1 year ago

I believe the existing functionality works as-is, and formatting of data, if required, should occur prior to calling _WD_SetElementValue. My recommendation is to add an entry in the remarks section of the header to document the situation.

Danp2 / au3WebDriver