Adds the ability to run bash script through appium-for-mac #30

davidmokos · 2017-11-16T14:10:27Z

we can now run bash script on the target machine running the appium-for-mac using the selenium driver.execute_script("mybashcommand")

…selenium driver.execute_script()

jsf-clabot · 2017-11-16T14:10:28Z

All committers have signed the CLA.

stuartbrussell-intuit

Thanks for adding a feature. This will be useful.

stuartbrussell-intuit · 2017-11-16T19:39:41Z

AppiumForMac/Server/Handlers/AfMHandlers.m

+- (AppiumMacHTTPJSONResponse *)post_execute:(NSString*)path data:(NSData*)postData
+{
+    return [self executeWebDriverCommandWithPath:path data:postData onMainThread:YES commandBlock:^(AfMSessionController *session, NSDictionary *commandParams, int *statusCode)
+            {


Not sure if the indentation is from github, but please ensure "return" and the block are aligned.

stuartbrussell-intuit · 2017-11-16T19:44:55Z

AppiumForMac/Server/Controller/AfMSessionController.m

+
+    NSFileHandle *file = [pipe fileHandleForReading];
+
+    [task launch];


The launch method is deprecated. Please use only supported APIs.
https://developer.apple.com/documentation/foundation/nstask/1414189-launch?language=objc

stuartbrussell-intuit · 2017-11-16T19:47:05Z

AppiumForMac/Server/Controller/AfMSessionController.m

+- (NSString *)runCommand:(NSString *)commandToRun
+{
+    NSTask *task = [[NSTask alloc] init];
+    [task setLaunchPath:@"/bin/sh"];


Are you sure 'sh' will run bash? Why not use /bin/bash to be explicit ?

stuartbrussell-intuit · 2017-11-16T19:48:59Z

AppiumForMac/Server/Controller/AfMSessionController.h

@@ -143,6 +143,7 @@ extern NSString * const kCookieDiagnosticsDirectory;
 - (BOOL)isElementDisplayed:(id)element;
 -(BOOL) clickElement:(id)element;
 -(void) closeWindow;
+- (NSString *)runCommand:(NSString *)commandToRun;


Please use a name like executeShellScript:(NSString *)script. It is more specific, and derived from the "post_execute" web driver command name.

stuartbrussell-intuit · 2017-11-16T20:00:29Z

AppiumForMac/Server/Handlers/AfMHandlers.m

+    return [self executeWebDriverCommandWithPath:path data:postData onMainThread:YES commandBlock:^(AfMSessionController *session, NSDictionary *commandParams, int *statusCode)
+            {
+                // The bash command to run
+                NSString *command = (NSString*)[commandParams objectForKey:@"script"];


Rename to 'script' or 'scriptString', or something else besides 'command'. In this context, command refers to the web driver command itself.

stuartbrussell-intuit · 2017-11-16T20:07:20Z

AppiumForMac/Server/Handlers/AfMHandlers.m

+            {
+                // The bash command to run
+                NSString *command = (NSString*)[commandParams objectForKey:@"script"];
+


If you can, please support the full "execute" command by allowing shell script arguments passed in through the 'args' commandParam. That will allow the same scriptString to have different behaviors without having to rebuild the string in your client script.

mykola-mokhnach · 2017-11-16T20:12:20Z

this might be a perfect backdoor %/

stuartbrussell-intuit · 2017-11-16T20:57:58Z

@mykola-mokhnach Yeah, AfM opens an unprotected port. In that sense AfM has been a form of back door since day one. This just improves that "feature". 😄

mykola-mokhnach · 2017-11-16T21:01:07Z

Do we want to keep it like this? I'd rather vote for closing such hole rather than extending it.

stuartbrussell-intuit · 2017-11-16T22:35:32Z

Would that mean a password-protected https port? I thought WebDriver ports were by definition open. Could be wrong. But if an open port is a bad thing, then AfM is a bad thing. AfM is unique in the sense that the driver's universe is the whole machine, rather than the inside of a web view. Does AfM therefore not meet a particular security standard? I'm all for making it more secure if we can, but a faulty password scheme is almost worse than none, because it makes people think it is "safe".

Curious: does Appium proper deal with this? Does it give you access to the whole device, or just one app at a time?

stuartbrussell-intuit · 2017-11-16T22:40:13Z

Another possibility is to set a user default on the AfM machine, so that only a specific set of IP addresses can connect.

mykola-mokhnach · 2017-11-17T07:15:18Z

@stuartbrussell-intuit In all the other driver we do limit the access to the command line, so the user cannot execute arbitrary shell commands on the target machine. Am I right @jlipps ?

davidmokos · 2017-11-17T16:34:09Z

I committed some changes, but I need to dig into the API to find a replacement for the NSTask launch method, which is deprecated but there is apparently no substitution.

About the security concern - what blocks me from opening the Terminal app and pass a command into it by driving the UI?

jlipps · 2017-12-05T21:45:37Z

Yeah I think it's less about secure access to AfM and more about what users can do once they have a session. With other drivers we try to limit what can be done via the driver to something other than general-purpose computing on the host machine :-)

The question I usually ask when people want this kind of feature is what their use case is. Often we can then simply build a feature specifically for that use case, and not blow open a huge security hole.

stuartbrussell-intuit · 2017-12-05T23:36:38Z

It's fine if we reject this change, but... this goes to the heart of AfM. Because of the way it is implemented, AfM has access to the entire UI of the machine, not just a specific app. So in its current form, it could easily launch Terminal and type into the window using standard webdriver commands. That's actually valuable because one script can control multi-app work flows, but with great power comes great... security holes? 😄 Seriously though, how should we treat AfM knowing how powerful it is? (btw, was this a surprise to people? I was explicit in my SelConf 2017 presentation that AfM is a fundamentally different beast from (e.g.) ChromeDriver, but not everyone saw it.)

jlipps · 2017-12-05T23:53:57Z

I guess if it's well-known that AfM can control the entire computer anyway then this doesn't add any security hole! So I'm fine with it in that case.

stuartbrussell-intuit · 2017-12-06T00:01:33Z

👍 However, I do think it would be prudent to add something that disallows access from any other machine, for people who want to restrict who can control the computer. (e.g. a white list).

#32

stuartbrussell-intuit · 2017-12-06T00:08:19Z

@davidmokos I'm totally swamped, give me a couple of days and then I can review your latest. If the code is good, I'll approve it.

mykola-mokhnach · 2017-12-14T13:01:57Z

I think we could use this feature together with the flag from appium/appium#9790, which should be implemented first on osx driver side

stuartbrussell-intuit · 2017-12-14T19:43:28Z

@mykola-mokhnach Just to clarify for my confused brain: The fix for #9790 would mean, before "unsafe" actions, the user has to set a preference on the server side, or launch the server from the command line with an arg? So by default it would not be allowed? Just making sure I understand.

mykola-mokhnach · 2017-12-14T20:08:25Z

@stuartbrussell-intuit Yes, your understanding is correct. The idea is that remote clients don't have access to the remote server, so they cannot change command line arguments there, but it should not be a problem if one runs Appium in a local environment. Check, for example, appium/appium-android-driver#293

mykola-mokhnach · 2018-02-09T20:15:01Z

AppiumForMac/Server/Handlers/AfMHandlers.m

@@ -240,6 +240,17 @@ - (AppiumMacHTTPJSONResponse *)post_url:(NSString*)path data:(NSData*)postData
 // POST /session/:sessionId/execute
 // Inject a snippet of JavaScript into the page for execution in the context of the currently selected frame. The executed script is assumed to be synchronous and the result of evaluating the script is returned to the client.

+- (AppiumMacHTTPJSONResponse *)post_execute:(NSString*)path data:(NSData*)postData


it should be also possible to pass additional environment variables

mykola-mokhnach · 2018-02-09T20:16:26Z

AppiumForMac/Server/Controller/AfMSessionController.m

@@ -574,6 +574,32 @@ -(void) closeWindow
    [self.currentWindow performAction:@"AXCancel"];
 }

+- (NSString *)executeShellScript:(NSString *)script
+{
+    NSTask *task = [[NSTask alloc] init];


I assume NSTask starts with an empty environment by default. Shouldn't we inherit the current system env by default?

Jiurong-Yang · 2019-05-29T01:29:00Z

Is anyone working on this? Just to clarify the CRs for this pull request are:
1). Replace the NSTask module with something that is not deprecated. I assume a popen module should suffice?
2). Comply to the execute mobile command API structure
http://appium.io/docs/en/commands/mobile-command/
3). Comply with the --relaxed-security server flag.

Not sure what is the current state of this project. I'd like some feedback if possible. Thank you
@mykola-mokhnach
@stuartbrussell-intuit

mykola-mokhnach · 2019-05-29T05:37:50Z

Hi @Jiurong-Yang . Thanks for your feedback
Unfortunately we don't have enough time and resources to support this driver properly, as we are mostly focused on mobile stuff. We'd be happy if there were more contributors to it.

mykola-mokhnach · 2019-12-19T18:10:11Z

Superseded by appium/appium-mac-driver#38

Adds the ability to run bash script through appium-for-mac using the …

6adbfea

…selenium driver.execute_script()

stuartbrussell-intuit suggested changes Nov 16, 2017

View reviewed changes

Improvements on the execute bash feature

262433b

mykola-mokhnach reviewed Feb 9, 2018

View reviewed changes

Jiurong-Yang mentioned this pull request Jun 6, 2019

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

Closed

mykola-mokhnach closed this Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the ability to run bash script through appium-for-mac #30

Adds the ability to run bash script through appium-for-mac #30

davidmokos commented Nov 16, 2017

jsf-clabot commented Nov 16, 2017 •

edited

Loading

stuartbrussell-intuit left a comment

stuartbrussell-intuit Nov 16, 2017

stuartbrussell-intuit Nov 16, 2017

stuartbrussell-intuit Nov 16, 2017

stuartbrussell-intuit Nov 16, 2017

stuartbrussell-intuit Nov 16, 2017

stuartbrussell-intuit Nov 16, 2017

mykola-mokhnach commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

mykola-mokhnach commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

mykola-mokhnach commented Nov 17, 2017

davidmokos commented Nov 17, 2017 •

edited

Loading

jlipps commented Dec 5, 2017

stuartbrussell-intuit commented Dec 5, 2017 •

edited

Loading

jlipps commented Dec 5, 2017

stuartbrussell-intuit commented Dec 6, 2017

stuartbrussell-intuit commented Dec 6, 2017

mykola-mokhnach commented Dec 14, 2017

stuartbrussell-intuit commented Dec 14, 2017

mykola-mokhnach commented Dec 14, 2017

mykola-mokhnach Feb 9, 2018

mykola-mokhnach Feb 9, 2018

Jiurong-Yang commented May 29, 2019

mykola-mokhnach commented May 29, 2019

mykola-mokhnach commented Dec 19, 2019


		NSFileHandle *file = [pipe fileHandleForReading];

		[task launch];

Adds the ability to run bash script through appium-for-mac #30

Adds the ability to run bash script through appium-for-mac #30

Conversation

davidmokos commented Nov 16, 2017

jsf-clabot commented Nov 16, 2017 • edited Loading

stuartbrussell-intuit left a 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

mykola-mokhnach commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

mykola-mokhnach commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

stuartbrussell-intuit commented Nov 16, 2017

mykola-mokhnach commented Nov 17, 2017

davidmokos commented Nov 17, 2017 • edited Loading

jlipps commented Dec 5, 2017

stuartbrussell-intuit commented Dec 5, 2017 • edited Loading

jlipps commented Dec 5, 2017

stuartbrussell-intuit commented Dec 6, 2017

stuartbrussell-intuit commented Dec 6, 2017

mykola-mokhnach commented Dec 14, 2017

stuartbrussell-intuit commented Dec 14, 2017

mykola-mokhnach commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiurong-Yang commented May 29, 2019

mykola-mokhnach commented May 29, 2019

mykola-mokhnach commented Dec 19, 2019

jsf-clabot commented Nov 16, 2017 •

edited

Loading

davidmokos commented Nov 17, 2017 •

edited

Loading

stuartbrussell-intuit commented Dec 5, 2017 •

edited

Loading