fix: cookies regression: send Page commands for cookies handling #1534

KazuCocoa · 2023-03-30T05:55:27Z

Using this.remote for cookies handling (except for setCookies) is current intentional behavior instead of JavaScript call. (https://gist.github.com/KazuCocoa/b935fbca6efa1abf4688cc94bc14366a for reference)

#1532

This change will send _rpc_forwardSocketData with Page. commands.

[HTTP] --> GET /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie
[HTTP] {}
[debug] [XCUITestDriver@17d4 (8cec8681)] Calling AppiumDriver.getCookies() with args: ["8cec8681-efd8-4b28-a207-393fcb59d679"]
[debug] [XCUITestDriver@17d4 (8cec8681)] Executing command 'getCookies'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:57553', page '1', target 'page-7' (id: 38): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 38): '{"cookies":[]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 34ms
[debug] [XCUITestDriver@17d4 (8cec8681)] Responding to client with driver.getCookies() result: []
[HTTP] <-- GET /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie 200 39 ms - 12
[HTTP]
[HTTP] --> DELETE /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie
[HTTP] {}
[debug] [XCUITestDriver@17d4 (8cec8681)] Calling AppiumDriver.deleteCookies() with args: ["8cec8681-efd8-4b28-a207-393fcb59d679"]
[debug] [XCUITestDriver@17d4 (8cec8681)] Executing command 'deleteCookies'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:57553', page '1', target 'page-7' (id: 40): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 40): '{"cookies":[]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 4ms
[debug] [XCUITestDriver@17d4 (8cec8681)] Responding to client with driver.deleteCookies() result: true
[HTTP] <-- DELETE /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie 200 7 ms - 14
[HTTP]
[HTTP] --> DELETE /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie/name
[HTTP] {}
[debug] [XCUITestDriver@17d4 (8cec8681)] Calling AppiumDriver.deleteCookie() with args: ["name","8cec8681-efd8-4b28-a207-393fcb59d679"]
[debug] [XCUITestDriver@17d4 (8cec8681)] Executing command 'deleteCookie'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:57553', page '1', target 'page-7' (id: 42): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 42): '{"cookies":[]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 31ms
[debug] [XCUITestDriver@17d4 (8cec8681)] Cookie 'name' not found. Ignoring.
[debug] [XCUITestDriver@17d4 (8cec8681)] Responding to client with driver.deleteCookie() result: true
[HTTP] <-- DELETE /session/8cec8681-efd8-4b28-a207-393fcb59d679/cookie/name 200 34 ms - 14

After setting cookies once with driver.manage.add_cookie(name: "key", value: "value")

[HTTP] --> GET /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie
[HTTP] {}
[debug] [XCUITestDriver@8956 (827a81cb)] Calling AppiumDriver.getCookies() with args: ["827a81cb-0306-4706-ad0a-ad4d1dad256d"]
[debug] [XCUITestDriver@8956 (827a81cb)] Executing command 'getCookies'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:58046', page '1', target 'page-7' (id: 40): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 40): '{"cookies":[{"name":"key","value":"value","domain":"0.0.0.0","path":"/","expires":0,"session":true,"httpOnly":false,"secure":false,"sameSite":"None"}]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 37ms
[debug] [XCUITestDriver@8956 (827a81cb)] Responding to client with driver.getCookies() result: [{"name":"key","value":"value","domain":"0.0.0.0","path":"/","expires":0,"session":true,"httpOnly":false,"secure":false,"sameSite":"None"}]
[HTTP] <-- GET /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie 200 42 ms - 149
[HTTP]
[HTTP] --> DELETE /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie
[HTTP] {}
[debug] [XCUITestDriver@8956 (827a81cb)] Calling AppiumDriver.deleteCookies() with args: ["827a81cb-0306-4706-ad0a-ad4d1dad256d"]
[debug] [XCUITestDriver@8956 (827a81cb)] Executing command 'deleteCookies'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:58046', page '1', target 'page-7' (id: 42): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 42): '{"cookies":[{"name":"key","value":"value","domain":"0.0.0.0","path":"/","expires":0,"session":true,"httpOnly":false,"secure":false,"sameSite":"None"}]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 40ms
[debug] [RemoteDebugger] Deleting cookie 'key' on 'http://0.0.0.0/'
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:58046', page '1', target 'page-7' (id: 44): 'Page.deleteCookie'
[debug] [RemoteDebugger] Received data response from send (id: 44): '{}'
[debug] [RemoteDebugger] Sending to Web Inspector took 4ms
[debug] [XCUITestDriver@8956 (827a81cb)] Responding to client with driver.deleteCookies() result: true
[HTTP] <-- DELETE /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie 200 48 ms - 14
[HTTP]
[HTTP] --> DELETE /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie/name
[HTTP] {}
[debug] [XCUITestDriver@8956 (827a81cb)] Calling AppiumDriver.deleteCookie() with args: ["name","827a81cb-0306-4706-ad0a-ad4d1dad256d"]
[debug] [XCUITestDriver@8956 (827a81cb)] Executing command 'deleteCookie'
[debug] [RemoteDebugger] Getting cookies
[debug] [RemoteDebugger] Sending '_rpc_forwardSocketData:' message to app 'PID:58046', page '1', target 'page-7' (id: 46): 'Page.getCookies'
[debug] [RemoteDebugger] Received data response from send (id: 46): '{"cookies":[]}'
[debug] [RemoteDebugger] Sending to Web Inspector took 4ms
[debug] [XCUITestDriver@8956 (827a81cb)] Cookie 'name' not found. Ignoring.
[debug] [XCUITestDriver@8956 (827a81cb)] Responding to client with driver.deleteCookie() result: true
[HTTP] <-- DELETE /session/827a81cb-0306-4706-ad0a-ad4d1dad256d/cookie/name 200 10 ms - 14

mykola-mokhnach · 2023-03-30T08:08:17Z

lib/commands/web.js

-    let script = 'return document.cookie';
-    let jsCookies = await this.executeAtom('execute_script', [script, []]);
+    // get the cookies from the remote debugger, or an empty object
+    const cookies = await this.remote.getCookies() || {cookies: []};


what remote.getCookies() returns if no cookies found?

{"cookies":[]} in my test, but not sure if it has a case that could return null for example since I haven't tested old versions.

This PR is from the previous deleted code as-is not to change anything right now. (Refactoring would be a separate PR)

mykola-mokhnach · 2023-03-30T08:09:15Z

lib/commands/web.js

-      for (let [name, value] of _.toPairs(cookieUtils.createJWPCookie(undefined, jsCookies))) {
-        cookies.push({name, value});
+    // the value is URI encoded, so decode it safely
+    const decodedCookieValues = cookies.cookies.map((cookie) => {


I assume we could destruct the object above, like const {cookies} =....

mykola-mokhnach · 2023-03-30T08:12:42Z

lib/commands/web.js

+    });
+
+    // zip cookies with decoded value, removing undefined cookie values
+    return _.zip(cookies.cookies, decodedCookieValues)


we may decode and return in a single chain. I don't see a need for the intermediate decodedCookieValues variable

mykola-mokhnach · 2023-03-30T08:13:49Z

lib/commands/web.js

-    let cookies = await this.getCookies();
-    if (_.indexOf(_.map(cookies, 'name'), cookieName) === -1) {
+    const cookies = await this.getCookies();
+    const cookie = cookies.find((cookie) => cookie.name === cookieName);


could be cookies.find(({name}) => name === cookieName)

mykola-mokhnach · 2023-03-30T08:14:26Z

lib/commands/web.js

-    if (_.indexOf(_.map(cookies, 'name'), cookieName) === -1) {
+    const cookies = await this.getCookies();
+    const cookie = cookies.find((cookie) => cookie.name === cookieName);
+    if (!cookie) {
      this.log.debug(`Cookie '${cookieName}' not found. Ignoring.`);
      return true;


should be always return true?

mykola-mokhnach · 2023-03-30T08:15:06Z

lib/commands/web.js

-      }
+    const cookies = await this.getCookies();
+    for (const cookie of cookies) {
+      await this._deleteCookie(cookie);


may we use B.all here to speed up the process?

mykola-mokhnach · 2023-03-30T08:15:33Z

lib/commands/web.js

-      }
+    const cookies = await this.getCookies();
+    for (const cookie of cookies) {
+      await this._deleteCookie(cookie);
    }
    return true;


should we always return true?

KazuCocoa · 2023-03-30T16:37:31Z

I'll do refactoring (for comments as well) in #1538 as a followup pr

boneskull · 2023-03-30T17:23:28Z

@KazuCocoa I don't understand if this PR is a verbatim revert of a previous changeset, and if so, which one?

KazuCocoa · 2023-03-30T17:33:44Z

This is for #1484 (comment). Cookies handling except for setCookies is via remote debugger's Page commands directly instead of Runtime.evaluate, ordinally, afaik. https://gist.github.com/KazuCocoa/b935fbca6efa1abf4688cc94bc14366a is the comparison in the comment.

I have checked the commit history as well, then noticed maybe something regression potentially happened in appium-ios-driver migration after Appium 1.22.3 if your debugging the mixin resolved wrongly. (I mean Runtime.evaluate was alive at that time, then, it was a regression as cookies)
XCUITest 4.19.1 was via the remote debugger's Page, so potentially unexpected regression for cookies handling existed between Appium 1.22.3 and XCUITest 4.19.1 or around. (I haven't done git bisect)

KazuCocoa · 2023-04-01T02:59:43Z

let me tweak the review in #1538 and merge this to keep the module behavior via Page command at this time

## [4.21.3](v4.21.2...v4.21.3) (2023-04-01) ### Bug Fixes * cookies regression: send Page commands for cookies handling ([#1534](#1534)) ([908ed1a](908ed1a))

github-actions · 2023-04-01T03:04:14Z

🎉 This PR is included in version 4.21.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

boneskull · 2023-04-03T21:29:42Z

@KazuCocoa The return true from deleteCookie violates the type defined in ExternalDriver. Did you need this for something? I don't see it used anywhere.

KazuCocoa · 2023-04-03T21:42:06Z

Oh, i see. should be void. I'll update it in #1538
https://github.com/appium/appium/blob/a11a6ede3320c0a58aad82fd7fc4c8b8ba75ae95/packages/types/lib/driver.ts#L1161

KazuCocoa added 2 commits March 30, 2023 04:50

chore: use remote debugger for cookies

f3fc192

fix naming

ef3eac6

KazuCocoa requested review from mykola-mokhnach and boneskull March 30, 2023 05:56

mykola-mokhnach reviewed Mar 30, 2023

View reviewed changes

KazuCocoa mentioned this pull request Mar 30, 2023

refactor: getCookies and deleteCookies #1538

Merged

KazuCocoa changed the title ~~chore: send Page commands for cookies handling~~ fix: cookies regression: send Page commands for cookies handling Mar 31, 2023

KazuCocoa mentioned this pull request Mar 31, 2023

fix: launchApp regression #1540

Merged

KazuCocoa merged commit 908ed1a into master Apr 1, 2023

KazuCocoa deleted the kazucocoa/cookies branch April 1, 2023 02:59

github-actions bot pushed a commit that referenced this pull request Apr 1, 2023

chore(release): 4.21.3 [skip ci]

126a3d6

## [4.21.3](v4.21.2...v4.21.3) (2023-04-01) ### Bug Fixes * cookies regression: send Page commands for cookies handling ([#1534](#1534)) ([908ed1a](908ed1a))

github-actions bot added the released label Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cookies regression: send Page commands for cookies handling #1534

fix: cookies regression: send Page commands for cookies handling #1534

KazuCocoa commented Mar 30, 2023 •

edited

Loading

mykola-mokhnach Mar 30, 2023

KazuCocoa Mar 30, 2023

mykola-mokhnach Mar 30, 2023

mykola-mokhnach Mar 30, 2023

mykola-mokhnach Mar 30, 2023

mykola-mokhnach Mar 30, 2023

mykola-mokhnach Mar 30, 2023

mykola-mokhnach Mar 30, 2023

KazuCocoa commented Mar 30, 2023 •

edited

Loading

boneskull commented Mar 30, 2023

KazuCocoa commented Mar 30, 2023 •

edited

Loading

KazuCocoa commented Apr 1, 2023

github-actions bot commented Apr 1, 2023

boneskull commented Apr 3, 2023

KazuCocoa commented Apr 3, 2023

fix: cookies regression: send Page commands for cookies handling #1534

fix: cookies regression: send Page commands for cookies handling #1534

Conversation

KazuCocoa commented Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KazuCocoa commented Mar 30, 2023 • edited Loading

boneskull commented Mar 30, 2023

KazuCocoa commented Mar 30, 2023 • edited Loading

KazuCocoa commented Apr 1, 2023

github-actions bot commented Apr 1, 2023

boneskull commented Apr 3, 2023

KazuCocoa commented Apr 3, 2023

KazuCocoa commented Mar 30, 2023 •

edited

Loading

KazuCocoa commented Mar 30, 2023 •

edited

Loading

KazuCocoa commented Mar 30, 2023 •

edited

Loading