New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid package error when there is asar path in the command passed to child_process.exec #5571

Closed
jviotti opened this Issue May 17, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@jviotti
Member

jviotti commented May 17, 2016

I'm hitting this strange issue when using the ELECTRON_RUN_AS_NODE=1 feature to run a script inside an asar, which re-runs itself with child_process.exec() as part of an elevation routine provided by sudo-prompt.

I've created a small snippet that reproduces the issue here. I tried creating a Gist, but it doesn't allow me to upload asar archives.

Installing the npm dependencies and running npm start outputs the following:

$ npm start

> electron-elevate-asar@1.0.0 start /Users/jviotti/Projects/playground/electron-elevate-asar
> ELECTRON_RUN_AS_NODE=1 electron app.asar/index.js

Elevating
Executing: /Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/electron-prebuilt/dist/Electron.app/Contents/MacOS/Electron /Users/jviotti/Projects/playground/electron-elevate-asar/app.asar/index.js
STDOUT: undefined
STDERR: undefined
/Users/jviotti/Projects/playground/electron-elevate-asar/app.asar/index.js:28
        throw error;
        ^

Error: Invalid package /usr/bin/sudo -n -E /Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/electron-prebuilt/dist/Electron.app/Contents/MacOS/Electron /Users/jviotti/Projects/playground/electron-elevate-asar/app.asar
    at invalidArchiveError (ELECTRON_ASAR.js:137:13)
    at Object.module.(anonymous function) [as execFile] (ELECTRON_ASAR.js:192:16)
    at Object.exports.exec (child_process.js:111:18)
    at Attempt (/Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/sudo-prompt/index.js:19:14)
    at Object.Exec (/Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/sudo-prompt/index.js:105:3)
    at Immediate.<anonymous> (/Users/jviotti/Projects/playground/electron-elevate-asar/app.asar/index.js:21:23)
    at Immediate.immediate._onImmediate (timers.js:590:18)
    at tryOnImmediate (timers.js:543:15)
    at processImmediate [as _immediateCallback] (timers.js:523:5)

sudo-prompt executes the following command: sudo -n -E /Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/electron-prebuilt/dist/Electron.app/Contents/MacOS/Electron /Users/jviotti/Projects/playground/electron-elevate-asar/app.asar/index.js, which as you can see above, seems to originate the issue, however copy-pasting this command into my terminal succeeds without any problem.

Basically, the elevation fails only then the command is executed from an electron process with ELECTRON_RUN_AS_NODE enabled.

The asar archive is of course valid, since the original script wouldn't run from it otherwise.

Some observations:

  • The code runs fine when the script is not packaged inside an asar.
  • The code works fine if we create another JavaScript file that executes sudo -n -E electron app.asar/index.js and we run it with NodeJS. Calling the same new file with ELECTRON_RUN_AS_NODE=1 electron exhibits the issue.

  • Electron version: Ran in v0.37.6, v0.37.8 and v1.1.0.
  • Operating system: OS X 10.11.4 and Ubuntu 14.04 x64.

@jviotti jviotti changed the title from Invalid package when executing a command from an electron process with ELECTRON_RUN_AS_NODE to "Invalid package" when executing a command from an electron process with ELECTRON_RUN_AS_NODE May 17, 2016

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 18, 2016

Error: Invalid package /usr/bin/sudo -n -E /Users/jviotti/Projects/playground/electron-elevate-asar/node_modules/electron-prebuilt/dist/Electron.app/Contents/MacOS/Electron /Users/jviotti/Projects/playground/electron-elevate-asar/app.asar

From the error information the whole command /usr/bin/sudo -n -E ... is passed to child_process.execFile, instead of just the file. After looking into the implementation of child_process.exec, I found it just passes the whole command as file path to child_process.execFile.

Before this is fixed you have to modify the module to use execFile directly instead of exec to work around it.

@zcbenz zcbenz changed the title from "Invalid package" when executing a command from an electron process with ELECTRON_RUN_AS_NODE to Invalid package error when there is asar path in the command passed to child_process.exec May 18, 2016

@jviotti

This comment has been minimized.

Member

jviotti commented May 18, 2016

Hi @zcbenz ,

Thanks for your input. Using execFile indeed works correctly, and I can see how node passes the whole command to execFile in child_process.js.

By looking at Electron's child_process.execFile API override, I see that the whole command is detected to be an asar by the splitPath() function, causing getOrCreateArchive() to be called on the whole command.

What do you think could be a real solution to this issue? Happy to work on it myself if you want. I can think of the following options right now:

  1. Override child_process.exec in lib/common/asar.js to split the command by white spaces, and call execFile by providing the first argument as the executable, although simply splitting the command might cause issues with paths including spaces (we could cover escaped white spaces though), and modifying the default child_process.exec behaviour might break existing code.
  2. Add a check to splitPath() to perform the splitting as mentioned before, and potentially return another parameter in the resulting array containing the array of arguments, omitting the executable. This means that overrideAPI should be changed to handle the new array of arguments accordingly.
  3. Add a check to splitPath to return isAsar == false if the command contains .asar, but the file path is part of a larger command, so the command is executable normally, without the asar layer.

The solution number 3 sounds like the most elegant one IMHO.

@jviotti

This comment has been minimized.

Member

jviotti commented May 18, 2016

Another potential workaround is to set process.noAsar = true right before the child_process.exec in sudo-prompt, and unset it afterwards.

jviotti pushed a commit to jviotti/sudo-prompt that referenced this issue May 18, 2016

Juan Cruz Viotti
Set `process.noAsar` when calling child_process.exec in Attempt()
I've encountered a very strange issue when executing a command
containing the path to an `asar` archive in Electron.

Turns out NodeJS implements `child_process.exec` by simply passing the
whole command to `child_process.execFile`. See:

- https://github.com/nodejs/node/blob/master/lib/child_process.js#L90
- https://github.com/nodejs/node/blob/master/lib/child_process.js#L99

Electron patches `child_process.execFile` to add support for `asar`
archives by injecting logic that extracts the required files from the
`asar` to a temporary location before delegating the work to the
original `child_process.execFile`.

In order to decide whether to inject the custom `asar` extracting logic,
Electron makes use of a helper function called `splitPath()`. See:

- https://github.com/electron/electron/blob/master/lib/common/asar.js#L37

If the first argument of the returned array equals `true`, means that
the path is considered to be an `asar` archive, and thus the extraction
logic takes place. The problem is that if the command passed to
`child_process.execFile` *contains* a path to an asar archive, padded
with other commands/arguments, `splitPath()` will consider it to be an
`asar` archive, and will try to extract it, throwing a rightfully
`Invalid package` error.

For example, let's say you pass the following command to
`sudoPrompt.exec`:

```sh
electron app.asar/index.js
```

The `Attempt()` function will then call `child_process.exec` with:

```sh
/usr/bin/sudo -n electron app.asar/index.js
```

Since the string `.asar` is present in the command, `splitPath()` will
consider `/usr/bin/sudo -n electron app.asar/index.js` to be a path to
an `asar` archive, and will fail with the above mentioned error.

A good enough workaround to this issue, at least until its correctly
fixed in Electron, its to manually disable `asar` support by setting
`process.noAsar` right before calling `child_process.exec`, and
reverting its value to its original one afterwards.

For more information about this issue, please take a look at the
following Electron issue: electron/electron#5571

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>
@jviotti

This comment has been minimized.

Member

jviotti commented May 18, 2016

@zcbenz I'm working on the patch by following the third mentioned approach, but identifying the parts of the command is not something we could accurately do, since splitting by white spaces doesn't consider cases like paths containing spaces.

Some thoughts:

  • We can split by whitespace and document a warning that commands containing white spaces lose asar support by default, which is of course not nice, and might break existing code.
  • Split by absolute paths. We can split the command using a RegExp to only consider absolute paths as separate parts of the command. For example echo /path/to/my/asar with spaces.asar would be split into [ echo, /path/to/my/asar with spaces.asar ] but echo my/asar with spaces.asar would not be split at all. Finding a RegExp that covers all cases, Windows included could be a bit tricky to get right.
  • We could be smarter in the way we detect something before or after the asar path. For example, if there is a space after .asar, which can very safely conclude there is something else afterwards. We could also check for an indicator of the beginning of an absolute or relative path before the .asar (for example: ./, /, <letter>:\, etc), and check if its index is 0 or -1. If that fails, we can assume there is something before.

What do you think?

I'm wondering if a better solution would be to accept a custom child_process.exec option to manually disable the asar support (e.g: electronNoAsar: true, or electronDisableOverride: true) and document it, since trying to make clever assumptions on the command mostly based on heuristics doesn't feel right, there are too many edge cases and ambiguities.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 19, 2016

I think we can probably patch child_process.exec so it calls Node's original execFile instead of Electron's node.

jviotti pushed a commit to jviotti/electron that referenced this issue May 19, 2016

Juan Cruz Viotti
Override `child_process.execFile` asar patch for `child_process.exec`
NodeJS implements `child_process.exec` by simply passing the whole
command to `child_process.execFile`. See:

- https://github.com/nodejs/node/blob/master/lib/child_process.js#L90
- https://github.com/nodejs/node/blob/master/lib/child_process.js#L99

Electron patches `child_process.execFile` to add support for `asar`
archives by injecting logic that extracts the required files from the
`asar` to a temporary location before delegating the work to the
original `child_process.execFile`.

In order to decide whether to inject the custom `asar` extracting logic,
Electron makes use of a helper function called `splitPath()`. See:

- https://github.com/electron/electron/blob/master/lib/common/asar.js#L37

If the first argument of the returned array equals `true`, means that
the path is considered to be an `asar` archive, and thus the extraction
logic takes place. The problem is that if the command passed to
`child_process.execFile` *contains* a path to an asar archive, padded
with other commands/arguments, `splitPath()` will consider it to be an
`asar` archive, and will try to extract it, throwing a rightfully
`Invalid package` error.

Fixes: electron#5571
Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

jviotti pushed a commit to jviotti/electron that referenced this issue May 19, 2016

Juan Cruz Viotti
Override `child_process.execFile` asar patch for `child_process.exec`
NodeJS implements `child_process.exec` by simply passing the whole
command to `child_process.execFile`. See:

- https://github.com/nodejs/node/blob/master/lib/child_process.js#L90
- https://github.com/nodejs/node/blob/master/lib/child_process.js#L99

Electron patches `child_process.execFile` to add support for `asar`
archives by injecting logic that extracts the required files from the
`asar` to a temporary location before delegating the work to the
original `child_process.execFile`.

In order to decide whether to inject the custom `asar` extracting logic,
Electron makes use of a helper function called `splitPath()`. See:

- https://github.com/electron/electron/blob/master/lib/common/asar.js#L37

If the first argument of the returned array equals `true`, means that
the path is considered to be an `asar` archive, and thus the extraction
logic takes place. The problem is that if the command passed to
`child_process.execFile` *contains* a path to an asar archive, padded
with other commands/arguments, `splitPath()` will consider it to be an
`asar` archive, and will try to extract it, throwing a rightfully
`Invalid package` error.

Fixes: electron#5571
Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

jviotti pushed a commit to jviotti/electron that referenced this issue May 19, 2016

Juan Cruz Viotti
Override `child_process.execFile` asar patch for `child_process.exec`
NodeJS implements `child_process.exec` by simply passing the whole
command to `child_process.execFile`. See:

- https://github.com/nodejs/node/blob/master/lib/child_process.js#L90
- https://github.com/nodejs/node/blob/master/lib/child_process.js#L99

Electron patches `child_process.execFile` to add support for `asar`
archives by injecting logic that extracts the required files from the
`asar` to a temporary location before delegating the work to the
original `child_process.execFile`.

In order to decide whether to inject the custom `asar` extracting logic,
Electron makes use of a helper function called `splitPath()`. See:

- https://github.com/electron/electron/blob/master/lib/common/asar.js#L37

If the first argument of the returned array equals `true`, means that
the path is considered to be an `asar` archive, and thus the extraction
logic takes place. The problem is that if the command passed to
`child_process.execFile` *contains* a path to an asar archive, padded
with other commands/arguments, `splitPath()` will consider it to be an
`asar` archive, and will try to extract it, throwing a rightfully
`Invalid package` error.

Fixes: electron#5571
Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

@zcbenz zcbenz closed this in #5611 May 20, 2016

davemackintosh added a commit to Floato/electron that referenced this issue May 31, 2016

Override `child_process.execFile` asar patch for `child_process.exec`
NodeJS implements `child_process.exec` by simply passing the whole
command to `child_process.execFile`. See:

- https://github.com/nodejs/node/blob/master/lib/child_process.js#L90
- https://github.com/nodejs/node/blob/master/lib/child_process.js#L99

Electron patches `child_process.execFile` to add support for `asar`
archives by injecting logic that extracts the required files from the
`asar` to a temporary location before delegating the work to the
original `child_process.execFile`.

In order to decide whether to inject the custom `asar` extracting logic,
Electron makes use of a helper function called `splitPath()`. See:

- https://github.com/electron/electron/blob/master/lib/common/asar.js#L37

If the first argument of the returned array equals `true`, means that
the path is considered to be an `asar` archive, and thus the extraction
logic takes place. The problem is that if the command passed to
`child_process.execFile` *contains* a path to an asar archive, padded
with other commands/arguments, `splitPath()` will consider it to be an
`asar` archive, and will try to extract it, throwing a rightfully
`Invalid package` error.

Fixes: electron#5571
Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment