1338327 - Native messaging hosts fail for .cmd files with spaces in path

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[[e-mail]]

This is somewhat related maybe to #1321637, but that is about space in file names for application manifests in the registry

I am porting a Chrome extension to firefox and have run into the following issues. My current development environment is : "Firefox 51.0.1 (32-bit)"

According to MDN (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging), the manifest for an external application requires a "path" key. On Windows, the path may be a full path or may be relative to the location of the manifest.

If I specify with out any path, the firefox will try to open on the root of the drive.
Assume the manifest is defined in "C:\Program Files\My Org\My App\app.json"
Then if "path" : "app.bat", 
I expect "C:\Program Files\My Org\My App\app.bat" to be started
Else if "path" : ".\\app.bat",
I expect the same

In reality (using Sys Internals procmon) I see a failure to find "C:\app.bat" for both use cases .

If instead I specify "path": "C:\\Program Files\\My Org\\My App\\app.bat", then the javascript runtime reports a error accessing "C:\Program".

The workaround is to specify the app as an 8.3 name:
"path": " "C:\\PROGRA~1\\MYORG~1\\MYAPP~1\\app.bat",

This works but is not really satisfactory

Comment 2

[[e-mail]]

I'm not sure how to reopen this, but I'm not seeing a fix
Environment:
  Windows 8.1/32
  Firefox 52.0 32-bit (52.0.0.6270)
(please excuse the 'Chrome' names; this is an extension being ported)


Application Manifest is in the directory: "C:\Program Files\OpenText\Office Editor"

If the path to the external host is:
1) "ChromeActionHandler.cmd" or
2) "C:\\Program Files\\OpenText\\Office Editor\\ChromeActionHandler.cmd" 
THEN the script is found (according to Sysinternals Procmon.exe" but is never executed;
AND the console log contains the entries:
    sendMessageToNativeApp  background.js:240:5
    stderr output from native app com.opentext.officeeditor.actionhandler: 'C:\Program' is not recognized as an internal or external command,
    stderr output from native app com.opentext.officeeditor.actionhandler: operable program or batch file.
    onNativeMessage: msg=(null);  background.js:300:5

if the path to the external host is:
3) "C:\\PROGRA~1\\OpenText\\OFFICE~1\\ChromeActionHandler.cmd"
THEN, the script is found and and launched; the extensions works as expected.

To me, this is clear evidence that 
- relative names are not working, if they resolve to a full path with embedded spaces
- absolute names do not work if they contain spaces.

This seems to be exactly the case reported in #1321637, but its apparently still not working correctly

Please advise

Comment 3

[[e-mail]]

Or perhaps the fix did not make it into #52

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1338327 Handle .cmd files with spaces in subprocess

Review commit: https://reviewboard.mozilla.org/r/148760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148760/

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8877372 [details]
Bug 1338327 Handle .cmd files with spaces in subprocess

https://reviewboard.mozilla.org/r/148760/#review154160

::: toolkit/components/extensions/test/xpcshell/head_native_messaging.js:95
(Diff revision 1)
>        do_register_cleanup(() => {
>          registry.shutdown();
>        });
>  
>        for (let script of scripts) {
> +        let scriptExtension = script.hasOwnProperty("scriptExtension") ?

Nit: Please don't use `hasOwnProperty` as a method. `script.scriptExtension || "bat"` should be fine here. Or:

    let {scriptExtension = "bat"} = script;

::: toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js:168
(Diff revision 1)
> -      });
> +    });
> -      port.postMessage(MSG);
> +    port.postMessage(MSG);
> -    }
> +  }
>  
> -    let extension = ExtensionTestUtils.loadExtension({
> +  let extension = ExtensionTestUtils.loadExtension({
> -      background,
> +    background: `(${background})(\"${app}\");`,

Nit: Backslashes are not necessary. But I'd rather have something like `${JSON.stringify(app)}` on account of being paranoid.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8877372 [details]
Bug 1338327 Handle .cmd files with spaces in subprocess

https://reviewboard.mozilla.org/r/148760/#review154160

> Nit: Please don't use `hasOwnProperty` as a method. `script.scriptExtension || "bat"` should be fine here. Or:
> 
>     let {scriptExtension = "bat"} = script;

I believe either of those will generate a strict mode warning?

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8877372 [details]
Bug 1338327 Handle .cmd files with spaces in subprocess

https://reviewboard.mozilla.org/r/148760/#review154160

> I believe either of those will generate a strict mode warning?

The `||` version won't. The destructuring version might. I don't know if that bug has been fixed yet.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8877372 [details]
Bug 1338327 Handle .cmd files with spaces in subprocess

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148760/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44aab3a73cbf
Handle .cmd files with spaces in subprocess r=kmag

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/44aab3a73cbf