move file movement commands by jlipps · Pull Request #605 · appium/appium-xcuitest-driver

jlipps · 2018-01-16T23:46:22Z

before, the ios commands were overriding pushFile and pullFile
these commands really belong in file-movement anyway
also, fix up the incorrect use of tempDir
also, fix up the fact that some clients send in weird byte arrays instead of strings, silly java

so much stuff was broken in here! @mykola-mokhnach we must not have tests for this, because pullFile was getting executed by the ios-driver code 🤕

jlipps · 2018-01-16T23:46:45Z

+ * @param {string} base64Data - Base-64 encoded content of the file to be uploaded.
+ */
+async function pushFileToSimulator (device, remotePath, base64Data) {
+  if (_.isArray(base64Data)) {


hard to tell what changed in the copy, this is where some of the new code is

mykola-mokhnach · 2018-01-17T06:47:30Z

+  if (_.isArray(base64Data)) {
+    // some clients (ahem) java, send a byte array encoding utf8 characters
+    // instead of a string, which would be infinitely better!
+    base64Data = Buffer.from(base64Data).toString('utf8');


I asume the encoding is binary/ascii here

according to the code used in the java client, the client sends a byte stream of bytes, each byte is a utf8 char, which is part of a base64 string. so to get the b64-encoded string we need to decode utf8 bytes. silly java client!

mykola-mokhnach · 2018-01-17T06:48:14Z

+    // instead of a string, which would be infinitely better!
+    base64Data = Buffer.from(base64Data).toString('utf8');
+  }
+  const buf = Buffer.from(base64Data, 'base64');


is Buffer.from supposed to work before Node8? Or Babel takes care of this?

yeah, it's supported from node 5

mykola-mokhnach · 2018-01-17T06:50:18Z

+ * @param {string} base64Data - Base-64 encoded content of the file to be uploaded.
+ */
+async function pushFileToRealDevice (device, remotePath, base64Data) {
+  await verifyIFusePresence();


should we do the same base64 transformation here? perhaps, the transformation itself might be moved to appium-support, since it might be useful for Android as well?

ah, yes we should. i mean really what we should do is fix the client, because it should be sending a string. i just needed something i could fix faster than the java client :-)

I'll take care of it. But there are still many older versions in the wild

usually people update server much more frequently than the client

mykola-mokhnach · 2018-01-17T07:07:52Z

Thanks @jlipps

imurchie · 2018-01-17T14:38:24Z

Looks good, though the file movement tests fail.

For the other test, see #606

mykola-mokhnach · 2018-01-23T08:17:31Z

@jlipps Do you have time to finish this PR?

jlipps · 2018-01-23T22:04:56Z

@mykola-mokhnach yeah i'll take a look at the failed tests

before, the ios commands were overriding pushFile and pullFile these commands really belong in file-movement anyway also, fix up the incorrect use of tempDir also, fix up the fact that some clients send in weird byte arrays instead of strings, silly java

jlipps · 2018-01-23T23:28:23Z

ok, fixes pushed. a couple things to note:

there are some screen recording unit tests failing, seems unrelated
this is the first time this package will export the pushFile and pullFile commands that are now being tested. they have a different API than ios-driver's, so this should be technically considered a breaking change
pullFolder has not been rewritten according to the new style, so it retains ios-driver's API, which is probably confusing that it would work differently. @mykola-mokhnach want to add pullFolder support on this branch?

mykola-mokhnach · 2018-01-24T06:52:44Z

pullFolder has not been rewritten according to the new style, so it retains ios-driver's API, which is probably confusing that it would work differently. @mykola-mokhnach want to add pullFolder support on this branch?

Maybe later when I have time. I don't see this API endpoint is used often.

mykola-mokhnach · 2018-01-24T08:34:09Z

+        log.debug(`The destination folder '${path.dirname(dstPath)}' does not exist. Creating...`);
+        await fs.mkdir(path.dirname(dstPath));
+      }
+      await fs.writeFile(dstPath, new Buffer(base64Data, 'base64').toString('binary'), 'binary');


so how about adding base64-data patch here?

perhaps, it makes sense to fix it on higher level, inside pushFile call, so then both methods (for a real device and Simulator) will get the fixed data

mykola-mokhnach · 2018-01-24T18:49:59Z

        it('should be able to fetch the Address book', async function () {
-          let file = 'Library/AddressBook/AddressBook.sqlitedb';
-          let stringData = await pullFileAsString(file);
+          let file = '../../../../../Library/AddressBook/AddressBook.sqlitedb';


@jlipps I think you've just found a nice security hole there, which allows to read/write any file on the server. The push/pull methods should contain a security verification, which confirms that absolute path is a subpath of the corresponding container.

* move file movement commands before, the ios commands were overriding pushFile and pullFile these commands really belong in file-movement anyway also, fix up the incorrect use of tempDir also, fix up the fact that some clients send in weird byte arrays instead of strings, silly java * clean up debug code * fix up file movement tests * Add path verification * Use mkdirp instead of mkdir

jlipps commented Jan 16, 2018

View reviewed changes

mykola-mokhnach reviewed Jan 17, 2018

View reviewed changes

mykola-mokhnach mentioned this pull request Jan 17, 2018

Fix the second argument type for pushFile appium/java-client#812

Merged

4 tasks

mykola-mokhnach mentioned this pull request Jan 18, 2018

Add client issue workaround for pushFile endpoint appium/appium-android-driver#312

Merged

jlipps added 3 commits January 23, 2018 14:05

move file movement commands

2b7b9da

before, the ios commands were overriding pushFile and pullFile these commands really belong in file-movement anyway also, fix up the incorrect use of tempDir also, fix up the fact that some clients send in weird byte arrays instead of strings, silly java

clean up debug code

8e55d5d

fix up file movement tests

2181b1d

jlipps force-pushed the jlipps-pushFile-fix branch from ddf6112 to 2181b1d Compare January 23, 2018 23:27

mykola-mokhnach reviewed Jan 24, 2018

View reviewed changes

mykola-mokhnach closed this Feb 6, 2018

imurchie deleted the jlipps-pushFile-fix branch July 23, 2018 13:15

Uh oh!

Conversation

jlipps commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach commented Jan 17, 2018

Uh oh!

imurchie commented Jan 17, 2018

Uh oh!

mykola-mokhnach commented Jan 23, 2018

Uh oh!

jlipps commented Jan 23, 2018

Uh oh!

jlipps commented Jan 23, 2018

Uh oh!

mykola-mokhnach commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mykola-mokhnach Jan 24, 2018 •

edited

Loading