Allow AfM driver to execute shell scripts through the Execute script API

[Jiurong-Yang]

Init commit. Functionally working. Replacing pull request https://github.com/appium/appium-for-mac/pull/30

[Jiurong-Yang]

@mykola-mokhnach @KazuCocoa. Let me know what I need to change to comply with the standard. And also if this C popen module is a suitable replacement for the deprecated NSTask library.

[mykola-mokhnach]

popen only returns stdout, but people might also need stderr and the exit code: https://stackoverflow.com/questions/280571/how-to-control-popen-stdin-stdout-stderr-redirection, so I would rather return the following map from execute:

{
  stdout
  stderr
  code
}

[jlipps]

Seems like this could be a security hole, no? If so we should put it behind --relaxed-security (and I'm about to raise a PR that adds --allow-insecure=foo,bar so we can give each insecure feature a name and whitelist it)

[Jiurong-Yang]

@mykola-mokhnach. Fair point. I'll change the return and use NSTask with launchAndReturnError as @KazuCocoa suggested. As for @jlipps I think this was already discussed in the earlier pull request https://github.com/appium/appium-for-mac/pull/30. AfM it self is pretty much a giant security hole since you can launch a terminal and run anything you have the privilege to run. We can put the execute_script functionality behind --relaxed-security but it would be a false sense of security. Regardless if you guys want it I'll add it. Will you be replacing the --relaxed-security with --allow-insecure or would the --relaxed-security server flag suffice?

[mykola-mokhnach]

I don't force you to use NSTask, - you may use whenever API you think would be appropriate to solve the task. I'm only concerned about what the resulting data should contain, since I already have some experience implementing similar features

[Jiurong-Yang]

Right but KazuCocoa brought up the alternative with NSTask launchAndReturnError method and I'm just going to do both at the same time.

[Jiurong-Yang]

@KazuCocoa Apparently launchAndReturnError is only available in Mac OS 10.13 or newer. Hope that's alright. I'm not sure what is the expected backward compatibility is for AfM.

[jlipps]

@Jiurong-Yang ah I missed that earlier conversation. Yeah, that makes sense. It makes me want to move to Appium 2.0 more quickly, since it means that if anyone is running an exposed Appium server and happens to have AfM installed, then they are vulnerable to remote code execution. But seems OK to just regard all of AfM as a big security risk.

[Jiurong-Yang]

Reworked it to meet all the demands. Tested, functional.

[KazuCocoa]

thanks! > https://github.com/appium/appium-for-mac/pull/64#issuecomment-500592183

I have not enough experience for this feature, which OS we should support. It's fine to go as the current implementation. thanks for your confirmation!

[mykola-mokhnach]

The implementation looks fine to me, although I would rather wait until the corresponding relaxedSecurity flag is added to the nodejs driver, so it is safe to merge

[Jiurong-Yang]

@mykola-mokhnach are you refering to the appium-mac-driver? Is someone working on this?

[KazuCocoa]

Merged the driver PR

[Jiurong-Yang]

Dose this mean this pull request can be merged?

[KazuCocoa]

Looks yes. Will merge this later soon.

[mykola-mokhnach]

@KazuCocoa I still didn't see the PR in the mac driver, which would allow to invoke this endpoint under allowable feature name

[KazuCocoa]

How about https://github.com/appium/appium-mac-driver/pull/31/files? execute can run when relaxed security is enabled. Ah, or the new isFeatureEnabled?

[mykola-mokhnach]

Superseded by https://github.com/appium/appium-mac-driver/pull/38