Skip to content
This repository has been archived by the owner on Sep 23, 2021. It is now read-only.

Issues on windows when the username contains & #97

Open
zvin opened this issue Jun 3, 2019 · 13 comments
Open

Issues on windows when the username contains & #97

zvin opened this issue Jun 3, 2019 · 13 comments

Comments

@zvin
Copy link

zvin commented Jun 3, 2019

If the username contains a & (like Alice & Bob's Laptop), sudo-prompt fails on windows.
Most probably because the temporary file is in the user's folder and &s are not escaped.

@jorangreef
Copy link
Owner

Thanks @zvin , can you please provide any sample error messages? That will help me narrow this down.

@zvin
Copy link
Author

zvin commented Jun 4, 2019

It errors here https://github.com/jorangreef/sudo-prompt/blob/master/index.js#L589
test file:

const sp = require('sudo-prompt');

const options = { name: 'test' };

sp.exec('echo ok', options, (error, stdout, stderr) => {
        console.log({ error, stdout, stderr });
});

The elevate script looks like this and fails when I run it with cmd /c ...:

@echo off
call "C:\Users\KRS&KR~1\AppData\Local\Temp\9f9c5cce3172619ac463295721953625\command.bat" > "C:\Users\KRS&KR~1\AppData\Local\Temp\9f9c5cce3172619ac463295721953625\stdout" 2> "C:\Users\KRS&KR~1\AppData\Local\Temp\9f9c5cce3172619ac463295721953625\stderr"
(echo %ERRORLEVEL%) > "C:\Users\KRS&KR~1\AppData\Local\Temp\9f9c5cce3172619ac463295721953625\status"

@jorangreef
Copy link
Owner

Thanks @zvin, I could not reproduce any error.

To make sure it's not a 8.3 short name issue, I created a directory called k&ritchie, and referenced it using its 8.3 short name of K&RITC~1.

I then created elevate.bat as follows:

@echo off
call "C:\Users\Joran\sudo-prompt\K&RITC~1\command.bat" > "C:\Users\Joran\sudo-prompt\K&RITC~1\stdout" 2> "C:\Users\Joran\sudo-prompt\K&RITC~1\stderr"
(echo %ERRORLEVEL%) > "C:\Users\Joran\sudo-prompt\K&RITC~1\status"

This should be exactly the same as your example, except with a different 8.3 short name.

command.bat:

echo ok

I then ran this with:

cmd /c elevate.bat

Everything ran smoothly, the status code returned was 0, and the stdout file contained ok.

Also, the example you posted looks like it is properly escaped.

Are you sure this is not an issue with your command itself?

@zvin
Copy link
Author

zvin commented Jun 4, 2019

I can reproduce the issue reliably.
I must have been wrong in my diagnosis above.
It is throwing an error here https://github.com/jorangreef/sudo-prompt/blob/master/index.js#L589 because the stdout, stderr and status files do not exist (I disabled the removal of temp files and checked in the folder).
The powershell command looks like

powershell.exe Start-Process -FilePath "'C:\Users\KRS&KR~1\AppData\Local\Temp\81a1d4081334e8e3ac6906e9bd180f37\execute.bat'" -WindowStyle hidden -Verb runAs

Launching it by hand does not create the stdout, stderr and status files.

@zvin
Copy link
Author

zvin commented Jun 4, 2019

The command itself is echo ok, my whole script is in my comment above.

@jorangreef
Copy link
Owner

I can reproduce it now.

Ignoring anything specific to sudo-prompt and PowerShell, here's a simple VBS script which will run a file in the same directory called elevate.bat with administrator permissions.

Set objShell = CreateObject("Shell.Application")
objShell.ShellExecute "elevate.bat", "", "", "runas", 0

Just save this as elevate.vbs and put it in the same directory as elevate.bat.

The interesting thing here is we are relying on the current working directory to pass the full path to the elevate.bat script. This leaves all escaping to Windows.

If the full path to these files has no ampersand, everything will work.

But, if the full path includes an ampersand, Windows itself won't escape the ampersand when ShellExecute expands the path to elevate.bat.

@jorangreef
Copy link
Owner

Whether we elevate using VBScript or PowerShell, the issue is actually in runAs with both of them, not with sudo-prompt.

We are passing the path correctly here: https://github.com/jorangreef/sudo-prompt/blob/master/index.js#L523

But PowerShell is not escaping the path when it proceeds to call cmd.exe /C itself later (you can see what PowerShell does by clicking "Show more details" in the permissions prompt UI dialog, you can actually see the full command and path constructed by PowerShell itself there).

I don't know how we can do the escaping for PowerShell. If we try, PowerShell doesn't seem to find the file we are trying to run (I think it stats it first before calling cmd.exe).

The only way seems to be to put the elevate.bat in a path somewhere on the system without ampersands. Any ideas? These will probably all be restricted paths.

Maybe you can give the escaping a shot to see if you come up with another solution?

Alternatively, in your case, you might try not using the 8.3 short name, i.e. use the long path, since that will probably not have any ampersands in it. We could also try something where if we see the path is an 8.3 short name, then we find a way to expand it (any ideas?), to get rid of the ampersand.

@zvin
Copy link
Author

zvin commented Jun 5, 2019

The ampersand is in the user name so it is also in the name of his home folder, short names only add the ~.

@Tyriar
Copy link

Tyriar commented Jun 5, 2019

@bitcrazed could we get some advice from your team on this? The issue is that an ampersand in the username is breaking the script for both powershell and vbscript, see #97 (comment)

@jorangreef
Copy link
Owner

@bitcrazed, the issue seems to be something internal to powershell and vbscript, since we're letting them do the escaping of the ampersand in #97 (comment), by referencing a file in the same cwd. It's only the full path to the cwd that contains the ampersand. The idea behind this was to rule out any escaping errors on our side.

@jorangreef
Copy link
Owner

Ping @bitcrazed, I'm pretty sure this is a bug in Windows' shell escaping...

If you can put anyone else at Microsoft onto this it would be much appreciated.

@bitcrazed
Copy link

Hey @jorangreef - could I ask you to file an issue in https://github.com/microsoft/terminal/ referencing this issue? We'll triage and comment there.

@jorangreef
Copy link
Owner

Thanks @bitcrazed, done: microsoft/terminal#2419

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants